Repository navigation
fix(is): report ambiguous selector matches as ambiguity, not absence (#2870) - #3340
Conversation
…RN text pair at the read door Two coupled fixes for #2870. 1. The uniqueness rows (is <predicate> except exists/absent, and get attrs) reported a multi-node match as selector_not_found. To an agent that reads as proof the element does not exist, on a screen where it is plainly on display. The shared door now keys on the pipeline outcome kind: none stays selector_not_found, ambiguous becomes AMBIGUOUS_MATCH with the new typed reason selector_ambiguous, details.matches, and bounded candidate snapshot lines shaped like the acting rows' refusal. 2. React Native reports one authored <Text> twice on a regular capture: the paragraph view plus its accessibility-element mirror, identical label and rect, both carrying a hittability fact. Label selectors on RN text therefore always matched two nodes and every fail-closed read refused. resolveElementReportedTwice recognises that pair at the shared selectors resolution door and keeps the OUTER reporter — the node whose testID the app authored and the row snapshot -i already publishes, because collectIosRepeatedStaticSuppression has always suppressed the mirror in the interactive projection. The replay verification gate consumes the same rule, so live and replay resolve identically. find list and snapshot --json still show both nodes; only rows that refuse to choose collapse.
thymikee
added
the
ready-for-human
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
… a new reason Review correction: #2870's read-path refusal should add no new machine vocabulary. The code union (packages/kernel/src/errors.ts) already owns AMBIGUOUS_MATCH with the exact semantics, and CLI/MCP/replay render its candidates and the acting path's partial-refs publication key on that code. The ambiguity outcome now carries code + matches + bounded candidates only; the selectorAmbiguous reason is removed from INTERACTION_ERROR_REASONS. Also: closest-negative commands surface asserts no matches/candidates leak into the selector_not_found outcome, and the offset-rect negative is proven at the command surface, not only the pipeline.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
- Eager-closure budget: the daemon entry gained one static edge for the new failure-door module. The gate names its own fix -- give the code a home in a module the closure already evaluates -- so the builders move into selector-read-shared.ts (the strict reads' existing shared-failure home) and the standalone module is deleted. No baseline touched, no lazy import bolted onto an error path. - Contract fields are now written AFTER caller details, so no caller spread order can clobber them; both strict rows prove they report the matched alternative as details.selector for the same capture. - The ambiguity hint no longer echoes the selector into a single-quoted find command (label="It's here" broke the outer quoting). - The text-echo collapse gains the reportage clause: every non-reporter candidate must be the accessibility element the platform reports for the reporter (isReportedAccessibilityElement). Same label + same frame with an authored (view-backed) descendant stays ambiguous -- the closest-negative fixture pins exactly that. The offset-rect fixture gains the live RN roles so the rect is the only differing fact. - The 5-candidate cap moves to kernel/errors as ELEMENT_MATCH_CANDIDATE_LIMIT beside the ElementMatchCandidateDetails type the surfaces read; all three producers (acting refusal, find refusal, read door) consume it. The shared- shape comment now names what is actually shared. - Help/docs: 'Uniqueness-based reads' replaces internal 'nominating reads'; ref guidance scoped to ref-taking commands (is rejects refs); the RN pair bullet qualified as the observed iOS accessibility shape.
|
Review pass addressed on Failing check fixed at the invariant. Cubic findings: all seven addressed — replies on each thread. The two structural ones: (a) the text-echo collapse now additionally requires every non-reporter candidate to carry the reported-accessibility-element role/subrole ( replay-e2e.md boundary, stated for the reviewer: an |
…ach code answers Pre-dispatch target binding can stop an annotated step before the command ever runs (identity set >1 with no isolating signal, or no capturable fresh snapshot), and both guards answer IDENTITY_UNVERIFIABLE, not AMBIGUOUS_MATCH. Scope the dispatched-ambiguity claim accordingly rather than conflating the two codes -- ADR 0012 retired exactly that conflation.
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
New docs finding fixed on Four owed confirmations, current head
Process note acknowledged: every cubic root thread on this PR now has a non-bot reply from me (verified via the thread API — 8/8), including where a finding led to a design change. Nothing merged or approved. |
|
Smoke red triaged as #3342's flake class — job 113543477378, step "Preflight iOS runner through public CLI", typed Independent confirmation the lane cost isn't my diff:
Still owed and unchanged at |
…and read routes The acting route (selector-readiness.ts) and the strict reads (this branch's new read door) built the same four things -- COMMAND_FAILED, formatSelectorFailure, the typed reason, selectorFailureHint -- from two modules, so a hint or message edit in one would silently drift from the other. One builder now lives in selector-read-shared.ts, the module the acting chain already reaches through resolution.ts and the reads already import -- no new module, no new static edge (eager-closure budgets: 774/774 green). The two axes the routes genuinely differ on stay explicit parameters: unique (message shape) and dispatched -- the acting row proves 'no' via discloseDispatch, the read route proves nothing about reaching the device and omits the field rather than asserting one. Both halves are pinned: selector-readiness.test asserts dispatched 'no' on the one-attempt press refusal, selector-read-policy asserts its absence on the read refusal, so a silent add or drop on either side fails locally.
|
Cubic P2 on the duplicated One correction to the coordinator's ledger: the replay-e2e |
|
The ambiguity fix looks right, but one problem needs to change before merge. At 9730825, After the usual I read For live validation, the existing iOS run in the PR body was at e04e85b, not 9730825, and I did not reproduce it. Please run the RN Catalog screen at the fixed head. Not blocking, and fine to take or leave: the Could the RN-pair rule live in snapshot presentation instead, so a label selector is unique for every row? The author chose to keep refs and counts stable, which seems reasonable. Could the read door also reuse The cubic-dev-ai threads are fixed at this head and can be resolved: the All 21 checks pass. The Android live lane that uses the changed |
is/get attrs printed candidate @refs minted from their internal full-tree capture without issuing them, while the acting refusal publishes the same shape through markSessionPartialRefsIssued + refsGeneration. After the usual snapshot -i the authorized frame is the interactive tree, so a press of a printed candidate was admitted and resolved against an EARLIER tree where the same ref body names a different node -- the docs actively route callers there. That is a wrong-node bind, worse than the flattened not-found this PR fixes. ADR 0014's issuance rule for ambiguity refusals now has one implementation beside the partial-frame primitive it wraps (the issueSettleRefs precedent): publishAmbiguousMatchCandidateRefs issues the candidate bodies as a PARTIAL frame over the capture the failing request just consumed and returns the frozen epoch as refsGeneration. The acting touch runtime consumes it (its interaction-ambiguity-publication module folded into the rule) and the is/get dispatches run it through toDaemonResponse's issuance parameter, which the routes that never print candidates (wait, read-only find) leave unset. Both surfaces already pin printed candidates from refsGeneration, so the retry resolves against the tree that listed it; the plain body still requires a complete frame and is refused with the suggested pinned form. Pinned by the reviewer's sequence as a test: snapshot -i frame -> ambiguous is -> press <candidate ref> acts on the listed node's point, not the earlier tree's positional twin; and the unpinned body is refused, never retargeted. Also takes the reviewer's non-blocking items: elementMatchCandidateDetails owns the cap + snapshot-line rendering the three AMBIGUOUS_MATCH producers shared as copies, the floating doc block folds into observationReadFailure, and the RN-pair doc bullet names every collapsing row (crop-on, replay identity binding), not only the two reads.
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
The eager-closure ratchet failed at the issuance head: importing ELEMENT_MATCH_CANDIDATE_LIMIT as a value made snapshot-lines.ts (and through it snapshot-diff.ts) evaluate kernel/errors.ts and its redaction chain from the daemon's eager rows, for one constant used at one call site inside the builder the three AMBIGUOUS_MATCH producers already share. The cap, the slice, the renderer, and the pairing are one contract, so the declaration belongs in the module that owns that contract; surfaces compute "+N more" from matches - candidates.length and never needed the constant. Kernel keeps the detail type; the builder's docblock states the laziness rationale (ADR 0019).
|
check:affected recorded at |
…(ADR 0014) The review's P1: updateSessionSnapshot deliberately skips storing a sparse-quality capture, so candidates minted from that tree were issued against the PREVIOUS stored tree's generation -- the same wrong-node bind the issuance rule exists to prevent, arriving through the sparse path. issueSettleRefs already carries the opposite ruling beside the same primitive: what was not stored issues nothing. The second implementation of the rule had dropped that guard. Printing and issuing become one decision at the rule, keyed on the typed fact the surfaces already read: the candidate bodies may only be authorized when the capture the failing request consumed IS the tree the session stores under the generation being frozen (node identity -- the command layer stores an annotation copy of the same array). A sparse/unstored capture, a retired or sessionless ref, or a session with no generation now degrades to the count-only refusal: truthful matches count, no candidate refs, and a hint that cannot advertise an affordance the frame could not honor. An advertised-but-unusable ref is worse than no list. Both consumer seams hand the rule the capture their request consumed: the selector routes pass the capture runtime's consumedSnapshot slot through toDaemonResponse's issuance parameter, and the touch runtime owns a per-dispatch consumedCapture slot filled where its own captures land. Runner-native refusals (direct iOS) carry no candidate shape and pass through untouched. The unit door previously over-claimed a sessionless assertion it never made; it now pins retired-ref, sessionless, no-generation, and unstored-capture refusals separately, and the reviewer's end-to-end sequence gains its sparse twin. commands.md scopes the predicate claim to the uniqueness rows (exists/absent never resolve one element) and documents the count-only form.
Head
|
|
Board reconciliation: the red |
|
The ambiguity fix in c6acd1e looks right, and the three earlier inline threads from cubic-dev-ai are now fixed at this head. All 21 checks pass at c6acd1e. The earlier red Coverage run was on 23bc27d, before the cap placement change. The Android live lane that reads the changed One thing is still missing. The live run asked for in the previous review was not done at this head, and the only live run (e04e85b) predates ref issuance. Not blocking, and you can take or leave these: no test drives an ambiguous press through On the earlier threads, the three from cubic-dev-ai are fixed at this head and can be resolved: the sparse issuance case now falls to a count-only refusal #3340 (comment), the sessionless and no-generation tests were added #3340 (comment), and the I did not run the daemon tests, so their pass status comes from CI and your recorded check run. I read the touch-route Before merge, the live iOS RN Catalog run above needs to be posted at the fixed head. |
Live iOS RN Catalog gate — executed at head
|
… follow-up to #3340) (#3347) * test(daemon): pin the acting route's own ambiguity issuance; narrow the rule's docblock The review's two follow-ups, both confirmed at c6acd1e: 1) The acting seam had no test of its own. Deleting the touch runtime's consumed-capture slot degraded every press/fill ambiguity to count-only with no test red -- the existing press test pressed an is-minted ref, traveling the selector-route seam. An ambiguous label= press through handleInteractionCommands now asserts its OWN refusal carries refsGeneration and a refFrameScope equal to the printed candidate bodies. Mutation-checked: removing the slot fills makes exactly this test fail. 2) The rule's docblock claimed every AMBIGUOUS_MATCH producer runs through the helper; find's refusal (buildAmbiguousMatchError, #1597) prints candidates without issuing them and never called it. Narrowed to the routes that consume the rule, with the find shape named and its contract stated: it prints no refsGeneration and its hint routes the caller to narrow the locator, so it advertises no issued-ref affordance. Routing find through the rule would make its candidates issuable -- a separate contract change, left to the maintainer. * docs(daemon): state find's missing generation pin, not an affordance claim The review nit was right that the old phrasing leaned on an inference ("advertises no issued-ref affordance") that a reader could overread as "find's candidates cannot be acted on" -- they are printed as ordinary snapshot lines, and a plain ref against an unrelated complete frame is a pre-existing admission the rule does not govern. The builder's own message ("Use a more specific locator or selector.") and absent refsGeneration remain the facts; state those directly and defer the issuance question as the contract decision it is.
1 participant
Summary
Closes #2870. Two coupled fixes for
ison a selector that matches more than one node.Ambiguity is no longer reported as absence. The strict reads (
ispredicates other thanexists/absent, andget attrs— bothreadUniquerows) turned a multi-node match intoCOMMAND_FAILED/selector_not_foundwith an empty candidate list — which to an agent reads as "the element does not exist" on a screen where it is plainly on display. A new shared door keys on the pipeline outcome kind, never message text:nonestaysselector_not_found;ambiguousnow answers with the existingAMBIGUOUS_MATCHcode plusmatchesand up to fiveformatSnapshotLinecandidates — ADR 0011's bounded-disclosure shape and the acting path's exact shape (selector-action-resolution.ts), so no new machine vocabulary is introduced and CLI/MCP/replay render candidates through the one existing reader.screenshot --crop-onalready had its own typed refusal (CROP_TARGET_AMBIGUOUS) and keeps it. Out of scope by declared policy:get text,is exists,wait.RN text reads as one node. React Native reports one authored
<Text>twice on a regular capture (paragraph view + accessibility-element mirror; measured live viasnapshot --raw: identical label, byte-identical rects, bothhittable: true).resolveElementReportedTwiceextends the read door's EXISTING structural collapse rule (wait still refuses an unverified-hittability wrapper chain (native runner path) #2498 wrapper control: one ancestor-descendant chain) with a text-echo branch keyed on label identity + wrapper-slack rects + no semantic touch target, keeping the outer reporter — the node whose testID the app authored and the rowsnapshot -ipublishes. Nothing in snapshot presentation changed: the client-serialization dedup invariant stays untouched, refs andfind list/snapshot --jsoncounts are unchanged; only fail-closed rows collapse. The replay gate consumes the same function, so live and replay resolve identically.Validation
pnpm check:affected --run(one run per pushed head, recorded in comments): all checks pass at headc6acd1e5e(1,441 unit files, layering, eager-closure ratchet, fallow, wire-compat, command-docs).e04e85be2:is visible|text 'label="Catalog scroll: top"'pass;is visible 'role=text'→AMBIGUOUS_MATCH matches=22+ 5 candidates ++17 more, no reason field; a true miss →selector_not_foundwith nomatches/candidates. Not re-run at the issuance heads — the daemon test below is the verification for those; a live Catalog re-run at this head is left to a human.selector-runtime-ambiguity-issuance.test.ts):snapshot -iframe → ambiguousis→press <printed candidate ref>acts on the listed node's point, not the earlier tree's positional twin; the unpinned body is refused, never retargeted; the sparse-capture twin prints no candidates and issues nothing. Unit door pins stored / retired-ref / sessionless / no-generation / unstored-capture branches separately.matches: 2; zero matches carries no count and no candidates.Known risk
test/integration/android-emulator-e2e/live-assertions.tsaccepts onlyselector_not_found/predicate_faileddetails reasons, so an ambiguous Android selector now fails loudly instead of scrolling. Lane unrun here.