-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Patch runtime enable #1512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Patch runtime enable #1512
Conversation
Version Packages
|
|
Too many files changed for review. |
1 similar comment
|
Too many files changed for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 issues found across 101 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/package.json">
<violation number="1" location="packages/core/package.json:2">
P0: Critical: Package identity changes appear to be fork-specific and should not be merged. The package name is changed from `@browserbasehq/stagehand` to `@natewong1313/stagehand`, and repository URLs point to a different GitHub account. This would change the published package namespace and could break existing consumers or constitute an unintentional ownership transfer.</violation>
<violation number="2" location="packages/core/package.json:107">
P1: Bugs URL points to issues on a different repository (`natewong1313/stagehand`) rather than the actual project's issue tracker (`browserbase/stagehand`).</violation>
</file>
<file name="packages/core/lib/v3/understudy/page.ts">
<violation number="1" location="packages/core/lib/v3/understudy/page.ts:692">
P1: Calling `Runtime.disable` immediately after `Runtime.enable` will disable the Runtime CDP domain, preventing `Runtime.consoleAPICalled` events from being emitted. This breaks console message tracking functionality.</violation>
</file>
<file name="packages/core/lib/v3/v3.ts">
<violation number="1" location="packages/core/lib/v3/v3.ts:970">
P1: Proxy authentication is incorrectly nested inside the downloads behavior conditional. Users who configure proxy credentials without download options will silently have no proxy authentication. Move the proxy auth block outside the downloads conditional.</violation>
<violation number="2" location="packages/core/lib/v3/v3.ts:1010">
P2: Missing `.catch(() => {})` on `session.send("Fetch.continueRequest", ...)`. Other CDP calls in this block have error handling to match the best-effort pattern. Add `.catch(() => {})` for consistency.</violation>
</file>
<file name="packages/core/lib/v3/understudy/executionContextRegistry.ts">
<violation number="1" location="packages/core/lib/v3/understudy/executionContextRegistry.ts:61">
P1: Disabling Runtime immediately after enabling it will prevent the fallback event listener from receiving `executionContextCreated` events. If the execution context isn't already in the cache after the enable/disable sequence, the Promise below (line 65+) will always timeout because no Runtime events are fired while Runtime is disabled.</violation>
</file>
<file name="packages/core/CHANGELOG.md">
<violation number="1" location="packages/core/CHANGELOG.md:7">
P2: Commit links point to `natewong1313/stagehand` (a fork) instead of the main `browserbase/stagehand` repository. This is inconsistent with all other changelog entries and may lead to broken links or confusion for users. The commit links should use `browserbase/stagehand` to match the PR links and existing changelog format.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| { | ||
| "name": "@browserbasehq/stagehand", | ||
| "version": "3.0.7", | ||
| "name": "@natewong1313/stagehand", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Critical: Package identity changes appear to be fork-specific and should not be merged. The package name is changed from @browserbasehq/stagehand to @natewong1313/stagehand, and repository URLs point to a different GitHub account. This would change the published package namespace and could break existing consumers or constitute an unintentional ownership transfer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/package.json, line 2:
<comment>Critical: Package identity changes appear to be fork-specific and should not be merged. The package name is changed from `@browserbasehq/stagehand` to `@natewong1313/stagehand`, and repository URLs point to a different GitHub account. This would change the published package namespace and could break existing consumers or constitute an unintentional ownership transfer.</comment>
<file context>
@@ -1,6 +1,6 @@
{
- "name": "@browserbasehq/stagehand",
- "version": "3.0.7",
+ "name": "@natewong1313/stagehand",
+ "version": "3.0.8",
"description": "An AI web browsing framework focused on simplicity and extensibility.",
</file context>
| "name": "@natewong1313/stagehand", | |
| "name": "@browserbasehq/stagehand", |
| }, | ||
| "bugs": { | ||
| "url": "https://github.com/browserbase/stagehand/issues" | ||
| "url": "https://github.com/natewong1313/stagehand/issues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Bugs URL points to issues on a different repository (natewong1313/stagehand) rather than the actual project's issue tracker (browserbase/stagehand).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/package.json, line 107:
<comment>Bugs URL points to issues on a different repository (`natewong1313/stagehand`) rather than the actual project's issue tracker (`browserbase/stagehand`).</comment>
<file context>
@@ -100,11 +100,11 @@
},
"bugs": {
- "url": "https://github.com/browserbase/stagehand/issues"
+ "url": "https://github.com/natewong1313/stagehand/issues"
},
"homepage": "https://stagehand.dev"
</file context>
| "url": "https://github.com/natewong1313/stagehand/issues" | |
| "url": "https://github.com/browserbase/stagehand/issues" |
| if (this.consoleHandlers.has(key)) return; | ||
|
|
||
| void session.send("Runtime.enable").catch(() => {}); | ||
| void session.send("Runtime.disable").catch(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Calling Runtime.disable immediately after Runtime.enable will disable the Runtime CDP domain, preventing Runtime.consoleAPICalled events from being emitted. This breaks console message tracking functionality.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/page.ts, line 692:
<comment>Calling `Runtime.disable` immediately after `Runtime.enable` will disable the Runtime CDP domain, preventing `Runtime.consoleAPICalled` events from being emitted. This breaks console message tracking functionality.</comment>
<file context>
@@ -689,6 +689,7 @@ export class Page {
if (this.consoleHandlers.has(key)) return;
void session.send("Runtime.enable").catch(() => {});
+ void session.send("Runtime.disable").catch(() => {});
const handler = (evt: Protocol.Runtime.ConsoleAPICalledEvent) => {
</file context>
| eventsEnabled: true, | ||
| }) | ||
| .catch(() => {}); | ||
| // Proxy authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Proxy authentication is incorrectly nested inside the downloads behavior conditional. Users who configure proxy credentials without download options will silently have no proxy authentication. Move the proxy auth block outside the downloads conditional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/v3.ts, line 970:
<comment>Proxy authentication is incorrectly nested inside the downloads behavior conditional. Users who configure proxy credentials without download options will silently have no proxy authentication. Move the proxy auth block outside the downloads conditional.</comment>
<file context>
@@ -965,6 +967,58 @@ export class V3 {
eventsEnabled: true,
})
.catch(() => {});
+ // Proxy authentication
+ if (lbo.proxy?.username && lbo.proxy?.password) {
+ const page = await this.ctx?.awaitActivePage();
</file context>
| if (cached) return cached; | ||
|
|
||
| await session.send("Runtime.enable").catch(() => {}); | ||
| await session.send("Runtime.disable").catch(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Disabling Runtime immediately after enabling it will prevent the fallback event listener from receiving executionContextCreated events. If the execution context isn't already in the cache after the enable/disable sequence, the Promise below (line 65+) will always timeout because no Runtime events are fired while Runtime is disabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/executionContextRegistry.ts, line 61:
<comment>Disabling Runtime immediately after enabling it will prevent the fallback event listener from receiving `executionContextCreated` events. If the execution context isn't already in the cache after the enable/disable sequence, the Promise below (line 65+) will always timeout because no Runtime events are fired while Runtime is disabled.</comment>
<file context>
@@ -58,6 +58,7 @@ export class ExecutionContextRegistry {
if (cached) return cached;
await session.send("Runtime.enable").catch(() => {});
+ await session.send("Runtime.disable").catch(() => {});
const after = this.getMainWorld(session, frameId);
if (after) return after;
</file context>
| const requestPausedHandler = ({ | ||
| requestId, | ||
| }: Protocol.Fetch.RequestPausedEvent) => { | ||
| session.send("Fetch.continueRequest", { requestId }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Missing .catch(() => {}) on session.send("Fetch.continueRequest", ...). Other CDP calls in this block have error handling to match the best-effort pattern. Add .catch(() => {}) for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/v3.ts, line 1010:
<comment>Missing `.catch(() => {})` on `session.send("Fetch.continueRequest", ...)`. Other CDP calls in this block have error handling to match the best-effort pattern. Add `.catch(() => {})` for consistency.</comment>
<file context>
@@ -965,6 +967,58 @@ export class V3 {
+ const requestPausedHandler = ({
+ requestId,
+ }: Protocol.Fetch.RequestPausedEvent) => {
+ session.send("Fetch.continueRequest", { requestId });
+ };
+
</file context>
| session.send("Fetch.continueRequest", { requestId }); | |
| session.send("Fetch.continueRequest", { requestId }).catch(() => {}); |
| @@ -1,5 +1,23 @@ | |||
| # @browserbasehq/stagehand | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Commit links point to natewong1313/stagehand (a fork) instead of the main browserbase/stagehand repository. This is inconsistent with all other changelog entries and may lead to broken links or confusion for users. The commit links should use browserbase/stagehand to match the PR links and existing changelog format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/CHANGELOG.md, line 7:
<comment>Commit links point to `natewong1313/stagehand` (a fork) instead of the main `browserbase/stagehand` repository. This is inconsistent with all other changelog entries and may lead to broken links or confusion for users. The commit links should use `browserbase/stagehand` to match the PR links and existing changelog format.</comment>
<file context>
@@ -1,5 +1,23 @@
+
+### Patch Changes
+
+- [#1486](https://github.com/browserbase/stagehand/pull/1486) [`692ffa0`](https://github.com/natewong1313/stagehand/commit/692ffa0346ad3d121686aba503c0a22844293efa) Thanks [@tkattkat](https://github.com/tkattkat)! - improve logging in agent
+
+- [#1495](https://github.com/browserbase/stagehand/pull/1495) [`72ac775`](https://github.com/natewong1313/stagehand/commit/72ac775a831d6f0f376ceda4426525f93cc21452) Thanks [@tkattkat](https://github.com/tkattkat)! - export tool function & type to simplify defining custom tools
</file context>
| # @browserbasehq/stagehand | |
| - [#1486](https://github.com/browserbase/stagehand/pull/1486) [`692ffa0`](https://github.com/browserbase/stagehand/commit/692ffa0346ad3d121686aba503c0a22844293efa) Thanks [@tkattkat](https://github.com/tkattkat)! - improve logging in agent |
|
Too many files changed for review. |
8 similar comments
|
Too many files changed for review. |
|
Too many files changed for review. |
|
Too many files changed for review. |
|
Too many files changed for review. |
|
Too many files changed for review. |
|
Too many files changed for review. |
|
Too many files changed for review. |
|
Too many files changed for review. |
why
what changed
test plan
Summary by cubic
Adds proxy authentication support and stabilizes CDP Runtime usage by enabling then immediately disabling around core operations. Also simplifies CI and updates the package scope to @natewong1313.
New Features
Refactors
Written for commit 460136a. Summary will update on new commits.