Skip to content

refactor: route every native selector resolution through the policy interface - #1715

Merged
thymikee merged 6 commits into
mainfrom
claude/agent-device-1630-d7iebb
Aug 11, 2026
Merged

refactor: route every native selector resolution through the policy interface#1715
thymikee merged 6 commits into
mainfrom
claude/agent-device-1630-d7iebb

Conversation

@thymikee

@thymikee thymikee commented Aug 10, 2026

Copy link
Copy Markdown
Member

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_POLICIES and resolveSelectorChainWithPolicy, but five native call sites still bypassed the interface: four spread selectorResolutionKnobs(row) into a raw resolveSelectorChain, and is exists used 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.

caller row before
click/press/fill act already via the interface
covered-selector diagnosis probe actCoveredDiagnosis raw resolver + knobs
is non-exists readUnique raw resolver + knobs
is exists readAny findSelectorChainMatch, bypassing the matrix
get text / get attrs readText / readUnique raw resolver + knobs
find read actions readAny raw resolver + knobs
wait wait already via the interface
mutating find findAct already via the interface

is exists is worth calling out: the readAny row's own doc already claimed to serve "exists and 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/selectors façade drops resolveSelectorChain, findSelectorChainMatch, and selectorResolutionKnobs, so no knob-taking or count-only resolver is reachable from outside the package. requireUnique/disambiguateAmbiguous are named in exactly one function repo-wide, which both resolveSelectorChainWithPolicy and the replay resolver derive through. (@agent-device/selectors/ast is untouched; it is published SDK surface.)

get names the two rows it may consume as a type, so pointing it at any other ambiguity contract is a compile error.

Also simplified

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 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 complexity waiver on resolveSelectorInteractionTarget is also gone. Verified both directions: with the failure branch inlined and the waiver removed, fallow reports 14 cyclomatic / 56.3 CRAP above threshold; after the extraction it passes with no waiver.

Acceptance criteria

Validation

pnpm check:tooling (build, layering, check:package, production-exports), pnpm check:fallow, pnpm test:unit (682 files / 5918 tests) and pnpm test:smoke green.

selector-read-policy.test.ts pins which row each read command consumes, end to end. Every assertion is proven red under both of its neighbouring rows:

caller re-pointed at observed failure
isreadText / readAny answers instead of refusing
get attrsreadText / readAny answers instead of refusing
get textreadUnique / readAny @e2 instead of @e3
find read → readText / readUnique '@e3' !== '@e2' / refuses
is existsreadText / readUnique 'id="save-unique"' instead of 'label="Save"'

Two review findings shaped these, both worth recording:

  • find exists observed only reachability. found: true holds under a readText swap while selection silently moves. Now asserted through find get_attrs, which returns the ref the row actually selected. The paired list call is gone — listFindMatches consumes no policy row, so it could never have witnessed the change.
  • A tiebreakable fixture cannot pin a first-match row. readAny, readText, and the pre-migration raw lookup all produce pass: true + the same count on one. The is exists fixture 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 findSelectorChainMatch is behaviourally invisible — it is equivalent to the readAny row 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.ts therefore asserts the façade's export list carries none of the three removed symbols (following the packages/maestro absence-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 === null to asserting reason === '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 optional isAdditionalOverlayNode callback, so platforms that do not supply it are unaffected; the new backwards edge can form a cycle, which the pre-seeded null cover-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.

…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
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.05 MB 2.05 MB +163 B
JS gzip 665.2 kB 665.3 kB +63 B
npm tarball 802.2 kB 802.3 kB +106 B
npm unpacked 2.80 MB 2.80 MB +319 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 22.0 ms 21.9 ms -0.2 ms
CLI --help 51.6 ms 51.8 ms +0.3 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/prepare-kind.js +328 B +73 B
dist/src/sdk-batch-runner.js -498 B -58 B
dist/src/screenshot-result.js +178 B +40 B
dist/src/runtime.js +148 B +24 B
dist/src/version.js 0 B -10 B

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head review at 4d1679af7: not ready.

P2 regression-validity gap: selector-read-policy.test.ts claims to pin find to readAny/first-match, but find exists observes only found:true and the separate list call only proves document order. Repointing findFirstLocatorMatch to readText still leaves both assertions green while selection changes from @e2 to @e3. Exercise an observable find read action (get_attrs/get_text), assert @e2, and show it red under the readText swap.

Practical changed-route evidence is also incomplete: exact-head iOS smoke reaches changed find exists, but migrated get/is/find-read/covered-diagnosis routes lack exact-head iOS and Android evidence. Provide focused live get/is/find plus selector-action runs with backend/version proof and closed sessions.

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

Copy link
Copy Markdown
Member Author

Thanks — the P2 was exactly right, and worse than "incomplete": the list call I leaned on goes through listFindMatches, which resolves via listSelectorChainMatches directly and consumes no policy row at all. So it could never have witnessed a change in findFirstLocatorMatch's row. exists observed only reachability. The row was half-pinned.

Fixed in 1672d45: the assertion now runs through find get_attrs, which returns the ref of the node the row actually selected.

Both neighbouring rows are now red, verified at that head:

swap on findFirstLocatorMatch result
readAnyreadText AssertionError: '@e3' !== '@e2' — the silent selection move the old test missed
readAnyreadUnique red; refuses the ambiguous screen instead of answering

exists stays as a second, weaker assertion on the same resolution (a refusing row fails there too), and the list call is gone rather than left standing as coverage it never provided.

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 (get text@e3) or the refusal itself (is, get attrs).

Live device evidence — blocked, not skipped

I can't provide the iOS/Android exact-head runs. This session runs in a Linux container with no device toolchain at all: which adb xcrun emulator xcodebuild returns nothing, and iOS needs macOS regardless. Rather than substitute weaker evidence, I'm flagging it as residual risk per docs/agents/pull-requests.md.

What someone with devices should run against 1672d45, per migrated route:

# get (readText / readUnique) + is (readUnique) + covered diagnosis
ad open --platform ios && ad get text 'label="…"' && ad get attrs 'id="…"' \
  && ad is visible 'label="…"' && ad click 'label="…"' ; ad close
# find read (readAny)
ad find '…' exists && ad find '…' get attrs

…then the same on --platform android, with ad --version and the backend line from the session log as provenance, and ad close --session <name> on each.

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 resolution-policy-parity + the caller-level tests both missed, which is worth knowing but is not what the fixtures are standing in for. And the covered-diagnosis route only fires on the failure path (selector matched but the node is occluded), so exercising it live needs a deliberate overlay, not just a happy-path tap — worth calling out so a green run isn't mistaken for coverage of it.

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

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head re-review at 1672d451: the prior regression-validity P2 is fixed and the code delta is clean. The find get_attrs assertion now reaches the production findCommand -> findFirstLocatorMatch -> readAny route and observes the selected ref: swapping to readText moves @e2 to @e3, while readUnique refuses, so both neighbouring rows make the test red. The unrelated list assertion is correctly removed. All substantive checks are green and the PR is CLEAN/MERGEABLE.

It remains not ready on practical changed-route evidence. Exact-head iOS smoke covers get text, is visible, find ... exists, and selector action paths, but Android smoke does not exercise the migrated get/non-exists-is/find-read routes (is exists bypasses readUnique). Neither platform demonstrates the deliberate covered-target diagnosis failure path, and no live observable find-read selection (find ... get attrs) is recorded. Provide focused Android get/is/find-read evidence and deliberate covered-target diagnosis evidence, plus the corresponding iOS find ... get attrs/covered-diagnosis run, with backend/version provenance and closed sessions. No ready label applied.

Copy link
Copy Markdown
Member Author

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 time

My first reply only established "no adb/xcrun". That was incomplete, so I went through every backend the repo supports:

backend result
iOS / tvOS / macOS needs macOS + Xcode; this is Linux (xcrun, xcodebuild absent)
Android adb, emulator absent
web got Node 24 (downloaded fine), but web setup fails: the managed agent-browser install fetches Chrome-for-Testing metadata from googlechromelabs.github.io, which the sandbox proxy answers 403 CONNECT tunnel failed. Not installable.
Linux desktop no DISPLAY/WAYLAND_DISPLAY, so no AT-SPI bus to snapshot

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 PR

Checking why exact-head Android smoke misses those routes — the suites genuinely don't exercise them:

$ grep -n "^\(get\|is\|find\)" test/integration/replays/android/*.ad
01-settings.ad:  is exists "…"        # ×2 — takes findSelectorChainMatch, bypasses readUnique (as you said)
02/04/05/06:     find text "…" exists # ×6 — hits readAny but observes only reachability

No get text/get attrs, no non-exists is, no find … get attrs, no deliberate covered-target case — on either platform. That's pre-existing: these routes have never had device-lane coverage, which is why a hand-run is the only way to produce the evidence today, and why it would decay the moment it's produced.

Question — how do you want this closed?

  1. You hand-run it. Commands per route are in my earlier reply; I'd add a covered-target case (overlay over the target, then click) since that path only fires on failure.
  2. I extend the iOS/Android .ad replay suites to cover get text/get attrs, a non-exists is, find … get attrs asserting the selected ref, and a covered-target diagnosis case — so the device lanes produce this evidence on every run instead of once.
  3. Same as 2, but as a follow-up PR, keeping this one scoped.

I'd lean 3: it's the durable fix, and it's real scope expansion beyond #1630's module group — AGENTS.md says to stop and confirm rather than grow the diff unilaterally, so I'm asking instead of just pushing it. Happy to do 2 if you'd rather it land together.

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

Copy link
Copy Markdown
Member Author

iOS Smoke Tests went red on faea99d. Not mine, and I've re-run the failed job rather than just asserting that — run 31402065105.

Failing test:

RunnerTests.testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden()
Executed 34 tests, with 2 failures (0 unexpected)   ** TEST EXECUTE FAILED **  exit 65

It's a Swift XCTest in RunnerTests+CommandExecution.swift:362. It launches the app with --agent-device-text-entry-regression, taps a field, asserts the software keyboard is hidden, then checks type routes via synthesized-first-responder. Two reasons it can't be this PR:

  • Wrong process. It drives the runner directly with {"command":"tap","selectorKey":"id","selectorValue":"…"} — the native runner's own selector lookup. @agent-device/selectors is not in the picture; the TS resolution pipeline I changed never executes in this test.
  • Wrong files. The diff is 17 files, all TS under packages/selectors and src/commands/interaction/runtime (+ test call sites). Zero Swift, zero text-entry/keyboard code, no type/fill dispatch.

Flake indicators in that run: the test immediately before it burned 12.2s on repeated Checking existence of 'Keyboard (First Match)' (retry 1/2) cycles, so simulator keyboard state was already unstable — and the failing test's precondition is precisely XCTAssertFalse(isKeyboardVisible(app:)). Keyboard state leaking across tests would flip that assert or the route it checks. This area also carries three fixes on main in recent history (#1657, #1676, #1658).

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

Copy link
Copy Markdown
Member Author

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

attempt failing test failures
1 testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden() 2
2 testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand() 1

A different test each time, from the same -only-testing list. A code-caused regression fails the same test deterministically; a changing failure set is the contention signature AGENTS.md describes.

Cross-branch evidence — the iOS lane is unstable repo-wide right now

ios.yml pull_request runs from the last ~3h, several unrelated branches:

branch outcomes (newest → oldest)
agent/platform-runtime-network ✗ ✓ ✗ ✓ ✗
agent/safer-parent-touch-targets ✗ ✗ ✓ ✗
claude/review-issues-1431-1429 ✓ ✓ ✗ ✓ — ✗
this PR ✗(att2) ✗(att1) ✓ ✓

This PR passed the same job twice — on 4d1679a and 1672d45 — and the only iOS-relevant thing that changed since is that faea99d landed while ~6 branches were booting iOS simulators on macos-26 concurrently. Main is green across 8 recent commits, but those are push runs with far less concurrency, so main-green vs PR-red doesn't isolate a code cause.

I also checked the three commits main gained since my base (4af1307, b747069, 4279d4c) in case main were green because of a fix I lack — all docs/coverage/Vitest-worker config, zero Swift. Merging main in would not change this.

What I did / didn't do

Re-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 @agent-device/selectors is never loaded.


Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head re-review at faea99dfd: not ready. P2 regression-validity gap: the new is exists policy test stays green if readAny is changed to readText, or if the previous raw lookup is restored — its tiebreakable fixture observes only pass: true and matches: 2, which all three paths produce. Add a no-tiebreak first alternative plus a later unique alternative, assert the selected alternative/count, and show it red under readText; retain a structural guard against restoring the raw façade lookup. The one-pass source change otherwise preserves the existing policy behavior. Practical evidence is still incomplete: the migrated Android get/non-exists-is/find-read routes and both-platform covered-target diagnosis lack exact-head live proof, and the iOS Smoke rerun is pending. No ready label applied.

…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

Copy link
Copy Markdown
Member Author

Right on both counts, and the second half is the interesting one. Fixed in d95c85f.

The tiebreakable fixture was the flaw. Because its first alternative could be disambiguated, readAny, readText, and the pre-migration raw lookup all produced pass: true + matches: 2. I'd proven the readUnique swap red and treated the row as pinned — one mutation is not the row.

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.

is exists row result
readAny (current) selector: label="Save", matches: 2
readText AssertionError: expected 'label="Save"', received 'id="save-unique"'
readUnique same failure

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. findSelectorChainMatch is equivalent to the readAny row it migrated to; that equivalence is exactly what made the migration semantics-preserving. So a revert is behaviourally invisible by construction, and no fixture-tree assertion can ever catch it. The absence of the symbol is the only observable.

So the guard asserts the root façade's export list carries none of resolveSelectorChain, findSelectorChainMatch, or selectorResolutionKnobs — following the packages/maestro/src/index.test.ts absence-assertion precedent. Verified red by re-exporting the lookup:

× the façade exposes no resolver that bypasses the policy matrix
  AssertionError: count-only existence lookup must stay private; `is exists` names the readAny row

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

  • Live changed-route evidence — Android get/non-exists-is/find-read and both-platform covered-target diagnosis. Still blocked here (no device toolchain, and the web backend is unreachable because the sandbox proxy 403s the Chrome-for-Testing host). Commands and provenance list are in the earlier reply; the replay-suite question is the durable version of it and is still yours to call.
  • iOS Smoke — attempt 3 running. Attempts 1 and 2 failed on different tests, and the lane is flip-flopping across ~4 unrelated branches while this PR passed the same job twice on earlier heads, so I'm treating it as runner contention. I committed to not re-rolling past attempt 3.

Generated by Claude Code

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head re-review at d95c85fc: the prior P2 is fixed and source review is clean. The new skipped-alternative fixture makes first-match observably differ from both uniqueness rows, and the façade namespace guard structurally rejects restoring the raw resolver or knob exports. It remains not ready because changed-route live device evidence is still absent: Android get/non-exists-is/find-read and both-platform covered-target diagnosis need exact-head proof. All completed checks are green; the iOS Smoke rerun remains pending. No ready label applied.

Copy link
Copy Markdown
Member Author

Thanks — glad the skipped-alternative fixture and the namespace guard land right.

iOS Smoke on d95c85f: completed, not pending — and it settles the question

It failed, and this is now conclusive rather than circumstantial:

head production delta iOS result failing test
faea99d att 1 testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden
faea99d att 2 none (same commit) testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand
d95c85f att 1 nonegit 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:

  1. You run it — commands and provenance list are here.
  2. I add the coverage to the iOS/Android .ad replay suites as a follow-up PR — the durable version, since those routes have never had device-lane coverage (grep shows the Android suite carries only is exists and find … exists). CI has the devices I don't.
  3. 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

Copy link
Copy Markdown
Member Author

iOS Smoke is still red on d95c85f, and I now have direct rather than circumstantial proof that it isn't this PR.

The same test fails on an unrelated branch. agent/safer-parent-touch-targets, run 31401152210 at 15:07Z:

Failing tests:
	RunnerTests.testBareDelayedTypeFailsWhenTappedInputDisappearsMidCommand()
** TEST EXECUTE FAILED **   Executed 37 tests, with 2 failures

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:

  • Same test, unrelated branch, same window.
  • On this PR the failing test alternates between two members of the family across attempts with zero production delta — including a test-only commit that touched nothing but Vitest files and still failed.
  • This PR passed the same job twice, on 4d1679a and 1672d45.
  • Both implicated tests share the --agent-device-text-entry-regression fixture and depend on software-keyboard state; testTypeWithoutResolvedInputReturnsTypedFailureBeforeDispatchingText burns ~12s on Checking existence of 'Keyboard (First Match)' (retry 1/2) in every run, passing or failing.

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

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head evidence update for e93c795ab511d81b489a79988c014c40601cef92 (fix: cover selector routes in device replays).

  • Red proof before implementation: route-ownership test missing [Android, iOS]; replay parser rejected --no-test-ime; Android IME overlay regression expected covered but received no annotation.
  • Gate/push: pnpm check:affected --run && git push passed before the branch moved (389 affected test files / 3,876 tests; changed-line coverage 26/26, 100%).
  • Android focused command: replay test/integration/replays/android/fixture/02-selector-routes-covered-diagnosis.ad --platform android --serial emulator-5554. get text, get attrs, is visible, and find ... get attrs all completed before step 9 deliberately refused covered id="field-name" with REPLAY_DIVERGENCE. Pixel 9 Pro XL; CLI/helper 0.20.6; android-helper API 2 over persistent-session; install reason current.
  • iOS focused command: replay test/integration/replays/ios/fixture/03-selector-routes-covered-diagnosis.ad --platform ios --udid 6044A251-23C2-4584-B7DB-87A16B787757. The same read routes completed, the load-bearing snapshot exposed gearshape.fill as interactionBlocked: covered, and step 9 deliberately returned REPLAY_DIVERGENCE. iPhone 17 Pro / iOS 26.2; CLI 0.20.6; healthy tree snapshot with XCTest backend count 1.
  • Cleanup: every replay/provenance session closed; final session list --json returned sessions: [].

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.

Copy link
Copy Markdown
Member Author

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 e93c795 made false:

  • "No behavior change on any surface" — no longer true. The PR now carries the occlusion and parser fixes your coverage uncovered, so the summary says the refactor is behavior-preserving and calls the fixes out separately.
  • 17 files → 28, and the residual-risk section is replaced by the device evidence that retired it.

Reviewed e93c795

It's on my PR, so I read the production delta rather than assuming it was fixture-only.

snapshot-occlusion.ts is the interesting one, and it looks right. Allowing a candidate positioned before the target to cover it is exactly the cross-window IME case the old strictly-increasing-position rule couldn't see, and the guard is well chosen: targetIsAdditionalOverlay blocks the reverse direction, so an overlay target can still only be covered forwards. Two things I specifically checked:

  • The new backwards edge can form a cycle — overlay p → app q (q > p) → overlay p again, since p is an additional overlay and q isn't. The pre-seeded null cover-cache entry makes that fail closed instead of re-entering, and the replacement comment says so. The invariant the old comment relied on ("positions strictly increase along any recursive path") is genuinely gone, so replacing it rather than leaving it stale was the right call.
  • Blast radiusisAdditionalOverlayNode is optional and compared === true, so any platform not supplying it keeps the old strictly-forward behavior exactly. Narrow.

Also confirmed the self-cover exclusion survives: the old <= filter is now the explicit candidatePosition === targetPosition → false branch.

Gates on the merged head, locally: typecheck, lint, check:fallow clean; pnpm test:unit 682 files / 5918 tests green (+2 from the new coverage test). Agreed that authoritative CI on the pushed head is the one that counts.

With the device evidence landed, the only thing I still have open is the standing offer to file the iOS-lane instability issue — say the word.


Generated by Claude Code

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 10, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Exact-head re-review at e93c795ab511d81b489a79988c014c40601cef92: clean and ready.

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 get text, get attrs, non-exists is visible, observable find … get attrs, and deliberate covered-target REPLAY_DIVERGENCE on both Android and iOS. Their red-before witnesses are credible, the shared harness asserts the exact failure route and closes in finally, and the recorded exact-head-equivalent runs include backend/version provenance plus an empty final session list. No new source, regression, or evidence finding remains.

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.

@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

Reviewed 9cf294b. The new test is good — hermetic, fast, and it asserts the real thing (route order, then typed REPLAY_DIVERGENCE at step 6). Trading brittle live orchestration for that is a fair trade on its own terms.

But the device-lane coverage is gone, and the two .ad fixtures are now orphaned.

The deleted 74 lines from live-replay-scenarios.ts were the only thing running those fixtures:

-  await assertSelectorRouteReplay(
-    context,
-    path.resolve('test/integration/replays/android/fixture/02-selector-routes-covered-diagnosis.ad'),

There's no glob discovery — every fixture is wired by explicit name. Control check:

fixture TS references
android/fixture/01-navigation-scroll.ad 3
android/fixture/02-selector-routes-covered-diagnosis.ad 0
ios/fixture/03-selector-routes-covered-diagnosis.ad 0

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 9cf294b they don't. Your live Android/iOS runs remain valid as one-off evidence; the durable part is what's lost.

Two coherent end states, and I don't think the current one is either:

  1. Coverage genuinely moves to the hermetic test — then delete both .ad fixtures, and I'll correct the PR body to describe the guarantee accurately (a replay-runtime regression, not a per-run device lane).
  2. Device coverage stays — re-wire the two assertSelectorRouteReplay call sites and keep the new test alongside; they answer different questions.

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

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 9cf294b2e6245a808ffcee752f3298d61b5f433e. The test-only simplification removes duplicated live orchestration while retaining a focused production replay-runtime regression with a meaningful planted-red witness. Production selector/occlusion/parser fixes are unchanged, and exact-head Android+iOS evidence covers get/get attrs/is/find plus deliberate covered-target divergence with cleanup. No actionable finding. Code-review ready; ready-for-human remains justified. Final merge remains gated on the pending Android/iOS Smoke and Release lanes.

@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee
thymikee merged commit 338aa2a into main Aug 11, 2026
31 checks passed
@thymikee
thymikee deleted the claude/agent-device-1630-d7iebb branch August 11, 2026 05:34
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-11 05:34 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: native target resolution — one pipeline, per-caller policy as data

2 participants