feat: scaffold Stagehand code-mode MCP host - #2597
Open
shrey150 wants to merge 2 commits into
Open
Conversation
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Confidence score: 3/5
- In
packages/integrations/src/codemode/executor.ts, the native executor path appears non-interruptible once execution starts because abort is only checked pre-run and bothexecute/closesharethis.queue; a hung snippet can block shutdown and tie up the worker indefinitely—add in-flight cancellation checks and ensureclosecan preempt or bypass a blocked execution path. - In
packages/integrations/src/codemode/executor.ts, the 256 KB result cap is enforced only after fullJSON.stringify, so very large snippet outputs can still cause memory spikes before truncation, with potential OOM/instability under hostile or accidental large returns—enforce limits during serialization/streaming or short-circuit oversized structures before full stringification. - Across
packages/integrations/src/codemode/tool-contract.ts,packages/integrations/src/codemode/executor.ts,packages/integrations/scripts/generate-codemode-content.mjs, andpackages/integrations/src/codemode/snippet.ts, key new contract/executor/escaping/binding behaviors lack registered focused tests, increasing regression risk for public codemode behavior and CI blind spots—add and register targeted tests for queueing/cancellation, schema/result formatting, content escaping with--check, andexecuteStagehandSnippetvalidation paths.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/integrations/src/codemode/tool-contract.ts">
<violation number="1" location="packages/integrations/src/codemode/tool-contract.ts:24">
P3: Current CI cannot exercise this new public contract because the package has no registered tests. Add focused schema and result-formatting tests, then register a package test task.
(Based on your team's feedback about unit tests for new behavior.) .</violation>
</file>
<file name="packages/integrations/src/codemode/executor.ts">
<violation number="1" location="packages/integrations/src/codemode/executor.ts:25">
P3: This new executor has no committed focused tests for queueing, lazy lifecycle, error cleanup, or serialization edge cases, leaving its public native/MCP behavior unguarded. Add executor-level tests covering a normal persistent call and key failure/close paths.
(Based on your team's feedback about unit tests for new behavior.) .</violation>
<violation number="2" location="packages/integrations/src/codemode/executor.ts:84">
P2: For the native (non-MCP) executor path, a non-terminating snippet can never be interrupted: the AbortSignal is only checked once before the code starts, and `execute`/`close` both serialize through `this.queue`. A snippet that loops forever leaves the in-flight call unresolved, so `close()` (which waits on the queue) never resolves and the persistent browser/process leaks on shutdown. Consider passing the signal through to the snippet so an abort can terminate the run, or otherwise bound execution time.</violation>
<violation number="3" location="packages/integrations/src/codemode/executor.ts:188">
P2: The size guard only kicks in after `JSON.stringify` has fully built the result string in memory, so a snippet that returns a very large value still consumes unbounded memory before the 256 KB truncation is applied. This undermines MAX_RESULT_BYTES as a safety/memory bound in an agent-facing code executor that has no sandbox. Truncating incrementally (e.g., streaming serialization) or imposing a forced/capped serialization would keep memory usage proportional to the configured limit.</violation>
</file>
<file name="packages/integrations/scripts/generate-codemode-content.mjs">
<violation number="1" location="packages/integrations/scripts/generate-codemode-content.mjs:28">
P3: Generated-content escaping and stale-file validation have no automated coverage; add focused tests for special characters plus current and stale `--check` output.
(Based on your team's feedback about unit tests for new behavior.)</violation>
</file>
<file name="packages/integrations/src/codemode/snippet.ts">
<violation number="1" location="packages/integrations/src/codemode/snippet.ts:11">
P3: Regression coverage is absent for `executeStagehandSnippet`, including binding validation and injected dependency behavior. Add focused unit tests for normal execution, reserved/invalid names, and optional `stagehand`.
(Based on your team's feedback about adding unit tests for new behavior.) .</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
shriyatheunicorn
force-pushed
the
shrey/stg-2765-codemode-package
branch
from
August 5, 2026 07:14
dab19ae to
171765f
Compare
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
shrey150
marked this pull request as draft
August 5, 2026 19:57
shrey150
force-pushed
the
shrey/stg-2765-codemode-package
branch
from
August 6, 2026 08:34
8517905 to
2cfa7b0
Compare
shrey150
changed the title
This was referenced Aug 6, 2026
shrey150
marked this pull request as ready for review
August 6, 2026 08:57
Contributor
There was a problem hiding this comment.
6 issues found across 15 files
Confidence score: 3/5
- In
packages/integrations/src/codemode/stdio-lifecycle.ts,shutdowncan reject whenresource.close()throws synchronously duringallSettledinput construction, which breaks the intendedfalsefailure path and can leave teardown behavior inconsistent—wrap each close call in a promise chain so sync throws are captured as settled failures. - In
packages/integrations/src/codemode/stdio-server.ts, installing signal/stdin handlers only afterconnectCodeModeStdio(server)resolves means early termination can skip the shutdown path, risking missed cleanup and incorrect exit-code handling—register handlers before the startup await. - In
.github/workflows/ci.yml, the new path behavior can run thebrowser-tsworkflow (including secret-backed/paid Browserbase smoke steps) for integrations-only changes without an explicit approval gate, creating cost and secret-exposure risk on untrusted contributions—add fork/approval guards or tighten path conditions. - The test reliability/caching changes around
turbo.json,packages/integrations/package.json, andpackages/integrations/tests/stdio-server.test.tsweaken CI signal: missinginputscan cause stale cache hits,test:unitdepends on prebuilt artifacts, and one stderr assertion is timing-sensitive—restore explicit task inputs, maketest:unitself-contained (or enforce build), and make stderr assertions deterministic.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/integrations/tests/stdio-server.test.ts">
<violation number="1" location="packages/integrations/tests/stdio-server.test.ts:107">
P3: The `stderr` assertion in the third test races the async delivery of the child's stderr output against the MCP connect handshake. Since stderr and the init handshake are separate pipes on the parent event loop, the 'data' event may not have fired by the time `connect` resolves, making this test intermittently flaky. Wait for the ready line (e.g. poll `stderr` in a helper that resolves once it contains the ready message) before asserting to make it deterministic.</violation>
</file>
<file name="packages/integrations/src/codemode/stdio-server.ts">
<violation number="1" location="packages/integrations/src/codemode/stdio-server.ts:5">
P2: Early termination can bypass your shutdown path because signal/stdin handlers are installed only after `connectCodeModeStdio(server)` resolves. Installing handlers before the startup await keeps cleanup and exit-code behavior consistent even when the process is interrupted during initialization.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:40">
P2: Integrations-only PRs now trigger the `browser-ts` workflow path, which includes the Browserbase smoke step with secret-backed/paid-resource config and no explicit maintainer approval gate. Consider adding a fork approval condition (for example `safe-to-test`) for that job or splitting integrations into a separate output that only runs non-secret checks.
(Based on your team's feedback about maintainer approval gates for external PR workflows using secrets or paid resources.)</violation>
</file>
<file name="packages/integrations/src/codemode/stdio-lifecycle.ts">
<violation number="1" location="packages/integrations/src/codemode/stdio-lifecycle.ts:12">
P2: Shutdown can reject instead of returning `false` when a closer throws synchronously. `resource.close()` is evaluated while building the `allSettled` input, so wrapping each call in a promise chain keeps sync throws contained as rejected results.</violation>
</file>
<file name="packages/integrations/package.json">
<violation number="1" location="packages/integrations/package.json:18">
P3: The package's `test:unit` script is not self-contained: unlike the `test` script, it runs vitest without building first, yet `tests/stdio-server.test.ts` imports the compiled artifact `../dist/codemode/stdio-server.mjs`. In a clean checkout (no `dist/` yet), invoking `pnpm test:unit` directly fails, and it only works because the Turbo task `@browserbasehq/stagehand-integrations#test:unit` is configured with `dependsOn: ["^build", ...#build]`. This hidden ordering makes the standalone script silently depend on a prior build. Consider making `test:unit` self-contained (e.g., build first like `test`) or documenting the Turbo dependency so the behavior isn't surprising.</violation>
</file>
<file name="turbo.json">
<violation number="1" location="turbo.json:73">
P2: The new `@browserbasehq/stagehand-integrations#test:unit` task adds a `dependsOn` but drops the `inputs` block that every other cacheable test task in this repo defines. Because the package's test script runs `vitest run --root ../..`, it resolves the repo-root `vitest.config.ts` — which is not in turbo's `globalDependencies` — so changes to that config (including the `packages/integrations/tests` glob added in this PR) won't invalidate the turbo cache, and it can replay stale results. It also leaves `dist/**`/`.turbo/**` in the task's file hash, unlike the stable cache-key setup used by the extension, sdk-ts, protocol, and docs test tasks. Consider giving this task the same explicit `inputs` (including `$TURBO_ROOT$/vitest.config.ts` and the `!dist/**`, `!.turbo/**`, `!node_modules/**` exclusions) to match the repo convention and keep the cache key stable.</violation>
</file>
Architecture diagram
sequenceDiagram
participant MCP_Client as MCP Client
participant StdioTransport as Stdio Transport
participant ServerEntry as stdio-server.ts
participant McpHost as McpServer Instance
participant Shutdown as closeCodeModeStdio()
Note over MCP_Client,Shutdown: NEW: Stagehand code-mode MCP host startup and lifecycle
MCP_Client->>StdioTransport: spawn process (node dist/codemode/stdio-server.mjs)
StdioTransport->>ServerEntry: stdin/stdout/stderr connected
ServerEntry->>ServerEntry: createCodeModeMcpHost()
ServerEntry->>McpHost: NEW: McpServer("stagehand-codemode", "4.0.0")
McpHost-->>ServerEntry: ready
ServerEntry->>ServerEntry: connectCodeModeStdio(server)
ServerEntry->>McpHost: connect(new StdioServerTransport())
McpHost-->>ServerEntry: connected
ServerEntry->>StdioTransport: stderr: "Stagehand code-mode MCP host listening on stdio"
Note over StdioTransport,McpHost: MCP initialization handshake
MCP_Client->>StdioTransport: initialize request
StdioTransport->>McpHost: forward initialize
McpHost-->>StdioTransport: capabilities (no tools)
StdioTransport-->>MCP_Client: capabilities
Note over MCP_Client,Shutdown: Shutdown scenarios
alt stdin EOF (normal shutdown)
ServerEntry->>ServerEntry: stdin "end" event
ServerEntry->>McpHost: close()
ServerEntry->>Shutdown: closeCodeModeStdio([server])
Shutdown->>McpHost: close()
alt cleanup completes within 5s
McpHost-->>Shutdown: resolved
Shutdown-->>ServerEntry: true
ServerEntry->>ServerEntry: process.exit(0)
else cleanup timeout
Shutdown->>Shutdown: setTimeout(5000)
Shutdown-->>ServerEntry: false
ServerEntry->>StdioTransport: stderr: "Failed to close..."
ServerEntry->>ServerEntry: process.exit(1)
end
else SIGINT (Ctrl+C)
ServerEntry->>ServerEntry: signal handler
ServerEntry->>McpHost: close()
ServerEntry->>Shutdown: closeCodeModeStdio([server])
Shutdown-->>ServerEntry: result
ServerEntry->>ServerEntry: process.exit(130)
else SIGTERM
ServerEntry->>ServerEntry: signal handler
ServerEntry->>McpHost: close()
ServerEntry->>Shutdown: closeCodeModeStdio([server])
Shutdown-->>ServerEntry: result
ServerEntry->>ServerEntry: process.exit(143)
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| import { closeCodeModeStdio } from "./stdio-lifecycle.js"; | ||
|
|
||
| const server = createCodeModeMcpHost(); | ||
| await connectCodeModeStdio(server); |
Contributor
There was a problem hiding this comment.
P2: Early termination can bypass your shutdown path because signal/stdin handlers are installed only after connectCodeModeStdio(server) resolves. Installing handlers before the startup await keeps cleanup and exit-code behavior consistent even when the process is interrupted during initialization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/src/codemode/stdio-server.ts, line 5:
<comment>Early termination can bypass your shutdown path because signal/stdin handlers are installed only after `connectCodeModeStdio(server)` resolves. Installing handlers before the startup await keeps cleanup and exit-code behavior consistent even when the process is interrupted during initialization.</comment>
<file context>
@@ -0,0 +1,22 @@
+import { closeCodeModeStdio } from "./stdio-lifecycle.js";
+
+const server = createCodeModeMcpHost();
+await connectCodeModeStdio(server);
+let closing = false;
+
</file context>
| - '.github/**' | ||
| sdk-ts: | ||
| - 'packages/sdk-ts/**' | ||
| - 'packages/integrations/**' |
Contributor
There was a problem hiding this comment.
P2: Integrations-only PRs now trigger the browser-ts workflow path, which includes the Browserbase smoke step with secret-backed/paid-resource config and no explicit maintainer approval gate. Consider adding a fork approval condition (for example safe-to-test) for that job or splitting integrations into a separate output that only runs non-secret checks.
(Based on your team's feedback about maintainer approval gates for external PR workflows using secrets or paid resources.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 40:
<comment>Integrations-only PRs now trigger the `browser-ts` workflow path, which includes the Browserbase smoke step with secret-backed/paid-resource config and no explicit maintainer approval gate. Consider adding a fork approval condition (for example `safe-to-test`) for that job or splitting integrations into a separate output that only runs non-secret checks.
(Based on your team's feedback about maintainer approval gates for external PR workflows using secrets or paid resources.) </comment>
<file context>
@@ -37,6 +37,7 @@ jobs:
- '.github/**'
sdk-ts:
- 'packages/sdk-ts/**'
+ - 'packages/integrations/**'
- 'packages/extension/**'
- 'packages/protocol/**'
</file context>
Comment on lines
+12
to
+14
| const cleanup = Promise.allSettled(resources.map((resource) => resource.close())).then( | ||
| (results) => results.every((result) => result.status === "fulfilled"), | ||
| ); |
Contributor
There was a problem hiding this comment.
P2: Shutdown can reject instead of returning false when a closer throws synchronously. resource.close() is evaluated while building the allSettled input, so wrapping each call in a promise chain keeps sync throws contained as rejected results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/src/codemode/stdio-lifecycle.ts, line 12:
<comment>Shutdown can reject instead of returning `false` when a closer throws synchronously. `resource.close()` is evaluated while building the `allSettled` input, so wrapping each call in a promise chain keeps sync throws contained as rejected results.</comment>
<file context>
@@ -0,0 +1,25 @@
+ timeoutMs = STDIO_SHUTDOWN_GRACE_MS,
+): Promise<boolean> {
+ let timeout: NodeJS.Timeout | undefined;
+ const cleanup = Promise.allSettled(resources.map((resource) => resource.close())).then(
+ (results) => results.every((result) => result.status === "fulfilled"),
+ );
</file context>
Suggested change
| const cleanup = Promise.allSettled(resources.map((resource) => resource.close())).then( | |
| (results) => results.every((result) => result.status === "fulfilled"), | |
| ); | |
| const cleanup = Promise.allSettled( | |
| resources.map((resource) => Promise.resolve().then(() => resource.close())), | |
| ).then((results) => results.every((result) => result.status === "fulfilled")); |
| "dependsOn": ["^build"] | ||
| }, | ||
| "@browserbasehq/stagehand-integrations#test:unit": { | ||
| "dependsOn": ["^build", "@browserbasehq/stagehand-integrations#build"] |
Contributor
There was a problem hiding this comment.
P2: The new @browserbasehq/stagehand-integrations#test:unit task adds a dependsOn but drops the inputs block that every other cacheable test task in this repo defines. Because the package's test script runs vitest run --root ../.., it resolves the repo-root vitest.config.ts — which is not in turbo's globalDependencies — so changes to that config (including the packages/integrations/tests glob added in this PR) won't invalidate the turbo cache, and it can replay stale results. It also leaves dist/**/.turbo/** in the task's file hash, unlike the stable cache-key setup used by the extension, sdk-ts, protocol, and docs test tasks. Consider giving this task the same explicit inputs (including $TURBO_ROOT$/vitest.config.ts and the !dist/**, !.turbo/**, !node_modules/** exclusions) to match the repo convention and keep the cache key stable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At turbo.json, line 73:
<comment>The new `@browserbasehq/stagehand-integrations#test:unit` task adds a `dependsOn` but drops the `inputs` block that every other cacheable test task in this repo defines. Because the package's test script runs `vitest run --root ../..`, it resolves the repo-root `vitest.config.ts` — which is not in turbo's `globalDependencies` — so changes to that config (including the `packages/integrations/tests` glob added in this PR) won't invalidate the turbo cache, and it can replay stale results. It also leaves `dist/**`/`.turbo/**` in the task's file hash, unlike the stable cache-key setup used by the extension, sdk-ts, protocol, and docs test tasks. Consider giving this task the same explicit `inputs` (including `$TURBO_ROOT$/vitest.config.ts` and the `!dist/**`, `!.turbo/**`, `!node_modules/**` exclusions) to match the repo convention and keep the cache key stable.</comment>
<file context>
@@ -61,6 +66,12 @@
+ "dependsOn": ["^build"]
+ },
+ "@browserbasehq/stagehand-integrations#test:unit": {
+ "dependsOn": ["^build", "@browserbasehq/stagehand-integrations#build"]
+ },
"@browserbasehq/stagehand-docs#typecheck": {},
</file context>
| try { | ||
| await client.connect(transport); | ||
| expect(client.getServerCapabilities()).not.toHaveProperty("tools"); | ||
| expect(stderr).toContain("Stagehand code-mode MCP host listening on stdio"); |
Contributor
There was a problem hiding this comment.
P3: The stderr assertion in the third test races the async delivery of the child's stderr output against the MCP connect handshake. Since stderr and the init handshake are separate pipes on the parent event loop, the 'data' event may not have fired by the time connect resolves, making this test intermittently flaky. Wait for the ready line (e.g. poll stderr in a helper that resolves once it contains the ready message) before asserting to make it deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/tests/stdio-server.test.ts, line 107:
<comment>The `stderr` assertion in the third test races the async delivery of the child's stderr output against the MCP connect handshake. Since stderr and the init handshake are separate pipes on the parent event loop, the 'data' event may not have fired by the time `connect` resolves, making this test intermittently flaky. Wait for the ready line (e.g. poll `stderr` in a helper that resolves once it contains the ready message) before asserting to make it deterministic.</comment>
<file context>
@@ -0,0 +1,112 @@
+ try {
+ await client.connect(transport);
+ expect(client.getServerCapabilities()).not.toHaveProperty("tools");
+ expect(stderr).toContain("Stagehand code-mode MCP host listening on stdio");
+ } finally {
+ await client.close();
</file context>
| "scripts": { | ||
| "build": "tsdown", | ||
| "test": "pnpm run build && vitest run --root ../.. packages/integrations/tests", | ||
| "test:unit": "vitest run --root ../.. packages/integrations/tests", |
Contributor
There was a problem hiding this comment.
P3: The package's test:unit script is not self-contained: unlike the test script, it runs vitest without building first, yet tests/stdio-server.test.ts imports the compiled artifact ../dist/codemode/stdio-server.mjs. In a clean checkout (no dist/ yet), invoking pnpm test:unit directly fails, and it only works because the Turbo task @browserbasehq/stagehand-integrations#test:unit is configured with dependsOn: ["^build", ...#build]. This hidden ordering makes the standalone script silently depend on a prior build. Consider making test:unit self-contained (e.g., build first like test) or documenting the Turbo dependency so the behavior isn't surprising.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/package.json, line 18:
<comment>The package's `test:unit` script is not self-contained: unlike the `test` script, it runs vitest without building first, yet `tests/stdio-server.test.ts` imports the compiled artifact `../dist/codemode/stdio-server.mjs`. In a clean checkout (no `dist/` yet), invoking `pnpm test:unit` directly fails, and it only works because the Turbo task `@browserbasehq/stagehand-integrations#test:unit` is configured with `dependsOn: ["^build", ...#build]`. This hidden ordering makes the standalone script silently depend on a prior build. Consider making `test:unit` self-contained (e.g., build first like `test`) or documenting the Turbo dependency so the behavior isn't surprising.</comment>
<file context>
@@ -0,0 +1,33 @@
+ "scripts": {
+ "build": "tsdown",
+ "test": "pnpm run build && vitest run --root ../.. packages/integrations/tests",
+ "test:unit": "vitest run --root ../.. packages/integrations/tests",
+ "typecheck": "tsc --noEmit -p tsconfig.json"
+ },
</file context>
Suggested change
| "test:unit": "vitest run --root ../.. packages/integrations/tests", | |
| "test:unit": "pnpm run build && vitest run --root ../.. packages/integrations/tests", |
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The code-mode product work spans three distinct review domains: process hosting, browser code execution, and agent guidance. This bottom PR isolates the package and Model Context Protocol (MCP) host so its build, transport, and shutdown behavior can be reviewed without the execution engine or prompt content.
Stack
code_executeregistrationSKILL.md,REFERENCE.md, generated exports, package assets, and guidance loading checksEach PR is intended to build, test, and make a truthful claim independently.
What changed
@browserbasehq/stagehand-integrationsworkspace packageSIGINTandSIGTERMIntentionally not included
E2E Test Matrix
pnpm --filter @browserbasehq/stagehand-integrations typecheck && pnpm --filter @browserbasehq/stagehand-integrations build && pnpm --filter @browserbasehq/stagehand-integrations test:unit{"initialized":true,"toolsCapability":null,"readyMessage":true}pnpm exec turbo run test:unit --filter=@browserbasehq/stagehand-integrationspnpm checkChangeset
None. This introduces a private workspace package and does not publish a release.