fix(browser): accept tab aliases in action target ids#99182
Conversation
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close this PR as superseded: its useful Browser Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Canonical path: Use #96178 as the landing path for the Browser So I’m closing this here and keeping the remaining discussion on #96178. Review detailsBest possible solution: Use #96178 as the landing path for the Browser Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main resolves the request tab through alias-aware selection, then rejects alias spellings with raw equality before dispatch; the canonical PR also provides real Chrome/CDP before-after proof. Is this the best way to solve the issue? No for this PR as the landing path. The same fix is already carried by an older open canonical PR with broader tests and real browser proof, so maintaining this parallel branch is not the best solution. Security review: Security review cleared: No concrete security or supply-chain concern was found; the diff changes only Browser route target-id normalization and tests. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 203a896b27c5. |
Onyx_bot ReviewOld behavior confirmed. Validation rejected any action.targetId differing from tab.targetId — even valid aliases like t1 that resolveTargetIdFromTabs would correctly resolve. resolveTargetIdFromTabs confirmed correct. Checks exact matches then case-insensitive prefix match. Friendly aliases like t1 resolve correctly when unambiguous. New functions confirmed sound. normalizeActionTargetIdsForSelectedTab recursively walks the action tree (top-level + batch), resolves each targetId via resolveTargetIdFromTabs, and normalizes mismatches to ACT_TARGET_ID_MISMATCH. Test coverage note. Two new alias tests cover the happy path. A useful addition: test for ambiguous aliases (multiple tabs match prefix) to confirm the ambiguous rejection path — but not a blocker since existing mismatch tests cover rejection. Assessment: Correct and ready. Narrow, well-structured fix with proper recursive tree walk for batch actions. |
|
Closing as superseded per ClawSweeper: the same Browser /act target-id alias fix is already covered by the canonical PR #96178, which has sufficient proof and is ready for maintainer review. |
Summary
t1before validating top-level and nested actiontargetIdfieldsVerification
node scripts/run-vitest.mjs extensions/browser/src/browser/server.agent-contract-core.test.tsReal behavior proof
The browser
/tabsresponse tells agents to use friendly refs such ast1, but/actpreviously compared nested actiontargetIdvalues directly with the raw CDP target id. That made valid requests liketargetId: "t1"fail withACT_TARGET_ID_MISMATCHbefore dispatch. The new tests cover accepted friendly refs at the top level and inside batch actions, while existing mismatch tests continue to reject real cross-tab drift.