test: port v4 integration coverage - #2551
Merged
miguelg719 merged 7 commits intoAug 4, 2026
Merged
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.
16 issues found across 29 files
Confidence score: 3/5
- In
packages/sdk-ts/tests/integration/locator-content-methods.test.ts, theinputValue()expectation for a plain<div>appears misaligned withlocator.inputValueforwarding to Playwright, so the test may codify the wrong contract and hide a real behavior mismatch—update the assertion to match intended SDK semantics (including error behavior if applicable). - In
packages/server/understudy/a11y/snapshot/capture.ts, sanitization is applied intryScopedSnapshotbut not consistently in the merge path, which can leak malformed outline data into merged accessibility snapshots—apply the same well-formed sanitization in both code paths. - Several integration tests (
wait-for-timeout,clipboard, and popup URL checks indefault-page-tracking) rely on tight timing, live network, or navigation-completion assumptions, increasing flaky CI risk and reducing signal on regressions—relax timing assertions, make fixtures hermetic, and wait for explicit load states before asserting URLs. - In
.github/workflows/ci.yml, unapproved fork PRs can trigger broad integration fan-out while changes toscripts/test-utils.tscan bypass relevant coverage, creating both CI resource-exposure and blind-spot risk—gate discovery for external PRs and includescripts/test-utils.tsin integration-trigger 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/sdk-ts/tests/integration/keyboard.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/keyboard.test.ts:320">
P3: The "modifier state clears on keyPress error" test swallows any rejection from keyPress("Cmd+InvalidKey123") without asserting that an error actually occurred, so the scenario it is named for is never validated — if the SDK doesn't throw here, the test passes trivially and would not catch a regression in modifier cleanup. Consider asserting the error (expect(keyPress(...)).rejects.toThrow()) before checking that subsequent typing succeeds.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:20">
P3: The 7 specs each inline a near-identical drag-and-drop fixture (draggable source, drop-zone, and the same dragstart/drop/dragover listeners), differing only by element ids/names and the drop-result element. Extracting a small shared fixture helper (e.g. a function that builds the HTML given source/target/result ids and optional initial content) would cut roughly 250 lines of duplicated boilerplate and make the behavioral differences between the cases much easier to see. This is a maintainability suggestion; the duplicated HTML risks drift when the common fixture changes.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-content-methods.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-content-methods.test.ts:234">
P1: The "inputValue() returns empty string for non-input elements" test calls inputValue() on a plain <div> and expects "". The SDK's locator.inputValue routes to the Playwright locator.inputValue (runtime.ts:667), which throws 'Node is not an <input>, <textarea> or [contenteditable] element' for non-input nodes rather than returning an empty string, so this test will fail at runtime. Adjust the expectation to assert the thrown error (or drop the div case) so the suite reflects actual behavior.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/default-page-tracking.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/default-page-tracking.test.ts:62">
P3: Each popup test asserts `page.url()` right after `waitForActivePage` returns. That helper only reports that the active page *changed* (target created), not that the popup finished navigating, so `page2.url()`/`page3.url()` can race and briefly return `about:blank`, making the suite flaky. Consider polling for the expected URL instead of reading it once, e.g. `await expect.poll(async () => (await page2.url())).toBe(new URL("/page2", fixture.url).href)`.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-scroll.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-scroll.test.ts:40">
P3: These integration specs poll with a fixed `setTimeout` (200ms / 100ms) to wait for scroll and drag/drop events to settle, and that wait helper is duplicated across seven scroll cases and five drag/drop cases. Because the PR fans these tests into 20 shards across 4 browser lanes, timing-based sleeps are the most likely source of flaky shards: on a loaded CI runner the wheel/drag events may not settle within the fixed window, and on a fast runner the sleep is pure waste. Consider extracting a small `sleep(ms)` helper (e.g. into `_support.ts`) and, where practical, waiting on the actual condition (`window.scrollY` changing or the drop/result element's `textContent`) instead of a hard-coded delay so the assertions are deterministic rather than clock-dependent.</violation>
<violation number="2" location="packages/sdk-ts/tests/integration/page-scroll.test.ts:114">
P3: This test's `#marker` element is never referenced and the assertion (`scrollY > 0`) duplicates the first vertical test, so it doesn't meaningfully validate scrolling at a specific coordinate. Either have the test actually verify the expected scroll offset relative to the marker, or drop the redundant case/dead markup to keep the suite honest and easier to maintain.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/timeouts.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/timeouts.test.ts:24">
P3: These tests can silently pass via the SDK's 10-second RPC command-timeout fallback instead of the intended 5ms model timeout, so a regression in the short-timeout plumbing would not fail the suite and would inflate each run to ~10s. Consider asserting the rejection arrives well under the 10s fallback (or add an explicit vitest timeout lower than commandTimeoutMs) so the short timeout path is actually covered.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/clipboard.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/clipboard.test.ts:6">
P2: These tests depend on a live third-party URL and a 15s network navigation even though the HTML is fully produced by the test itself (document.body is overwritten right after the goto). This is non-hermetic, inconsistent with the rest of the suite (which uses the local startFixtureServer), and fails whenever the eval site or CI network is unreachable. Consider serving the textarea/button markup from a local fixture server instead of the external page; localhost is a secure context, so the navigator.clipboard user-gesture case still works.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-selector.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-selector.test.ts:202">
P3: The timeout test asserts wall-clock bounds (450ms–2000ms) around a 500ms RPC timeout. The 2000ms ceiling is tight enough that a slow, shard-contended CI run can occasionally overshoot it and flake the suite. Consider asserting only the lower bound (that it did not return early) or widening the upper margin, or dropping the absolute elapsed ceiling and instead verifying the error/timeout path only.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:43">
P3: Edits to `scripts/test-utils.ts` skip the suite even though the runner imports its discovery and argument-processing helpers. Include that dependency in `integration` so runner behavior is covered.</violation>
<violation number="2" location=".github/workflows/ci.yml:383">
P2: SDK-only external PRs now start discovery plus up to 20 integration runners without maintainer approval. Gate discovery like the existing `extension-drift` job to avoid unreviewed fork code consuming the fan-out.
(Based on your team's feedback about approval gates for external-contributor workflows.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-screenshot.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:85">
P2: The dialog top-layer test claims to verify that masking works inside a `<dialog>` top layer, but it only asserts `bytes.length > 0` and that the temporary `[data-stagehand-mask]` overlay nodes are removed afterward. Neither assertion proves the mask actually rendered: the test would still pass if the mask silently did nothing to the captured image (e.g., if the top-layer element was never overlaid). Consider asserting a pixel value inside the masked region (or comparing against an unmasked capture) so the test would actually fail if top-layer masking regressed.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts:39">
P2: The wall-clock timing assertions in these tests are tight and can flake on a loaded CI runner. `waitForTimeout(0)` still requires a full RPC round trip through the v4 service-worker protocol (plus browser-side scheduling), so the `Date.now()` window measured around the call includes protocol latency and serialization overhead, not just the 0ms wait. On a slow shard the elapsed time can exceed the 50ms bound even though `waitForTimeout` behaves correctly. The same risk applies to the 200ms/190ms lower-bound check. Since the PR fans the suite into 20 shards, prefer asserting observable effects (e.g., the counter/DOM update tests already do this well) over sub-second elapsed-time thresholds for the fast paths.</violation>
</file>
<file name="packages/server/understudy/a11y/snapshot/capture.ts">
<violation number="1" location="packages/server/understudy/a11y/snapshot/capture.ts:35">
P3: The type name `WellFormedString` is misleading: the values passed through it are not well-formed — they are the inputs being sanitized via `.toWellFormed()`. The intersection type is really just a workaround to call a standard `String.prototype` method that the project's TS lib version doesn't expose, so it neither guarantees well-formedness nor documents intent well. Consider enabling `ES2024` (or later) in the TS `lib` so `string.toWellFormed()` is typed natively, or rename/comment this to make clear it only exposes the sanitization method rather than representing a well-formed string.</violation>
<violation number="2" location="packages/server/understudy/a11y/snapshot/capture.ts:798">
P2: The well-formed sanitization is applied inconsistently across the two snapshot paths. In tryScopedSnapshot, the sanitized wellFormedOutline is assigned to both combinedTree and perFrame[0].outline, but in mergeFramesIntoSnapshot only combinedTree is sanitized — the perFrame[].outline values returned below (lines 804-806) still carry the raw, possibly non-well-formed strings with unpaired surrogates. The main multi-frame/merged snapshot path therefore still emits per-frame outlines that the scoped path now cleans. If the goal of this change is to guarantee well-formed outline output (so downstream model/transport handling doesn't hit unpaired surrogates), the same sanitization should be applied to the per-frame outlines echoed in the merged snapshot to keep both paths consistent.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-addInitScript.test.ts:318">
P3: This spec (along with `page-addInitScript.test.ts`, which uses `EXAMPLE_URL = "https://example.com"`, and several popup cases here that open `https://example.com/`) performs live network navigations to a public site, while the rest of the v4 suite is built on the hermetic `startFixtureServer` helper and the PR's stated goal is “adapt legacy cases to hermetic v4 fixtures”. Since this integration suite is being fanned out to 20 CI shards, an external dependency like example.com is a real source of flakiness: it requires outbound network access, can be rate-limited or down, and can make shards fail nondeterministically. Consider replacing these navigations with routes served by `startFixtureServer` (the fixture server already serves `http://127.0.0.1:<port>/`), so the popup/init-script behavior is exercised against controlled, offline content.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
5 issues found across 32 files
Confidence score: 3/5
- In
.github/workflows/ci.yml, external SDK PRs can trigger the full integration fan-out without a maintainer gate, which can burn runner capacity and let untrusted PRs start broad CI workloads — add the repositorysafe-to-testguard to discovery so dependent shards stay gated. - In
.github/workflows/ci.yml, integration selection can miss or overrun coverage: changes toscripts/test-utils.tsmay skip the integration workflow entirely, and an emptytestNamesgroup can cause vitest to run all files in that group — includescripts/test-utils.tsin the paths filter and fail/guard whentestNamesis empty. - In
packages/sdk-ts/tests/integration/locator-input-methods.test.ts, the#transparent(opacity:0) expectation conflicts with howlocatorIsVisibledelegates to Playwrightlocator.isVisible(), so the test may assert behavior Playwright does not guarantee and create false confidence or intermittent failures — align the fixture/assertion with Playwright’s documented visibility semantics. - In
packages/sdk-ts/tests/integration/wait-for-selector.test.ts, fixed post-gotosleeps (waitForTimeout(100/300)) add unnecessary latency and can still be timing-sensitive under load — remove the fixed delays and rely onwaitForSelector/state-based waits to keep tests faster and more stable.
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=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:43">
P2: Changes to the runner's shared argument/discovery helper will skip integration coverage entirely. Include `scripts/test-utils.ts` in this filter so those changes exercise the workflow they affect.</violation>
<violation number="2" location=".github/workflows/ci.yml:383">
P2: External SDK PRs can now start the full integration fan-out without maintainer approval, consuming up to 20 hosted runners. Gate discovery with the repository's `safe-to-test` condition so its dependent shards remain skipped until approved.
(Based on your team's feedback about maintainer approval before external CI.)</violation>
<violation number="3" location=".github/workflows/ci.yml:434">
P2: The integration matrix runs each group's `paths` through vitest with no file-positional filtering when the array is empty. If a group ever has an empty `testNames` list (e.g., a maintainer adds a placeholder group), that shard silently executes the ENTIRE integration suite instead of skipping — duplicating every other shard and likely blowing the 20-minute per-job timeout. `groupIntegrationTests` rejects missing/duplicate/unknown tests but accepts an empty group without error. Consider guarding against empty groups so the matrix can't accidentally run everything.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-input-methods.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-input-methods.test.ts:104">
P2: The `#transparent` div uses `opacity:0`, but the SDK routes `locatorIsVisible` straight to Playwright's `locator.isVisible()`, and Playwright's documented visibility check is only "non-empty bounding box and not visibility:hidden" — CSS `opacity` is explicitly not a factor. So this element is considered visible and `expect(transparent).toBe(false)` will fail (and if it is currently passing, the SDK is not actually exercising Playwright's isVisible path). Consider dropping the opacity case or asserting `toBe(true)` to match Playwright semantics.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-selector.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-selector.test.ts:240">
P3: The post-`goto` `waitForTimeout(100/300)` sleeps are redundant fixed delays; each `waitForSelector` call already polls until its own timeout, so the sleeps only add wall-clock latency and can become flaky if a lane is slow. Relying on the selector polling (with an adequate `timeout`) would be more robust and faster.</violation>
</file>
Architecture diagram
sequenceDiagram
participant GH as GitHub Actions
participant Disc as Integration Discovery (test-integration.ts)
participant CI as Integration Shard Job
participant Vitest as Vitest Integration Tests
participant Stage as Stagehand SDK
participant Fix as Fixture Server
participant Br as Local Browser
participant Snap as Hybrid Snapshot Capture
participant LLM as LLM Client
Note over GH,LLM: v4 integration test architecture and runtime flow
GH->>GH: path-filter integration paths
alt integration paths changed
GH->>Disc: pnpm run test:integration -- --list-groups
Disc->>Disc: discover test files and assign semantic groups
alt missing or duplicate group ownership
Disc->>Disc: fail discovery with ownership error
else ten stable groups emitted
Disc-->>GH: integration matrix with grouped paths
loop each integration group
GH->>CI: matrix entry with INTEGRATION_PATHS
CI->>CI: decode JSON paths list
CI->>Vitest: pnpm run test:integration <paths...>
Vitest->>Stage: createStagehand({ browser, model })
Stage->>Br: launch local browser headless
Vitest->>Fix: startFixtureServer(routes or handler)
Fix-->>Vitest: fixture URL
Vitest->>Stage: page/locator/context APIs
Stage->>Br: CDP commands, navigation, input
Br->>Fix: HTTP requests with context/page headers
Fix-->>Br: fixture responses
Br-->>Stage: DOM state and event results
opt observe/extract/act
Stage->>Snap: capture hybrid DOM + accessibility snapshot
Snap->>Snap: normalize Unicode to well-formed strings
Snap-->>Stage: sanitized snapshot
Stage->>LLM: generate(snapshot prompt)
alt model responds
LLM-->>Stage: structured result
else model hangs or times out
LLM-->>Stage: timeout error
end
end
Vitest-->>CI: shard pass/fail
end
CI->>GH: upload CTRF report
end
else integration paths unchanged
GH->>Disc: skip integration discovery
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
7 issues found and verified against the latest diff
Confidence score: 3/5
- In
.github/workflows/ci.yml,determine-changesignores the existingsafe-to-testapproval path, so approved fork PRs can be blocked from running check/build/integration jobs and merge without expected validation coverage — include the approval signal in that gate condition. - In
packages/server/controllers/stagehandController.ts(act()), timeout handling rejects the request but does not cancelactService.act(), so browser actions may still execute after the SDK reports a timeout, creating state drift and surprising side effects — propagate cancellation/abort to the underlying action. - Several integration tests in
packages/sdk-ts/tests/integration/page-screenshot.test.ts,locator-nth.test.ts, andpage-drag-and-drop.test.tsdon’t assert the behavior named in their titles (masking effectiveness, iframe targeting, non-left mouse buttons), which can let regressions ship undetected — add assertions that directly verify the intended user-visible outcome. packages/sdk-ts/tests/integration/timeouts.test.tsappears timing-sensitive (5ms budget consumed during setup rather than while awaiting the hang), so results may be flaky and may not validate server checkpoint timeout behavior reliably — anchor assertions to the guarded operation phase instead of incidental setup timing.
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/integration/page-screenshot.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:85">
P3: The top-layer mask test validates that a masked screenshot is produced and overlays are cleaned up, but it never verifies the mask actually obscured the #secret input. If the locator failed to resolve inside the dialog top layer or masking silently no-op'd, this test would still pass. Consider asserting on the resulting image content (e.g., pixel sampling under the input) so the test actually guards the behavior its name describes.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:239">
P3: This test is titled "works correctly with iframe selectors" but only asserts the main-frame buttons; the frame's own buttons are written but never checked, so iframe-selector behavior isn't actually exercised. Add assertions against the frame's buttons (e.g. via the frame locator or by verifying the frame content) so the named case is covered.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:369">
P3: This test is named "different mouse buttons" but only passes `button: "left"` (the default) and the in-page drop handler hardcodes the text to "left" without reading event.button, so right/middle button handling is never actually exercised and the assertion couldn't detect failure even if a different button were used. Either test right/middle explicitly (reading e.button in the handler) or rename the case to reflect that it only verifies the default left-button path.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:24">
P1: Approved fork PRs never reach CI because this condition ignores the existing `safe-to-test` approval signal; all check, build, and integration jobs depend on `determine-changes`. Include the approval signal here so a maintainer-approved PR can run the required workflow.
(Based on your team's feedback about maintainer approval gates.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/sdk-ts/tests/integration/timeouts.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/timeouts.test.ts:24">
P3: These timeouts tests pass only if the 5ms operation budget is consumed during page setup/snapshot before the hanging model is awaited. Because the server enforces timeouts via checkpoint guards (createTimeoutGuard, which requires the flow to reach a post-step checkpoint) rather than racing the model call with a timer, a genuinely pending model is never bounded, so on a fast machine the test could hang until the 5s vitest timeout and fail/abort for the wrong reason. Consider asserting more directly that the timeout races the model call, or documenting the reliance on the setup-time >5ms condition so a future speedup doesn't silently turn this into flaky hangs.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-addInitScript.test.ts:88">
P3: The `page.addInitScript` and `context.addInitScript` suites each inline the same DOMContentLoaded-gated payload-marking helper (function source that writes a `dataset` marker on `DOMContentLoaded`, plus the matching `evaluate` JSON read-back), and the popup-await polling loops are near-identical across the addInitScript / default-page-tracking specs. Since these tests were just added together, consider extracting small shared helpers into `tests/integration/_support.ts` (e.g. an `installPayloadInitScript(ctx, key, payload)` plus a generic `waitForPage(context, predicate, timeoutMs)`) and reusing them in both suites. This keeps the growing integration suite consistent and avoids several copies of the same polling/scaffolding logic drifting apart.</violation>
</file>
<file name="packages/server/controllers/stagehandController.ts">
<violation number="1" location="packages/server/controllers/stagehandController.ts:54">
P2: When `act()` hits its `timeout`, the controller rejects and the SDK surfaces a 'timed out' error, but the underlying `actService.act()` is not cancelled, so the action can still be executed in the browser after the caller already received the timeout. That can cause unexpected side effects or duplicate actions if the user retries after a timeout. `withTimeout` only races the timers; it never signals or aborts the wrapped operation. Consider adding real cancellation/abort for `act()` (or at least documenting that a timed-out action may still run) so the reported timeout accurately reflects whether the action was applied.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
14 issues found across 33 files
Confidence score: 3/5
packages/server/controllers/stagehandController.tschanges howact/observe/extractare wrapped, but there’s no corresponding integration coverage for these API-facing behavior changes, so regressions can slip through undetected for clients — add focused integration tests underpackages/server/testfor normal, error, and timeout flows.- In
packages/server/controllers/stagehandController.ts,withTimeout()reports timeout to callers without canceling the underlying operation (notablyact()), which can leave work running after the request has failed and cause unintended side effects or resource pressure — propagate an abort/cancel signal and verify cleanup in tests. - The fork gating logic in
.github/workflows/ci.ymlcurrently skipsdetermine-changes(and downstream jobs) for approved external PRs and also undermines theextension-driftopt-in path, creating a real chance of unvalidated changes merging — include the existingsafe-to-testlabel in the gate and reconcile per-job conditions. - Several integration tests in
packages/sdk-ts/tests/integration/*and related test tooling (scripts/test-integration.ts) rely on timing-sensitive or incomplete assertions, which can produce flaky runs and miss real regressions in timeout/button/snapshot/grouping behavior — tighten assertions (event-driven waits, upper bounds, full option coverage) and document/enforce grouping invariants near the code.
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/integration/page-drag-and-drop.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:324">
P3: This test is titled "with different mouse buttons" but only exercises `{ button: "left" }`, which is the default, so it never verifies that the `button` option actually changes behavior and the name overstates the coverage. Consider either testing the right (and ideally middle) button paths — e.g. checking that a right-button drag does not trigger the left-button drop handler — or renaming the test to reflect that it only covers the default button.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-selector.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-selector.test.ts:201">
P3: The "respects custom timeout" assertion only checks a lower bound (>=450ms), so a waitForSelector that ignores the supplied timeout and waits far longer would still pass. Add an upper-bound check so the test actually verifies the custom timeout is honored, not merely that the call was slow.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-domain-policy.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-domain-policy.test.ts:38">
P3: The non-allowed host is derived by swapping in 'localhost', which only works where localhost resolves to 127.0.0.1 (the fixture binds IPv4 only). The reachability assertion guards a silent pass but turns this into an environment-dependent failure on IPv6-first hosts. If the two-host distinction isn't essential, register a second route/host on the same bound address, or explicitly require an IPv4 localhost mapping in a comment.</violation>
<violation number="2" location="packages/sdk-ts/tests/integration/context-domain-policy.test.ts:103">
P2: This negative assertion (that a blocked popup was not retained) relies on a fixed 500ms wall-clock sleep before inspecting `context.pages()`. Fixed sleeps to assert that something did *not* happen are inherently timing-dependent: on a slow CI lane the popup may simply not have been created/teardown yet, or a slow close may leave the page visible past the window, making the assertion flaky in either direction. Consider replacing the raw sleep with a deterministic wait (e.g. `expect.poll` with a timeout that checks whether the target page ever appears, or waiting on the navigation promise) so the assertion is robust to environment timing.</violation>
</file>
<file name="packages/server/understudy/a11y/snapshot/capture.ts">
<violation number="1" location="packages/server/understudy/a11y/snapshot/capture.ts:262">
P3: Lone-surrogate sanitization lacks coverage for selector-scoped observe/extract requests. `tryScopedSnapshot` returns before `mergeFramesIntoSnapshot`; add a focused Unicode case so this separate path stays well-formed.
(Based on your team's feedback about adding unit tests for new behavior.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/server/understudy/a11y/snapshot/capture.ts:799">
P3: The well-formed Unicode sanitization is now implemented twice: once in `tryScopedSnapshot` (line 262, `wellFormedOutline`) and once in `mergeFramesIntoSnapshot` (the `toWellFormed` arrow at line 799, used at 800 and 810). Both do the identical `(value as StringWithToWellFormed).toWellFormed()` cast. Consider extracting a single shared module-level helper (e.g., `wellFormed(value: string): string`) so the two snapshot paths stay consistent and the `StringWithToWellFormed` type/comment lives in one place, making it easier to keep behavior aligned if the sanitization ever needs to change.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:22">
P2: Approved fork PRs cannot run any CI: their head repo never equals `github.repository`, so `determine-changes` and every dependent job are skipped. Include the existing `safe-to-test` approval label in this gate.
(Based on your team's feedback about maintainer approval for fork CI.) [FEEDBACK_USED].</violation>
<violation number="2" location=".github/workflows/ci.yml:23">
P3: This new fork gate on `determine-changes` correctly blocks the whole pipeline for external PRs, but it also silently breaks the existing per-job opt-in for `extension-drift`. That job's condition still offers `safe-to-test` as 'a way for a maintainer to opt a specific external PR in' and checks `contains(github.event.pull_request.labels.*.name, 'safe-to-test')` — but when `determine-changes` is skipped the `needs.determine-changes.outputs.sdk-go` term is empty, so `extension-drift` is skipped even when the label is present. If an internal-promotion handoff is now the only intended path for fork PRs, that's fine, but the dead `safe-to-test` condition and its comment in `extension-drift` (and the now-redundant fork gate in `go-windows`) should be removed or updated so the workflow isn't documenting an opt-in that can no longer fire.</violation>
<violation number="3" location=".github/workflows/ci.yml:389">
P3: After switching `discover-integration` to the new `integration` output, the `server` job output (`server: ${{ steps.filter.outputs.server }}`) and its `server` path-filter key have no remaining consumer in this workflow. It's now dead config that a future change could mistake for an active gate. Since the `integration` filter already covers the same server/protocol/lock paths, consider dropping the orphaned `server` output (and the `server` filter key) rather than keeping two parallel filters where only one is wired up.</violation>
</file>
<file name="scripts/test-integration.ts">
<violation number="1" location="scripts/test-integration.ts:50">
P3: The semantic-group mapping introduces a hard invariant that is not documented near the code: every discovered integration test must appear in exactly one group, and `--list-groups` / `groupIntegrationTests` hard-fails ("Integration tests missing a group") otherwise. That means a future maintainer who adds a new `.test.ts` file but forgets to register it here will break integration discovery in CI with a non-obvious failure. Consider adding a co-located comment above `integrationTestGroups` explaining that new test files must be registered in exactly one group (and that discovery will throw otherwise), and that the list must stay in sync with `packages/sdk-ts/tests/integration`.</violation>
<violation number="2" location="scripts/test-integration.ts:86">
P3: The `assigned.has(testName)` check also fires when the same test name is listed twice within a single group (e.g. a typo like `["timeouts", "wait-for-selector", "timeouts"]`), but the raised message always claims "has multiple groups", which would mislead a maintainer debugging the failure. Consider refining the message (or detecting intra-group duplicates separately) so the diagnostic names the actual problem.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-addInitScript.test.ts:119">
P3: The add-init-script injection boilerplate (the `setPayload`/`markVisit` helper that checks `document.readyState === "loading"` before wiring a `DOMContentLoaded` listener) is duplicated verbatim across at least four tests in this file and again in `page-addInitScript.test.ts` and `default-page-tracking`/`user-data-dir` style setups. Consider extracting a shared helper (e.g. a `withInitScriptPayload(name, payload)` helper in `_support.ts`) so a future change to the navigation/readyState logic — or to the protocol method under test — only has to be made in one place, and so the tests assert on a single consistent witness.</violation>
</file>
<file name="packages/server/controllers/stagehandController.ts">
<violation number="1" location="packages/server/controllers/stagehandController.ts:53">
P2: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
The controller now wraps `act`, `observe`, and `extract` calls with `withTimeout()`, introducing a new timeout-enforcement layer on the server surface. This changed controller code path needs integration-test coverage under `packages/server/tests/` that exercises a request with a timeout option and asserts the resulting timeout error. Existing service-level timeout tests do not cover the controller wrapping.</violation>
<violation number="2" location="packages/server/controllers/stagehandController.ts:65">
P2: The new withTimeout() wrapper reports a timeout to the caller but does not cancel the underlying act/observe/extract operation. For `act()` specifically, actService.act continues running in the background after the caller receives a TimeoutError, so the action can still be applied to the page even though the user believes it timed out; it also keeps page/service resources (e.g., an unresolved LLM generate) alive until the underlying promise settles. This is different from the existing screenshot use of withTimeout, which wraps a passive read-only operation. Consider either aborting the underlying operation on timeout (and awaiting/ignoring its late result) or documenting that a timeout does not guarantee the act was not performed, so callers can verify outcome before proceeding.</violation>
</file>
Architecture diagram
sequenceDiagram
participant GHA as GitHub Actions CI
participant Detect as determine-changes filter
participant Discover as test-integration discovery
participant Vitest as Vitest shard runner
participant Tests as Integration test suite
participant Fixture as Fixture HTTP server
participant SDK as Stagehand SDK
participant Ctx as BrowserContext
participant Page as Page/Locator
participant Snap as Hybrid snapshot capture
participant Ctrl as Stagehand server controller
participant LLM as Client LLM stub
Note over GHA,Discover: CI integration discovery and sharding
alt internal PR or push
GHA->>Detect: evaluate changed paths
Detect->>Detect: match integration path patterns
Detect-->>GHA: integration=true
GHA->>Discover: pnpm test:integration -- --list-groups
Discover->>Discover: discoverIntegrationTests(testsDir)
Discover->>Discover: groupIntegrationTests into 10 local/* groups
Discover-->>GHA: JSON group names and paths
GHA->>GHA: build CI matrix from group paths
loop each CI shard
GHA->>Vitest: run with INTEGRATION_PATHS json
Vitest->>Tests: execute group test files
end
else external fork PR
GHA->>GHA: job skipped until maintainer promotion
end
Note over Tests,LLM: Hermetic runtime flow exercised by the expanded suite
Tests->>Fixture: startFixtureServer(routes/html/handler)
Tests->>SDK: createStagehand with browser/model options
SDK->>Ctx: launch local browser and create context
Ctx->>Page: firstPage / newPage(url)
Page->>Fixture: goto with optional extra HTTP headers
Ctx->>Ctx: addInitScript / setDomainPolicy / clipboard
Ctx->>Page: apply context policies to new and child pages
Page->>Snap: capture hybrid DOM + a11y snapshot
Snap->>Snap: toWellFormed() on combinedTree/perFrame outlines
Snap-->>Ctrl: HybridSnapshot
Ctrl->>LLM: act/observe/extract model.generate
Ctrl->>Ctrl: withTimeout(servicePromise, options.timeout)
alt operation timeout configured
Ctrl-->>Tests: reject with timed out
else model responds
LLM-->>Ctrl: structured LLM result
Ctrl-->>Tests: act/observe/extract result
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
7 issues found across 33 files
Confidence score: 2/5
- In
packages/server/controllers/stagehandController.ts,act()timeout handling usesPromise.race, so browser work can continue after aTimeoutErroris returned; this can trigger late side effects and state drift for callers who already treated the action as failed — propagate cancellation/deadline into the underlying action path so timed-out operations are actually aborted. - In
packages/sdk-ts/tests/integration/locator-input-methods.test.ts, the visibility assertion conflicts with Playwright semantics (opacity: 0can still be visible), which is likely to produce deterministic test failures and block CI — update the assertion to match Playwright’s visibility rules or change the fixture to use truly non-visible conditions (display:none/visibility:hidden). - Across
packages/sdk-ts/tests/integration/page-screenshot.test.ts,page-drag-and-drop.test.ts,locator-count.test.ts,context-addInitScript.test.ts, andscripts/test-integration.ts, several low-severity test gaps/duplication reduce signal (non-assertive screenshot checks, drag-button path not exercised, timing-based flake risk, duplicated setup, untested--list-groupswiring); these won’t usually break runtime behavior immediately but can hide regressions — add focused assertions/helpers and targeted CLI coverage 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/sdk-ts/tests/integration/locator-input-methods.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-input-methods.test.ts:104">
P1: This assertion will fail: Playwright's locator.isVisible() treats an element with opacity:0 as visible, because visibility is defined by a non-empty bounding box and not being display:none/visibility:hidden — opacity is ignored. The #transparent div (normal size, opacity:0) will report visible, so `expect(transparent).toBe(false)` fails whenever the test runs. Either assert the actual behavior (`.toBe(true)`) or restyle the element so it is genuinely non-visible.</violation>
</file>
<file name="packages/server/controllers/stagehandController.ts">
<violation number="1" location="packages/server/controllers/stagehandController.ts:54">
P1: A timed-out `act()` can still complete its in-flight browser action after the caller has received `TimeoutError`. `Promise.race` only stops awaiting; propagate cancellation/deadline into the action operation so a timed-out request cannot later mutate the page.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-addInitScript.test.ts:119">
P3: The addInitScript-with-args `setPayload`/DOMContentLoaded boilerplate is duplicated verbatim in three tests, and the `knownTargetIds`+`waitForPopupPage` polling pattern in four. Extracting these into shared helpers in `_support.ts` (or local module-level helpers) would keep the suite consistent and let a future browser-behavior change to the init-script timing be fixed in one place rather than in every copy.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-screenshot.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:66">
P3: The advanced-screenshot test validates mask/omitBackground/style only by asserting non-empty bytes and that the temporary overlay attributes are cleaned up. It never verifies the mask replacement color, transparency, or style actually appear in the rendered output, so a no-op regression in any of these options would go undetected. Consider decoding a pixel from the returned buffer (e.g., assert the masked region resolves to the configured maskColor or that the background is transparent) to make the assertion meaningful.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:369">
P3: The "drag and drop with different mouse buttons" test does not actually exercise the `button` option. The fixture's `drop` handler unconditionally sets `#buttonUsed` to `'left'`, and the test only passes `{ button: "left" }` (the default) before asserting that value. As written, the assertion is always true and the test would pass even if Stagehand ignored the `button` parameter. Consider having the fixture record `e.button` from the drop event and asserting right/middle-button behavior too, or drop the misleading "different buttons" framing.</violation>
</file>
<file name="scripts/test-integration.ts">
<violation number="1" location="scripts/test-integration.ts:111">
P3: The new `--list-groups` CLI wiring (parsing the flag, stripping it from `remainingArgs`, and choosing groups vs. entries output inside `isDirectExecution`) is not covered by any unit test — only `groupIntegrationTests` itself is unit-tested. Since CI depends on this exact flag path (`pnpm --silent run test:integration -- --list-groups` in the discover-integration job), a regression in the arg parsing could silently change the matrix JSON. Consider extracting the flag-parsing/output-selection into a small exported function and asserting it directly (for `--list-groups`, `--list`, and neither), matching the existing granular tests for `groupIntegrationTests`.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-count.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-count.test.ts:84">
P3: The shadow-DOM setup here runs synchronously during page load, so by the time `page.goto(..., { waitUntil: "load" })` resolves the `button`s already exist inside the shadow root. The added `setTimeout(100)` (with the 'wait a bit' comment) is therefore dead synchronization and a potential flake source; other shadow-DOM specs in this suite navigate the same way and query immediately with no raw sleep. Consider removing the timer, or if a wait is genuinely needed, poll deterministically (e.g. `expect(locator).toHaveCount(2)` or `page.waitForSelector`) rather than sleeping a fixed 100ms.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as GitHub CI
participant DET as Determine Changes
participant DISC as Discover Integration Tests
participant SDKT as SDK TS Tests
participant SERVER as Server
participant CONTROLLER as Stagehand Controller
participant TIMEOUT as withTimeout
participant LLM as LLM Provider
participant BROWSER as Browser
Note over CI,BROWSER: PR expands integration coverage from 5 to 31 files across 10 semantic groups
CI->>DET: Trigger on PR targeting main
DET->>DET: Evaluate conditional (external PR skip)
alt workflow == server or sdk or package files changed
DET->>DISC: Pass integration=true flag
DISC->>DISC: pnpm test:integration --list-groups
Note over DISC: NEW: outputs 10 stable semantic groups<br/>(local/browser-lifecycle, local/context-network,<br/>local/input, local/locators-read, etc.)
DISC-->>CI: JSON matrix of groups with test paths
CI->>CI: For each group, run shard with paths from INTEGRATION_PATHS
CI->>SDKT: pnpm test:integration -- [group paths]
end
Note over SDKT,BROWSER: Integration test execution for each group
SDKT->>SDKT: createStagehand() with optional browser/model overrides
SDKT->>BROWSER: localBrowser.launch({ headless: true })
BROWSER-->>SDKT: Browser instance
SDKT->>SDKT: Stagehand.create({ browser, model })
alt Timeout tests (timeouts.test.ts)
SDKT->>SDKT: Use hangingModel (never resolves)
SDKT->>CONTROLLER: observe() / extract() / act() with timeout: 5
CONTROLLER->>TIMEOUT: NEW: Wrap service call with withTimeout()
TIMEOUT->>LLM: Await model.generate()
Note over TIMEOUT,LLM: Promise never resolves
TIMEOUT-->>CONTROLLER: Reject with timeout error
else Unicode well-formed tests (unicode-well-formed.test.ts)
SDKT->>BROWSER: Navigate to page with malformed Unicode
SDKT->>SDKT: CHANGED: snapshot.capture() calls toWellFormed() on outlines
SDKT->>LLM: observe() prompt includes repaired Unicode
LLM-->>SDKT: Structured content with elements
else Standard integration tests
SDKT->>BROWSER: Navigate to fixture URL or data: URI
BROWSER-->>SDKT: Page ready
SDKT->>BROWSER: Click / type / scroll / hover / drag-and-drop
SDKT->>BROWSER: evaluate() to read DOM state
BROWSER-->>SDKT: DOM values (click counts, text content, etc.)
end
Note over SDKT: Fixture server pattern (startFixtureServer)<br/>supports static HTML, route maps, or request handlers
alt Fixture with FixtureHandler
SDKT->>SDKT: startFixtureServer((request) => body)
SDKT->>SERVER: HTTP GET /pixel.svg or /
SERVER-->>SDKT: Response with headers from handler
end
Note over SDKT: Popup window interaction tested with<br/>waitForPopupPage polling pattern
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
8 issues found across 33 files
Confidence score: 3/5
- In
packages/server/controllers/stagehandController.ts,act()still doesn’t enforceoptions.timeoutwhile an LLM request is in flight, so hanging-model calls can block until the 60s RPC fallback instead of failing fast as configured — apply the same wall-clock timeout enforcement used in the other controller paths. - In
packages/server/controllers/stagehandController.ts, theobserve/extractresponse wrapping changes alter Stagehand REST API client/server behavior without the required integration coverage, which raises regression risk for consumers expecting previous payload shapes — add an integration test underpackages/server/testthat exercises client-server compatibility for these endpoints. - Several SDK integration tests are brittle (
packages/sdk-ts/tests/integration/context-domain-policy.test.ts,.../user-data-dir.test.ts) due to localhost IPv4 assumptions, fixed sleeps, and immediate profile cleanup, which can create nondeterministic CI failures and hide real regressions — switch to deterministic synchronization/host selection and retry-safe teardown. - Assertion strength is weak in
packages/sdk-ts/tests/integration/locator-nth.test.tsand.../wait-for-selector.test.ts, so tests may pass on unrelated errors or default timeout behavior rather than the intended semantics — assert specific error messages/types and both lower/upper timing bounds to verify the targeted behavior.
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/integration/user-data-dir.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/user-data-dir.test.ts:19">
P3: Removing the freshly-used Chrome profile in afterEach right after closeStagehand can flake on platforms where the browser process has not fully released its user-data-dir files, since `force: true` does not retry or ignore EBUSY/EPERM. Consider wrapping the rmSync in a try/catch or retrying briefly to keep the teardown robust across all CI lanes.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-addInitScript.test.ts:119">
P3: The `setPayload()` + `document.readyState === "loading"` DOMContentLoaded guard is duplicated verbatim across four tests in this file (the "args to new pages", "newPage(url)", "link clicks", and "window.open" cases), differing only in the value they serialize. Consider extracting a small shared helper (e.g. `installInitPayload(window, payload)`) so the four tests express only their distinct setup, which also makes it easier to spot that the cross-process/popup tests share the same plumbing.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-domain-policy.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-domain-policy.test.ts:38">
P2: this case hard-depends on `localhost` resolving to the IPv4 loopback that the fixture server is bound to (`server.listen(0, "127.0.0.1", ...)`), so the navigation may fail in environments where localhost resolves to ::1. Consider binding the fixture to a host that is guaranteed to be independently addressable, or structuring the allowed/blocked assertion without relying on hostname routing, so the test validates domain policy rather than local DNS/loopback behavior.</violation>
<violation number="2" location="packages/sdk-ts/tests/integration/context-domain-policy.test.ts:103">
P2: The popup-rejection test relies on a fixed `waitForTimeout(500)` sleep before asserting that no new page is retained. The popup lifecycle being verified is asynchronous and its latency varies across CI lanes, so this timing window can flake (popup still present when the assertion runs on a slow lane). Consider polling until the retained page count settles instead of sleeping a hard-coded duration, e.g. `await expect.poll(...)` on the filtered retained page ids until it equals `[]`, or awaiting the popup-close event.</violation>
</file>
<file name="packages/server/controllers/stagehandController.ts">
<violation number="1" location="packages/server/controllers/stagehandController.ts:78">
P1: `act()` still ignores timeout while an LLM request is pending, so the new hanging-model timeout case waits for the RPC's 60-second fallback rather than rejecting after `options.timeout`. Apply the same wall-clock timeout to `actService.act` (or move it to a common controller path).</violation>
<violation number="2" location="packages/server/controllers/stagehandController.ts:78">
P2: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
The `observe` and `extract` controller methods now wrap their service calls in `withTimeout`, adding a new server-level timeout boundary. The existing service tests cover internal timeout checkpoints, but no integration test exercises the controller-level wrapper to ensure the Promise.race timeout correctly fires and produces the expected `TimeoutError` for the full operation. Consider adding an integration test in `packages/server/tests/` that invokes the controller (or the route handler that calls it) with a short `options.timeout` and asserts the timeout behavior.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:145">
P3: The out-of-bounds test only asserts that *some* exception was thrown, so it would pass even if textContent() failed for an unrelated reason and does not actually verify out-of-range semantics. Consider asserting rejection directly and narrowing the error type/message so the test fails on the wrong failure mode. Also note a raw action on an out-of-range nth can burn the full default action timeout before throwing, slowing the suite.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-selector.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-selector.test.ts:201">
P3: The elapsed assertion has only a lower bound, so it can't actually confirm the custom 500ms timeout was honored — if the option were ignored and the default (seconds-scale) timeout were used, elapsed would still be >= 450ms and the test would pass. Consider adding an upper bound (e.g. `toBeLessThan(3000)`) so the test meaningfully discriminates custom-vs-default timeout behavior.</violation>
</file>
Architecture diagram
sequenceDiagram
participant PR as Contributor PR
participant CI as GitHub Actions CI
participant Detect as determine-changes
participant Discover as test-integration.ts
participant Matrix as Integration matrix job
participant Vitest as Vitest integration suite
participant Stagehand as Stagehand SDK runtime
participant Fixture as Local fixture server
participant Browser as Chrome browser lane
participant Server as Stagehand controller
participant Snapshot as Snapshot capture
participant LLM as Stub LLM model
PR->>CI: PR events (opened, synchronize, labeled, unlabeled)
CI->>Detect: Evaluate head repo and safe-to-test label
alt External fork without safe-to-test label
Detect-->>CI: Skip PR (no checkout or execution)
end
Detect-->>CI: integration=true for SDK/server/protocol/scripts paths
CI->>Discover: pnpm test:integration -- --list-groups
Discover->>Discover: Discover files and enforce unique semantic group ownership
Discover-->>CI: Ten stable integration groups with paths
CI->>Matrix: INTEGRATION_PATHS=<JSON group paths>
Matrix->>Vitest: pnpm test:integration -- <group paths...>
Vitest->>Stagehand: createStagehand({ browser, model })
Stagehand->>Browser: Launch headless Chrome (optional userDataDir)
Vitest->>Fixture: startFixtureServer(routes or request handler)
Fixture-->>Vitest: Local fixture URLs
Vitest->>Stagehand: Drive page/locator actions (click, type, clipboard, etc.)
Stagehand->>Server: observe()/extract()/act() via runtime adapter
Server->>Server: CHANGED: withTimeout(observe/extract, options.timeout)
Server->>LLM: model.generate (possibly hanging stub)
alt Timeout fires
Server-->>Stagehand: Reject with timeout error
else Model returns
LLM-->>Server: Structured LLM content
Server->>Snapshot: Build hybrid DOM + accessibility snapshot
Snapshot->>Snapshot: CHANGED: toWellFormed() on outline/combinedTree
Snapshot-->>Server: Sanitized snapshot
Server-->>Stagehand: Observation/extraction result
end
Stagehand-->>Vitest: DOM state / action result
Vitest-->>CI: Pass/fail + CTRF report
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
9 issues found across 34 files
Confidence score: 4/5
- In
packages/sdk-ts/tests/integration/wait-for-timeout.test.ts, reliance on fixed wall-clock timing and not guaranteeingtimeoutPromisecleanup can make CI failures flaky and harder to diagnose due to masked root errors—switch to condition-based polling and always await/cleanup in afinallypath. - In
packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts, the “explicit left mouse button” case does not actually read the event button, so it can pass even if button handling regresses—assert againstevent.button/captured event data to validate real behavior. - In
packages/sdk-ts/tests/integration/page-screenshot.test.tsandpackages/sdk-ts/tests/integration/locator-content-methods.test.ts, assertions are coupled to internal DOM markers or incomplete versus test intent, which risks false failures during refactors and gaps in behavior coverage—assert via public outcomes and add the missingtextContentexpectation. - In
packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts(fixture duplication) plus naming/scope drift inkeyboard.test.tsandframe-get-location-and-click.test.ts, maintainability is the main risk because intent and coverage are harder to trust over time—deduplicate fixtures and align suite/file names with what is actually tested.
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/integration/page-screenshot.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:69">
P3: These tests correctly verify that temporary screenshot overlays are removed from the page, but they assert this by hard-coding the extension's internal DOM marker attributes (`data-stagehand-mask` and `data-stagehand-style`). Those attribute names are implementation details of the masking mechanism and not part of the public SDK contract, so a benign internal rename or a switch to a different masking technique would break these stability-focused integration tests even when behavior is correct. Consider verifying the user-visible outcome (for example, that the masked element's computed style is back to normal or that no visual residue remains) instead, or keep a single documented constant for the marker names if the DOM-based check must stay.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/keyboard.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/keyboard.test.ts:9">
P3: The suite label "V3 keyboard shortcuts and typing" is a leftover from the ported legacy spec; this file is part of the v4 integration coverage (Stagehand.create + public Page API). Rename the describe block to "v4 keyboard shortcuts and typing" so test-group output and failure reports don't mislead.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts:56">
P3: The assertion that the interval-driven counter reaches >= 3 after a 350ms wait depends on wall-clock page-timer behavior, which can be throttled or delayed on a loaded CI browser. Consider polling for the expected text with `expect.poll`/`waitForSelector` instead of a fixed timeout so the test doesn't flake (slow-down) and stays robust under load.</violation>
<violation number="2" location="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts:144">
P3: If any of the intermediate `page.evaluate` / `textContent` steps fail, `timeoutPromise` is never awaited, producing a secondary unhandled-rejection error that obscures the actual failure. Awaited the timeout in a `finally` (or via `Promise.all`) so it is always settled when the test exits.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:16">
P3: The file repeats the same draggable+drop-zone HTML fixture and centroid-coordinate boilerplate seven times, with only minor ID/style variations. Since _support.ts already provides a shared fixture server, consider extracting a shared draggable fixture (and a small helper returning source/target centroids) to cut the duplication and make each test's intent clearer.</violation>
<violation number="2" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:371">
P3: The 'explicit left mouse button' test doesn't actually verify the button option: the drop handler sets #buttonUsed to 'left' unconditionally and never inspects the event's button, so the assertion passes whether or not dragAndDrop honored { button: "left" }. Consider reading e.button in the drop handler and asserting on that value so the test meaningfully covers the parameter.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-count.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-count.test.ts:84">
P3: This shadow-DOM test coordinates via a fixed 100 ms sleep rather than a real wait for the element to be present. Since the `<script>` runs synchronously before the `load` event that `goto(..., { waitUntil: "load" })` resolves on, the shadow buttons are already attached when this line executes, so the delay is redundant; on slower CI the fixed sleep is also a classic source of flake if the script ever becomes async. Prefer polling with `waitForSelector`/`expect(...).toHaveCount` so the test waits only as long as needed and never races the DOM.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-content-methods.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-content-methods.test.ts:210">
P3: The comment claims textContent includes script content, but nothing asserts it. Only innerText's exclusion (`not.toContain("console.log")`) is actually verified, so the comparison this test is named for isn't validated; add `expect(textContent).toContain("console.log")` (and optionally the style content) to lock in the distinguishing behavior.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/frame-get-location-and-click.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/frame-get-location-and-click.test.ts:5">
P3: The filename is `frame-get-location-and-click.test.ts`, but the spec contains no frame, iframe, or frame `getLocation` coverage — it only tests coordinate-based clicking on a plain button with no frames involved (the describe block is correctly titled "Coordinate-based clicking"). The name implies frame/location behavior that this test does not exercise, which misleads anyone scanning the suite for frame coverage. Consider renaming the file to something like `coordinate-click.test.ts`, or porting the intended frame `getLocation` case so the file matches its name.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as GitHub Actions CI
participant Det as Determine Changes Job
participant Fi as Path Filter
participant Disc as Discover Tests Job
participant Test as Integration Test Job
participant SDK as SDK-TS (Integration Tests)
participant Server as Stagehand Server
participant Model as ClientLLM (Stub)
Note over CI,Test: NEW: CI pipeline with integration detection
CI->>Det: Trigger (PR opened/reopened/synchronize/labeled)
Det->>Fi: Filter changed paths
alt Safe-to-test (internal PR or label present)
Fi-->>Det: Output integration flag (true/false)
else External PR without label
Det-->>CI: Skip (job not run)
end
Det-->>Disc: Pass integration flag
alt integration == true
Disc->>Disc: pnpm test:integration -- --list-groups
Disc-->>Test: Output group matrix
end
loop Each group in matrix
Test->>Test: Setup Chrome (setup-chrome-verified action)
Test->>SDK: Run pnpm test:integration -- [group paths]
SDK->>SDK: Create Stagehand instance via createStagehand()
SDK->>Server: Initialize Stagehand runtime
Server->>Server: Set up runtime state, model adapter
SDK->>SDK: Launch local browser (headless)
SDK->>SDK: Create fixture server (startFixtureServer)
alt Click-count tests
SDK->>SDK: Navigate to double-click fixture
SDK->>SDK: locator.click() / page.click() with clickCount options
Note over SDK: CHANGED: click method updated for multi-click
else Clipboard tests
SDK->>SDK: Write/read clipboard via context.clipboard
SDK->>SDK: paste()/copy()/cut() operations
else Context addInitScript tests
SDK->>SDK: ctx.addInitScript() on context
SDK->>SDK: Navigate, create new pages, open popups
SDK-->>SDK: Verify script injection across navigations
else Domain policy tests
SDK->>SDK: setDomainPolicy() with allowed/blocked domains
SDK->>SDK: Verify image loading blocked/allowed
else Extra HTTP headers tests
SDK->>Server: setExtraHTTPHeaders()
SDK->>Server: Navigate to fixture server
Server-->>SDK: Verify header in request
else Page tracking tests
SDK->>SDK: Open popups, close pages
SDK-->>SDK: Check activePage() returns correct page
else Keyboard tests
SDK->>SDK: keyPress(), type() for various keys
SDK-->>SDK: Verify input values
else Locator content tests
SDK->>SDK: textContent(), innerHtml(), innerText(), inputValue()
SDK-->>SDK: Verify returned content
else Locator count/nth tests
SDK->>SDK: count() and nth() for CSS/XPath/text selectors
SDK-->>SDK: Verify element counts and text content
else Locator input tests
SDK->>SDK: fill(), type(), hover(), isVisible(), isChecked()
SDK-->>SDK: Verify state changes
else selectOption tests
SDK->>SDK: selectOption() on select elements
SDK-->>SDK: Verify selected values
else Observe element ID tests
SDK->>Model: Observe instruction
Model-->>SDK: Return element with 0-ordinal ID
SDK->>SDK: Verify ID format preserved
else Page addInitScript tests
SDK->>SDK: page.addInitScript() scoped to page
SDK-->>SDK: Verify script runs only on target page
else Drag-and-drop tests
SDK->>SDK: dragAndDrop() with coordinates/options
SDK-->>SDK: Verify drop event triggered
else Hover tests
SDK->>SDK: hover() at coordinates
SDK-->>SDK: Verify mouseover/hover styles
else Screenshot tests
SDK->>SDK: screenshot() with various options
SDK-->>SDK: Verify output and cleanup of overlays
else Scroll tests
SDK->>SDK: scroll() with delta values
SDK-->>SDK: Verify scroll position changed
else Timeout tests
SDK->>Model: action (act/observe/extract)
Model-->>SDK: Never resolves (hanging)
SDK-->>SDK: Verify timeout exception thrown
else Unicode tests
SDK->>Model: observe page with malformed unicode
Model->>SDK: Verify text is well-formed before model call
Note over SDK,Model: CHANGED: toWellFormed() sanitization in snapshot
else User-data-dir tests
SDK->>SDK: Launch Chrome with userDataDir
SDK-->>SDK: Verify Default/Local State files exist
end
Test-->>CI: Upload CTRF report
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Confidence score: 4/5
- In
packages/sdk-ts/tests/integration/_support.ts,localBrowser.launchcan succeed whileStagehand.createthrows, leaving an untracked browser process running and potentially hanging Node/CI runs — wrap creation intry/finally(or equivalent cleanup path) so the launched handle is always closed on failure. - In
packages/sdk-ts/tests/integration/page-screenshot.test.ts, the screenshot coverage is weaker than the test names imply: one assertion depends on Playwright-specific error text, and the masking test never verifies pixels were actually masked, so real regressions could slip through or fail for wording-only changes — assert on the zod enum error shape and validate masked output content. - In
packages/sdk-ts/tests/integration/locator-nth.test.ts, the iframe fixture cannot currently distinguish whether main-page locators incorrectly include child-frame elements, so the test may pass in both correct and incorrect implementations — adjust fixture ordering/content so the two behaviors produce different observable results. - In
packages/server/understudy/a11y/snapshot/capture.ts, malformed Unicode handling is only exercised for root-page/full-merge and same-page scoped paths, leaving child-frame merge behavior untested and a regression path open — add a child-frame malformed Unicode case to the regression suite.
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/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:230">
P3: This iframe test claims main-page locators should not include child-frame elements, but with the current fixture the assertion can't distinguish the two behaviors: both `Main Button` elements sort before the frame buttons in DOM order, so a frame-piercing implementation would still return "Main Button 1"/"Main Button 2" for `nth(0)`/`nth(1)`. To actually validate main-frame scoping, assert the scoped match count so frame elements would break the test if included.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-screenshot.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:32">
P2: This 'rejects unsupported image types' assertion was ported from Playwright wording ('expected one of'), but v4 validates screenshot options server-side through a zod schema whose enum error is `Invalid enum value. Expected 'png' | 'jpeg', received 'webp'` — it never contains 'expected one of'. As written the regex likely never matches, so the test could silently fail or be a false-negative. Anchor the expectation to the actual zod error text instead.</violation>
<violation number="2" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:89">
P3: For a test named around masking, this only checks that a screenshot is returned and that the DOM is left clean; it never asserts the masked element was actually hidden in the output image. If masking silently no-ops, the test still passes. Consider a stronger assertion (e.g. pixel/region check or masking a distinctive background) to cover the behavior it claims to test.</violation>
</file>
<file name="packages/server/understudy/a11y/snapshot/capture.ts">
<violation number="1" location="packages/server/understudy/a11y/snapshot/capture.ts:801">
P3: Malformed Unicode in a child frame's accessibility outline is not covered by the new regression test. `unicode-well-formed.test.ts` exercises the full merge path only with a root page and the scoped fast path with the same page, so it does not verify that `injectSubtrees` preserves the replacement behavior when a nested iframe outline is merged, nor that the corresponding `perFrame` outline is well formed. A small nested-iframe fixture with a lone surrogate would make this `mergeFramesIntoSnapshot` behavior regression-proof.
(Based on your team's feedback about focused tests for new behavior and key edge cases.) [FEEDBACK_USED].</violation>
</file>
<file name="packages/sdk-ts/tests/integration/_support.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/_support.ts:34">
P3: If `localBrowser.launch` succeeds but `Stagehand.create` (its arg is not actually awaited, but creation may throw) rejects, the launched browser handle is a lost local and never closed, which can hang the Node process and stall an integration CI lane since each of the new specs depends on this helper. Wrap the launch in try/catch and close the browser on the failure path so a failed spec doesn't leak a live browser.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
5 issues found across 34 files
Confidence score: 4/5
- In
.github/workflows/ci.yml, addinglabeled/unlabeledtopull_requestcan restart CI on any label change, and with concurrency cancellation this can interrupt in-flight validation and create noisy/unstable signal—limit triggers to the intended event path or gate execution so onlysafe-to-testlabel changes run the costly jobs. - In
.github/workflows/ci.yml, the integration filter currently misses roottsconfig.jsonchanges, so transpilation/runtime regressions could merge without integration coverage—includetsconfig.jsonin theintegrationpath set so those checks always run when compiler settings move. - In
packages/sdk-ts/tests/integration/observe-element-id-format.test.ts, asserting a hardcoded internal observe-instruction sentence makes the test fragile to harmless prompt wording refactors, which can cause false failures and slow refactors—assert stable behavioral output or a narrower contract instead of exact internal phrasing. - In
packages/server/tests/stagehand-controller-timeout.test.tsandpackages/sdk-ts/tests/integration/_support.ts, coverage misses the non-timeout baseline path and launch defaults rely on spread-order behavior, leaving room for subtle regressions in timeout handling or browser options precedence—add baseline timeout cases and an explicit precedence test forheadlessoverrides.
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/integration/observe-element-id-format.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/observe-element-id-format.test.ts:54">
P2: The mock LLM asserts against a hardcoded copy of the internal observe instruction sentence. Since that prompt text is an implementation detail (not a public API contract), a wording refactor would fail this test even though the element-ID round-trip behavior is correct. Consider asserting on the stable, intent-relevant part (e.g., that the prompt contains the target text and a bracketed `[0-N]` element ID) rather than the exact sentence, so the test guards behavior instead of internal copy.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:5">
P3: Adding `labeled`/`unlabeled` to the `pull_request` trigger means ANY label change now restarts this workflow, not just `safe-to-test` (GitHub `types` cannot be filtered by label name). Because the workflow has `concurrency.cancel-in-progress: true` keyed on `github.ref`, routine label churn — such as the documented `preview` label add/remove flow — will spawn a new run that cancels any in-progress CI for that PR and burns runner minutes on a run that mostly gets skipped at `determine-changes`. Consider whether the redundant run/cancel behavior on unrelated label events is acceptable, or move the `safe-to-test` gating so label events that don't introduce/remove the gate don't disrupt in-flight runs.</violation>
<violation number="2" location=".github/workflows/ci.yml:39">
P3: Changes to root TypeScript compiler settings can alter integration-test transpilation while this filter leaves the integration suite skipped. Include `tsconfig.json` in `integration` so those runtime checks still run.</violation>
</file>
<file name="packages/server/tests/stagehand-controller-timeout.test.ts">
<violation number="1" location="packages/server/tests/stagehand-controller-timeout.test.ts:64">
P3: The new controller timeout tests only cover the rejection path (timeout fires). They do not exercise the regression-sensitive baseline where `options.timeout` is omitted or non-positive, in which `withTimeout` passes the operation straight through. Adding a focused case that mocks a resolving service and `expect(await call).toEqual(result)` would lock in that the timeout wrap does not alter normal act/observe/extract behavior when no timeout is requested.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/_support.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/_support.ts:34">
P3: The default `headless: true` in `localBrowser.launch({ headless: true, ...options?.browser })` is placed before the caller-provided spread, so a future browser config that includes a `headless` field (or an explicit `headless: undefined`) will silently override the hermetic default, which could cause CI to run headed and behave differently than intended. Consider placing the spread first so any caller config is applied and then the harness's `headless: true` guarantee is reasserted last for hermetic tests.</violation>
</file>
Architecture diagram
sequenceDiagram
participant PR as External PR
participant GH as GitHub Actions CI
participant Det as determine-changes job
participant Disc as Integration discovery
participant Vite as Vitest runner
participant SDK as Stagehand SDK v4 tests
participant Fixture as Hermetic fixture server
participant Ctrl as Stagehand controller
participant Svc as act/observe/extract services
participant Snap as Hybrid snapshot capture
Note over PR,GH: CI security gate and semantic grouping
PR->>GH: pull_request event (labeled/synchronize)
GH->>Det: determine-changes job
alt external fork without safe-to-test label
Det-->>GH: job skipped (maintainer approval required)
else upstream PR or safe-to-test label
Det->>Det: CHANGED: integration path filter
Det-->>GH: integration=true
end
GH->>Disc: CHANGED: pnpm test:integration --list-groups
Disc->>Disc: NEW: groupIntegrationTests() validates unique ownership
alt valid grouping
Disc-->>GH: 10 semantic integration groups
else invalid grouping
Disc-->>GH: fail (missing/multiple/unknown test)
end
Note over GH,Vite: Sharded integration execution
GH->>Vite: CHANGED: INTEGRATION_PATHS env drives pnpm test:integration paths
Vite->>SDK: run v4 integration specs
SDK->>Fixture: NEW: createStagehand(options) + startFixtureServer(handler)
Fixture-->>SDK: hermetic fixture routes/responses
SDK->>SDK: page/locator interactions (click, keyboard, clipboard)
SDK->>Ctrl: act/observe/extract request
Ctrl->>Ctrl: NEW: withTimeout(serviceCall, options.timeout)
Ctrl->>Svc: delegate operation
Svc->>Snap: capture hybrid snapshot
Snap->>Snap: NEW: toWellFormed() sanitizes combinedTree/outline
alt operation completes before timeout
Snap-->>Svc: sanitized snapshot
Svc-->>Ctrl: result
Ctrl-->>SDK: result
else timeout fires
Ctrl-->>SDK: reject operation() timed out after Nms
end
SDK-->>Vite: assertions pass
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
8 issues found across 39 files
Confidence score: 2/5
- In
packages/server/dom/screenshotScripts/resolveMaskRect.ts, mask coordinates can be computed in the wrong coordinate space for transformed/filter/contained dialog/popover roots, so redaction boxes may land offset and leave sensitive content visible — preserve root-relative coordinates for those fixed-containing-block cases. - In
packages/server/understudy/screenshotUtils.ts, deep-mask matching currently narrows to the first matched element, which can miss other intended targets and leak unmasked secrets in captures — keep the default deep-mask locator unnarrowed and only narrow when explicitly requested (for example vianth()). packages/server/understudy/screenshotUtils.tscan hang screenshot capture on backgrounded tabs by awaiting an animation frame without a fallback, which can stall both capture and overlay cleanup — add a bounded timeout/fallback path so execution can continue when rAF is suspended.- Test reliability/coverage gaps in
packages/sdk-ts/tests/integration/locator-input-methods.test.tsandpackages/sdk-ts/tests/integration/user-data-dir.test.ts, plus missing targeted regressions for deep-mask/a11y timeout paths, reduce confidence in catching these regressions early — align visibility expectations with Playwright semantics and add focused polling/regression tests for the new behaviors.
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/understudy/a11y/snapshot/capture.ts">
<violation number="1" location="packages/server/understudy/a11y/snapshot/capture.ts:264">
P3: Scoped snapshot Unicode repair has no regression coverage; the existing test only covers merged frames. Add a focused `tryScopedSnapshot`/public snapshot test where `scopeApplied` is true and a malformed surrogate appears, asserting both `combinedTree` and `perFrame[0].outline` are well-formed.
(Based on your team's feedback about adding unit tests for new behavior.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-input-methods.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-input-methods.test.ts:104">
P2: The `#transparent` (opacity:0) case asserts `isVisible()` returns false, but the underlying Playwright-based isVisible treats opacity:0 elements as visible (visibility only considers non-empty bounding box and `visibility:hidden`). This assertion contradicts the visibility definition and would fail; the element is itself visible, so the expectation should be `true` (or the case should use `visibility:hidden` to exercise the hidden path).</violation>
</file>
<file name="packages/sdk-ts/tests/integration/user-data-dir.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/user-data-dir.test.ts:42">
P3: The assertion for `Local State` runs synchronously right after the poll confirms only `Default` exists, so it can flake on slow CI runners where the second profile artifact is written a moment later. Consider polling for both artifacts (e.g., Promise.allFulfilled over two polls, or a single poll checking both paths) so the test waits on the artifact it actually asserts.</violation>
</file>
<file name="packages/server/dom/screenshotScripts/resolveMaskRect.ts">
<violation number="1" location="packages/server/dom/screenshotScripts/resolveMaskRect.ts:61">
P1: Masks misalign in dialogs/popovers styled with `transform`, `filter`, or containment: viewport coordinates are applied relative to that root’s fixed-position containing block. Preserve root-relative coordinates for such roots, or use an absolutely positioned overlay relative to the root.</violation>
</file>
<file name="packages/server/understudy/screenshotUtils.ts">
<violation number="1" location="packages/server/understudy/screenshotUtils.ts:200">
P3: The newly supported deep-mask path has no regression coverage, so iframe-hop resolution and pixel redaction can silently break. Add focused screenshot cases for a deep locator and a narrowed `nth()` deep locator.
(Based on your team's feedback about adding unit tests for new behavior.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/server/understudy/screenshotUtils.ts:200">
P1: A deep mask matching multiple elements now covers only its first match, leaving other matched secrets visible in the screenshot. Preserve an unnarrowed Locator for default deep masks; narrow only when the caller explicitly uses `nth()`.</violation>
<violation number="3" location="packages/server/understudy/screenshotUtils.ts:269">
P2: Screenshots of a backgrounded tab can hang after inserting mask overlays because this awaits an animation frame with no fallback. Add a bounded fallback so capture and cleanup can proceed when frame callbacks are suspended.</violation>
</file>
<file name="packages/server/controllers/stagehandController.ts">
<violation number="1" location="packages/server/controllers/stagehandController.ts:65">
P3: When act()/observe()/extract() hit their timeout, the underlying service operation (including any in-flight LLM request) is not cancelled — only the caller-facing promise rejects while the background work continues to run. This means a timed-out LLM call can still incur cost/side effects after the client already received a timeout error. Consider noting this behavior or wiring an abort signal through the services if cancellation is desired.</violation>
</file>
Architecture diagram
sequenceDiagram
participant CI as GitHub Actions CI
participant Detect as determine-changes Job
participant Discover as Discover Integration Tests Job
participant Runner as Run Integration Shards Job
participant SDK as SDK TypeScript Tests
participant Page as Page API
participant Context as BrowserContext API
participant Stagehand as Stagehand Instance
participant Model as Client LLM Model
participant Fixture as Fixture Server
participant Snapshot as Snapshot Engine
participant Mask as Mask Overlay Engine
participant Timeout as withTimeout Wrapper
Note over CI,Runner: NEW: CI Integration Pipeline Changes
CI->>Detect: Pull request event (opened, labeled, synchronize)
alt Label event is NOT "safe-to-test"
Detect->>Detect: Skip CI execution
else Label is "safe-to-test" or non-label event
Detect->>Detect: Check head repo is internal or safe-to-test
Detect->>Detect: Determine changes using paths filter
alt Integration path changes detected
Detect-->>Discover: Pass integration=true
Discover->>Discover: pnpm test:integration --list-groups
Note over Discover: NEW: Group-based discovery replaces sharding
Discover->>Discover: Validate every test belongs to exactly one group
alt Missing group or duplicate test
Discover->>Discover: Throw error
end
Discover-->>Runner: Emit integration test groups matrix
loop For each test group
Runner->>Runner: mapfile -t integration_paths from matrix
Runner->>SDK: pnpm run test:integration -- {paths}
end
end
end
Note over SDK,Mask: NEW: Integration Test Fixtures & Page API
SDK->>Stagehand: createStagehand(options?)
Stagehand->>Stagehand: Launch local browser (headless by default)
Stagehand-->>SDK: Stagehand instance
Note over SDK,Fixture: CHANGED: Fixture server supports function handlers
SDK->>Fixture: startFixtureServer(routesOrHtml)
Fixture->>Fixture: Create HTTP server
alt routesOrHtml is string
Fixture->>Fixture: Map "/" to static HTML response
else routesOrHtml is function (handler)
Fixture->>Fixture: Map "*" to dynamic handler
end
Fixture-->>SDK: { url, close }
Note over SDK,Page: Test: click-count
Page->>Page: page.click() and locator.click() with clickCount option
Page->>Page: Dispatch single, double, and triple clicks
Page-->>SDK: Verify event.detail counts
Note over SDK,Context: Test: clipboard
Context->>Context: clipboard.writeText(), readText()
Context->>Context: paste() to focused textarea
Context->>Context: copy() and cut() selected text
alt Active page vs explicit page option
Context->>Context: Default to active page
else
Context->>Context: Respect page option parameter
end
Note over SDK,Context: Test: addInitScript (context-level)
Context->>Context: addInitScript(script, arg?)
loop Navigation to new pages
Page->>Context: Execute init script on DOMContentLoaded
alt Cross-process popup
Page->>Page: Inject via child process, survives reload
end
end
Note over SDK,Context: Test: setDomainPolicy
Context->>Context: setDomainPolicy({ blockedDomains, allowedDomains })
alt Blocked domains take precedence
Context->>Context: Block matching requests
else Allowed domains only
Context->>Context: Allow matching, block others
end
Note over SDK,Context: Test: setExtraHTTPHeaders
Context->>Context: setExtraHTTPHeaders(headers)
Page->>Fixture: Goto with custom headers
Fixture-->>Page: Echo request headers
Note over SDK,Page: Test: page.setExtraHTTPHeaders
alt Updated headers replace previous
Page->>Page: setExtraHTTPHeaders replaces, not merges
end
Note over SDK,Page: Test: keyboard
Page->>Page: keyPress(key), type(text), key sequencing
alt Modifier keys (Cmd, Shift, Ctrl+key)
Page->>Page: Dispatch key events with modifiers
else Invalid key chords
Page->>Page: Clear modifier state to avoid stuck keys
end
Note over SDK,Page: Test: locator content methods
Page->>Page: textContent(), innerText(), innerHtml(), inputValue()
alt Visibility differences
Page->>Page: textContent includes hidden elements
Page->>Page: innerText excludes display:none and visibility:hidden
end
Note over SDK,Page: Test: locator count, nth, selectOption
Page->>Page: count() returns matching element total
Page->>Page: nth(index) returns specific element
alt Nth out of bounds
Page->>Page: Throw Error
end
Page->>Page: selectOption(value) on <select> elements
Note over SDK,Page: Test: page addInitScript (scoped)
Page->>Page: addInitScript scoped to single page only
alt Other pages
Page->>Page: Not affected by page-scoped init script
end
Note over SDK,Page: Test: drag and drop
Page->>Page: dragAndDrop(fromX, fromY, toX, toY, options)
alt returnXpath true
Page-->>SDK: Return [fromXpath, toXpath]
else
Page-->>SDK: Return ["", ""]
end
Note over SDK,Snapshot: Test: observe element ID format
Stagehand->>Stagehand: observe(instruction)
Stagehand->>Snapshot: Capture accessibility snapshot
Snapshot->>Snapshot: Map elements with 0-ordinal IDs (0-1, 0-2, etc.)
Snapshot-->>Stagehand: Combined tree with element IDs
Stagehand->>Model: Send prompt with target text and element IDs
Model-->>Stagehand: Return element ID and action
Stagehand->>Page: act(selector)
Page-->>Stagehand: Success state
Note over SDK,Snapshot: CHANGED: Unicode sanitization in snapshots
Snapshot->>Snapshot: Capture per-frame outlines
alt Malformed Unicode (e.g. lone high surrogate)
Snapshot->>Snapshot: toWellFormed() repair on combined tree
Snapshot->>Snapshot: toWellFormed() repair on per-frame outlines
end
Snapshot-->>Stagehand: Well-formed hybrid snapshot
Note over SDK,Mask: Test: page screenshot with mask
Page->>Page: screenshot(options)
Page->>Mask: applyMaskOverlays(locators, color)
alt Locator is DeepLocatorDelegate
Mask->>Mask: await locator.real() to resolve underlying locator
end
Mask->>Mask: resolveMaskRect for top-layer elements (dialogs)
Note over Mask: CHANGED: Use viewport coordinates for top-layer masks
Mask->>Mask: Create overlay divs with position:fixed for top-layer
Mask-->>Page: Cleanup function to remove overlays
Note over SDK,Timeout: Test: operation timeouts
Stagehand->>Timeout: act / observe / extract with timeout option
Timeout->>Timeout: Create race between service promise and timeout
alt Service completes within timeout
Timeout-->>Stagehand: Return result
else Timeout expires
Timeout-->>Stagehand: Throw "timed out" error
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
3 issues found across 40 files
Confidence score: 3/5
- In
packages/server/controllers/stagehandController.ts, the operation timeout currently rejects the caller without canceling the underlyingact/observe/extractwork, so work can continue after the deadline and cause hidden side effects, resource contention, or overlapping operations—wire the timeout to an actual abort/cancellation path for the in-flight task. - In
packages/server/types/private/screenshot.ts,DeepLocatorDelegatemask handling can regress without detection because tests exercise a plainLocatorpath rather thanpage.deepLocator(...), which risks shipping broken cross-iframe mask resolution—add an integration case that usesdeepLocator(including all-iframe pages) to cover the real path. - In
packages/sdk-ts/tests/integration/page-screenshot.test.ts, duplicated pixel-validation code across screenshot tests makes future mask assertions easier to desynchronize and harder to maintain, increasing the chance of false confidence—extract a shared image/mask assertion helper and reuse it in both scenarios.
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/types/private/screenshot.ts">
<violation number="1" location="packages/server/types/private/screenshot.ts:20">
P3: Regression in `DeepLocatorDelegate` mask acceptance/resolution can reach release undetected: current coverage passes a `Locator`, not `page.deepLocator(...)`. Add a cross-iframe `deepLocator` mask case, including all-match or `nth()` behavior.
(Based on your team's feedback about adding unit tests for new behavior.)</violation>
</file>
<file name="packages/server/controllers/stagehandController.ts">
<violation number="1" location="packages/server/controllers/stagehandController.ts:53">
P2: The new operation-level timeout rejects the caller's promise at the deadline, but it does not abort the underlying act/observe/extract work — it only races against a timer and clears it in `finally`. So after a timeout the LLM call (and any server-side action it is mid-way through) keeps executing in the background: the failure is silently discarded by the Promise.race, yet token spend continues and, for `act()`, a real page action can still complete after the caller has been told the operation timed out. Consider threading a cancellation signal (e.g. an AbortSignal into the services / the LLM call) so the timeout actually cancels the work, and note the non-aborting behavior in the timeoutConfig code with a comment.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-screenshot.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-screenshot.test.ts:103">
P3: The pixel-analysis logic for verifying masks is duplicated between the "dialog top layer" test and the "deep-locator/nth()" test: both re-implement the base64→Image.decode→canvas→getImageData→raw-pixel-loop sequence (plus the `data:image/png;base64,...` data-URL construction). Since these mask tests are likely to grow, consider extracting a small shared helper (e.g. decode the screenshot bytes to a `Uint8ClampedArray` with the image dimensions, and optionally a `sampleCenter` helper) into the support file or a local module-level function so mask assertions stay consistent and the duplicated ~15 lines are not maintained in two places.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as SDK Client
participant Controller as StagehandController
participant Timeout as withTimeout
participant Service as act/observe/extract Service
participant Snapshot as Hybrid Snapshot Capture
participant LLM as Client LLM
participant Page as Understudy Page
participant Deep as DeepLocatorDelegate
participant Mask as applyMaskOverlays
participant Frame as CDP Frame
Note over Client,Frame: LLM operation timeout and snapshot Unicode repair
Client->>Controller: act/observe/extract with options.timeout
Controller->>Timeout: NEW: withTimeout(serviceCall, timeout, "observe()")
Timeout->>Service: run operation
Service->>Page: resolve page
Page->>Snapshot: capture hybrid snapshot
Snapshot->>Snapshot: CHANGED: toWellFormed() repairs malformed Unicode
Snapshot-->>Page: well-formed snapshot
Page-->>Service: snapshot + page context
Service->>LLM: generate()
LLM-->>Service: response
Service-->>Timeout: result
alt timeout fires
Timeout-->>Controller: reject "observe() timed out after Xms"
else operation completes
Timeout-->>Controller: result
end
Controller-->>Client: result or error
Note over Client,Frame: Deep-locator screenshot masking
Client->>Page: screenshot with mask deepLocator
Page->>Deep: locator.real()
Deep->>Deep: CHANGED: nthIndex=-1 keeps all matches
Deep-->>Page: resolved locator(s)
Page->>Mask: applyMaskOverlays(locators, color)
Mask->>Frame: evaluate resolveMaskRects
Frame-->>Mask: rects per frame
Mask->>Frame: inject overlays and wait for rAF paint
Frame-->>Mask: overlays painted
Mask-->>Page: cleanup tasks
Page-->>Client: screenshot bytes
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Confidence score: 2/5
- In
.github/workflows/ci.yml, thesafe-to-testflow can allowsynchronizeto run CI on new, unreviewed fork commits after approval, which creates a concrete trust-boundary/security risk in CI execution—bind approval to the exact commit/event or force re-approval by removing/re-adding the label on new pushes. - In
packages/server/dom/screenshotScripts/resolveMaskRect.ts,packages/server/understudy/deepLocator.ts, andpackages/server/types/private/screenshot.ts, the new mask-coordinate and deep-locator behavior lacks focused regression tests, so future changes could silently misalign masks or apply them to the wrong elements—add targeted cases for dialog/popover containing-block roots, defaultdeepLocatorvs.nth(n), and apage.deepLocator(...)screenshot mask path. - In
packages/sdk-ts/tests/integration/context-addInitScript.test.ts, the popup open/wait/verify sequence is duplicated three times, increasing maintenance overhead and the chance of inconsistent future edits—extract a shared helper to keep assertions and timing behavior consistent.
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/dom/screenshotScripts/resolveMaskRect.ts">
<violation number="1" location="packages/server/dom/screenshotScripts/resolveMaskRect.ts:58">
P3: This coordinate-order change has no focused regression coverage. Add cases for dialog/popover roots, including an attribute/style-dependent root, so masks remain aligned when the root establishes the containing block.
(Based on your team's feedback about adding unit tests for changed behavior.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/server/understudy/deepLocator.ts">
<violation number="1" location="packages/server/understudy/deepLocator.ts:118">
P3: The new unscoped default has no focused regression coverage. Add cases verifying a default `deepLocator` masks every match while `deepLocator(...).nth(n)` still masks only match `n`.
(Based on your team's feedback about unit tests for changed behavior.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-addInitScript.test.ts:179">
P3: The popup-wait-and-verify-injection sequence (capture knownTargetIds, click, waitForPopupPage, evaluate, reload, re-evaluate) is duplicated verbatim three times. Extracting a small helper (e.g. `openPopupAndAssertInjection(opener, selector, expected, { withReload })`) would make the three cross-process/in-process cases read as intent and remove the copy-paste, so future changes to popup flow only need to be made once.</violation>
</file>
<file name="packages/server/types/private/screenshot.ts">
<violation number="1" location="packages/server/types/private/screenshot.ts:20">
P3: Deep-locator masks have no regression coverage, so a future schema/type or `real()`-resolution change can silently break this newly supported path. Add a focused screenshot test with a `page.deepLocator(...)` mask, ideally crossing an iframe.
(Based on your team's feedback about unit tests for new behavior.) [FEEDBACK_USED]</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:36">
P1: A fork contributor can push arbitrary new commits after `safe-to-test` is applied, and later `synchronize` events execute CI for that unreviewed head. Tie approval to the exact commit/event, or require removing and reapplying the label after each push.
(Based on your team's feedback about maintainer approval gates for external PR workflows.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
9 issues found across 41 files
Confidence score: 3/5
- In
.github/workflows/ci.yml, the shared concurrency group can still replace a pending run when unrelated labels change, which can silently drop the latest CI signal for a PR and let regressions slip through—use a workflow-specific concurrency key so pending runs are not discarded by unrelated events. - In
packages/server/understudy/screenshotUtils.ts, resolving mask-overlay readiness after a singlerequestAnimationFramecan happen before the compositor commit, so screenshots may intermittently capture without masks—wait for a paint-safe point (for example a stronger frame/commit gate) and keep the timeout as fallback. packages/server/understudy/screenshotUtils.tsalso lacks deterministic coverage for the rAF-vs-timeout timing paths, so future changes could reintroduce flaky or incorrect masking without being caught—add focused tests that exercise both completion routes.- Several integration tests (
packages/sdk-ts/tests/integration/locator-nth.test.ts,context-addInitScript.test.ts,default-page-tracking.test.ts, andcontext-domain-policy.test.ts) rely on timing-sensitive or partially guarded assumptions, which raises CI flake risk and can obscure real failures—replace fixed sleeps/assumptions with state-based waits and guard the full page-listing + popup flow.
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/integration/user-data-dir.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/user-data-dir.test.ts:33">
P3: The page-navigation lines don't feed the assertion: `firstPage` + `page.goto("about:blank")` result in a `page` value that is never used, and Chrome writes the userDataDir profile structure at launch regardless of navigation. Dropping these lines leaves the test verifying the same behavior and removes dead/confusing setup.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:17">
P2: An unrelated label change can still discard a pending CI run for the same PR: `cancel-in-progress: false` preserves a running run but GitHub concurrency replaces the existing pending run in this shared group. Give ignored label events a separate concurrency group (for example, include `github.run_id` for non-`safe-to-test` label actions) so label churn cannot prevent the latest queued CI from starting.</violation>
</file>
<file name="packages/server/understudy/screenshotUtils.ts">
<violation number="1" location="packages/server/understudy/screenshotUtils.ts:226">
P3: Mask capture timing now depends on the rAF/fallback race, but this behavior has no deterministic regression coverage. Add focused tests for both rAF completion and the 100 ms fallback so screenshot masking does not regress silently.
(Based on your team's feedback about unit tests for new behavior.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/server/understudy/screenshotUtils.ts:269">
P3: The mask-overlay paint wait resolves after a single `requestAnimationFrame`, which fires *before* the compositor paints and commits the frame to the buffer CDP captures. Since the 100ms timeout only engages when rAF is delayed/throttled, an active page will typically capture after just one rAF, leaving the overlay's inclusion in the screenshot timing-dependent rather than guaranteed. Waiting for two rAFs (or a short delay) after appending the overlays would make the mask reliably visible in the captured frame.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-addInitScript.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-addInitScript.test.ts:38">
P3: In `waitForPopupPage`, only the `activePage()` call is guarded by the try/catch while the `ctx.pages()` call on the line above is not. During a cross-process popup the RPC to list pages can transiently reject (same churn the `activePage` guard exists for), which would abort the polling loop and fail the test instead of waiting out the transient state. Wrap both discovery calls in the same guarded block so the helper keeps polling.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/page-drag-and-drop.test.ts:118">
P3: This test asserts event.button === 0 on the `drop` handler after passing `{ button: "left" }`, but HTML5 drag-and-drop drop events always report button 0 regardless of the starting mouse button. As written the assertion passes no matter what the `button` option is set to, so it gives false confidence that the option is honored; consider removing the misleading name or asserting a behavior that actually differentiates the button option.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/context-domain-policy.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/context-domain-policy.test.ts:112">
P3: The popup assertion assumes `window.open()` returns a WindowProxy that the domain policy later closes. When a blocked domain makes the browser return `null` from `window.open()` instead, `__blockedPopup` stays null and `?.closed ?? false` is never true, so the test times out even though the popup was correctly not retained. Robustness suggestion: treat null as an equally valid pass (e.g., resolve true when the reference is null or closed) so the test verifies intent regardless of the open-then-close vs blocked-at-creation behavior.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/default-page-tracking.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/default-page-tracking.test.ts:53">
P3: This assertion can race on slow CI: server newPage() brings the new tab to front best-effort (errors swallowed) and activePage() resolves via Chrome's asynchronously-updated active target, so there is no guarantee the new page is already active when the RPC returns. For consistency with the popup tests and to de-flake, poll for the active-page transition before asserting, e.g. `const active = await waitForActivePage(stagehand.context, initial.pageId); expect(active?.pageId).toBe(created.pageId);`.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:112">
P2: This test introduces a fixed 100ms delay before asserting on the shadow-DOM buttons. Since the shadow root is attached synchronously by a script that runs during parsing, and the `page.goto(..., { waitUntil: "load" })` already resolves only after load, the sleep is unnecessary and makes the test timing-dependent and flaky under slow CI. The sibling `locator-count.test.ts` shadow-DOM case uses `expect.poll(...)` instead, which is the more robust pattern. Consider dropping the sleep (load wait is sufficient) or switching to `expect.poll` for consistency with the rest of the suite.</violation>
</file>
Architecture diagram
sequenceDiagram
participant GH as GitHub PR Event
participant CI as CI Pipeline
participant DET as determine-changes Job
participant DIS as Discover Tests Job
participant RUN as Run Integration Shard
participant SDK as SDK-ts (test fixture)
participant CONT as Controller
participant BO as Backend Operation
participant LLM as LLM (client stub)
participant CH as Chrome Browser
Note over GH,CH: PR: expand v4 integration coverage
GH->>CI: pull_request event (opened, labeled, etc.)
CI->>CI: Evaluate cancel-in-progress guard
alt label event & label != 'safe-to-test'
CI->>CI: Skip CI (no cancel)
else other PR events or safe-to-test label
CI->>CI: Allow concurrent runs
end
CI->>DET: Determine changes
DET->>DET: Check integration filter (SDK-ts, server, scripts, config)
alt Fork PR & no 'safe-to-test' label
DET-->>CI: Skip CI (external contributor gate)
else Main repo or labeled safe
DET->>DET: Set output integration=true
DET-->>CI: Proceed
end
CI->>DIS: Discover integration tests
DIS->>DIS: pnpm test:integration -- --list-groups
Note over DIS: NEW: switched from --list to --list-groups
DIS->>DIS: Group tests into 10 stable semantic groups
DIS-->>CI: Emit grouped matrix (e.g., local/input, local/locators-read)
loop For each integration group
CI->>RUN: Run integration shard
RUN->>RUN: mapfile + pnpm test:integration [paths]
RUN->>SDK: Execute test file(s)
Note over SDK,CH: Test lifecycle per file
SDK->>SDK: createStagehand(options?)
SDK->>SDK: Launch browser (localBrowser.launch)
SDK->>CH: headless browser instance
SDK->>SDK: Create Stagehand instance
alt Fixture server needed
SDK->>SDK: startFixtureServer(routes | handler | string)
Note over SDK: NEW: supports FixtureHandler callbacks
SDK->>SDK: createServer → handleRequest
end
SDK->>SDK: firstPage(stagehand)
SDK->>CH: navigate to test page (data: or fixture)
CH-->>SDK: Page loaded
alt Click-count tests
SDK->>CH: locator.click() or page.click(x,y)
CH->>CH: Dispatch click/dblclick events
CH-->>SDK: Verify event counts
else Clipboard tests
SDK->>CH: context.clipboard.writeText/readText/paste/copy/cut
CH-->>SDK: Clipboard operations via CDP
else addInitScript tests
SDK->>CH: context.addInitScript / page.addInitScript
SDK->>CH: Navigate or open popup
CH-->>SDK: Script runs before document scripts
else Domain policy tests
SDK->>CH: context.setDomainPolicy({blockedDomains / allowedDomains})
SDK->>CH: Navigate to target domain
alt Blocked domain
CH-->>SDK: chrome-error:// page
else Allowed domain
CH-->>SDK: Normal page load
end
else Extra HTTP headers tests
SDK->>CH: context.setExtraHTTPHeaders / page.setExtraHTTPHeaders
SDK->>CH: Navigate to fixture
SDK->>CH: Read request headers via fixture echo
else Locator content/input/nth tests
SDK->>CH: locator.textContent() / innerText() / innerHtml() / inputValue()
SDK->>CH: locator.fill() / type() / hover() / isVisible() / isChecked()
SDK->>CH: locator.nth(n) / locator.first()
CH-->>SDK: Return element state
else selectOption tests
SDK->>CH: locator.selectOption(value/label)
CH->>CH: Fire change event
CH-->>SDK: Verify selection
else Keyboard tests
SDK->>CH: page.keyPress(key + modifiers)
CH->>CH: Dispatch keydown/keyup/input
CH-->>SDK: Verify caret position & value
else Drag-and-drop tests
SDK->>CH: page.dragAndDrop(fromX, fromY, toX, toY)
CH->>CH: Dispatch dragstart/dragover/drop
CH-->>SDK: Verify drop status
else Scroll tests
SDK->>CH: page.scroll(x, y, deltaX, deltaY)
CH->>CH: Dispatch wheel event
CH-->>SDK: Verify scroll position
else Screenshot tests
SDK->>CH: page.screenshot(options)
Note over SDK: CHANGED: mask supports DeepLocatorDelegate
CH->>CH: Capture with mask/clip/caret
CH-->>SDK: Return screenshot bytes
SDK->>SDK: Inspect pixels (verify masks)
else waitForSelector tests
SDK->>CH: page.waitForSelector(selector, state/timeout)
CH->>CH: Poll DOM until state achieved
alt Timeout
CH-->>SDK: Throw TimeoutError
else Success
CH-->>SDK: Return true
end
else Snapshots (observe/act with LLM)
SDK->>CONT: stagehand.observe / stagehand.act / stagehand.extract
Note over CONT: CHANGED: operations wrapped in withTimeout()
CONT->>BO: call service function
alt Timeout provided
BO->>BO: Race promise against deadline
alt Timeout expires
BO-->>CONT: Throw error (operation timed out)
end
end
BO->>LLM: Generate LLM call
LLM-->>BO: Response with elementId/action
BO->>CH: Execute action (click/hover)
CH-->>BO: Action result
BO-->>CONT: Return result
CONT-->>SDK: Return data
else unicode-well-formed tests
SDK->>CONT: stagehand.observe with malformed Unicode
Note over CONT: CHANGED: toWellFormed() applied in capture.ts
CONT->>BO: Build snapshot
BO->>BO: Repair malformed Unicode in outline strings
BO-->>CONT: Well-formed snapshot
end
SDK->>SDK: closeStagehand(stagehand)
SDK->>CH: browser.close()
RUN->>RUN: Collect results (CTRF report)
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Confidence score: 4/5
- In
packages/sdk-ts/tests/integration/locator-nth.test.ts, the hard-codedsetTimeout(..., 100)can make the shadow DOM test timing-dependent even though creation happens beforeload, which risks avoidable CI flakes and hides true regressions—replace the sleep with an event/state-based wait tied to DOM readiness. - In
packages/sdk-ts/tests/integration/wait-for-selector.test.ts, assertingelapsed < 3_000uses wall-clock timing that can fail under CI contention despite correct timeout behavior, creating noisy failures and lower signal from the suite—assert on semantic timeout behavior with a looser or non-wall-clock bound. - In
packages/server/types/private/screenshot.ts, deep-locator mask behavior lost regression coverage, so iframe/hop locator resolution bugs in screenshot masking could ship unnoticed—add an integration test that passespage.deepLocator(...)viamaskand verifies masking is applied.
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/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:112">
P2: The `setTimeout(..., 100)` in the shadow DOM test is a hard-coded sleep. Since the shadow root is created by a synchronous inline <script> that runs before the `load` event, the `waitUntil: "load"` navigation already guarantees attachment, so this sleep is both unnecessary and timing-flaky. Prefer removing it (or using a deterministic locator wait / polling on `attachShadow` state) so the test does not depend on wall-clock timing.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-selector.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-selector.test.ts:202">
P2: The wall-clock assertion `expect(elapsed).toBeLessThan(3_000)` in "respects custom timeout duration" can flake on loaded CI lanes: the 500ms timeout resolving later than 3s is not a behavior failure, just resource starvation. Consider asserting only that elapsed is at least the configured timeout (~450ms) or poll for a bounded range around 500ms with looser margins, rather than an absolute upper cap that couples the test to machine speed.</violation>
</file>
<file name="packages/server/types/private/screenshot.ts">
<violation number="1" location="packages/server/types/private/screenshot.ts:20">
P3: Deep-locator masks now have no regression coverage, so failures resolving iframe/hop locators during screenshot masking can ship undetected. Add an integration case that passes `page.deepLocator(...)` in `mask` and verifies the masked output.
(Based on your team's feedback about adding unit tests for new behavior.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Confidence score: 3/5
- In
packages/server/dom/screenshotScripts/resolveMaskRect.ts,rectandrootRectcan be sampled from different DOM states ifdata-stagehand-mask-rootgeometry changes mid-flow, which can shift masks and produce visibly incorrect screenshots — capture both rectangles before any root mutation/observer side effects (or recompute both after mutation) to keep coordinates consistent. - In
packages/server/understudy/screenshotUtils.ts, the new render-wait branch lacks targeted regression tests, so masked captures could silently regress to firing before overlays paint — add focused tests for both the rAF callback path and the 100ms fallback. - In
packages/sdk-ts/tests/integration/locator-select-option.test.tsandpackages/sdk-ts/tests/integration/locator-nth.test.ts, repeated data-URL setup and an unnecessary fixed 100ms sleep increase maintenance and flake risk in CI — extract the shareddataUrl(html)helper and remove the hard-coded delay in favor of deterministic readiness.
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/dom/screenshotScripts/resolveMaskRect.ts">
<violation number="1" location="packages/server/dom/screenshotScripts/resolveMaskRect.ts:58">
P2: Masks can be offset when page CSS or an attribute observer changes root geometry for `data-stagehand-mask-root`: `rect` is from before that mutation but `rootRect` is from after it. Capture both rectangles before the temporary attribute mutation (or recapture both afterward) so the coordinate subtraction uses one layout state.</violation>
</file>
<file name="packages/server/understudy/screenshotUtils.ts">
<violation number="1" location="packages/server/understudy/screenshotUtils.ts:269">
P3: The new render-wait path has no focused regression coverage. A unit test for an rAF callback and one for the 100ms fallback would keep masked screenshots from regressing to capture before overlays render.
(Based on your team's feedback about adding unit tests for new behavior.) [FEEDBACK_USED].</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-select-option.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-select-option.test.ts:20">
P3: These two locator specs repeat the `"data:text/html," + encodeURIComponent(...)` goto boilerplate in nearly every test, whereas the sibling keyboard.test.ts in the same PR introduces a `dataUrl(html)` helper for the identical purpose. Extracting a shared `dataUrl()` (or `gotoHtml(page, html)`) helper would cut ~15 near-identical `page.goto` blocks and keep the suite consistent, making the tests easier to read and update. Since the helper lives in keyboard.test.ts today, the cleanest fix is to hoist it into `_support.ts` and reuse it across these specs.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:112">
P3: The shadow DOM is attached synchronously by the inline script before the page reaches `load` (which the `goto(waitUntil: "load")` already awaits), so the hard-coded `setTimeout(resolve, 100)` sleep is unnecessary and introduces a timing-based flake in CI. Prefer the deterministic `expect.poll(...)` pattern already used in the `locator-count.test.ts` shadow-DOM test, or rely on the locator action's own waiting, rather than a fixed sleep.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Confidence score: 4/5
- In
packages/sdk-ts/tests/integration/wait-for-timeout.test.ts, assertions tied to real wall-clock thresholds can become flaky under CI scheduling jitter, causing intermittent failures and masking true regressions in timeout behavior—de-risk by using fake timers, wider tolerances, or polling-based assertions that don’t depend on exact elapsed milliseconds. - In
packages/sdk-ts/tests/integration/click-count.test.ts, stale comments referring to a textarea while the fixture uses<div id="target">can mislead future test maintenance and debugging—update the spec wording to match the actual DOM fixture so intent stays clear.
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/integration/click-count.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/click-count.test.ts:63">
P3: Comments in this spec reference a "textarea", but the fixture's clickable element is a `<div id="target">` and there is no textarea anywhere in the HTML. This stale wording (left over from the ported Playwright test) is misleading for anyone reading the fixture; suggest updating the comments to reference the `#target` div.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/wait-for-timeout.test.ts:26">
P3: Two of these tests assert on real wall-clock timing (elapsed >= 190 after waitForTimeout(200)/waitForTimeout(50)x4, and an in-page setInterval incremented counter reaching >= 3 after 350ms). Under CI load, foreground/background tab throttling or a slow scheduler can delay those timeouts/intervals, making the run intermittently red even though the underlying API is correct. Consider driving these off a deterministic signal (e.g. a page-set counter or a fixture server response) instead of asserting on absolute elapsed time / interval counts, or at least loosening to a generous lower-bound/poll window.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
4 issues found across 40 files
Confidence score: 4/5
- In
scripts/test-integration.ts, CI discovery still skips the new semantic groups for SDK-only changes, so one-owner validation may never run and regressions can slip through unnoticed—wire discovery to--list-groupsso grouped suites execute in automation. - In
packages/sdk-ts/tests/integration/locator-nth.test.ts, the fixed 100ms sleep before shadow-root assertions can be too short on slow lanes, causing intermittent failures and noisy CI—replace the hard wait with a condition-based wait for shadow-root readiness. - In
packages/server/understudy/screenshotUtils.ts, overriddenrequestAnimationFramecan bypass the new paint synchronization path, making mask-render timing flaky in some pages—keep the timer fallback when the rAF call throws to preserve deterministic waiting. - In
packages/sdk-ts/tests/integration/_support.ts,createStagehandcurrently overwrites caller-providedbrowseroptions (includingheadless), which can hide configuration intent and block future test scenarios—merge options in the opposite order so caller settings are honored.
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/integration/_support.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/_support.ts:34">
P3: The new `createStagehand` helper exposes a `browser` option but then always forces `headless: true` after spreading the caller's options, so any caller that passes `browser: { headless: false }` (or any future option that implies a headed run) gets its value silently overridden. There is no test in this batch that passes `headless`, so the impact is currently zero, but this is a footgun for a shared helper that all integration specs reuse. Prefer spreading `headless` through or documenting that it is intentionally pinned.</violation>
</file>
<file name="packages/server/understudy/screenshotUtils.ts">
<violation number="1" location="packages/server/understudy/screenshotUtils.ts:278">
P3: Pages that override `requestAnimationFrame` can bypass the paint wait, making the new mask-render synchronization flaky. Preserve the timer fallback when the rAF call throws.</violation>
</file>
<file name="scripts/test-integration.ts">
<violation number="1" location="scripts/test-integration.ts:121">
P2: Integration CI still discovers individual entries and remains skipped for SDK-only changes, so the new semantic groups and their one-owner validation never run in automation. Wire the discovery job to `--list-groups` and include the SDK change condition when these groups are intended to provide the integration matrix.</violation>
</file>
<file name="packages/sdk-ts/tests/integration/locator-nth.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locator-nth.test.ts:112">
P2: This shadow-DOM test relies on a hard-coded 100ms sleep to wait for the shadow root to be attached before running its assertions. On slower or loaded CI lanes that pause can be exceeded, making the nth/textContent assertions flaky. The same PR already establishes a more robust pattern in `locator-count.test.ts` (`await expect.poll(() => locator.count()).toBe(2)`); consider using `expect.poll` or `page.waitForSelector` here as well so the test waits on an actual condition rather than a fixed timer.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 40 files
Architecture diagram
sequenceDiagram
participant Test as Integration Test (Vitest)
participant Support as _support.ts
participant Stagehand as Stagehand SDK
participant Page as Page (Understudy)
participant Locator as Locator / DeepLocatorDelegate
participant Controller as StagehandController
participant Timeout as withTimeout()
participant Service as Service (act/observe/extract)
participant Snapshot as Snapshot Capture
participant Screenshot as Screenshot Utils
Note over Test,Screenshot: NEW: Expanded integration test suite (31 files)
Test->>Support: createStagehand({ browser?, model? })
Support->>Stagehand: Launch browser & create Stagehand
Support-->>Test: Stagehand instance
Note over Test,Page: Click-count tests (locator & page)
Test->>Page: goto(doubleClickFixtureUrl)
Test->>Locator: click({ clickCount: 2 })
Locator->>Page: Dispatch click events
Page-->>Test: Verify click count
Note over Test,Page: Clipboard tests
Test->>Stagehand: context.clipboard.writeText("hello")
Stagehand->>Page: Write to clipboard
Test->>Stagehand: context.clipboard.readText()
Stagehand-->>Test: "hello"
Test->>Stagehand: context.clipboard.paste()
Stagehand->>Page: Paste into focused element
Note over Test,Page: addInitScript tests (context & page)
Test->>Stagehand: context.addInitScript(script)
Stagehand->>Page: Inject script
Page->>Page: Run on navigation
Test->>Stagehand: context.newPage()
Stagehand->>Page: Apply script to new page
Note over Test,Page: Domain policy tests
Test->>Stagehand: context.setDomainPolicy({ blockedDomains })
Stagehand->>Page: Block matching requests
alt Blocked domain
Page->>Page: Request blocked
Page-->>Test: Image fails to load
else Allowed domain
Page->>Page: Request allowed
Page-->>Test: Image loads
end
Note over Test,Page: Extra HTTP headers
Test->>Stagehand: context.setExtraHTTPHeaders()
Stagehand->>Page: Add headers
Test->>Page: goto(fixtureUrl)
Page->>FixtureServer: Request with custom headers
FixtureServer-->>Page: Echo headers
Page-->>Test: Verify header received
Note over Test,Page: Keyboard tests
Test->>Page: type("Hello World")
Test->>Page: keyPress("Cmd+A")
Test->>Page: keyPress("Delete")
Page-->>Test: Verify input cleared
Note over Test,Page: Screenshot with masking
Test->>Page: screenshot({ mask: [locator] })
Page->>Screenshot: applyMaskOverlays()
Screenshot->>Locator: resolve locators
alt DeepLocatorDelegate
Screenshot->>DeepLocatorDelegate: real() -> Locator
DeepLocatorDelegate->>Locator: Resolve all matches (nth: -1)
end
Locator->>Page: Create overlay elements
Page->>Screenshot: Capture with masks
Screenshot-->>Page: Cleanup masks (double rAF + fallback)
Page-->>Test: Screenshot bytes
Note over Test,Snapshot: Unicode well-formed snapshot
Test->>Stagehand: observe("Find banner text")
Stagehand->>Controller: Observe request
Controller->>Timeout: withTimeout(observe())
Timeout->>Service: observe()
Service->>Snapshot: tryScopedSnapshot()
Snapshot->>Snapshot: toWellFormed(outline) - repair malformed UTF-16
Snapshot->>Snapshot: mergeFramesIntoSnapshot()
Snapshot->>Snapshot: toWellFormed() on per-frame outlines
Snapshot-->>Service: Well-formed snapshot text
Service-->>Timeout: Result
Timeout-->>Controller: Result (or timeout error)
Controller-->>Stagehand: Observe result
Stagehand-->>Test: elementId in "0-N" format
Note over Test,Timeout: Operation timeout tests
Test->>Stagehand: observe("find", { timeout: 5 })
alt Timeout exceeds
Stagehand->>Controller: observe()
Controller->>Timeout: withTimeout(observe(), 5, "observe()")
Timeout-->>Controller: TimeoutError
Controller-->>Stagehand: Reject with timeout message
Stagehand-->>Test: "timed out" error
else No timeout
Controller->>Timeout: withTimeout(observe(), undefined)
Timeout->>Service: Pass through
Service-->>Test: Result
end
Note over Test,Page: Drag and drop tests
Test->>Page: dragAndDrop(fromX, fromY, toX, toY)
Page->>Page: Dispatch drag events
Page-->>Test: Verify drop success
Note over Test,Page: Default page tracking
Test->>Stagehand: context.newPage()
Stagehand->>Page: Create new page
Test->>Stagehand: context.activePage()
Stagehand-->>Test: Most recent page
Test->>Page: close()
Stagehand-->>Test: Previous page becomes active
Note over Test,Page: Text selector innermost matching
Test->>Page: locator("text=Click me")
Page->>Page: Match only innermost element
Page-->>Test: Single element result
Note over Test,Page: Wait-for-selector tests
Test->>Page: waitForSelector("#delayed-btn")
Page->>Page: Poll for element
alt Element appears
Page-->>Test: true
else Timeout
Page-->>Test: Timeout error
end
alt pierceShadow: true
Page->>Page: Search open shadow DOM
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
akeimach
approved these changes
Aug 3, 2026
miguelg719
force-pushed
the
codex/v4-integration-parity
branch
from
August 3, 2026 05:16
b8eb13c to
457e142
Compare
miguelg719
changed the title
miguelg719
force-pushed
the
codex/v4-integration-parity
branch
from
August 3, 2026 20:19
6bfd3a9 to
3d906c8
Compare
miguelg719
changed the base branch from
v4-spike
to
codex/v4-python-input-doc-types
August 4, 2026 19:15
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
2 participants
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.
Summary
Scope boundary
This PR is mechanical test transfer only. It has no packages/server production changes, generated extension update, or .github/workflows diff from v4-spike.
Three transferred specs that expose runtime gaps travel with their standalone fixes:
Together the stack grows the suite to 31 files. CI orchestration and external-contributor approval policy remain isolated in #2553.
The remaining main integration specs are not silently omitted: Agent/streaming/cache cases require the v4 Agent surface; Browserbase/CDP lifecycle, downloads, OOPIF, and connection cases require dedicated browser infrastructure; logger and FlowLogger cases target lifecycle code removed from v4.
Verification
Review order
#2551 → #2555 → #2556 → #2557 → #2553