fix: demote non-unique ids from writer identity/selector chain (#1269)

[thymikee]

Summary

Fixes #1269 — Android list-row get replays bind the wrong row because the recorder uses the non-unique framework resource id android:id/title (matchCount 11 on Settings root) as the primary identity; positional drift on replay then makes the identity verifier correctly refuse with identity-mismatch (measured FDR 1.0, see issue).

The rule: capture-time uniqueness, not a namespace blocklist. Demote an id from identity whenever it matches more than one node in the record-time tree. This is not Android-specific in principle — a fixture below reproduces the same class on iOS via an RN FlatList reusing one testID across rows.

The two edit sites (both required — either alone is insufficient)

• computeTargetEvidence (src/daemon/session-target-evidence.ts): the target-v1 identity tuple now falls back to role+label when the recorded id's own capture-time match count exceeds one. Reuses the existing filterIdentitySet domain machinery already computed there — passing an empty ancestry degrades its ancestry-prefix check to vacuously-true, turning it into a plain "how many nodes in the record-time tree carry this id" scan.
• buildSelectorChainForNode (src/selectors/build.ts): the recorded selector chain now omits a non-unique id rather than leading with it. Every writer call site (get/press/fill recording in resolution.ts/selector-read.ts, plus the replay divergence-suggestion path in session-replay-divergence.ts) now passes the record-time tree so there's something to count against. Omitting nodes (e.g. an isolated test node) preserves prior behavior — no regression for call sites that can't supply a tree.

What's explicitly untouched

resolveSelectorChain and live press/fill resolution are unchanged, per ADR 0012's deliberately-disclosed-not-changed disambiguation. This fix is writer-side / replay-scoped only — it changes what gets recorded, never how a live selector is resolved.

ADR 0012 amendment

Decision 3 gets one added sentence (plus a status-bullet entry): "An id may serve as identity (and lead the selector chain) only when it uniquely denotes the target in the record-time tree."

Fixtures

• Android duplicated-android:id/title list (the measured repro shape: label-less LinearLayout/RecyclerView/FrameLayout ancestry, 3 rows sharing the id) — demoted, and still verifies via the now-selective label (src/daemon/__tests__/session-target-evidence.test.ts, src/selectors/build.test.ts).
• iOS/RN duplicated-testID FlatList shape (one shared accessibility identifier across Cell rows) — same demotion, proving the rule isn't an android:id/* namespace check.
• Regression case: an already-unique id is still recorded as identity (no change for the already-clean case).

Explicitly out of scope

The Android list-press class is a separate, unfixed defect: the clickable row is a label-less role="linearlayout" container with no id at all to demote (matchCount: 12, still diverges). Id-demotion doesn't apply since there's no id in the first place — it needs a different design (deriving identity from the labeled descendant). Not attempted here; needs its own issue per the maintainer scope decision on #1269.

Test plan

• pnpm typecheck
• pnpm exec oxlint --deny-warnings (repo-wide)
• pnpm format:check (project's oxfmt scope: src test skills ...)
• pnpm check:fallow --base origin/main (no dead-code/unused-export issues)
• pnpm check:affected --base origin/main --run — 166 test files / 1745 tests, all green
• New fixtures in src/daemon/__tests__/session-target-evidence.test.ts and src/selectors/build.test.ts

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.7 MB	1.7 MB	+593 B
JS gzip	557.5 kB	557.4 kB	-88 B
npm tarball	673.0 kB	673.1 kB	+46 B
npm unpacked	2.4 MB	2.4 MB	+593 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.7 ms	25.9 ms	-0.8 ms
CLI --help	54.2 ms	54.1 ms	-0.1 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/sdk-selectors.js	+22.1 kB	+7.3 kB
dist/src/tv-remote.js	+13.3 kB	+4.6 kB
dist/src/session.js	+84 B	+48 B
dist/src/runtime.js	+42 B	+23 B
dist/src/selector-runtime.js	+48 B	+17 B

[thymikee]

Live post-fix validation (the record/replay proof requested in review)

Built a daemon from this branch (head 84c2dc75), recorded an Android Settings root get-flow (get "Network & internet", "Connected devices", "Apps") on Pixel 9 Pro XL API 37, and replayed it.

PROOF 1 — the generated .ad omits android:id/title. On a real live capture (where android:id/title matches every row, matchCount > 1), the recorder now emits neither the id clause in the selector chain nor an id in the target-v1 tuple:

# agent-device:target-v1 {"role":"textview","label":"Network & internet","ancestry":[...]}   # no "id"
get "text" "role=\"textview\" label=\"Network & internet\" || label=\"Network & internet\" || value=..."   # no id=

grep -c android:id/title on the generated script = 0.

PROOF 2 — it replays cleanly; the identity class is eliminated.

• Default-annotated recording of the same screen (main): 20/20 identity-mismatch divergences (FDR 1.0).
• This fixed recording: 8/8 clean in the classifying harness; across ~12 total replays, 0 identity-mismatch.

Honest nuance: one earlier replay hit a step-2 wait → action-failure — that is the orthogonal #1270 slow-dumpsys-scroll-hint timeout (an action-failure, not an identity-mismatch), not this fix's concern; it did not recur in the clean 8-run batch. So flow-level 100%-clean on Android remains bounded by #1270 until that's fixed, but this fix's own defect class (the android:id/title identity-mismatch) is eliminated, verified live — not just via fixtures.

Scope reminder: the Android list-press class (label-less role="linearlayout", no id) is still out of scope here and needs the separate design call noted on #1269.

[thymikee]

Review of 84c2dc75e: right design, right scope, right fixtures — one consistency defect to fix, one load-bearing test missing, one evidence claim to clarify.

Conformance verified: capture-time uniqueness (not a namespace blocklist) at both edit sites; resolver untouched per ADR-0012's disclosed-not-changed posture; all four production buildSelectorChainForNode writer call sites pass the record-time tree (checked — the only bare caller left is a web-provider test); the press-class defect correctly scoped out. Also affirming a choice the PR undersells: omitting the non-unique id from the chain (vs demoting it below label) is the safer call — as a trailing fallback clause, a non-selective id that fires after a label miss would disambiguate to an arbitrary wrong node, which is worse than failing.

Finding 1 — the uniqueness predicate exists twice with non-identical semantics, and the fix's correctness depends on the two agreeing. demoteNonUniqueId counts via filterIdentitySet(nodes, byIndex, identity, []); selectableId counts via a raw normalizeSelectorText(candidate.identifier) scan. These disagree in at least two edge classes:

1. filterIdentitySet — even with empty ancestry — still requires !observed.broken (it builds a Math.max(ancestry.length, 1)-deep parent walk), so an id-sharing candidate with broken parent linkage is EXCLUDED from the identity-side count but INCLUDED in the chain-side count. The "degrades to vacuously-true" comment is not quite accurate.
2. The identity side compares under boundedLocalIdentity's normalization (NFC + byte cap); the chain side under normalizeSelectorText. Ids differing only in Unicode normalization form (or exceeding the byte cap) count as equal on one side and distinct on the other.

Either divergence yields the bad half-state this fix exists to prevent: chain demotes while identity keeps the id (or vice versa) → resolution and verification key on different identities → the mismatch class persists. Fix: extract ONE shared idMatchCount(nodes, id) predicate with one normalization used by both sites — and add the cross-invariant test regardless: for any (node, tree), evidence.id === undefined ⟺ the chain contains no id= clause, exercised over fixtures including a broken-parent candidate and a non-NFC id.

Finding 2 — the load-bearing replay-time test is missing. The new tests prove record-time demotion + record-time self-check. The scenario that motivated the whole issue — record, then the tree REORDERS (a row inserted before the target), then replay-time verification — is untested. One test in the replay verification suite: id-demoted evidence (role+label identity), mutated tree with the target at a different position → path 4 verified, action proceeds. That's the FDR-1.0 → 0 claim, pinned in code.

Finding 3 — clarify the "measured stripped-id 20/20 clean" comment in session-target-evidence.test.ts. The n=20 clean run recorded on #1269 was the label-only control, which carries NO annotations (verification off). If a genuine stripped-id arm (id removed, annotations LIVE) has since been run to n=20, link the artifact on #1269 — that's the fix-efficacy evidence and it should be cited. If not, the comment is mislabeling the weaker control and should say so until the post-merge re-run exists.

Nit: the synthetic Settings fixture is fine, but the archived real capture from the E1 run (wave3/ evidence) run through computeTargetEvidence would make the regression fixture match the measured reality exactly — worth doing while the artifact is fresh.

Back-compat note for the record: nodes being optional means any FUTURE writer call site that forgets it silently regresses to trust-the-id. The doc comment covers it; if this recurs, make the writer path take the tree required.

[thymikee]

Thanks — all three addressed in 35ff81f2e. Design kept as endorsed (id omitted from the chain, not reordered below the label).

Ask 1 — one shared uniqueness predicate (the correctness fix)

Confirmed the two predicates diverged, as you said. demoteNonUniqueId counted via filterIdentitySet(nodes, byIndex, identity, []), which is not a plain id scan: it (a) normalizes both sides through boundedLocalIdentity (NFC + 256-byte cap) and (b) excludes broken-parent-walk candidates (!observed.broken). selectableId did a raw normalizeSelectorText scan — trim, no NFC, no cap, no exclusion. So the counts diverge on non-NFC ids, >256-byte ids, whitespace, and broken linkage — the half-demoted split.

Fix: extracted one predicate idMatchCountInTree(nodes, id) (in src/replay/target-identity-node.ts) that counts over the canonical readNodeLocalIdentity id the replay verifier keys on, with no ancestry/parent-walk exclusion (an id is non-selective the moment two nodes carry it, wherever they sit). Both demoteNonUniqueId and selectableId now call it. Fixed the inaccurate "vacuously-true / plain id scan" comment.

Cross-invariant test (src/selectors/build.test.ts): for the same node+tree, evidence.id === undefined ⟺ the built chain has no id= clause — across a demoted case, a unique-id case, and a non-NFC edge (café vs café, equal under NFC). I verified it fails under the old raw-scan (evidence demotes, chain keeps the id → false !== true) and passes under the unified predicate.

Ask 2 — the load-bearing reorder test (FDR 1.0 → 0)

Added an end-to-end mechanism test (src/daemon/handlers/__tests__/session-replay-target-classification.test.ts): record against a 3-row tree sharing android:id/title (writer demotes id in both tuple and chain), then a different replay tree where a conditional row is inserted and the shared-id rows reorder (recorded row drifts from 2nd to 3rd). classifyReplayTarget (the divergence path's verification entry point) binds the correct row via role+label — verified, matchCount 1. Contrast in the same test: id="android:id/title" resolves to null (ambiguous across all 4 rows) while role="textview" label="Connected devices" resolves uniquely — the id path can't discriminate under the reorder, the demoted identity survives it.

Ask 3 — removed the conflated live number

Deleted the // ...measured "get-only stripped-id: 20/20 clean" fix. comment from session-target-evidence.test.ts; it now justifies itself by the mechanism (role+label selectivity after demotion). The annotations-live evidence stays on #1269.

Behavior preservation (your STOP-and-report gate)

Not triggered. For ordinary ascii ids the canonical count equals the old raw count, so the already-clean unique-id path is unchanged — unique ids still lead the chain, and the kept clause still emits the chain's own normalizeSelectorText id string (only the decision is shared, not the emitted string). The only outcomes that change are the edge cases the old split mishandled (non-NFC, broken parent walk), where demotion is the correct result.

Layering: selectors → replay is a legal value import (replay is an unranked peripheral zone; no cycle) — pnpm check:layering green.

Gates: tsc, oxlint --deny-warnings, format:check, check:layering, pnpm check:fallow --base origin/main (10 changed files, no issues), and pnpm check:affected --base origin/main --run (166 files / 1747 tests) all green.

[thymikee]

Rebased onto latest origin/main (e58cbcdb5). New head: fcc9b605d; origin/main is confirmed an ancestor.

One heads-up: the rebase was not fully conflict-free. git rebase reported no textual conflict, but it produced a semantic break — #1217 (typed direct Maestro engine) had extracted buildReplayDivergenceSuggestionForNode out of resolveSuggestionCandidate in session-replay-divergence.ts and added a second caller in session-replay-maestro-failure.ts. My nodes argument to buildSelectorChainForNode rode into the extracted helper, where nodes was no longer in scope (tsc: Cannot find name 'nodes').

Resolved in a clearly-labeled third commit (fcc9b605d): threaded the record-time tree as a required nodes param on the helper and passed it from both callers (each already had it in scope). Net effect: non-unique-id demotion now also applies to the typed-Maestro suggestion path — consistent with the fix's thesis, no behavior change for the already-unique-id case. If you'd prefer the conservative variant (exclude the Maestro suggestion path, leaving its pre-#1217 behavior untouched), say the word and I'll pass nodes: undefined there instead.

Gates on the rebased head: tsc, oxlint --deny-warnings, format:check, check:layering, check:fallow --base origin/main (11 changed files, no new issues; the one duplication finding is pre-existing/inherited in resolution.ts↔selector-read-shared.ts, unrelated), and check:affected --base origin/main --run (165 files / 1678 tests) all green.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-16 06:20 UTC