feat(crewai): add Stagehand code-mode MCP example - #2628
Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…into shrey/stg-2765-codemode-crewai # Conflicts: # .github/workflows/codemode-framework-examples.yml # packages/integrations/README.md
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Confidence score: 5/5
- In
packages/integrations/examples/crewai/e2e.py, malformed MCP-result handling is only exercised by a secret-backed live smoke test, so parser/contract regressions could slip through and be hard to reproduce locally when failures occur — add focused local tests for valid results, invalid JSON, and explicit failure 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/examples/crewai/e2e.py">
<violation number="1" location="packages/integrations/examples/crewai/e2e.py:14">
P3: Malformed MCP-result handling is only covered by the secret-backed live smoke, making parser/contract regressions hard to diagnose or exercise locally. Add focused tests for accepted results and invalid JSON, failed, and non-object value payloads.
(Based on your team's feedback about adding unit tests for new behavior.)</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| from sandbox import StagehandSandboxLease | ||
|
|
||
|
|
||
| def successful_result(raw_result: Any) -> dict[str, Any]: |
There was a problem hiding this comment.
P3: Malformed MCP-result handling is only covered by the secret-backed live smoke, making parser/contract regressions hard to diagnose or exercise locally. Add focused tests for accepted results and invalid JSON, failed, and non-object value payloads.
(Based on your team's feedback about adding unit tests for new behavior.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/examples/crewai/e2e.py, line 14:
<comment>Malformed MCP-result handling is only covered by the secret-backed live smoke, making parser/contract regressions hard to diagnose or exercise locally. Add focused tests for accepted results and invalid JSON, failed, and non-object value payloads.
(Based on your team's feedback about adding unit tests for new behavior.) </comment>
<file context>
@@ -0,0 +1,135 @@
+from sandbox import StagehandSandboxLease
+
+
+def successful_result(raw_result: Any) -> dict[str, Any]:
+ result = json.loads(str(raw_result))
+ assert result["ok"] is True, result
</file context>
There was a problem hiding this comment.
Addressed in 60e1c451. Added four secret-free parser tests covering an accepted result, invalid JSON, a failed result, and a non-object value. Failures now use a typed fixed-message error without reflecting raw MCP payloads.
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
There was a problem hiding this comment.
3 issues found across 12 files (changes from recent commits).
Confidence score: 4/5
- In
packages/integrations/examples/shared/vercel_sandbox_lease.py,parse_connectionnow enforces stricter shape and token-length rules, buttest_connection_parser_rejects_untrusted_shapesonly exercises a narrow invalid case; edge shapes could slip through and either over-reject valid inputs or accept malformed ones in production — expand parser tests to cover each rejected component and boundary token lengths. - In
packages/integrations/examples/vercel-sandbox/src/sandbox.ts, the new cancellation contract is untested for abort-before-setup and abort-during-readiness paths, so cancellation regressions could leak listeners or leave sandbox startup state inconsistent — add focused mocked sandbox tests for both abort timings and explicit listener cleanup. - In
packages/integrations/examples/crewai/agent.py, there is no focused coverage for dual-failure scenarios where discovery/yielded-body work fails andstop()also fails, which risks silently dropping one error and obscuring root cause during teardown — addFakeAdaptertests that force both failures and assert both are preserved.
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/examples/vercel-sandbox/src/sandbox.ts">
<violation number="1" location="packages/integrations/examples/vercel-sandbox/src/sandbox.ts:90">
P3: The new cancellation contract has no focused test, leaving aborted-before-setup and aborted-during-readiness behavior unguarded. Add mocked sandbox tests for both paths and listener cleanup.
(Based on your team's feedback about unit tests.) .</violation>
</file>
<file name="packages/integrations/examples/crewai/agent.py">
<violation number="1" location="packages/integrations/examples/crewai/agent.py:79">
P3: Dual-failure behavior can regress unnoticed: no test makes discovery or yielded-body work fail while `stop()` also fails, so neither new group verifies both preserved failures. Add focused `FakeAdapter` tests for setup and run branches, including member order/types.
(Based on your team's feedback about adding unit tests for new behavior.) .</violation>
</file>
<file name="packages/integrations/examples/shared/vercel_sandbox_lease.py">
<violation number="1" location="packages/integrations/examples/shared/vercel_sandbox_lease.py:237">
P3: The new `parse_connection` hardening added username/password/query/fragment rejection and a fixed 43-character token-length bound, but the parser test (`test_connection_parser_rejects_untrusted_shapes`) only covers non-JSON, non-https scheme, non-`/mcp` path, and empty token. The newly added branches aren't exercised, so a regression in those specific checks (for example accidentally allowing credentials or a query string in the URL) wouldn't be caught. Consider adding cases for `https://user:pass@host/mcp`, `https://host/mcp?x=1`, a URL with a fragment, and an out-of-length token (e.g. 42 or 44 chars) so the new validation is encoded in executable form alongside the other new contract tests.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| readinessTimeoutMs?: number; | ||
| sandboxTimeoutMs?: number; | ||
| cleanupTimeoutMs?: number; | ||
| signal?: AbortSignal; |
There was a problem hiding this comment.
P3: The new cancellation contract has no focused test, leaving aborted-before-setup and aborted-during-readiness behavior unguarded. Add mocked sandbox tests for both paths and listener cleanup.
(Based on your team's feedback about unit tests.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/examples/vercel-sandbox/src/sandbox.ts, line 90:
<comment>The new cancellation contract has no focused test, leaving aborted-before-setup and aborted-during-readiness behavior unguarded. Add mocked sandbox tests for both paths and listener cleanup.
(Based on your team's feedback about unit tests.) .</comment>
<file context>
@@ -87,6 +87,7 @@ export type StagehandSandboxOptions = {
readinessTimeoutMs?: number;
sandboxTimeoutMs?: number;
cleanupTimeoutMs?: number;
+ signal?: AbortSignal;
};
</file context>
There was a problem hiding this comment.
Added in e1b860ee. Contracts now cover an already-aborted signal before sandbox creation and cancellation during readiness. The latter uses a fully mocked sandbox, verifies polling stops, stop/delete run, and no abort listeners remain. The sandbox contract suite passes 15/15.
| try: | ||
| stop_adapter(adapter) | ||
| except StagehandCrewAICleanupError as cleanup_error: | ||
| raise BaseExceptionGroup( |
There was a problem hiding this comment.
P3: Dual-failure behavior can regress unnoticed: no test makes discovery or yielded-body work fail while stop() also fails, so neither new group verifies both preserved failures. Add focused FakeAdapter tests for setup and run branches, including member order/types.
(Based on your team's feedback about adding unit tests for new behavior.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/examples/crewai/agent.py, line 79:
<comment>Dual-failure behavior can regress unnoticed: no test makes discovery or yielded-body work fail while `stop()` also fails, so neither new group verifies both preserved failures. Add focused `FakeAdapter` tests for setup and run branches, including member order/types.
(Based on your team's feedback about adding unit tests for new behavior.) .</comment>
<file context>
@@ -23,52 +43,86 @@ def stagehand_code_tools(
+ try:
+ stop_adapter(adapter)
+ except StagehandCrewAICleanupError as cleanup_error:
+ raise BaseExceptionGroup(
+ "CrewAI MCP setup and cleanup both failed",
+ [primary_error, cleanup_error],
</file context>
There was a problem hiding this comment.
Added in e1b860ee. Focused setup+cleanup and run+cleanup tests now assert two-member BaseExceptionGroups, preserve member order, and verify the typed connection/run-primary and cleanup members. CrewAI tests pass 10/10.
| or parsed.path != "/mcp" | ||
| or parsed.username is not None | ||
| or parsed.password is not None | ||
| or parsed.query |
There was a problem hiding this comment.
P3: The new parse_connection hardening added username/password/query/fragment rejection and a fixed 43-character token-length bound, but the parser test (test_connection_parser_rejects_untrusted_shapes) only covers non-JSON, non-https scheme, non-/mcp path, and empty token. The newly added branches aren't exercised, so a regression in those specific checks (for example accidentally allowing credentials or a query string in the URL) wouldn't be caught. Consider adding cases for https://user:pass@host/mcp, https://host/mcp?x=1, a URL with a fragment, and an out-of-length token (e.g. 42 or 44 chars) so the new validation is encoded in executable form alongside the other new contract tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/examples/shared/vercel_sandbox_lease.py, line 237:
<comment>The new `parse_connection` hardening added username/password/query/fragment rejection and a fixed 43-character token-length bound, but the parser test (`test_connection_parser_rejects_untrusted_shapes`) only covers non-JSON, non-https scheme, non-`/mcp` path, and empty token. The newly added branches aren't exercised, so a regression in those specific checks (for example accidentally allowing credentials or a query string in the URL) wouldn't be caught. Consider adding cases for `https://user:pass@host/mcp`, `https://host/mcp?x=1`, a URL with a fragment, and an out-of-length token (e.g. 42 or 44 chars) so the new validation is encoded in executable form alongside the other new contract tests.</comment>
<file context>
@@ -194,16 +215,28 @@ def parse_connection(line: str) -> StagehandSandboxConnection:
+ or parsed.path != "/mcp"
+ or parsed.username is not None
+ or parsed.password is not None
+ or parsed.query
+ or parsed.fragment
+ ):
</file context>
There was a problem hiding this comment.
Added in e1b860ee. The parser regression table now includes userinfo, query, fragment, and both 42- and 44-character tokens, alongside the earlier scheme/path/empty-token cases. Shared lease tests pass 8/8.
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Confidence score: 4/5
- In
packages/integrations/examples/vercel-sandbox/src/lease.test.mjs, the expected{ code: 143, signal: null }appears inconsistent withlease.tsSIGTERM-forwarding behavior, which can cause misleading test failures or validate the wrong contract — update the assertion to match the intended signal/exit semantics. - In
packages/integrations/examples/shared/vercel_sandbox_lease.py, the new three-way setup-error handling only has coverage for two branches, leaving one path unguarded against regressions — add a regression test for the uncovered setup-error branch to de-risk future changes.
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/examples/vercel-sandbox/src/lease.test.mjs">
<violation number="1" location="packages/integrations/examples/vercel-sandbox/src/lease.test.mjs:116">
P2: The assertion `{ code: 143, signal: null }` does not match the lease's signal-forwarding semantics. lease.ts propagates SIGTERM via `process.kill(process.pid, 'SIGTERM')` after removing listeners, so the child dies *by signal* and the parent `close` event reports `{ code: null, signal: 'SIGTERM' }` (128+signal=143 is a shell convention, not what Node's child_process reports). As written, this test will fail on the signal path it is meant to validate. Assert `{ code: null, signal: 'SIGTERM' }` instead to encode actual signal-style exit semantics.</violation>
</file>
<file name="packages/integrations/examples/shared/vercel_sandbox_lease.py">
<violation number="1" location="packages/integrations/examples/shared/vercel_sandbox_lease.py:145">
P3: The new setup-error condition splits three ways, but only two of the paths have regression coverage. `test_keyboard_interrupt_during_setup_is_preserved` exercises the `not isinstance(error, Exception)` branch (preserve control flow), and `test_setup_timeout_is_typed_and_closes_the_owner` exercises the `StagehandSandboxLeaseError` preservation path (queue.Empty is wrapped into a typed error already inside the inner try). Nothing covers the remaining `else` branch, which is the exact new behavior that sanitizes an unexpected setup `Exception` into the fixed `"Stagehand sandbox lease failed during setup"` message. That branch is reachable (for example a ValueError escaping `parse_connection`/`urlsplit` on a malformed-but-string URL), and it is the primary server-side change in this delta, so a focused test would protect it from regression. Consider adding a test that injects a plain `ValueError` via `Queue.get` and asserts the re-raised `StagehandSandboxLeaseError` equals `"Stagehand sandbox lease failed during setup"` and that the owner's stdin was closed before the error.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| await waitForFile(readyPath); | ||
| child.kill("SIGTERM"); | ||
| const exit = await waitForExit(child, "Lease did not preserve the setup signal"); | ||
| assert.deepEqual(exit, { code: 143, signal: null }); |
There was a problem hiding this comment.
P2: The assertion { code: 143, signal: null } does not match the lease's signal-forwarding semantics. lease.ts propagates SIGTERM via process.kill(process.pid, 'SIGTERM') after removing listeners, so the child dies by signal and the parent close event reports { code: null, signal: 'SIGTERM' } (128+signal=143 is a shell convention, not what Node's child_process reports). As written, this test will fail on the signal path it is meant to validate. Assert { code: null, signal: 'SIGTERM' } instead to encode actual signal-style exit semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/examples/vercel-sandbox/src/lease.test.mjs, line 116:
<comment>The assertion `{ code: 143, signal: null }` does not match the lease's signal-forwarding semantics. lease.ts propagates SIGTERM via `process.kill(process.pid, 'SIGTERM')` after removing listeners, so the child dies *by signal* and the parent `close` event reports `{ code: null, signal: 'SIGTERM' }` (128+signal=143 is a shell convention, not what Node's child_process reports). As written, this test will fail on the signal path it is meant to validate. Assert `{ code: null, signal: 'SIGTERM' }` instead to encode actual signal-style exit semantics.</comment>
<file context>
@@ -45,3 +48,97 @@ test("lease setup failure exits while the parent keeps stdin open", async () =>
+ await waitForFile(readyPath);
+ child.kill("SIGTERM");
+ const exit = await waitForExit(child, "Lease did not preserve the setup signal");
+ assert.deepEqual(exit, { code: 143, signal: null });
+ } finally {
+ if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL");
</file context>
| assert.deepEqual(exit, { code: 143, signal: null }); | |
| assert.deepEqual(exit, { code: null, signal: "SIGTERM" }); |
There was a problem hiding this comment.
I verified this against the actual launcher rather than changing the assertion: direct node self-SIGTERM reports {code:null, signal:"SIGTERM"}, but the production path uses the tsx executable, which translates the forwarded signal to {code:143, signal:null}. Reproduction with node_modules/.bin/tsx -e 'process.kill(...)' yields 143, and the black-box lease test passes that real path. 73911f32 adds a comment documenting the tsx signal-proxy convention so it is not confused with direct child_process semantics.
| except BaseException as error: # noqa: BLE001 -- cleanup must preserve any primary failure. | ||
| primary_error = ( | ||
| error | ||
| if not isinstance(error, Exception) |
There was a problem hiding this comment.
P3: The new setup-error condition splits three ways, but only two of the paths have regression coverage. test_keyboard_interrupt_during_setup_is_preserved exercises the not isinstance(error, Exception) branch (preserve control flow), and test_setup_timeout_is_typed_and_closes_the_owner exercises the StagehandSandboxLeaseError preservation path (queue.Empty is wrapped into a typed error already inside the inner try). Nothing covers the remaining else branch, which is the exact new behavior that sanitizes an unexpected setup Exception into the fixed "Stagehand sandbox lease failed during setup" message. That branch is reachable (for example a ValueError escaping parse_connection/urlsplit on a malformed-but-string URL), and it is the primary server-side change in this delta, so a focused test would protect it from regression. Consider adding a test that injects a plain ValueError via Queue.get and asserts the re-raised StagehandSandboxLeaseError equals "Stagehand sandbox lease failed during setup" and that the owner's stdin was closed before the error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/integrations/examples/shared/vercel_sandbox_lease.py, line 145:
<comment>The new setup-error condition splits three ways, but only two of the paths have regression coverage. `test_keyboard_interrupt_during_setup_is_preserved` exercises the `not isinstance(error, Exception)` branch (preserve control flow), and `test_setup_timeout_is_typed_and_closes_the_owner` exercises the `StagehandSandboxLeaseError` preservation path (queue.Empty is wrapped into a typed error already inside the inner try). Nothing covers the remaining `else` branch, which is the exact new behavior that sanitizes an unexpected setup `Exception` into the fixed `"Stagehand sandbox lease failed during setup"` message. That branch is reachable (for example a ValueError escaping `parse_connection`/`urlsplit` on a malformed-but-string URL), and it is the primary server-side change in this delta, so a focused test would protect it from regression. Consider adding a test that injects a plain `ValueError` via `Queue.get` and asserts the re-raised `StagehandSandboxLeaseError` equals `"Stagehand sandbox lease failed during setup"` and that the owner's stdin was closed before the error.</comment>
<file context>
@@ -142,7 +142,8 @@ def start(self) -> StagehandSandboxConnection:
primary_error = (
error
- if isinstance(error, StagehandSandboxLeaseError)
+ if not isinstance(error, Exception)
+ or isinstance(error, StagehandSandboxLeaseError)
else self._lease_error("failed during setup")
</file context>
There was a problem hiding this comment.
Added in 73911f32. A plain sentinel-bearing ValueError injected from Queue.get now asserts the exact fixed Stagehand sandbox lease failed during setup error and proves stdin was closed before propagation. Shared lease tests pass 9/9.
1 participant
Why
Stagehand code mode executes model-generated JavaScript. This integration keeps that execution behind the package-installed Vercel Sandbox boundary from #2626 while using CrewAI's native remote MCP client.
Stack
@browserbasehq/stagehand-codemodepackage and CLIWhat changed
MCPServerAdapterover authenticated Streamable HTTP{ url, token }connectioncode_executedescription instead of copying the schema, executor, or skill1.15.13Proof matrix
from sandbox import StagehandSandboxLeaseusage resolves without exposing repository internals.pnpm checkopenai/gpt-5-minimodel-selected call, independent DOM verification, credential isolation, and ordered cleanup before emittingPASS.The live job intentionally fails closed when repository Browserbase, Vercel, or OpenAI credentials are absent. Repository Vercel credentials are not currently configured for this workflow, so no live pass is claimed yet.
Changeset
None. This adds an integration example and CI proof surface without changing a published API.