refactor: route every native selector resolution through the policy interface - #1715
Conversation
…nterface #1649 declared the per-caller ambiguity matrix; four native call sites still bypassed it, spreading `selectorResolutionKnobs(row)` into a raw `resolveSelectorChain` instead of naming the row. That left the "one interface" claim aspirational: a caller could restate its contract as engine knobs and nothing would notice. - `is` non-exists, `get text`/`get attrs`, find's read actions, and the covered-selector diagnosis probe now call `resolveSelectorChainWithPolicy` with their existing row. Semantics are byte-identical: the knob-backed branch of that interface forwards to the same engine call the call sites built by hand. - The façade drops `resolveSelectorChain` and `selectorResolutionKnobs`, so no knob-taking resolver is reachable from outside the package and a call site cannot re-acquire the knobs even by accident. `requireUnique`/`disambiguateAmbiguous` are now named in exactly one function, which `resolve-with-policy.ts` and the replay resolver both derive through. - `get` names the two rows it may consume as a type, so pointing it at any other ambiguity contract is a compile error. Tests: selector-read-policy.test.ts pins which row each read command consumes, end to end, on one ambiguous fixture — the only tree the rows disagree on. Each assertion was proven red by re-pointing its caller at a neighbouring row. The knob-consistency check moves into the package beside the now-private helper. Test call sites that used the raw resolver move to `resolveRecordedTarget`, the same knobs and the path that actually replays a recorded chain. Extracting the failure branch drops `resolveSelectorInteractionTarget` below the complexity threshold; its `fallow-ignore` waiver is removed (verified load-bearing before the extraction, unnecessary after). Closes #1630. Structural stages (occlusion, off-screen, promotion, poll budget) stay per-caller pipeline code, tracked in #1656. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Exact-head review at P2 regression-validity gap: Practical changed-route evidence is also incomplete: exact-head iOS smoke reaches changed No production or guarantee-matrix defect found; checks are green. No ready label applied. |
#1715 review, P2: the find row assertion was only half a pin. `find exists` returns `found: true` for any resolved node, and the `list` call it leaned on goes through listFindMatches — a path that consumes no policy row at all. So repointing findFirstLocatorMatch at `readText` left both assertions green while selection silently moved from the document-order head to the tiebreak winner. Assert through `find get_attrs`, which returns the ref of the node the row actually selected. Both neighbouring rows are now red: `readText` fails '@e3' !== '@e2' (the move the old test missed), `readUnique` fails by refusing the ambiguous screen. `exists` stays as a second, weaker assertion on the same resolution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD
|
Thanks — the P2 was exactly right, and worse than "incomplete": the Fixed in Both neighbouring rows are now red, verified at that head:
I also re-checked the other three assertions against the same standard — each is red under both of its neighbours, since each observes either the selected ref ( Live device evidence — blocked, not skippedI can't provide the iOS/Android exact-head runs. This session runs in a Linux container with no device toolchain at all: What someone with devices should run against …then the same on Two notes on how I'd read that evidence. The migrated routes are pure in-memory resolution — same engine call, same arguments — so a live divergence would mean the refactor was not behavior-preserving in a way Not claiming ready. Happy to keep iterating on the test standard if any of the remaining three assertions still look under-pinned to you. Generated by Claude Code |
|
Exact-head re-review at It remains not ready on practical changed-route evidence. Exact-head iOS smoke covers |
|
Thanks for the re-review. Two things, then a question — I don't want to keep restating the same blocker. Live evidence is terminal in this environment, and I checked properly this timeMy first reply only established "no adb/xcrun". That was incomplete, so I went through every backend the repo supports:
So there is no backend of any kind reachable from here — not a "didn't bother" but a hard limit. No sessions were opened, nothing to close. The gap you named is in the repo's device lanes, not just in this PRChecking why exact-head Android smoke misses those routes — the suites genuinely don't exercise them: No Question — how do you want this closed?
I'd lean 3: it's the durable fix, and it's real scope expansion beyond #1630's module group — Either way I can't produce the run itself from here, so this PR stays at residual risk on live evidence until someone with devices executes it. The body's Validation section now says exactly that rather than claiming device evidence was unnecessary. Generated by Claude Code |
…ch pass Follow-up tightening on the same seam. `is exists` reached findSelectorChainMatch directly while the `readAny` row's own doc claimed to serve "`exists` and find's read-only actions" — true of the docs, not of the code, which is the unverifiable-claim shape #1656's review called out. It now names `readAny`, the row it always described. Equivalent by construction: both take the first alternative with any match under requireRect: false, and disclose that alternative's count. That leaves the root façade with no consumer for findSelectorChainMatch, so it goes the way of resolveSelectorChain — dropped from the string-only façade, kept on the published ./ast surface. Its façade-twin type SelectorChainMatch dies with it (fallow caught it). resolveSelectorChainWithPolicy matched twice on the uniqueness path: once via resolveSelectorChain, then again to fill matchedNodes. Hoisting the single list call above the row switch removes that second pass, collapses two duplicated ambiguous literals into one helper, and drops a `?? [resolution.node]` fallback that was unreachable — a resolution implies its alternative matched, so the list is never null there. While hoisting: the resolved arm's matchedNodes can describe a different alternative than resolution.selector, because uniqueness skips an ambiguous alternative to try the next one. Unreachable today (only first-match callers read it, where both come from one list), and left as-is rather than silently changed — but the doc claimed "the alternative it came from", so it now says what is actually true. Tests: is exists gets a caller-level pin on the shared ambiguous fixture — passes with matches: 2 where its fail-closed siblings refuse — proven red by pointing it at readUnique. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD
|
iOS Smoke Tests went red on Failing test: It's a Swift XCTest in
Flake indicators in that run: the test immediately before it burned 12.2s on repeated I can't reproduce locally to prove it against the base branch — no macOS/Xcode in this environment, same limit as the device-evidence discussion above — so the re-run is the empirical check. I'll report the result either way; if it reproduces I'll stop calling it a flake and dig in properly. Everything else on this head is green: Lint & Format, Fallow, Static Checks, Layering Guard, Maestro Conformance, Replay-Compat, Bundle Size, iOS Release, CodeQL. Generated by Claude Code |
|
Re-run failed too, so as promised I stopped calling it a flake and dug in. The conclusion holds — it isn't this PR — but my proposed mechanism was wrong and I want to retract it before anyone spends time on it. Correction: I said keyboard state was probably leaking from the preceding test. That was speculation from one log and I shouldn't have offered it as the likely cause. Ignore it. What the two attempts actually show
A different test each time, from the same Cross-branch evidence — the iOS lane is unstable repo-wide right now
This PR passed the same job twice — on I also checked the three commits main gained since my base ( What I did / didn't doRe-ran the failed job once more (attempt 3). I'm not going to keep burning macOS runner minutes past that — if attempt 3 is red I'll report it rather than re-roll again. This looks like it deserves its own instability issue against the iOS lane (the text-entry family under simulator contention) rather than anything on this PR. Happy to open one with the run links if useful — say the word; I'd rather not file it unprompted. Nothing about the diff changed in this round: still 17 TS files, no Swift, and the failing tests drive the runner in-process where Generated by Claude Code |
|
Exact-head re-review at |
…tructurally #1715 review, second regression-validity gap. The `is exists` pin observed only `pass: true` and `matches: 2` on a fixture whose first alternative was merely TIEBREAKABLE — so disambiguation succeeded there and reported the same count first-match would. `readAny`, `readText`, and the pre-migration raw lookup all produced that, and only the readUnique swap I had checked went red. One mutation proven is not the same as the row being pinned. `exists` exposes no node ref, so the row has to be read off WHICH alternative answered. New fixture: alternative one matches two nodes that are genuinely indistinguishable (same depth, same area, both on screen) so the tiebreak declines; alternative two matches exactly one. First-match answers from alternative one; every uniqueness row skips the undecidable alternative and answers from alternative two. Asserting the selector now separates them — readText and readUnique both fail with `id="save-unique"` where `label="Save"` is expected. Restoring the raw lookup stays behaviourally invisible, though: findSelectorChainMatch is equivalent to the readAny row it migrated to, which is precisely why that migration preserved semantics. No fixture assertion can catch that revert, so the guard is structural — the façade's export list must not carry resolveSelectorChain, findSelectorChainMatch, or selectorResolutionKnobs. Follows the packages/maestro index.test.ts absence-assertion precedent. Verified red by re-exporting the lookup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD
|
Right on both counts, and the second half is the interesting one. Fixed in The tiebreakable fixture was the flaw. Because its first alternative could be disambiguated,
On the raw-lookup revert — you were right that it needs a structural guard, and I want to be explicit about why nothing else works. So the guard asserts the root façade's export list carries none of That covers all three removed symbols, so the whole migration is now protected from piecemeal revert rather than just this one caller. Gates: format, lint, typecheck, fallow clean; full unit suite 682 files / 5916 tests green. Still open, unchanged
Generated by Claude Code |
|
Exact-head re-review at |
|
Thanks — glad the skipped-alternative fixture and the namespace guard land right. iOS Smoke on
|
| head | production delta | iOS result | failing test |
|---|---|---|---|
faea99d att 1 |
— | ✗ | testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden |
faea99d att 2 |
none (same commit) | ✗ | testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand |
d95c85f att 1 |
none — git diff --name-only faea99d..d95c85f minus tests is empty |
✗ | back to testBareType…KeyboardIsHidden |
d95c85f touches three Vitest files and nothing else. A test-only TypeScript commit cannot change a Swift XCTest outcome, and the failing test alternates between two members of the same family across attempts with zero code delta. That is nondeterminism in the lane, not a property of this diff. Combined with the earlier cross-branch data (four unrelated branches flip-flopping in the same window; this PR green on the same job on two earlier heads), I don't think there's a code-cause hypothesis left standing.
I'm not re-running it again. I said I'd stop at attempt 3 and I'd rather not spend more macOS minutes fishing for a green.
Standing offer, unchanged: I'll open an iOS-lane instability issue with the run links and the alternating-failure table if you want one. Still not filing it unprompted — it's a repo-wide call.
Live device evidence — I need a decision, not another round
This is the third re-review blocking on it, so let me be direct about why I keep asking instead of delivering.
I cannot produce it from here. Verified, not assumed: no adb/xcrun/emulator; iOS needs macOS; the web backend is unreachable because the sandbox proxy returns 403 CONNECT tunnel failed for the Chrome-for-Testing host; the Linux backend has no DISPLAY, so no AT-SPI bus. There is no fourth backend.
Three ways this closes, and only you can pick:
- You run it — commands and provenance list are here.
- I add the coverage to the iOS/Android
.adreplay suites as a follow-up PR — the durable version, since those routes have never had device-lane coverage (grepshows the Android suite carries onlyis existsandfind … exists). CI has the devices I don't. - Merge on residual risk, with the gap recorded as it currently is in the PR body.
To be explicit about why I haven't just done (2) unilaterally: I can't execute .ad scripts locally, so I'd be pushing unvalidated cases onto a device lane I have no way to run — which is how you break the lane for everyone else. It needs to be a deliberate follow-up someone can watch, not a silent add-on here.
Everything else on d95c85f is green.
Generated by Claude Code
|
iOS Smoke is still red on The same test fails on an unrelated branch. That is one of the exact two tests failing here. Previously I could only show a flip-flop pattern across branches; this is failure identity on a branch with no relationship to selector resolution. Everything now points one way:
The family looks genuinely unstable under simulator contention, not broken by any one branch. I'd now recommend filing an iOS-lane instability issue rather than leaving it to be re-discovered per PR — I have the run links, the alternating-failure table, and the cross-branch match ready to go. Still not filing unprompted, since it's a repo-wide call: say the word and it's up in a minute. For this PR specifically, nothing has changed on my side — 29 of 30 checks green, and I'm not re-running iOS again. Generated by Claude Code |
|
Exact-head evidence update for
The PR body now carries the same concise evidence. Authoritative CI for the pushed head is running; I will report its final state rather than substituting local results. |
|
I clobbered your body edit — sorry, and it's restored. You added the device evidence at ~17:12Z; I rewrote the body at ~18:27Z to drop a stale bundle-size figure and overwrote your section without checking whether the body had moved under me. My mistake — I should have re-read before a full-body replace, exactly as I would before overwriting a file. I've reconstructed the evidence from your comment above; if I lost nuance, your wording wins. While restoring I also corrected two things the body still claimed that
Reviewed
|
|
Exact-head re-review at The corrected body now accurately separates the behavior-preserving selector-policy refactor from the Android occlusion and replay-parser fixes uncovered by durable device coverage. The new fixtures exercise GitHub is MERGEABLE/UNSTABLE only because Android/iOS Smoke and Android/iOS Release are still running. Every completed substantive check is green; no check is red. Final merge remains gated on those authoritative lanes. |
|
Authoritative CI follow-up for exact head e93c795: all reported checks completed successfully. Android Smoke Tests: https://github.com/callstack/agent-device/actions/runs/31418923303/job/93554666393. iOS Smoke Tests: https://github.com/callstack/agent-device/actions/runs/31418923262/job/93554666524. |
|
Simplification follow-up at exact head 9cf294b: replaced 217 lines of fixture scanning and duplicated live-device orchestration with one 68-line replay-runtime regression (31%). The new test executes open, get text, get attrs, is visible, find get attrs, then asserts the covered click becomes typed REPLAY_DIVERGENCE. A planted removal of find failed before restoration. Focused tests passed 46/46; pnpm check:affected --run passed 390 files / 3,877 tests with 26/26 changed lines covered. Exact-head Android and iOS scripts both completed their read routes and deliberately diverged at step 9; android-helper 0.20.6/API 2 and XCTest backends were proven, all sessions were closed, and temporary Metro/reverse setup was removed. Authoritative CI is running on this head. |
|
Reviewed But the device-lane coverage is gone, and the two The deleted 74 lines from There's no glob discovery — every fixture is wired by explicit name. Control check:
So both files still sit on disk, and nothing executes them. Why I think this matters rather than being cosmetic: the new test writes its own inline script. It never reads the fixtures, so it cannot notice them rotting, and the device lanes no longer exercise the migrated routes at all. That retires the specific guarantee this PR was blocked on — my body text (which you endorsed) says "the lanes reproduce this evidence on every run rather than once," and after Two coherent end states, and I don't think the current one is either:
I lean 1, honestly: the iOS lane's instability is well documented on this PR, so anchoring a load-bearing guarantee to it buys less than it looks like, and a hermetic test that runs on every PR is the stronger regression. But leaving orphaned fixtures is the worst option — they read as coverage while running never. Your call; happy to do either. Gates on this head are clean either way: typecheck, lint, fallow, and the new test green. Generated by Claude Code |
|
Re-reviewed exact head |
|
Authoritative CI follow-up for simplified exact head 9cf294b: all reported checks completed successfully, including Android Smoke Tests and iOS Smoke Tests. The PR is mergeable with a clean merge state. |
|
Summary
Closes #1630. The selector-resolution refactor is behavior-preserving; the device coverage added on top of it (
e93c795) also carries two real fixes it uncovered — see Device coverage below.#1649 declared
SELECTOR_RESOLUTION_POLICIESandresolveSelectorChainWithPolicy, but five native call sites still bypassed the interface: four spreadselectorResolutionKnobs(row)into a rawresolveSelectorChain, andis existsused a separate count-only lookup entirely. So the row was consulted, yet the engine call was still hand-assembled at the call site — a caller could restate its contract as knobs and nothing would catch it.Every consumer now names a row and nothing else.
click/press/fillactactCoveredDiagnosisisnon-existsreadUniqueis existsreadAnyfindSelectorChainMatch, bypassing the matrixget text/get attrsreadText/readUniquefindread actionsreadAnywaitwaitfindfindActis existsis worth calling out: thereadAnyrow's own doc already claimed to serve "existsand find's read-only actions". That was true of the docs and false of the code — the same unverifiable-claim shape #1656's review objected to, sitting inside the matrix being cleaned up.The bypass is closed, not just unused. The
@agent-device/selectorsfaçade dropsresolveSelectorChain,findSelectorChainMatch, andselectorResolutionKnobs, so no knob-taking or count-only resolver is reachable from outside the package.requireUnique/disambiguateAmbiguousare named in exactly one function repo-wide, which bothresolveSelectorChainWithPolicyand the replay resolver derive through. (@agent-device/selectors/astis untouched; it is published SDK surface.)getnames the two rows it may consume as a type, so pointing it at any other ambiguity contract is a compile error.Also simplified
resolveSelectorChainWithPolicymatched twice on the uniqueness path — once viaresolveSelectorChain, then again to fillmatchedNodes. Hoisting the single list call above the row switch removes that second pass, collapses two duplicatedambiguousliterals into one helper, and deletes a?? [resolution.node]fallback that was unreachable (a resolution implies its alternative matched). Net −20 lines there, and one matching pass instead of two on every read.The
fallow-ignore complexitywaiver onresolveSelectorInteractionTargetis also gone. Verified both directions: with the failure branch inlined and the waiver removed, fallow reports14 cyclomatic / 56.3 CRAPabove threshold; after the extraction it passes with no waiver.Acceptance criteria
resolveSelectorChainWithPolicyis the façade's only resolution entry.resolution-policy-parity.test.ts(from refactor: declare selector resolution policy as data #1649), joined by caller-level coverage below.findno longer chains two resolution engines — landed in find: stop chaining two resolution engines on mutating actions #1654/refactor(find): resolve a mutating find's target once, not twice (#1654) #1669.disambiguateAmbiguous/requireUniqueliterals at call sites — enforced by the type system, not convention.Validation
pnpm check:tooling(build, layering,check:package, production-exports),pnpm check:fallow,pnpm test:unit(682 files / 5918 tests) andpnpm test:smokegreen.selector-read-policy.test.tspins which row each read command consumes, end to end. Every assertion is proven red under both of its neighbouring rows:is→readText/readAnyget attrs→readText/readAnyget text→readUnique/readAny@e2instead of@e3findread →readText/readUnique'@e3' !== '@e2'/ refusesis exists→readText/readUnique'id="save-unique"'instead of'label="Save"'Two review findings shaped these, both worth recording:
find existsobserved only reachability.found: trueholds under areadTextswap while selection silently moves. Now asserted throughfind get_attrs, which returns the ref the row actually selected. The pairedlistcall is gone —listFindMatchesconsumes no policy row, so it could never have witnessed the change.readAny,readText, and the pre-migration raw lookup all producepass: true+ the same count on one. Theis existsfixture now makes its first alternative an undecidable tie (equal depth, equal area, both on screen) with a unique second alternative, so only a first-match row answers from alternative one.Thanks @thymikee for both.
One guard is structural, deliberately. Restoring
findSelectorChainMatchis behaviourally invisible — it is equivalent to thereadAnyrow it migrated to, which is exactly what made the migration semantics-preserving — so no fixture assertion can catch that revert.packages/selectors/src/index.test.tstherefore asserts the façade's export list carries none of the three removed symbols (following thepackages/maestroabsence-assertion precedent), verified red by re-exporting one.Test call sites that used the raw resolver move to
resolveRecordedTarget— same knobs, and the path that actually replays a recorded chain, so they exercise production rather than an approximation. Two tighten from=== nullto assertingreason === 'ambiguous'.Device coverage (e93c795, simplified by 9cf294b)
The Android and iOS .ad fixtures remain as concise reproduction recipes for get text, get attrs, is visible, find get attrs, and deliberate covered-target diagnosis. The permanent regression protection is smaller: session-replay-selector-routes.test.ts executes those read routes through the replay runtime and asserts the final typed REPLAY_DIVERGENCE, while android-input-method-overlays.test.ts remains the load-bearing semantic guard for the Android IME occlusion defect.
The simplification removes 217 lines of fixture scanning and duplicated live-suite orchestration and replaces them with one 68-line executing regression (31% of that machinery). A planted removal of the find step failed on the exact missing invocation before restoration.
Android exact-head evidence at 9cf294b: Pixel 9 Pro XL / emulator-5554 completed all four read routes before step 9 deliberately refused covered id=field-name with REPLAY_DIVERGENCE. Backend proof: android-helper 0.20.6, API 2, persistent-session, install reason current.
iOS exact-head evidence at the same head: iPhone 17 Pro / iOS 26.2 completed the same reads before step 9 deliberately refused covered id=gearshape.fill. Replay diagnostics recorded XCTest for all four captures; a focused snapshot recorded XCTest backend count 1.
Gate before push: pnpm check:affected --run && git push (390 affected test files / 3,877 tests; changed-line coverage 26/26). Cleanup: Android returned to portrait, temporary Metro/reverse setup was removed, every session closed, and final session list returned empty.
Two fixes the coverage uncovered
Writing the covered-target case surfaced genuine defects, so this PR is no longer purely behavior-preserving:
snapshot-occlusion.ts— an Android IME overlay lives in a separate window that can precede the app target in cross-window traversal, so the old strictly-increasing-position rule never considered it and the target came back unannotated. A candidate positioned before the target may now cover it when the candidate is an additional overlay and the target is not. Gated behind the optionalisAdditionalOverlayNodecallback, so platforms that do not supply it are unaffected; the new backwards edge can form a cycle, which the pre-seedednullcover-cache entry makes fail closed rather than re-enter.open-script.ts— the replay parser rejected--no-test-ime.Scope and follow-ups
25 files: the selector-resolution family, the test call sites the façade change reached, and the device-replay coverage plus the two fixes it uncovered.
Structural pipeline stages — occlusion routing, the off-screen guard, hittable-ancestor promotion, the wait poll budget — remain per-caller pipeline code, tracked in #1656, whose point is that they must be consumed by the interface rather than re-added as unverifiable columns. Maestro stays separate per ADR 0015.