fix(extension): preserve native XPath predicates - #2706
Conversation
Signed-off-by: KXH <shepherdlaurie238@gmail.com>
🦋 Changeset detectedLatest commit: 303ed5f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 4/5
- In
packages/extension/dom/locatorScripts/xpathResolver.ts(thehasShadownative-vs-composed selection), preferring native XPath can changecount()and index resolution when matching tags exist in both light DOM and composed trees, which may cause subtle locator mismatches in shadow-heavy pages—add/adjust regression tests for mixed light+shadow cases and clarify/guard the preference rule.
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/extension/dom/locatorScripts/xpathResolver.ts">
<violation number="1" location="packages/extension/dom/locatorScripts/xpathResolver.ts:80">
P3: The new native-vs-composed preference in the `hasShadow` path is non-trivial — native XPath never pierces shadow roots, so preferring it changes `count()`/index results whenever a tag appears in both light and composed trees, and the index-zero and `position()` semantics differ between the native engine and the subset parser. None of this rationale is documented next to the code; only the parser's docstring mentions it. Add an inline comment explaining the ordering (shadow-hop → native light-DOM → composed fallback), why native wins, and that it intentionally does not merge shadow matches, so future edits don't silently regress it.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Test/User Code
participant SDK as SDK-TS (locatorCount.test.ts)
participant Ext as Stagehand Extension
participant Parser as XPath Parser
participant Resolver as XPath Resolver
participant Native as Native XPath (Browser)
participant Composed as Composed-Tree Fallback
Note over Client,Composed: XPath Resolution with Shadow Root Present
Client->>SDK: locator("xpath=//div[position() > 1]").count()
SDK->>Ext: resolveXpath(xpath, targetIndex)
Ext->>Parser: parseXPathSteps(xpath)
Parser-->>Ext: steps with predicates (including "unsupported" type for position())
Note over Ext,Resolver: Primary path: try native first
Ext->>Resolver: resolveXPathAtIndex(xpath, index)
Resolver->>Native: document.evaluate(xpath, ...)
alt Native succeeds and returns >0 results
Native-->>Resolver: XPathResult with snapshotLength > 0
Resolver-->>Ext: Element from native result
Ext-->>SDK: Native match (preserves predicate semantics)
SDK-->>Client: count = 2 (position() > 1 filters correctly)
else Native returns 0 or errors
Native-->>Resolver: 0 results or throws
Resolver-->>Composed: Fall back to composed-tree lookup
Composed->>Parser: evaluatePredicate(element, predicate)
alt predicate type is "unsupported"
Parser-->>Composed: return false (no match)
Composed-->>Resolver: Empty results
Resolver-->>Ext: null (no element found)
else predicate type is "index" with value 0
Parser-->>Composed: return false (preserves zero-index semantics)
Composed-->>Resolver: Empty results
Resolver-->>Ext: null
end
Ext-->>SDK: No match (or fewer matches than expected)
SDK-->>Client: count = 0 for unsupported/index=0 predicates
end
Note over Client,Composed: Fallback path when native fails entirely
Client->>SDK: locator("xpath=//div[0]").count()
SDK->>Ext: resolveXpath(xpath, targetIndex)
Ext->>Parser: parseXPathSteps("//div[0]")
Parser-->>Ext: step with { type: "index", index: 0 }
Ext->>Resolver: countXPathMatches(xpath)
Resolver->>Native: document.evaluate("//div[0]", ...)
alt Native succeeds (could match if predicate is valid)
Native-->>Resolver: XPathResult with results
Resolver-->>Ext: native count (may include index-zero behavior)
else Native errors out
Native-->>Resolver: throws (e.g., malformed query)
Resolver-->>Composed: countXPathComposed(xpath)
Composed->>Parser: evaluatePredicate for each element
alt predicate type "index" with value 0
Parser-->>Composed: return false (never matches)
Composed-->>Resolver: count = 0
Resolver-->>Ext: count = 0
end
Ext-->>SDK: count = 0
SDK-->>Client: count resolves to 0 (correctly excluding index 0)
end
Note over Client,Composed: Integration test with shadow root
Client->>SDK: locator("xpath=//div[position() > 1]").first().textContent()
SDK->>Ext: resolveXpath with index 0 (for first())
Ext->>Resolver: resolveXPathAtIndex(xpath, 0)
Resolver->>Native: document.evaluate with position()
alt Native succeeds with shadow root present
Native-->>Resolver: Returns correct Element (A2)
Resolver-->>Ext: Element <div>B2</div>
Ext-->>SDK: Element
SDK-->>Client: textContent = "B2" (position() > 1 semantics preserved)
else Native returns 0 (unrelated shadow root interferes)
Native-->>Resolver: 0 matches (shadow root causes native failure)
Resolver-->>Composed: Fallback
Composed->>Parser: evaluatePredicate for "unsupported" type
Parser-->>Composed: return false for all elements
Composed-->>Resolver: count = 0
Resolver-->>Ext: null
Ext-->>SDK: No match
SDK-->>Client: Error or null (test expects failure)
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const shadowHopCount = resolveStagehandShadowHopMatches(xp, shadowCtx.getShadowRoot).length; | ||
| if (shadowHopCount > 0) return shadowHopCount; | ||
|
|
||
| const native = resolveNativeCountWithError(xp); |
There was a problem hiding this comment.
P3: The new native-vs-composed preference in the hasShadow path is non-trivial — native XPath never pierces shadow roots, so preferring it changes count()/index results whenever a tag appears in both light and composed trees, and the index-zero and position() semantics differ between the native engine and the subset parser. None of this rationale is documented next to the code; only the parser's docstring mentions it. Add an inline comment explaining the ordering (shadow-hop → native light-DOM → composed fallback), why native wins, and that it intentionally does not merge shadow matches, so future edits don't silently regress it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/dom/locatorScripts/xpathResolver.ts, line 80:
<comment>The new native-vs-composed preference in the `hasShadow` path is non-trivial — native XPath never pierces shadow roots, so preferring it changes `count()`/index results whenever a tag appears in both light and composed trees, and the index-zero and `position()` semantics differ between the native engine and the subset parser. None of this rationale is documented next to the code; only the parser's docstring mentions it. Add an inline comment explaining the ordering (shadow-hop → native light-DOM → composed fallback), why native wins, and that it intentionally does not merge shadow matches, so future edits don't silently regress it.</comment>
<file context>
@@ -74,6 +77,9 @@ export function countXPathMatches(rawXp: string, options?: XPathResolveOptions):
const shadowHopCount = resolveStagehandShadowHopMatches(xp, shadowCtx.getShadowRoot).length;
if (shadowHopCount > 0) return shadowHopCount;
+ const native = resolveNativeCountWithError(xp);
+ if (!native.error && native.count > 0) return native.count;
+
</file context>
|
Thanks for picking this up, and for going at the parser instead of around it. Four things I would want checked before this lands, roughly in the order they worry me. 1. The new integration test uses a 2. Native-first gated on 3. 4. Minor: the parser test passes Happy to run any of this against my repro if it helps. |
|
On point 4, to be concrete instead of just critical: I would rather grouping did not get pulled into this PR. It is a different code path. I will send it as its own PR against |
Merge native light-DOM results with shadow-root matches, surface unsupported composed predicates, preserve grouped XPath expressions, and exercise the HTTP failure path. Signed-off-by: KXH <shepherdlaurie238@gmail.com>
|
Thanks for the careful repro review. All four points were valid, and I have updated the PR in
I also added the positive If you can rerun the updated branch against your original repro, that would be very helpful. |
|
Grouping is covered here, so there is no separate PR coming from me. Thanks for turning it around this fast. The merged ordering reads right: native for light-DOM semantics, composed contributing only shadow-rooted elements, sorted by composed traversal. |
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 8 files (changes from recent commits).
Confidence score: 2/5
- In
packages/extension/dom/locatorScripts/xpathParser.ts,resolveXPathAtIndexcan throw in the shadow-root branch even when native XPath already found valid light-DOM results, which risks turning otherwise successful lookups into hard failures on pages that include shadow DOM — guard the composed-match evaluation and fall back to native matches when parsing/composition fails. - In
packages/extension/dom/locatorScripts/xpathResolver.ts,mergeXPathMatchescombines native and composed-fallback results that apply index predicates differently, so selectors like//div[2]can resolve to inconsistent elements and cause subtle mis-targeting — align predicate semantics before unioning, or gate merging to semantically equivalent cases. - In
packages/sdk-ts/tests/integration/locatorCount.test.ts, the current(//div)[2]check validates grouping behavior but not the shadow-fallback path tied toparseXPathSteps, leaving the regression risk under-covered — add a test that starts with/or//and exercises shadow-root fallback indexing directly.
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/locatorCount.test.ts">
<violation number="1" location="packages/sdk-ts/tests/integration/locatorCount.test.ts:73">
P3: The `(//div)[2]` assertion tests XPath grouping, a different code path from the shadow-root fallback this PR addresses. As discussed in review, `parseXPathSteps` only parses selectors starting with `/` or `//`, so a leading `(` never reaches the composed-tree traversal regardless of whether a shadow root exists — this selector's behavior is independent of this bug fix. Keep the shadow-root regression assertions here and move the grouping case to a dedicated PR for `xpathParser.ts` so a failure here isn't attributed to the fallback change.</violation>
</file>
<file name="packages/extension/dom/locatorScripts/xpathParser.ts">
<violation number="1" location="packages/extension/dom/locatorScripts/xpathParser.ts:385">
P1: When a page has any shadow root, this throw can abort XPath resolution even if native XPath already produced valid light-DOM matches. `resolveXPathAtIndex` evaluates composed matches unguarded in the shadow-root branch, so unsupported predicates like `position()` now bubble an exception and callers return null/throw instead of native results. Catch composed-fallback errors and preserve native results when native evaluation succeeded.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| export function evaluatePredicate(element: Element, predicate: XPathPredicate): boolean { | ||
| switch (predicate.type) { | ||
| case "unsupported": | ||
| throw new Error( |
There was a problem hiding this comment.
P1: When a page has any shadow root, this throw can abort XPath resolution even if native XPath already produced valid light-DOM matches. resolveXPathAtIndex evaluates composed matches unguarded in the shadow-root branch, so unsupported predicates like position() now bubble an exception and callers return null/throw instead of native results. Catch composed-fallback errors and preserve native results when native evaluation succeeded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/extension/dom/locatorScripts/xpathParser.ts, line 385:
<comment>When a page has any shadow root, this throw can abort XPath resolution even if native XPath already produced valid light-DOM matches. `resolveXPathAtIndex` evaluates composed matches unguarded in the shadow-root branch, so unsupported predicates like `position()` now bubble an exception and callers return null/throw instead of native results. Catch composed-fallback errors and preserve native results when native evaluation succeeded.</comment>
<file context>
@@ -382,7 +382,9 @@ function normalizeMaybe(value: string, normalize?: boolean): string {
switch (predicate.type) {
case "unsupported":
- return false;
+ throw new Error(
+ `Unsupported XPath predicate in composed-tree traversal: ${predicate.source}`,
+ );
</file context>
|
|
||
| await expect(page.locator("xpath=//div[2]").count()).resolves.toBe(2); | ||
| await expect(page.locator("xpath=//div[0]").count()).resolves.toBe(0); | ||
| await expect(page.locator("xpath=(//div)[2]").count()).resolves.toBe(1); |
There was a problem hiding this comment.
P3: The (//div)[2] assertion tests XPath grouping, a different code path from the shadow-root fallback this PR addresses. As discussed in review, parseXPathSteps only parses selectors starting with / or //, so a leading ( never reaches the composed-tree traversal regardless of whether a shadow root exists — this selector's behavior is independent of this bug fix. Keep the shadow-root regression assertions here and move the grouping case to a dedicated PR for xpathParser.ts so a failure here isn't attributed to the fallback change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk-ts/tests/integration/locatorCount.test.ts, line 73:
<comment>The `(//div)[2]` assertion tests XPath grouping, a different code path from the shadow-root fallback this PR addresses. As discussed in review, `parseXPathSteps` only parses selectors starting with `/` or `//`, so a leading `(` never reaches the composed-tree traversal regardless of whether a shadow root exists — this selector's behavior is independent of this bug fix. Keep the shadow-root regression assertions here and move the grouping case to a dedicated PR for `xpathParser.ts` so a failure here isn't attributed to the fallback change.</comment>
<file context>
@@ -52,22 +60,33 @@ describe("Locator count() method tests", () => {
- ),
+ await expect(page.locator("xpath=//div[2]").count()).resolves.toBe(2);
+ await expect(page.locator("xpath=//div[0]").count()).resolves.toBe(0);
+ await expect(page.locator("xpath=(//div)[2]").count()).resolves.toBe(1);
+ await expect(page.locator("xpath=//div[position() > 1]").count()).rejects.toThrow(
+ "Unsupported XPath predicate in composed-tree traversal: position() > 1",
</file context>
Keep native positional semantics when light-DOM matches exist, sanitize unsupported-predicate errors at the CDP boundary, and retain null/zero recovery for transient runtime failures. Signed-off-by: KXH <shepherdlaurie238@gmail.com>
5b660b3 to
adbc1ca
Compare
|
I reviewed the second Cubic pass against the resolver behavior and pushed The valid findings are addressed as follows:
Verification on the updated files:
I kept the grouped-expression regression in this PR because the issue author confirmed it is covered here and no separate PR is planned. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Distinguish successful empty native evaluation from native errors before considering composed traversal, and cover an unrelated shadow root with a zero-match last() selector. Signed-off-by: KXH <shepherdlaurie238@gmail.com>
60b789a to
303ed5f
Compare
|
The zero-native-match finding is valid and is fixed in
The HTTP fixture now covers that zero-match case and also reflects the final native-first rule for |
2 participants
Summary
Why
An unrelated shadow root currently switches every XPath locator to Stagehand's subset parser. Predicates such as
position()andlast()are then silently ignored, so the same locator can count or click different elements depending on unrelated page content.This keeps composed-tree lookup for selectors that need shadow traversal while preserving native XPath semantics whenever the browser can resolve a light-DOM match.
Testing
oxfmt --checkon all changed filesoxlinton all changed TypeScript filespackages/sdk-ts/tests/integration/locatorCount.test.tswith the new HTTP fixture; all 8 tests, including unchanged baselines, reach browser startup but are blocked locally because Chrome cannot resolve the extension source path under the workspace's non-ASCII parent directory (File path cannot be resolved). The HTTP and mixed light/shadow cases are included for CI on GitHub's ASCII workspace.Closes #2693
Summary by cubic
Preserves native XPath predicate semantics when pages include unrelated shadow roots. Previously the composed-tree fallback ignored positional predicates and treated index 0 as the first match; now grouped XPaths like
(//div)[2]and any path with positional/unsupported predicates evaluate natively, and unsupported predicates in composed traversal throw.(...)paths natively; if predicates are positional or unsupported, prefer native results; otherwise merge native light‑DOM results with shadow‑root matches, dedupe, and order by composed‑tree document order; distinguish empty native results from native errors before fallback.unsupportedpredicate that throws in composed traversal; keep XPath index‑zero semantics (index 0 matches nothing).evaluateXPathElement; sanitize “Unsupported XPath predicate in composed-tree traversal” at the CDP boundary; transient main‑world errors still return null/0.Migration
@browserbasehq/stagehand.Written for commit 303ed5f. Summary will update on new commits.