add _experimental_batch - #2608
Conversation
|
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 37 files
Confidence score: 3/5
- In
packages/server/tests/callback-batch.test.ts, the test expects camelCasepageIdeven thoughInProcessCommandClient.send/encodeWireValueforwards snake_case, so callback-batch contract checks can fail or validate the wrong shape — align the assertion with the wire-format path (or normalize before asserting). - In
packages/sdk-python/tests/test_cdp_client.py,run_callback_batchhas untested branches (valueIsUndefined→None,ok: false, and other post-eval outcomes), which risks silent behavior regressions for Python callers when responses vary — add branch-focused tests for each response form. - In
packages/sdk-ts/tests/object-wrapper.test.ts, the added batch test only verifies transport plumbing and never executes the callback body, so callback execution semantics could break without detection — add cases that run the callback and assert return/error propagation. - In
packages/sdk-ts/src/fileUpload.ts, the browser fallback base64 path does per-byte string concatenation, which can become very slow on large uploads and increase timeout risk in workers — switch to chunked conversion to keep output identical with lower runtime overhead.
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/sdk-ts/tests/object-wrapper.test.ts">
<violation number="1" location="packages/sdk-ts/tests/object-wrapper.test.ts:119">
P3: The new batch test only exercises transport plumbing (callbackSource stringification, input/timeout pass-through) because the fake runCallbackBatch never executes the callback. Consider adding focused cases that run the callback logic, assert pageId is forwarded when options.page is set, and cover the timeout/abort path and explicit-undefined envelope the PR calls out, so these behaviors are encoded rather than relying on manual verification.</violation>
</file>
<file name="packages/server/tests/callback-batch.test.ts">
<violation number="1" location="packages/server/tests/callback-batch.test.ts:42">
P2: This assertion expects camelCase `pageId`, but the params that reach the router come from `InProcessCommandClient.send`, which wire-encodes them to snake_case via `encodeWireValue`, producing `{ page_id: "page-1", selector: "button" }`. The test would therefore fail; assert against the snake_case key instead.</violation>
</file>
<file name="packages/sdk-ts/src/fileUpload.ts">
<violation number="1" location="packages/sdk-ts/src/fileUpload.ts:106">
P3: The browser fallback base64 encoder does per-byte string concatenation, which is very slow for large uploads and can increase timeout risk in worker execution. Chunked conversion keeps the same output with much lower overhead.</violation>
</file>
<file name="packages/sdk-python/tests/test_cdp_client.py">
<violation number="1" location="packages/sdk-python/tests/test_cdp_client.py:208">
P2: The new `run_callback_batch` behavior in `cdp_client.py` adds several distinct post-evaluation branches that the single added test does not exercise: the `valueIsUndefined: true` → `None` return path, the `ok: false` error envelope → `RuntimeError` reconstruction path, and the `exceptionDetails` → `RuntimeError` path. Likewise, the new `Stagehand._experimental_batch` validation (rejecting non-string/empty source, non-int/positive timeout, non-JSON-serializable input) and the `RPCClient.run_callback_batch` fallback guard for transports without a runner have no unit coverage. Adding focused tests for these branches (undefined envelope handling and error-envelope round-trip, plus at least one `_experimental_batch` validation case) would protect the serialization/error-contract logic from regressions.</violation>
</file>
Architecture diagram
sequenceDiagram
participant SDK as SDK Client (TS/Py/Go)
participant CDP as CDP Session (Worker)
participant SW as Service Worker
participant Router as RPCRouter
participant InProc as InProcessCommandClient
participant Page as Page / Locator Wrappers
participant Browser as Browser Runtime
Note over SDK,Browser: NEW: Experimental Batch Flow
SDK->>CDP: Runtime.evaluate (callback batch expression)
Note over SDK,CDP: Wraps callback source, serialized input, options with pageId & timeout
CDP->>SW: globalThis.__stagehandRunCallbackBatch(callback, input, options)
alt Another batch already active
SW-->>CDP: { ok: false, error: "Another batch running" }
CDP-->>SDK: Propagate error
else Valid batch
SW->>SW: Mark active, create AbortController, set timeout
SW->>InProc: new InProcessCommandClient(router, signal)
SW->>Router: context.pages (or context.active_page)
Router->>InProc: Return page list / active page
InProc-->>SW: Page reference
Note over SW,Page: Callback executes with worker-local objects
SW->>Page: callback({ page, context, act, observe, extract, metrics }, input)
Page->>InProc: locator.click({ selector: "button" })
InProc->>Router: handle({ method: "locator.click", params })
Router->>Browser: Execute CDP click command
Browser-->>Router: Result
Router-->>InProc: { clicked: true }
InProc-->>Page: Return result
Page->>InProc: page.title()
InProc->>Router: handle({ method: "page.title", params })
Router->>Browser: Execute CDP title command
Browser-->>Router: "Example Page"
Router-->>InProc: "Example Page"
InProc-->>Page: Return title
Page-->>SW: { title: "Example Page" }
SW->>SW: JSON round-trip result
alt Result is undefined
SW-->>CDP: { ok: true, valueIsUndefined: true }
else JSON-serializable result
SW-->>CDP: { ok: true, value: { title: "Example Page" } }
end
CDP-->>SDK: Decode envelope, return result
end
Note over SDK,SW: Key Constraints<br/>- context.close() excluded from callback scope<br/>- No recursive batch calls<br/>- Overall timeout via AbortController + cooperative check<br/>- CDP uses awaitPromise: true, returnByValue: true
Note over SDK,CDP: Host-only features excluded in worker:<br/>- screenshot path (no Node fs)<br/>- file upload local paths<br/>- Buffer → Uint8Array conversion
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 40 files
Confidence score: 3/5
- In
packages/sdk-ts/src/stagehand.ts, malformedoptions.page/pageIdcan silently fall back to the active page, which risks executing a callback batch against the wrong page instead of the caller’s target — add upfront validation and fail fast before payload construction. - In
packages/sdk-ts/src/stagehand.ts, method-reference callbacks may serialize into non-expression source and only fail later with a generic evaluation syntax error, making failures harder to diagnose and recover from — run a local parse/shape check and throw a clearTypeErrorbefore dispatch. - In
packages/sdk-ts/tests/object-wrapper.test.ts, the mockednode:fs/promisespath may not reliably intercept the runtime-concatenated dynamic import used bysetInputFiles, so the fallback-path test can pass or fail for tooling reasons rather than behavior — adjust the test to hook the actual import path or refactor loading to a mockable seam. - In
packages/server/tests/callback-batch.test.ts, failure and timeout runner paths are currently untested, leaving regression risk in the exact error/timeout handling called out by the test plan — add explicit timeout and runner-failure cases to de-risk callback batch robustness.
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/server/tests/callback-batch.test.ts">
<violation number="1" location="packages/server/tests/callback-batch.test.ts:84">
P3: This new test file covers the happy-path, undefined-envelope, extract-normalization, and callback-surface cases, but the runner's failure and timeout paths are untested. The PR test plan explicitly calls for validating worker error reconstruction, the timeout abort, and the one-batch-at-a-time guard, yet none of those branches in callbackBatch.ts (`{ ok: false, error }` envelope, `failure()`, the race between callbackPromise and the abort signal) are exercised here. Consider adding a couple of focused tests that make the in-process router throw and that simulate an abort/timeout, asserting the returned `ok:false` envelope.</violation>
</file>
<file name="packages/sdk-ts/src/stagehand.ts">
<violation number="1" location="packages/sdk-ts/src/stagehand.ts:141">
P2: Method-reference callbacks can fail late with a generic evaluation syntax error because their serialized source is not a valid expression. A local parse check before dispatch would surface a clear TypeError and avoid remote execution attempts.</violation>
<violation number="2" location="packages/sdk-ts/src/stagehand.ts:157">
P2: Malformed `options.page` can silently route the batch to the active page instead of failing fast. Validating `options.page` (or `pageId`) before building the payload would prevent wrong-page execution when caller input is invalid.</violation>
</file>
<file name="packages/sdk-ts/tests/object-wrapper.test.ts">
<violation number="1" location="packages/sdk-ts/tests/object-wrapper.test.ts:1487">
P3: This test forces the non-Node file-path fallback by mocking `node:fs/promises`, but `setInputFiles` loads that module through a runtime-concatenated `import(/* @vite-ignore */ ...)` which bypasses Vite's static import analysis. Whether `vi.doMock` intercepts that native dynamic import of a builtin is loader-dependent; if it doesn't, the test would actually open `example.txt` on disk and fail with "could not read file" instead of exercising the intended guard. Consider verifying the mock intercepts (e.g., also asserting no real fs access) or structuring the guard so the non-Node path is testable without relying on mocking a builtin used via `@vite-ignore`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant SDK as SDK Client (TS/Python/Go)
participant CDP as CDP Client / WebSocket
participant SW as Service Worker
participant CB as callbackBatch Runner
participant RPCR as RPCRouter (In-Browser)
participant Page as Page/Locator Wrapper
participant Context as BrowserContext
Note over SDK,Context: NEW: Experimental Batch Flow (avoiding SDK↔Server RTT per operation)
SDK->>CDP: sendCommand("Runtime.evaluate", { expression, awaitPromise, returnByValue })
Note over SDK,CDP: Expression wraps callback source + serialized input + options
CDP->>SW: Runtime.evaluate CDP command (target service worker session)
alt SW supports batches
SW->>SW: Check globalThis.__stagehandRunCallbackBatch exists
SW->>CB: invoke __stagehandRunCallbackBatch(callback, input, options)
CB->>CB: Check no other batch active (one-batch-at-a-time guard)
CB->>CB: Create InProcessCommandClient backed by RPCRouter
CB->>Context: Resolve selected page (by pageId or activePage())
Context->>RPCR: context.pages / context.active_page
RPCR-->>Context: Page list / active page ref
Context-->>CB: Page object
CB->>CB: Build callback context { page, context, act, observe, extract, metrics }
Note over CB: context.close() is intentionally excluded
CB->>CB: Invoke callback(stagehand, input)
loop Inside callback
Page->>CB: page.goto(url)
CB->>RPCR: stagehand.act / stagehand.observe / stagehand.extract
RPCR-->>CB: Result
Page->>CB: page.locator("h1").innerText()
CB->>RPCR: locator.innerText
RPCR-->>CB: Text value
end
alt Result is undefined
CB-->>SW: { ok: true, valueIsUndefined: true }
else Result is JSON-serializable
CB->>CB: JSON round-trip validation
CB-->>SW: { ok: true, value: <JSON> }
end
else SW incompatible
SW-->>CDP: { ok: false, error: "StagehandRuntimeIncompatibleError" }
end
SW-->>CDP: Runtime.evaluate result (envelope JSON)
CDP->>CDP: Parse CallbackBatchEnvelopeSchema
alt OK envelope
alt valueIsUndefined
CDP-->>SDK: undefined/null
else has value
CDP-->>SDK: Decoded result
end
else Error envelope
CDP-->>SDK: Reconstructed Error with name & message
end
Note over SDK,SW: Timeout handling: cooperative + CDP evaluation grace period
opt Timeout expires
CB->>CB: Abort controller triggers
CB-->>SW: { ok: false, error: "Stagehand callback batch timed out" }
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!Number.isFinite(timeout) || timeout <= 0) { | ||
| throw new RangeError("stagehand._experimental_batch() timeout must be greater than zero"); | ||
| } | ||
| const callbackSource = Function.prototype.toString.call(callback); |
There was a problem hiding this comment.
P2: Method-reference callbacks can fail late with a generic evaluation syntax error because their serialized source is not a valid expression. A local parse check before dispatch would surface a clear TypeError and avoid remote execution attempts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk-ts/src/stagehand.ts, line 141:
<comment>Method-reference callbacks can fail late with a generic evaluation syntax error because their serialized source is not a valid expression. A local parse check before dispatch would surface a clear TypeError and avoid remote execution attempts.</comment>
<file context>
@@ -110,6 +114,55 @@ export class Stagehand {
+ if (!Number.isFinite(timeout) || timeout <= 0) {
+ throw new RangeError("stagehand._experimental_batch() timeout must be greater than zero");
+ }
+ const callbackSource = Function.prototype.toString.call(callback);
+ if (nativeFunctionSourcePattern.test(callbackSource)) {
+ throw new TypeError(
</file context>
| return (await this.connectedRpcClient.runCallbackBatch({ | ||
| callbackSource, | ||
| input, | ||
| ...(options.page ? { pageId: options.page.pageId } : {}), |
There was a problem hiding this comment.
P2: Malformed options.page can silently route the batch to the active page instead of failing fast. Validating options.page (or pageId) before building the payload would prevent wrong-page execution when caller input is invalid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk-ts/src/stagehand.ts, line 157:
<comment>Malformed `options.page` can silently route the batch to the active page instead of failing fast. Validating `options.page` (or `pageId`) before building the payload would prevent wrong-page execution when caller input is invalid.</comment>
<file context>
@@ -110,6 +114,55 @@ export class Stagehand {
+ return (await this.connectedRpcClient.runCallbackBatch({
+ callbackSource,
+ input,
+ ...(options.page ? { pageId: options.page.pageId } : {}),
+ timeout,
+ signal: controller.signal,
</file context>
| installCallbackBatchRunner(scope, router); | ||
|
|
||
| await expect( | ||
| scope.__stagehandRunCallbackBatch?.(async () => undefined, null, { timeout: 1_000 }), |
There was a problem hiding this comment.
P3: This new test file covers the happy-path, undefined-envelope, extract-normalization, and callback-surface cases, but the runner's failure and timeout paths are untested. The PR test plan explicitly calls for validating worker error reconstruction, the timeout abort, and the one-batch-at-a-time guard, yet none of those branches in callbackBatch.ts ({ ok: false, error } envelope, failure(), the race between callbackPromise and the abort signal) are exercised here. Consider adding a couple of focused tests that make the in-process router throw and that simulate an abort/timeout, asserting the returned ok:false envelope.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/tests/callback-batch.test.ts, line 84:
<comment>This new test file covers the happy-path, undefined-envelope, extract-normalization, and callback-surface cases, but the runner's failure and timeout paths are untested. The PR test plan explicitly calls for validating worker error reconstruction, the timeout abort, and the one-batch-at-a-time guard, yet none of those branches in callbackBatch.ts (`{ ok: false, error }` envelope, `failure()`, the race between callbackPromise and the abort signal) are exercised here. Consider adding a couple of focused tests that make the in-process router throw and that simulate an abort/timeout, asserting the returned `ok:false` envelope.</comment>
<file context>
@@ -0,0 +1,247 @@
+ installCallbackBatchRunner(scope, router);
+
+ await expect(
+ scope.__stagehandRunCallbackBatch?.(async () => undefined, null, { timeout: 1_000 }),
+ ).resolves.toEqual({ ok: true, valueIsUndefined: true });
+ });
</file context>
| @@ -1354,6 +1483,44 @@ describe("Stagehand TS object wrapper", () => { | |||
| } | |||
| }); | |||
|
|
|||
| it("reports when file paths are unavailable outside Node.js", async () => { | |||
| vi.doMock("node:fs/promises", () => { | |||
There was a problem hiding this comment.
P3: This test forces the non-Node file-path fallback by mocking node:fs/promises, but setInputFiles loads that module through a runtime-concatenated import(/* @vite-ignore */ ...) which bypasses Vite's static import analysis. Whether vi.doMock intercepts that native dynamic import of a builtin is loader-dependent; if it doesn't, the test would actually open example.txt on disk and fail with "could not read file" instead of exercising the intended guard. Consider verifying the mock intercepts (e.g., also asserting no real fs access) or structuring the guard so the non-Node path is testable without relying on mocking a builtin used via @vite-ignore.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk-ts/tests/object-wrapper.test.ts, line 1487:
<comment>This test forces the non-Node file-path fallback by mocking `node:fs/promises`, but `setInputFiles` loads that module through a runtime-concatenated `import(/* @vite-ignore */ ...)` which bypasses Vite's static import analysis. Whether `vi.doMock` intercepts that native dynamic import of a builtin is loader-dependent; if it doesn't, the test would actually open `example.txt` on disk and fail with "could not read file" instead of exercising the intended guard. Consider verifying the mock intercepts (e.g., also asserting no real fs access) or structuring the guard so the non-Node path is testable without relying on mocking a builtin used via `@vite-ignore`.</comment>
<file context>
@@ -1354,6 +1483,44 @@ describe("Stagehand TS object wrapper", () => {
});
+ it("reports when file paths are unavailable outside Node.js", async () => {
+ vi.doMock("node:fs/promises", () => {
+ throw new Error("module resolution failed");
+ });
</file context>
There was a problem hiding this comment.
2 issues found across 40 files (changes from recent commits).
Confidence score: 4/5
- In
packages/sdk-go/rpc_client.go, the newstagehand.callback_batchtimeout routing isn’t covered by the timeout policy tests, so a regression could quietly revert to the default 10s deadline and cause unexpected request behavior for Go SDK users — add a focused timeout-policy test for this method path. - In
packages/sdk-ts/src/rpcClient.ts, the_experimental_batch/StagehandMethods.stagehandCallbackBatchtimeout branch is also untested inrpcClient.test.ts, which leaves deadline derivation changes in TypeScript undetected until runtime — extend the timeout matrix tests to include this method.
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/sdk-go/rpc_client.go">
<violation number="1" location="packages/sdk-go/rpc_client.go:249">
P3: `stagehand.callback_batch` now has custom timeout routing, but the timeout policy test does not verify this new method, so regressions here could silently fall back to the default 10s RPC deadline. Adding a focused `rpcResponseTimeout("stagehand.callback_batch", ...)` test (including a large-timeout clamp case) would make this behavior durable.
(Based on your team's feedback about adding unit tests for new behavior.) .</violation>
</file>
<file name="packages/sdk-ts/src/rpcClient.ts">
<violation number="1" location="packages/sdk-ts/src/rpcClient.ts:485">
P3: The new callback-batch timeout branch is untested in `rpcClient.test.ts`, so deadline derivation regressions for `_experimental_batch` can ship unnoticed. Consider adding `StagehandMethods.stagehandCallbackBatch` to the existing JSON-RPC deadline table test.
(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
| case "stagehand.act", | ||
| "stagehand.extract", | ||
| "stagehand.observe", | ||
| "stagehand.callback_batch", |
There was a problem hiding this comment.
P3: stagehand.callback_batch now has custom timeout routing, but the timeout policy test does not verify this new method, so regressions here could silently fall back to the default 10s RPC deadline. Adding a focused rpcResponseTimeout("stagehand.callback_batch", ...) test (including a large-timeout clamp case) would make this behavior durable.
(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/sdk-go/rpc_client.go, line 249:
<comment>`stagehand.callback_batch` now has custom timeout routing, but the timeout policy test does not verify this new method, so regressions here could silently fall back to the default 10s RPC deadline. Adding a focused `rpcResponseTimeout("stagehand.callback_batch", ...)` test (including a large-timeout clamp case) would make this behavior durable.
(Based on your team's feedback about adding unit tests for new behavior.) .</comment>
<file context>
@@ -263,6 +246,7 @@ func rpcResponseTimeout(method string, params json.RawMessage) (time.Duration, b
case "stagehand.act",
"stagehand.extract",
"stagehand.observe",
+ "stagehand.callback_batch",
"page.goto",
"page.reload",
</file context>
| case StagehandMethods.stagehandAct.name: | ||
| case StagehandMethods.stagehandExtract.name: | ||
| case StagehandMethods.stagehandObserve.name: | ||
| case StagehandMethods.stagehandCallbackBatch.name: |
There was a problem hiding this comment.
P3: The new callback-batch timeout branch is untested in rpcClient.test.ts, so deadline derivation regressions for _experimental_batch can ship unnoticed. Consider adding StagehandMethods.stagehandCallbackBatch to the existing JSON-RPC deadline table test.
(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/sdk-ts/src/rpcClient.ts, line 485:
<comment>The new callback-batch timeout branch is untested in `rpcClient.test.ts`, so deadline derivation regressions for `_experimental_batch` can ship unnoticed. Consider adding `StagehandMethods.stagehandCallbackBatch` to the existing JSON-RPC deadline table test.
(Based on your team's feedback about adding unit tests for new behavior.) .</comment>
<file context>
@@ -503,6 +482,7 @@ function rpcResponseTimeoutMs(method: string, params: unknown): number | undefin
case StagehandMethods.stagehandAct.name:
case StagehandMethods.stagehandExtract.name:
case StagehandMethods.stagehandObserve.name:
+ case StagehandMethods.stagehandCallbackBatch.name:
case StagehandMethods.pageGoto.name:
case StagehandMethods.pageReload.name:
</file context>
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Confidence score: 5/5
- In
packages/sdk-go/cdp_client_test.go,SkipsNonBatchMessagescurrently validates only the early prefilter path, so regressions in the JSON-decoding or method-dispatch handling for non-batch messages could slip through without test coverage; add a case that passes the marker check and still verifies the non-batch skip behavior after decode/dispatch logic is reached.
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/sdk-go/cdp_client_test.go">
<violation number="1" location="packages/sdk-go/cdp_client_test.go:577">
P3: The test named “SkipsNonBatchMessages” only exercises the prefilter short-circuit: `not-json` doesn’t contain the `stagehand.callback_batch` marker, so it returns before JSON decoding and the method dispatch. It would still pass if the method-check or error branches regressed, and it doesn’t show the intended case (a valid non-batch JSON message returning `ok=false`). Consider adding a case with a well-formed non-batch message, e.g. `{"method":"page.title"}`, to actually cover the “skip” path.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| func TestCallbackSourceFromMessageSkipsNonBatchMessages(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| source, ok, err := callbackSourceFromMessage(json.RawMessage(`not-json`)) |
There was a problem hiding this comment.
P3: The test named “SkipsNonBatchMessages” only exercises the prefilter short-circuit: not-json doesn’t contain the stagehand.callback_batch marker, so it returns before JSON decoding and the method dispatch. It would still pass if the method-check or error branches regressed, and it doesn’t show the intended case (a valid non-batch JSON message returning ok=false). Consider adding a case with a well-formed non-batch message, e.g. {"method":"page.title"}, to actually cover the “skip” path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk-go/cdp_client_test.go, line 577:
<comment>The test named “SkipsNonBatchMessages” only exercises the prefilter short-circuit: `not-json` doesn’t contain the `stagehand.callback_batch` marker, so it returns before JSON decoding and the method dispatch. It would still pass if the method-check or error branches regressed, and it doesn’t show the intended case (a valid non-batch JSON message returning `ok=false`). Consider adding a case with a well-formed non-batch message, e.g. `{"method":"page.title"}`, to actually cover the “skip” path.</comment>
<file context>
@@ -571,6 +571,15 @@ func TestCDPClientDeliversCallbackBatchWithRuntimeAttachment(t *testing.T) {
+func TestCallbackSourceFromMessageSkipsNonBatchMessages(t *testing.T) {
+ t.Parallel()
+
+ source, ok, err := callbackSourceFromMessage(json.RawMessage(`not-json`))
+ if err != nil || ok || source != "" {
+ t.Fatalf("callbackSourceFromMessage() = %q, %t, %v; want empty, false, nil", source, ok, err)
</file context>
…batch # Conflicts: # packages/extension/callbackBatch.ts # packages/extension/rpcRouter.ts # packages/extension/tests/callback-batch.test.ts # packages/extension/tests/stagehand-clients.test.ts # packages/protocol/stagehand.v4.json # packages/sdk-go/internal/extensionassets/stagehand-extension.zip # packages/sdk-go/models.gen.go # packages/sdk-python/src/stagehand/_generated/input_types.py # packages/sdk-python/src/stagehand/_generated/models.py # packages/sdk-ts/src/page.ts # rules/ast-grep/example-parity.test.ts
miguelg719
left a comment
miguelg719
left a comment
There was a problem hiding this comment.
Two blocking items before merge (correctness, not security — the architecture is sound). Remaining should-fix / nits to follow.
| callback_source=source, | ||
| input=_models.FieldSchema2.model_validate(input), | ||
| options=CallbackBatchOptions( | ||
| page_id=page.page_id if page is not None else None, |
There was a problem hiding this comment.
Blocking — Python default path is broken.
page_id=page.page_id if page is not None else None always sets the field, so model_dump(exclude_unset=True, by_alias=True) keeps it and the wire carries "pageId": null. The worker schema is pageId: z.string().min(1).optional() — optional but not nullable — so it rejects null. Every Python _experimental_batch() call without page= fails with invalid-params before the callback runs, including examples/batch.py.
options=CallbackBatchOptions(
**({"page_id": page.page_id} if page is not None else {}),
timeout=timeout,
),Matches the TS conditional spread and Go's *string + omitempty.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_experimental_batch_uses_registered_rpc_method( |
There was a problem hiding this comment.
Blocking — no wire-conformance coverage.
These tests assert against a recording fake, so nothing ever runs real serialized callback_batch params through the zod schemas the extension enforces — which is exactly why the pageId: null bug above slipped through. Please add a round-trip test (real RPCClient serialization → protocol schema parse) covering both page-omitted and page-provided cases, in Python and Go (shared golden wire fixtures would be ideal).
miguelg719
left a comment
miguelg719
left a comment
There was a problem hiding this comment.
rest of the review — should-fix + nits. none of these block on their own, but the page-pinning and extract heuristic ones are easy footguns.
| extract: async (...args) => { | ||
| const [instruction, schemaOrOptions, explicitOptions] = args; | ||
| const optionsOnly = | ||
| args.length < 3 && |
There was a problem hiding this comment.
extract() only treats arg2 as options if it strict-parses. a typo'd options obj ({ selctor, timeout }) fails the parse and gets shipped as the extraction schema instead — runs unscoped, returns garbage, no error (main sdk throws). reverse edge: a permissive {} schema parses as options and gets dropped. throw when keys intersect the options schema but don't fully validate.
| operationOptions ?? {}, | ||
| ); | ||
| return await client.send(StagehandMethods.stagehandAct, { | ||
| pageId: (operationPage ?? page).pageId, |
There was a problem hiding this comment.
(operationPage ?? page).pageId pins to the page resolved at batch start, but the outer sdk resolves activePage() per call. newPage + setActivePage + act in a callback silently drives the original page. resolve per-op or document the pinning.
| throw new RangeError("stagehand._experimental_batch() timeout must be greater than zero"); | ||
| } | ||
| const callbackSource = Function.prototype.toString.call(callback); | ||
| if (nativeFunctionSourcePattern.test(callbackSource)) { |
There was a problem hiding this comment.
nativeFunctionSourcePattern only catches native stubs. a valid shorthand method ({ async run(){} }).run serializes to async run(){...} → interpolates as (async run(){...}) → opaque v8 SyntaxError with no mention of _experimental_batch. validate with new Function(\return (${source})`)` and throw the clean TypeError. also worth a comment that this is dx-only, not a security guard — and it's missing from py/go.
| CallbackBatchResult, | ||
| ) | ||
| return ( | ||
| result.value.model_dump(mode="json") |
There was a problem hiding this comment.
value validates through FieldSchema3 (no StrictInt), so every int in the result comes back as float — {count: 7} → 7.0 — and inputs lose precision above 2^53. ts/go stay exact. use raw json pass-through (like go's json.RawMessage) or add StrictInt to the unions.
| ]); | ||
| if (result === undefined) return {}; | ||
| return { value: jsonRoundTrip(result) }; | ||
| } finally { |
There was a problem hiding this comment.
finally only clears the timeout, never aborts the controller after the callback settles. the InProcessCommandClient stays live and any un-awaited op the callback kicked off keeps running in the worker after the result returned — mutating page state under the user's next commands. controller.abort(...) in finally.
| } | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => { |
There was a problem hiding this comment.
timeout allows up to MAX_SAFE_INTEGER but goes straight to setTimeout, whose delay is signed 32-bit in chromium — anything > 2^31-1 ms wraps and fires immediately, aborting instantly instead of ~never. add .max(2**31-1) to the schema, mirror in py/go.
| metrics(): Promise<StagehandMetrics>; | ||
| }; | ||
|
|
||
| export function createCallbackBatchController(router: RPCRouter) { |
There was a problem hiding this comment.
the description + test plan claim one-batch-at-a-time protection, but nothing here / in rpcRouter / in any sdk serializes batches, and callback-batch.test.ts has a test asserting concurrent batches run. no new hazard vs concurrent act(), but the claim and the code disagree — implement an activeBatch gate or fix the description.
| } | ||
| const bytes = | ||
| typeof file.buffer === "string" | ||
| ? new TextEncoder().encode(file.buffer) |
There was a problem hiding this comment.
the 50 MiB pre-check for string buffers is gated on globalThis.Buffer, which doesn't exist in the worker. off-node, new TextEncoder().encode(...) fully allocates before the RangeError. compute utf-8 length without the full allocation for that path.
| ).resolves.toEqual({}); | ||
| }); | ||
|
|
||
| it("enforces the callback timeout in the worker", async () => { |
There was a problem hiding this comment.
the timeout test uses a never-settling callback and only asserts the race rejection, so throwIfAborted — the only thing stopping a timed-out callback from issuing more act/extract after failure — is never exercised; deleting it still passes. add a gated-callback test asserting a post-timeout send rejects and never reaches router.handle.
| "stagehand.callback_batch", | ||
| CallbackBatchParams( | ||
| callback_source=source, | ||
| input=_models.FieldSchema2.model_validate(input), |
There was a problem hiding this comment.
two small parity nits here: (1) non-string dict keys pass the json.dumps guard but then raise a raw pydantic ValidationError instead of the documented TypeError — model_validate_json(json.dumps(input)) coerces keys consistently. (2) a no-input callback sees input === undefined in ts (field omitted) but null in py/go. omit the field when unset or document the diff.
2 participants
why
client-server round trips can be expensive on remote browsers. this PR adds an experimental batch API that runs a callback inside the Stagehand extension service worker. commands issued by that callback route directly to the existing worker runtime, avoiding the client-server round trip between each operation
what changed
stagehand._experimental_batch()to the TypeScript and Python SDKs andStagehand.ExperimentalBatch()to GoglobalThis.__stagehandRunCallbackBatchin the service worker, which selects the active or explicitly requested page, constructs the callback context, applies the overall timeout, invokes the callback, and returns a JSON result/error envelopeStagehandCommandClientinterface and moved the publicPage,Locator,BrowserContext,Response, clipboard, and WebMCP wrappers onto itRPCRouterpage,context,act,observe,extract, andmetricsto callbacks. the callback context intentionally excludescontext.close()and does not expose Stagehand lifecycle or recursive batch operationsundefined, and reconstructed worker errors in the calling SDKpathcannot write to the caller's filesystem;Uint8Arrayrather than a NodeBufferExperimentalBatchas the equivalent of_experimental_batchin TypeScript and PythonTypeScript:
Python:
Go:
test plan
PageandLocatoroperations through the in-browser RPC routercontext.close()is absent from the callback surfaceundefinedresult envelopeawaitPromise/returnByValue.Summary by cubic
Adds an experimental batch API that runs a trusted JavaScript callback inside the Stagehand service worker for faster multi-step flows with direct access to
page/contextand built‑inact/observe/extract. Batches run via JSON‑RPCstagehand.callback_batchwith a runtime‑attachedcallback; screenshots now returnUint8Array.New Features
_experimental_batch(TS/Python) andExperimentalBatch(Go) expose{ page, context, act, observe, extract, metrics }, support optionalpagescoping andtimeout, return JSON‑serializable results, preserveundefined, and keep callback names via a bundled__namehelper.stagehand.callback_batchwith opaqueinput/value; CDP/transport attaches the executablecallbacktoRuntime.evaluatewhile using the normal pending RPC path; the service worker receives runtime attachments, enforces one active batch, blocks internal Stagehand/context APIs, normalizes extract, routes per‑operation to the correct page, propagates cancellation/timeouts, and forwards W3C trace context.StagehandCommandClientand moved public wrappers (Page,Locator,BrowserContext,Response, clipboard,WebMCP) onto it for in‑worker execution._experimental_batchto v4 reference and clarifiedpage.screenshot()returnsUint8Array.Bug Fixes
page.screenshot()returnsUint8Array; canonical base64 decoder with strict validation; disallowpathin batches; avoid double‑encoding file payloads; throwRangeErrorfor uploads >50MB; Node‑only FS access is dynamically imported with clear errors (including init scripts).pageIDs and null options; safely coerce batch timeouts across SDKs and includestagehand.callback_batchin timeout policy; prefilter input before JSON unmarshal in Go; fail fast on invalid worker responses; Python omitspage_idwhen no page is provided and checks for a closed RPC client before sending; correctedStagehandMetricstyping.Written for commit f176737. Summary will update on new commits.