Skip to content

[refactor]: use CDP for shadow root resolution#2032

Draft
seanmcguire12 wants to merge 13 commits intomainfrom
new-shadow-root-approach
Draft

[refactor]: use CDP for shadow root resolution#2032
seanmcguire12 wants to merge 13 commits intomainfrom
new-shadow-root-approach

Conversation

@seanmcguire12
Copy link
Copy Markdown
Member

@seanmcguire12 seanmcguire12 commented Apr 23, 2026

why

this pr replaces the old v3 shadow-piercing path with a cdp-based selector flow for closed shadow roots.

before this change, closed shadow root access depended on injected page-side code in files like packages/core/lib/v3/dom/piercer.runtime.ts and packages/core/lib/v3/understudy/piercer.ts.

after this change, selector resolution uses cdp to collect closed roots in packages/core/lib/v3/understudy/cdpClosedRoots.ts, then runs browser-side selector functions from packages/core/lib/v3/dom/selectorRuntime/index.ts without installing a persistent page global.

this avoids persistent window objects because cdp resolves real remote object handles for document, each closed shadow host, and each closed ShadowRoot. packages/core/lib/v3/understudy/selectorResolver.ts passes those handles into Runtime.callFunctionOn(...), and the browser-side helpers in packages/core/lib/v3/dom/selectorRuntime/index.ts build a temporary host-to-root map for that single call. no shadow state is stored on window, and nothing patches attachShadow.

what changed

  • added packages/core/lib/v3/understudy/cdpClosedRoots.ts

    • collectClosedShadowRoots(...) finds closed shadow roots with DOM.describeNode and resolves host/root handles with DOM.resolveNode
    • releaseRemoteObject(...) centralizes remote object cleanup for those handles
  • rewrote packages/core/lib/v3/understudy/selectorResolver.ts

    • resolveCss(...) and resolveText(...) now call queryElementsAcrossRoots(...)
    • resolveXPath(...) now goes through queryXPath(...)
    • countCss(...), countText(...), and countXPath(...) now use cdp-backed counting instead of the old injected locator invocation path
    • queryXPath(...) and countXPathMatches(...) choose between queryXPathNative / countXPathNative and queryXPathWithRoots / countXPathWithRoots based on whether hasOpenShadowRoots(...) or any closed roots are present
    • releaseClosedRootBundle(...) is used to clean up all host/root/document handles in one place
  • added packages/core/lib/v3/dom/selectorRuntime/index.ts

    • new shared browser-side functions for selector work:
      • queryCssWithRoots(...)
      • countCssWithRoots(...)
      • queryTextWithRoots(...)
      • countTextWithRoots(...)
      • queryXPathWithRoots(...)
      • countXPathWithRoots(...)
      • queryXPathNative(...)
      • countXPathNative(...)
      • hasOpenShadowRoots(...)
    • xpath parsing now reuses applyPredicates(...) and parseXPathSteps(...) from packages/core/lib/v3/dom/locatorScripts/xpathParser.ts
  • simplified packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts

    • resolveXPathAtIndex(...), countXPathMatches(...), and resolveXPathComposedMatches(...) now delegate to the shared selector runtime functions instead of carrying a separate xpath engine
  • updated packages/core/lib/v3/understudy/page.ts

    • waitForSelector(...) now uses FrameSelectorResolver for the shadow-aware path
    • when pierceShadow: false is passed, waitForSelector(...) uses a native-only in-page polling path so open and closed shadow dom are skipped
  • updated packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts

    • resolveObjectIdForXPath(...) and resolveObjectIdForCss(...) now use direct Runtime.evaluate calls in the frame's main world
    • the local helper names used there were cleaned up to resolveXPathSelector(...) and resolveCssSelectorDeep(...)
  • removed the old injected shadow-piercing path

    • deleted:
      • packages/core/lib/v3/understudy/piercer.ts
      • packages/core/lib/v3/understudy/locatorInvocation.ts
      • packages/core/lib/v3/dom/piercer.entry.ts
      • packages/core/lib/v3/dom/piercer.runtime.ts
      • packages/core/lib/v3/dom/rerenderMissingShadows.entry.ts
      • packages/core/lib/v3/dom/rerenderMissingShadows.runtime.ts
      • packages/core/lib/v3/dom/global.d.ts
      • packages/core/lib/v3/dom/index.ts
    • packages/core/lib/v3/understudy/context.ts no longer installs or preloads the piercer scripts
  • removed old selector helper files that are no longer used by the runtime

    • deleted:
      • packages/core/lib/v3/dom/locatorScripts/counts.ts
      • packages/core/lib/v3/dom/locatorScripts/selectors.ts
      • packages/core/lib/v3/dom/locatorScripts/waitForSelector.ts
  • updated packages/core/scripts/build-dom-scripts.ts

    • removed the old iife-string build path that was used for the piercer artifacts
    • added the new bootstrap-module target for selectorRuntime/index.ts

test plan

  • updated tests
    • packages/core/tests/integration/wait-for-selector.spec.ts was updated for the new pierceShadow: false behavior
    • packages/core/tests/unit/snapshot-a11y-resolvers.test.ts was updated to match the new helper naming

Summary by cubic

Replaced the v3 shadow-piercing path with a CDP-backed selector resolver for closed shadow DOM. This improves stability with handle cleanup, standardizes TimeoutError messages, and adds non-piercing text= support in waitForSelector.

  • New Features

    • CDP discovery of closed roots with ephemeral document/host/root handles; uses Promise.allSettled and cleans up partial objects.
    • New selectorRuntime for CSS/Text/XPath (native and composed) with hasOpenShadowRoots(...); XPath switches between native/composed based on open/closed roots.
    • Page.waitForSelector: pierceShadow: true uses FrameSelectorResolver; false uses native-only polling and supports text=; clearer TimeoutError messages.
    • A11y focus resolvers now use FrameSelectorResolver scoped to the target frame to return CSS/XPath objectIds.
  • Refactors

    • Removed injected piercer and legacy helpers; no preload in understudy/context.ts; deleted old DOM helpers and rerender scripts.
    • Removed dom/locatorScripts/xpathResolver.ts; XPath query/count now live in selectorRuntime.
    • Build script generates selectorRuntime.generated.ts and drops the IIFE build path.
    • Tests updated for CDP-based closed root discovery, pierceShadow: false behavior, a11y focus resolvers, and selectorRuntime XPath helpers.

Written for commit f76077f. Summary will update on new commits. Review in cubic

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 23, 2026

🦋 Changeset detected

Latest commit: f76077f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@browserbasehq/stagehand Patch
@browserbasehq/stagehand-evals Patch
@browserbasehq/stagehand-server-v3 Patch
@browserbasehq/stagehand-server-v4 Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@seanmcguire12
Copy link
Copy Markdown
Member Author

@cubic-dev-ai

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Apr 23, 2026

@cubic-dev-ai

@seanmcguire12 I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 22 files

Confidence score: 2/5

  • There is a concrete regression risk in packages/core/lib/v3/understudy/page.ts: !pierceShadow can pass text=, css=, or (-prefixed selectors into document.querySelector(), and the thrown error is swallowed, which can cause selector waits/lookups to fail in hard-to-debug ways.
  • Several high-confidence error-handling issues introduce generic/raw errors in packages/core/lib/v3/understudy/page.ts, packages/core/lib/v3/understudy/selectorResolver.ts, and packages/core/lib/v3/understudy/cdpClosedRoots.ts, which conflicts with typed/sanitized error requirements and raises integration/runtime consistency risk.
  • Pay close attention to packages/core/lib/v3/understudy/page.ts, packages/core/lib/v3/understudy/selectorResolver.ts, and packages/core/lib/v3/understudy/cdpClosedRoots.ts - selector parsing, sanitized exception flow, and CDP resource cleanup need tightening before merge.
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/core/lib/v3/understudy/page.ts">

<violation number="1" location="packages/core/lib/v3/understudy/page.ts:1326">
P1: The `!pierceShadow` inline expression doesn't handle `text=`, `css=`-prefixed selectors, or `(`-starting XPath expressions. These are silently passed to `document.querySelector()`, which throws, and the catch swallows the error — causing the call to poll until timeout with no useful diagnostic.

Consider reusing or mirroring `FrameSelectorResolver.parseSelector`'s prefix-stripping logic to keep both code paths consistent.</violation>

<violation number="2" location="packages/core/lib/v3/understudy/page.ts:1428">
P1: Custom agent: **Exception and error message sanitization**

`waitForSelector` introduces generic `new Error()` throws with raw selector values in messages, violating typed/sanitized error requirements.</violation>
</file>

<file name="packages/core/lib/v3/understudy/cdpClosedRoots.ts">

<violation number="1" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:49">
P2: Custom agent: **Exception and error message sanitization**

Generic `new Error()` is thrown in core code instead of a typed/sanitized custom error class as required.</violation>

<violation number="2" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:92">
P2: When one `DOM.resolveNode` succeeds but the other fails, `Promise.all` rejects and the catch block swallows the error—leaking the successfully-resolved remote object handle. Resolve them independently so you can release any handle that was allocated before continuing.</violation>
</file>

<file name="packages/core/lib/v3/understudy/selectorResolver.ts">

<violation number="1" location="packages/core/lib/v3/understudy/selectorResolver.ts:218">
P1: Custom agent: **Exception and error message sanitization**

New selector resolver CDP failures can bubble as raw unsanitized errors instead of typed/sanitized Stagehand errors.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant P as Page / Frame
    participant SR as FrameSelectorResolver
    participant CR as cdpClosedRoots
    participant CDP as CDP (Chrome DevTools Protocol)
    participant B as Browser (Page Context)

    Note over SR, B: NEW: CDP-Based Shadow Piercing Flow

    P->>SR: resolveSelector(selector, pierceShadow: true)
    
    SR->>CR: NEW: collectClosedShadowRoots(frame)
    CR->>CDP: DOM.describeNode(objectId: document, pierce: true)
    CDP-->>CR: Node tree (includes closed shadow roots)
    
    loop For each closed host/root pair
        CR->>CDP: NEW: DOM.resolveNode(backendNodeId)
        CDP-->>CR: RemoteObjectId (handle)
    end
    CR-->>SR: ClosedShadowRootBundle (handles)

    SR->>SR: NEW: select browser-side function (CSS/XPath/Text)
    
    SR->>CDP: CHANGED: Runtime.callFunctionOn(documentHandle)
    Note right of CDP: Arguments: [selector, ...hostRootPairs]
    
    CDP->>B: Execute selectorRuntime (injected bootstrap)
    B->>B: NEW: buildClosedRootsMap(pairs)
    B->>B: Query elements (piercing open + mapped closed roots)
    B-->>CDP: Return Element Handle(s) / Count
    
    CDP-->>SR: Response (RemoteObjectIds)

    SR->>SR: NEW: releaseClosedRootBundle()
    loop For each temporary handle (host/root/doc)
        SR->>CDP: Runtime.releaseObject(objectId)
    end

    SR-->>P: ResolvedNode(s)

    alt NEW: Page.waitForSelector (pierceShadow: false)
        P->>B: CHANGED: Runtime.evaluate (native-only polling)
        Note right of B: Skips all shadow DOM logic
    end
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread packages/core/lib/v3/understudy/page.ts Outdated
Comment thread packages/core/lib/v3/understudy/page.ts Outdated
let resultArrayId: Protocol.Runtime.RemoteObjectId | undefined;
try {
const called =
await this.frame.session.send<Protocol.Runtime.CallFunctionOnResponse>(
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Custom agent: Exception and error message sanitization

New selector resolver CDP failures can bubble as raw unsanitized errors instead of typed/sanitized Stagehand errors.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/selectorResolver.ts, line 218:

<comment>New selector resolver CDP failures can bubble as raw unsanitized errors instead of typed/sanitized Stagehand errors.</comment>

<file context>
@@ -108,260 +145,271 @@ export class FrameSelectorResolver {
+    let resultArrayId: Protocol.Runtime.RemoteObjectId | undefined;
+    try {
+      const called =
+        await this.frame.session.send<Protocol.Runtime.CallFunctionOnResponse>(
+          "Runtime.callFunctionOn",
+          {
</file context>
Fix with Cubic

Comment thread packages/core/lib/v3/understudy/cdpClosedRoots.ts Outdated
Comment thread packages/core/lib/v3/understudy/cdpClosedRoots.ts Outdated
@seanmcguire12
Copy link
Copy Markdown
Member Author

@cubic-dev-ai

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Apr 24, 2026

@cubic-dev-ai

@seanmcguire12 I have started the AI code review. It will take a few minutes to complete.

@seanmcguire12 seanmcguire12 changed the title [chore]: use CDP for shadow root resolution [refactor]: use CDP for shadow root resolution Apr 24, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 23 files

Confidence score: 3/5

  • There is some real merge risk: multiple medium-to-high severity findings (6–7/10 with high confidence) indicate concrete behavioral and error-handling problems rather than just style issues.
  • Most severe impact is user-facing error leakage in packages/core/lib/v3/understudy/page.ts and packages/core/lib/v3/understudy/cdpClosedRoots.ts, where typed exceptions include unsanitized selector/exception text that can expose runtime or CDP details.
  • Functional consistency issues may affect callers: countXPathNative in packages/core/lib/v3/dom/selectorRuntime/index.ts can overcount versus queryXPathNative, and visibility checks in packages/core/lib/v3/understudy/page.ts diverge between pierceShadow and non-pierceShadow paths.
  • Pay close attention to packages/core/lib/v3/understudy/cdpClosedRoots.ts, packages/core/lib/v3/understudy/page.ts, and packages/core/lib/v3/dom/selectorRuntime/index.ts - sanitize user-facing errors, ensure handle cleanup on throw paths, and align selector counting/visibility semantics.
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/core/lib/v3/understudy/cdpClosedRoots.ts">

<violation number="1" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:138">
P2: Remote object handles leak when `collectClosedShadowRoots` throws after acquiring `documentObjectId`. The catch block re-throws without releasing the document handle or any already-collected root/host handles in `roots`. Add cleanup before re-throwing to avoid accumulating leaked CDP remote objects.</violation>

<violation number="2" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:142">
P1: Custom agent: **Exception and error message sanitization**

User-facing typed error includes unsanitized underlying exception text, which can leak sensitive runtime/CDP details.</violation>
</file>

<file name="packages/core/lib/v3/dom/selectorRuntime/index.ts">

<violation number="1" location="packages/core/lib/v3/dom/selectorRuntime/index.ts:308">
P2: `countXPathNative` returns `snapshot.snapshotLength` which includes non-Element nodes, but `queryXPathNative` filters to `Element` instances only. This means `count` can report more matches than `query` actually returns for the same XPath. Either filter to Elements in the count, or use `XPathResult.ORDERED_NODE_SNAPSHOT_TYPE` and count only Element snapshot items.</violation>
</file>

<file name="packages/core/lib/v3/understudy/page.ts">

<violation number="1" location="packages/core/lib/v3/understudy/page.ts:1413">
P2: Visibility logic in the `!pierceShadow` inline expression diverges from `isElementVisible` used in the pierceShadow path. The inline check requires both `rect.width > 0` AND `rect.height > 0`, while `isElementVisible` only treats an element as invisible when `Math.max(width, height) === 0`. An element with one zero dimension (e.g. a horizontal rule with `height: 0`) would be considered invisible in the `!pierceShadow` path but visible in the `pierceShadow` path. Align the checks to avoid inconsistent `waitForSelector` behavior depending on the `pierceShadow` flag.</violation>

<violation number="2" location="packages/core/lib/v3/understudy/page.ts:1445">
P1: Custom agent: **Exception and error message sanitization**

`waitForSelector` throws a typed `TimeoutError` but leaks raw selector content in the user-facing error message instead of using a sanitized message.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant Client as SDK / User
    participant Page as Page / Frame
    participant Resolver as FrameSelectorResolver
    participant CDP as CDP Session (Server)
    participant Browser as Browser Runtime (Page)

    Note over Client,Browser: NEW: CDP-based Selector Resolution Flow (Closed Shadow DOM)

    Client->>Page: waitForSelector(".my-selector")
    Page->>Resolver: resolveFirst(selector)
    
    rect rgb(240, 240, 240)
        Note right of Resolver: NEW: Closed Shadow Root Discovery
        Resolver->>CDP: DOM.describeNode(document, depth: -1, pierce: true)
        CDP-->>Resolver: Node tree with backendNodeIds
        
        loop For each closed shadow host/root pair
            Resolver->>CDP: DOM.resolveNode(backendNodeId)
            CDP-->>Resolver: RemoteObjectId (host & root handles)
        end
    end

    rect rgb(230, 240, 255)
        Note right of Resolver: NEW: Ephemeral Runtime Execution
        Resolver->>CDP: Runtime.callFunctionOn(document)
        Note over Resolver,CDP: Arguments: [selector, ...hostRootHandles]
        
        CDP->>Browser: Execute injected selectorRuntime
        Browser->>Browser: Build temporary host-to-root map
        Browser->>Browser: Perform shadow-piercing CSS/XPath/Text query
        Browser-->>CDP: Array of matching Element handles
        CDP-->>Resolver: RemoteObjectIds
    end

    rect rgb(255, 240, 240)
        Note right of Resolver: NEW: Resource Cleanup
        Resolver->>CDP: Runtime.releaseObject(host/root handles)
        CDP-->>Resolver: OK
    end

    alt Element Found
        Resolver-->>Page: RemoteObjectId
        Page-->>Client: Success
    else Timeout / Not Found
        Resolver-->>Page: null
        Page->>Page: CHANGED: Retry or throw TimeoutError
    end
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread packages/core/lib/v3/understudy/page.ts
Comment thread packages/core/lib/v3/understudy/cdpClosedRoots.ts
Comment thread packages/core/lib/v3/understudy/cdpClosedRoots.ts
Comment thread packages/core/lib/v3/dom/selectorRuntime/index.ts Outdated
Comment thread packages/core/lib/v3/understudy/page.ts
@seanmcguire12
Copy link
Copy Markdown
Member Author

@cubic-dev-ai

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Apr 24, 2026

@cubic-dev-ai

@seanmcguire12 I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 23 files

Confidence score: 3/5

  • There is a concrete user-facing risk in packages/core/lib/v3/understudy/cdpClosedRoots.ts: raw underlying error text is propagated into StagehandDomProcessError, which conflicts with the stated sanitization requirement and could expose internal details.
  • Traversal logic in packages/core/lib/v3/understudy/cdpClosedRoots.ts appears fragile because the backendNodeId guard can block recursive shadow-root walking, which may cause missed nodes and inconsistent DOM processing behavior.
  • packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts may misclassify "fewer matches than target index" as an evaluation error, creating avoidable false-error paths when native XPath results are simply insufficient.
  • Pay close attention to packages/core/lib/v3/understudy/cdpClosedRoots.ts and packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts - error sanitization and shadow/XPath edge-case handling are the main merge risks.
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/core/lib/v3/understudy/cdpClosedRoots.ts">

<violation number="1" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:75">
P2: The `backendNodeId` guard inadvertently gates the recursive traversal into shadow roots. Move the `node.shadowRoots` iteration outside the `backendNodeId` check so subtree traversal always occurs, and only guard the `pairs.push` call that actually needs the host's `backendNodeId`.</violation>

<violation number="2" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:146">
P1: Custom agent: **Exception and error message sanitization**

Raw underlying error text is interpolated into a thrown `StagehandDomProcessError`, violating the sanitization requirement for user-facing errors.</violation>
</file>

<file name="packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts">

<violation number="1" location="packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts:54">
P2: Same pattern as `countXPathMatches`: `native.length > targetIndex` conflates "not enough matches" with "evaluation error". When no shadow DOM exists and the native evaluator found fewer matches than `targetIndex`, the old code returned `null`. The new code falls through to `queryXPathWithRoots`, which could return a spurious result from the custom XPath parser.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant P as Page / Frame
    participant SR as FrameSelectorResolver
    participant CDP as CDP (DOM / Runtime)
    participant JS as Browser Context (selectorRuntime)

    Note over P,JS: Shadow-Aware Selector Resolution Flow

    P->>SR: resolveFirst(selector, pierceShadow: true)
    
    rect rgb(240, 240, 240)
        Note right of SR: NEW: CDP-based Discovery
        SR->>CDP: collectClosedShadowRoots(frame)
        CDP->>CDP: DOM.describeNode (depth: -1, pierce: true)
        CDP-->>SR: Node tree with closedShadowRoots
        
        loop For each closed host/root pair
            SR->>CDP: DOM.resolveNode(backendNodeId)
            CDP-->>SR: hostObjectId, rootObjectId (handles)
        end
    end

    SR->>CDP: CHANGED: Runtime.callFunctionOn(documentObjectId)
    Note right of SR: Passes remote handles as arguments

    rect rgb(230, 245, 255)
        Note over JS: selectorRuntime execution
        JS->>JS: NEW: buildClosedRootsMap(handles)
        JS->>JS: queryCss/Text/XPathWithRoots()
        Note right of JS: Traverses DOM + Open/Closed ShadowRoots
    end

    CDP-->>SR: RemoteObjectId (Match)
    
    rect rgb(240, 240, 240)
        Note right of SR: NEW: Cleanup Handles
        SR->>CDP: releaseRemoteObject(host/root handles)
        SR->>CDP: releaseRemoteObject(resultArray)
    end

    SR-->>P: ResolvedNode (objectId)

    alt pierceShadow: false
        P->>JS: NEW: Native-only polling (no CDP overhead)
        JS->>JS: document.querySelector / findByText
        JS-->>P: boolean (settled)
    else pierceShadow: true (Polling)
        loop until timeout
            P->>SR: resolveFirst(...)
            SR-->>P: handle
            opt state is "visible"
                P->>CDP: Runtime.callFunctionOn(isElementVisible)
                CDP-->>P: boolean
            end
        end
    end
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

await releaseRemoteObject(frame, pair.rootObjectId);
}
if (error instanceof StagehandDomProcessError) throw error;
const message = error instanceof Error ? error.message : String(error);
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Custom agent: Exception and error message sanitization

Raw underlying error text is interpolated into a thrown StagehandDomProcessError, violating the sanitization requirement for user-facing errors.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/cdpClosedRoots.ts, line 146:

<comment>Raw underlying error text is interpolated into a thrown `StagehandDomProcessError`, violating the sanitization requirement for user-facing errors.</comment>

<file context>
@@ -0,0 +1,151 @@
+      await releaseRemoteObject(frame, pair.rootObjectId);
+    }
+    if (error instanceof StagehandDomProcessError) throw error;
+    const message = error instanceof Error ? error.message : String(error);
+    throw new StagehandDomProcessError(
+      `failed to collect closed shadow roots: ${message}`,
</file context>
Fix with Cubic

Comment thread packages/core/lib/v3/understudy/cdpClosedRoots.ts Outdated
Comment thread packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts Outdated
@seanmcguire12
Copy link
Copy Markdown
Member Author

@cubic-dev-ai

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Apr 28, 2026

@cubic-dev-ai

@seanmcguire12 I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 23 files

Confidence score: 3/5

  • There is some meaningful regression risk here: in packages/core/lib/v3/understudy/cdpClosedRoots.ts, raw caught error text is surfaced in a typed user-facing error, which can leak internal/CDP details and create confusing or unsafe error exposure.
  • packages/core/lib/v3/dom/selectorRuntime/index.ts mixes global document calls with functions typed as this: Document; using document.createTreeWalker/document.evaluate instead of this can produce incorrect behavior when called against non-global documents (for example iframe contexts).
  • The unused export in packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts looks low-severity housekeeping on its own, but the higher-severity document-context and error-sanitization issues keep this from being a low-risk merge.
  • Pay close attention to packages/core/lib/v3/understudy/cdpClosedRoots.ts and packages/core/lib/v3/dom/selectorRuntime/index.ts - sanitize surfaced errors and ensure all DOM APIs use the bound this: Document context consistently.
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/core/lib/v3/dom/selectorRuntime/index.ts">

<violation number="1" location="packages/core/lib/v3/dom/selectorRuntime/index.ts:278">
P2: Uses global `document.evaluate` instead of `this.evaluate`. Since the function binds `this: Document`, callers can pass a different document (e.g. an iframe's). Mixing the global `document` with a foreign context node throws `WRONG_DOCUMENT_ERR`.</violation>

<violation number="2" location="packages/core/lib/v3/dom/selectorRuntime/index.ts:322">
P2: Uses global `document.createTreeWalker` instead of `this.createTreeWalker`. Should use `this` consistently with the `this: Document` binding.</violation>
</file>

<file name="packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts">

<violation number="1" location="packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts:79">
P3: `resolveXPathComposedMatches` is exported but never imported or called anywhere in the codebase (including tests). Consider removing it to avoid maintaining dead code, or adding a test if it's intended as part of the public API.</violation>
</file>

<file name="packages/core/lib/v3/understudy/cdpClosedRoots.ts">

<violation number="1" location="packages/core/lib/v3/understudy/cdpClosedRoots.ts:149">
P1: Custom agent: **Exception and error message sanitization**

Raw caught error text is interpolated into a user-facing typed error, leaking unsanitized internal/CDP details.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant P as Page / Frame
    participant R as FrameSelectorResolver
    participant D as NEW: cdpClosedRoots
    participant CDP as Chrome DevTools Protocol
    participant B as Browser (Page Context)

    Note over P,B: Selector Resolution Flow (with Shadow Piercing)

    P->>R: resolveFirst(selector, pierceShadow: true)
    
    R->>D: NEW: collectClosedShadowRoots(frame)
    D->>CDP: DOM.describeNode(pierce: true, depth: -1)
    CDP-->>D: Node tree with BackendNodeIds
    
    loop For each closed shadow host/root pair
        D->>CDP: DOM.resolveNode(backendNodeId)
        CDP-->>D: RemoteObjectId (JS Handle)
    end
    D-->>R: ClosedShadowRootBundle (Handles)

    R->>CDP: CHANGED: Runtime.callFunctionOn(selector, ...handles)
    Note over CDP,B: Passes handles for Document, Hosts, and Roots
    
    B->>B: NEW: selectorRuntime.queryCssWithRoots()
    Note right of B: Reconstructs host-to-root map<br/>traverses shadow trees ephemeralally
    
    B-->>CDP: Result (Element RemoteObjectId)
    CDP-->>R: ResolvedNode Handle

    R->>D: NEW: releaseRemoteObject(bundle handles)
    D->>CDP: Runtime.releaseObject(objectId)

    R-->>P: Element Handle

    Note over P,B: Wait For Selector Flow (pierceShadow: false)

    alt pierceShadow: false
        P->>B: CHANGED: Runtime.evaluate(nativePollingScript)
        Note right of B: Uses native querySelector<br/>No shadow piercing or CDP discovery
        B-->>P: boolean (success/timeout)
    end

    Note over P,B: A11y / Snapshot Flow

    P->>CDP: CHANGED: Runtime.evaluate(idOnly serialization)
    Note right of B: resolveCssSelectorDeep() used<br/>instead of global piercer scripts
    CDP-->>P: Element ObjectId
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

if (error instanceof StagehandDomProcessError) throw error;
const message = error instanceof Error ? error.message : String(error);
throw new StagehandDomProcessError(
`failed to collect closed shadow roots: ${message}`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Custom agent: Exception and error message sanitization

Raw caught error text is interpolated into a user-facing typed error, leaking unsanitized internal/CDP details.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/cdpClosedRoots.ts, line 149:

<comment>Raw caught error text is interpolated into a user-facing typed error, leaking unsanitized internal/CDP details.</comment>

<file context>
@@ -0,0 +1,152 @@
+    if (error instanceof StagehandDomProcessError) throw error;
+    const message = error instanceof Error ? error.message : String(error);
+    throw new StagehandDomProcessError(
+      `failed to collect closed shadow roots: ${message}`,
+    );
+  }
</file context>

Comment thread packages/core/lib/v3/dom/selectorRuntime/index.ts Outdated
Comment thread packages/core/lib/v3/dom/selectorRuntime/index.ts Outdated
Comment thread packages/core/lib/v3/dom/locatorScripts/xpathResolver.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant