fix(browser): resolve act targetId aliases before mismatch check#96178
Conversation
The /act top-level and batch targetId guards compared the caller-supplied targetId against the resolved canonical tab.targetId with raw string equality. Any supported alias form (tabId, label, suggestedTargetId, or a unique id prefix) resolves to a different canonical id, so act requests that followed the documented 'prefer suggestedTargetId/tabId/label' guidance were rejected with 403 ACT_TARGET_ID_MISMATCH even though they named the correct tab. snapshot/open/close/tabs lack this guard and kept working, matching the reported symptom matrix. Resolve the action targetId through the same tab alias resolution the route used and reject only ids that resolve to a different tab.
|
Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 4:06 PM ET / 20:06 UTC. Summary PR surface: Source +7, Tests +101. Total +108 across 4 files. Reproducibility: yes. Source inspection shows current main and v2026.6.11 resolve the route tab through alias-aware selection, then raw-compare Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land this focused browser Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main and v2026.6.11 resolve the route tab through alias-aware selection, then raw-compare Is this the best way to solve the issue? Yes. Canonicalizing aliases at the AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a04b6ced4f96. Label changesLabel justifications:
Evidence reviewedPR surface: Source +7, Tests +101. Total +108 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (5 earlier review cycles)
|
…patch The /act gate accepted tabId/label/suggested/prefix aliases of the request tab but left them on action.targetId. The managed executor reads action.targetId ?? targetId for an exact page lookup (executeSingleAction -> getPageForTargetId), so an alias missed the lookup and broke the action at runtime whenever more than one page was open (single-page masked it via the pages.length===1 fallback). Canonicalize the action targetId (top-level and nested batch sub-actions) to the resolved tab id before dispatch; reject ids that resolve to a different tab. Replace the mock-masked contract assertions with executor-action assertions plus direct canonicalizer unit tests.
|
The prior review (🧂 / "patch is incorrect") was run against the first commit
Real browser proof (the requested behavior proof)Verified against a real headless Chrome (148) over CDP with two real tabs, driving the production functions the review cited — BEFORE: an un-canonicalized prefix alias fails the real Playwright exact-id lookup ( @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
|
Maintainer verification complete on I refactored the original callback/result/copy implementation into route-local canonicalization, reduced the production change from +55 to +7 net lines, and added a regression for ambiguous nested target prefixes. The final route resolves documented tab aliases against the selected managed-browser tab, rejects aliases that are not globally unique, and leaves the Playwright executor's exact-target invariant intact. Proof:
No known proof gaps remain. |
|
Merged via squash.
|
…nclaw#96178) * fix(browser): resolve act targetId aliases before mismatch check The /act top-level and batch targetId guards compared the caller-supplied targetId against the resolved canonical tab.targetId with raw string equality. Any supported alias form (tabId, label, suggestedTargetId, or a unique id prefix) resolves to a different canonical id, so act requests that followed the documented 'prefer suggestedTargetId/tabId/label' guidance were rejected with 403 ACT_TARGET_ID_MISMATCH even though they named the correct tab. snapshot/open/close/tabs lack this guard and kept working, matching the reported symptom matrix. Resolve the action targetId through the same tab alias resolution the route used and reject only ids that resolve to a different tab. * fix(browser): canonicalize act targetId aliases before Playwright dispatch The /act gate accepted tabId/label/suggested/prefix aliases of the request tab but left them on action.targetId. The managed executor reads action.targetId ?? targetId for an exact page lookup (executeSingleAction -> getPageForTargetId), so an alias missed the lookup and broke the action at runtime whenever more than one page was open (single-page masked it via the pages.length===1 fallback). Canonicalize the action targetId (top-level and nested batch sub-actions) to the resolved tab id before dispatch; reject ids that resolve to a different tab. Replace the mock-masked contract assertions with executor-action assertions plus direct canonicalizer unit tests. * test(browser): brace act-targetId guard clauses for curly lint * docs(changelog): note browser target alias fix * docs(changelog): note browser target alias fix * fix(browser): reject ambiguous batch target aliases * test(browser): type targetless act fixture * docs(changelog): move browser alias fix to unreleased * chore: drop nonessential browser changelog entry --------- Co-authored-by: Peter Steinberger <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>
What Problem This Solves
Fixes #96176. After interacting with a tab, every
browser actoperation(
evaluate,click,wait,select,fill,drag, ...) could fail with:403 action targetId must match request targetId(ACT_TARGET_ID_MISMATCH)while
snapshot,open,close, andtabskept working — exactly thesymptom matrix in the report.
Why This Change Was Made
The
/actroute resolves the request tab from the caller-suppliedtargetIdvia
ensureTabAvailable, which accepts any documented alias form —tabId,label,suggestedTargetId, the raw CDP id, or a unique id prefix(
browser-tool.schema.tsliterally tells the model to prefersuggestedTargetId/tabId/label). Resolution returns the canonicaltab.targetId.Both targetId guards then re-checked the request with raw string equality:
action.targetId !== tab.targetId(agent.act.ts)validateBatchTargetIds(actions, tab.targetId)Whenever the caller used an alias (the recommended path), the raw alias never
equals the resolved canonical id, so the guard returned 403 even though the
request named the correct tab.
snapshot/open/close/tabshave no suchguard, so they were unaffected — matching the reporter's table.
The fix resolves the action's targetId through the same tab alias resolution
the route already used, and rejects only ids that resolve to a different
tab. Malformed ids (e.g. a non-string that route selection drops but action
normalization stringifies) still correctly fail, since they resolve to no tab.
User Impact
browser actworks again when callers reference a tab bysuggestedTargetId,tabId,label, or a unique id prefix — including the form the tool schemarecommends. No config or API surface changes; genuine cross-tab drift in
batched actions is still rejected.
Evidence
Two new contract tests in
server.agent-contract-core.test.tsexercise thereal route + server:
act clickwith a unique prefix (abcd→ canonicalabcd1234)now returns
200and dispatches to the canonical target.Before/after on the unmodified production code (prod fix stashed):
Run via
node scripts/run-vitest.mjs extensions/browser/src/browser/server.agent-contract-core.test.ts.The existing
ACT_TARGET_ID_MISMATCHtests (non-string targetId, cross-tabbatch override) still pass unchanged.
Real browser proof (Chrome 148, real CDP)
Addressing the prior review's
[P1]: the canonicalization landed in49e8cfc,and the production dispatch path is now verified against a real headless
Chrome over CDP (two real tabs), driving the exact production functions the
review cited —
getPageForTargetId(pw-session.ts, exact-id page lookup) andexecuteActViaPlaywright/executeSingleAction(pw-tools-core.interactions.ts,effectiveTargetId = action.targetId ?? targetId):the real Playwright exact-id lookup (
BrowserTabNotFoundError, i.e. thereported runtime symptom). With ≥2 tabs the single-page best-effort fallback
cannot mask the miss.
canonicalizeActTargetIdsrewrites the alias to the canonical id,so
executeActViaPlaywrightevaluates in the exact intended tab and readsback its title (
PROOF_TAB_A, not the other tab).This was run with the real production code via a throwaway live harness (not
committed); the alias forms
tabId/label/suggestedTargetIdare covered bythe
canonicalizeActTargetIdsunit tests.AI-assisted.