fix(replay): unify the id-uniqueness predicate across both writer sites (#1269 review)

Address the maintainer review on #1272:

1. ONE shared uniqueness predicate. The two demotion sites were counting
   id matches with DIFFERENT semantics — `demoteNonUniqueId` via
   `filterIdentitySet` (NFC + 256-byte cap, and a broken-parent-walk
   exclusion), `selectableId` via a raw `normalizeSelectorText` scan (trim,
   no NFC/cap, no exclusion) — so the identity tuple and the selector chain
   could disagree and half-demote (id gone from one, kept in the other).
   Extract `idMatchCountInTree(nodes, id)` in target-identity-node.ts,
   counting over the canonical `readNodeLocalIdentity` id the replay
   verifier keys on, with no ancestry/parent-walk exclusion. Both
   `demoteNonUniqueId` and `selectableId` now call it. Corrects the
   inaccurate "vacuously-true / plain id scan" comment.

   Cross-invariant test (build.test.ts): for the same node+tree,
   evidence.id === undefined iff the built chain has no id= clause — across
   demoted, unique, and a non-NFC (decomposed vs precomposed) edge case.
   Verified it fails under the old raw-scan and passes under the unified
   predicate.

2. End-to-end reorder proof (session-replay-target-classification.test.ts):
   record against a tree whose rows share android:id/title, then classify
   against a DIFFERENT replay tree where the shared-id rows reorder — the
   demoted role+label identity rebinds the correct row (verified,
   matchCount 1) while `id="android:id/title"` resolves ambiguously (null).
   This pins the FDR 1.0 -> 0 mechanism, not just record-time demotion.

3. Removed the conflated "20/20 clean" live-number comment from the unit
   test; it now states the mechanism (role+label selectivity) instead.

Behavior for the already-clean unique-id path is unchanged: for ordinary
ascii ids the canonical count equals the old raw count. The only outcomes
that change are the edge cases the old split mishandled (non-NFC, broken
parent walk) — where demotion is the correct result. The kept clause still
emits the chain's own normalizeSelectorText id string, so unique ids lead
the chain exactly as before.

Author: thymikee

src/daemon/__tests__/session-target-evidence.test.ts

@@ -532,8 +532,8 @@ test('computeTargetEvidence: a shared Android framework id (matchCount 3 > 1) is
   assert.equal(evidence.label, 'Network & internet');
   assert.deepEqual(evidence.ancestry[0], { role: 'linearlayout' });
   // With the id demoted, role+label uniquely isolates this row among the
-  // three sharing the id, so the record-time self-check still verifies —
-  // this is the measured "get-only stripped-id: 20/20 clean" fix.
+  // three sharing the id, so the record-time self-check still verifies via
+  // the now-selective label rather than the non-selective id.
   assert.equal(evidence.verification, 'verified');
 });
 

src/daemon/handlers/__tests__/session-replay-target-classification.test.ts

@@ -1,7 +1,10 @@
 import { test } from 'vitest';
 import assert from 'node:assert/strict';
-import type { SnapshotNode } from '../../../kernel/snapshot.ts';
+import type { RawSnapshotNode, SnapshotNode } from '../../../kernel/snapshot.ts';
 import type { TargetAnnotationV1 } from '../../../replay/target-identity.ts';
+import { computeTargetEvidence } from '../../session-target-evidence.ts';
+import { buildSelectorChainForNode } from '../../../selectors/build.ts';
+import { parseSelectorChain, resolveSelectorChain } from '../../../selectors/index.ts';
 import { classifyReplayTarget } from '../session-replay-target-classification.ts';
 import {
   bottomTabsRealCaptureFixture,
@@ -464,3 +467,97 @@ test('classifyReplayTarget: document-order determinism for equal rect centers',
 });
 
 // ---------------------------------------------------------------------------
+// #1269 end-to-end mechanism (Ask 2): the load-bearing proof that id-demotion
+// makes replay clean — not just that the id is dropped at record time. Record
+// against a tree whose rows share `android:id/title`, then replay against a
+// DIFFERENT tree where those shared-id rows REORDER (positions drift). The
+// demoted role+label identity binds the correct row; the id path would not.
+// ---------------------------------------------------------------------------
+
+function androidSharedIdListFixture(
+  rows: { id: string; label: string; y: number }[],
+): SnapshotNode[] {
+  const raw: RawSnapshotNode[] = [
+    { index: 0, type: 'FrameLayout', depth: 0 },
+    { index: 1, type: 'RecyclerView', depth: 1, parentIndex: 0 },
+  ];
+  rows.forEach((row, position) => {
+    const wrapperIndex = 2 + position * 2;
+    const titleIndex = wrapperIndex + 1;
+    raw.push({ index: wrapperIndex, type: 'LinearLayout', depth: 2, parentIndex: 1 });
+    raw.push({
+      index: titleIndex,
+      type: 'TextView',
+      identifier: row.id,
+      label: row.label,
+      rect: { x: 0, y: row.y, width: 300, height: 48 },
+      depth: 3,
+      parentIndex: wrapperIndex,
+    });
+  });
+  return toSnapshotNodes(raw);
+}
+
+test('#1269 e2e: a demoted shared-id row rebinds by role+label after the shared-id rows reorder on replay', () => {
+  const ANDROID = 'android' as const;
+  // Record-time Settings root: three rows all carrying the framework id.
+  const recordNodes = androidSharedIdListFixture([
+    { id: 'android:id/title', label: 'Network & internet', y: 100 },
+    { id: 'android:id/title', label: 'Connected devices', y: 148 },
+    { id: 'android:id/title', label: 'Apps', y: 196 },
+  ]);
+  const target = recordNodes.find((node) => node.label === 'Connected devices')!;
+
+  // The writer demotes the non-unique id in BOTH the tuple and the chain.
+  const recorded = computeTargetEvidence({ node: target, preActionNodes: recordNodes })!;
+  assert.equal(recorded.id, undefined);
+  assert.equal(recorded.verification, 'verified');
+  const chain = buildSelectorChainForNode(target, ANDROID, { action: 'get', nodes: recordNodes });
+  assert.ok(!chain.some((entry) => entry.startsWith('id=')));
+  const token = chain.join(' || '); // the recorded selector positional the replay loop re-resolves
+
+  // Replay-time tree: a new conditional row appears at the top and the
+  // shared-id rows are in a DIFFERENT document/viewport order. The recorded
+  // row is now third, at a drifted position.
+  const replayNodes = androidSharedIdListFixture([
+    { id: 'android:id/title', label: 'Wi-Fi', y: 100 },
+    { id: 'android:id/title', label: 'Apps', y: 148 },
+    { id: 'android:id/title', label: 'Connected devices', y: 196 },
+    { id: 'android:id/title', label: 'Network & internet', y: 244 },
+  ]);
+  const expected = replayNodes.find((node) => node.label === 'Connected devices')!;
+
+  const result = classifyReplayTarget({
+    recorded,
+    token,
+    nodes: replayNodes,
+    platform: ANDROID,
+    refLabel: undefined,
+    requireRect: true,
+    allowDisambiguation: true,
+  });
+  assertVerified(result, { winnerRef: expected.ref, matchCount: 1 });
+
+  // Contrast — the reorder is genuinely hostile to the id path: the shared id
+  // matches all four rows (no unique bind → resolution refuses), while the
+  // demoted role+label resolves the correct row uniquely. This is the
+  // FDR 1.0 → 0 difference the demotion buys.
+  const idResolved = resolveSelectorChain(
+    replayNodes,
+    parseSelectorChain('id="android:id/title"'),
+    {
+      platform: ANDROID,
+      requireRect: true,
+      requireUnique: true,
+    },
+  );
+  assert.equal(idResolved, null); // non-unique: refuses to bind
+  const labelResolved = resolveSelectorChain(
+    replayNodes,
+    parseSelectorChain('role="textview" label="Connected devices"'),
+    { platform: ANDROID, requireRect: true, requireUnique: true },
+  );
+  assert.equal(labelResolved?.node.ref, expected.ref);
+});
+
+// ---------------------------------------------------------------------------

src/daemon/session-target-evidence.ts

@@ -17,7 +17,11 @@
 import type { SnapshotNode } from '../kernel/snapshot.ts';
 import { resolveRectCenter } from '../utils/rect-center.ts';
 import { findNearestScrollableContainer } from './snapshot-presentation/tree.ts';
-import { readNodeLocalIdentity, siblingOrdinal } from '../replay/target-identity-node.ts';
+import {
+  idMatchCountInTree,
+  readNodeLocalIdentity,
+  siblingOrdinal,
+} from '../replay/target-identity-node.ts';
 import {
   classifyTargetBindingMatch,
   matchesAncestryPrefix,
@@ -45,7 +49,7 @@ export function computeTargetEvidence(
   const { node, preActionNodes: nodes } = capture;
   if (typeof node.index !== 'number') return undefined;
   const byIndex = buildIndexMap(nodes);
-  const identity = demoteNonUniqueId(boundedLocalIdentity(node), nodes, byIndex);
+  const identity = demoteNonUniqueId(boundedLocalIdentity(node), nodes);
   const ancestryWalk = buildAncestryChain(node, byIndex, TARGET_ANNOTATION_MAX_ANCESTRY);
   const fullAncestry = ancestryWalk.chain;
   const sibling = computeSiblingOrdinal(nodes, node);
@@ -244,23 +248,18 @@ export function filterIdentitySet(
  * (Android's `android:id/title` matching every list row is the measured
  * case — #1269) is not selective: on replay the id-led identity set spans
  * every row, position drifts, and verification correctly refuses a
- * confident bind. Passing an empty `ancestry` to `filterIdentitySet`
- * degrades its ancestry-prefix check to vacuously-true, so it returns
- * exactly the nodes sharing this id anywhere in the tree — the id's own
- * capture-time match count, independent of structural context. When that
- * count is greater than one, fall back to role+label, exactly the identity
- * an unrecorded id already computes. The rule is capture-time uniqueness,
- * not an id-namespace heuristic: a reused RN `FlatList` `testID` hits the
- * same demotion on iOS.
+ * confident bind. `idMatchCountInTree` — the SAME predicate
+ * `buildSelectorChainForNode` uses for the selector chain — counts nodes
+ * sharing this canonical id across the whole tree; when more than one, fall
+ * back to role+label, exactly the identity an unrecorded id already computes.
+ * Both sites sharing one predicate is what keeps the tuple and the chain from
+ * disagreeing (demoting one but not the other). The rule is capture-time
+ * uniqueness, not an id-namespace heuristic: a reused RN `FlatList` `testID`
+ * hits the same demotion on iOS.
  */
-function demoteNonUniqueId(
-  identity: LocalIdentity,
-  nodes: readonly SnapshotNode[],
-  byIndex: Map<number, SnapshotNode>,
-): LocalIdentity {
+function demoteNonUniqueId(identity: LocalIdentity, nodes: readonly SnapshotNode[]): LocalIdentity {
   if (identity.id === undefined) return identity;
-  const idMatchCount = filterIdentitySet(nodes, byIndex, identity, []).length;
-  if (idMatchCount <= 1) return identity;
+  if (idMatchCountInTree(nodes, identity.id) <= 1) return identity;
   const { role, label } = identity;
   return { role, ...(label !== undefined ? { label } : {}) };
 }

src/replay/target-identity-node.ts

@@ -41,6 +41,33 @@ export function localIdentitiesEqual(a: LocalIdentity, b: LocalIdentity): boolea
   return a.id === b.id && a.role === b.role && a.label === b.label;
 }
 
+/**
+ * ADR 0012 decision 3 amendment (#1269): how many nodes in `nodes` carry the
+ * canonical identity `id`. THE single uniqueness predicate behind both
+ * id-demotion sites — `computeTargetEvidence`'s `target-v1` identity tuple
+ * and `buildSelectorChainForNode`'s selector chain. Sharing it is the
+ * correctness guarantee: a non-unique id is dropped from BOTH or NEITHER,
+ * never half-demoted (dropped from identity while still leading the chain, or
+ * the reverse — the exact split #1269's fix exists to close).
+ *
+ * `id` is a canonical identity id (`readNodeLocalIdentity(node).id`: NFC +
+ * 256-byte field cap, no trimming), and every candidate is measured through
+ * the SAME reader, so the count is over exactly the identities the replay
+ * verifier keys on. There is deliberately NO ancestry / parent-walk
+ * exclusion: an id is non-selective the moment two nodes anywhere in the tree
+ * carry it, independent of structural context or a broken parent linkage.
+ */
+export function idMatchCountInTree(
+  nodes: readonly Pick<RawSnapshotNode, 'type' | 'identifier' | 'label'>[],
+  id: string,
+): number {
+  let count = 0;
+  for (const node of nodes) {
+    if (readNodeLocalIdentity(node).id === id) count += 1;
+  }
+  return count;
+}
+
 /**
  * ADR 0012 decision 3: the STRUCTURAL denotation of a node within its capture
  * — the discriminators path 6 uses to isolate ONE member among several nodes

src/selectors/build.test.ts

@@ -2,6 +2,7 @@ import { test } from 'vitest';
 import assert from 'node:assert/strict';
 import type { SnapshotNode } from '../kernel/snapshot.ts';
 import { buildNodes } from '../__tests__/test-utils/snapshot-builders.ts';
+import { computeTargetEvidence } from '../daemon/session-target-evidence.ts';
 import { buildSelectorChainForNode } from './build.ts';
 
 function findByLabel(nodes: SnapshotNode[], label: string): SnapshotNode {
@@ -10,6 +11,24 @@ function findByLabel(nodes: SnapshotNode[], label: string): SnapshotNode {
   return found;
 }
 
+/**
+ * #1269 cross-invariant: the two writers demote through ONE shared
+ * uniqueness predicate, so for the same node+tree the `target-v1` identity
+ * tuple carries an `id` iff the built selector chain leads with an `id=`
+ * clause — never a half-demoted split (id in one, gone from the other).
+ */
+function assertIdParityAcrossWriters(nodes: SnapshotNode[], node: SnapshotNode): void {
+  const evidence = computeTargetEvidence({ node, preActionNodes: nodes });
+  const chain = buildSelectorChainForNode(node, 'android', { action: 'get', nodes });
+  const evidenceHasId = evidence?.id !== undefined;
+  const chainHasId = chain.some((entry) => entry.startsWith('id='));
+  assert.equal(
+    evidenceHasId,
+    chainHasId,
+    `id parity broken: evidence.id=${JSON.stringify(evidence?.id)} chain=${JSON.stringify(chain)}`,
+  );
+}
+
 // ---------------------------------------------------------------------------
 // #1269: a recorded id only leads the chain when it is unique in the
 // record-time tree. `android:id/title` — a shared Android framework
@@ -145,3 +164,111 @@ test('buildSelectorChainForNode: without a record-time tree, an id is trusted as
   const chain = buildSelectorChainForNode(nodes[0]!, 'ios', { action: 'get' });
   assert.deepEqual(chain, ['id="save"', 'role="button" label="Save"', 'label="Save"']);
 });
+
+// ---------------------------------------------------------------------------
+// #1269 cross-invariant (Ask 1): the identity tuple and the selector chain go
+// through ONE shared uniqueness predicate (`idMatchCountInTree`), so they
+// demote in lockstep. The pre-fix bug was two predicates with different
+// normalization/exclusions that could disagree — the non-NFC case below is
+// exactly where a raw `normalizeSelectorText` chain scan kept an id the
+// NFC-normalized identity tuple demoted.
+// ---------------------------------------------------------------------------
+
+test('#1269 cross-invariant: evidence.id present iff chain leads with id= — demoted, unique, and non-NFC edge cases', () => {
+  // Demoted: a shared framework id across 3 rows → both writers drop it.
+  const demoted = buildNodes([
+    { index: 0, type: 'FrameLayout', depth: 0 },
+    { index: 1, type: 'RecyclerView', depth: 1, parentIndex: 0 },
+    { index: 2, type: 'LinearLayout', depth: 2, parentIndex: 1 },
+    {
+      index: 3,
+      type: 'TextView',
+      identifier: 'android:id/title',
+      label: 'Network & internet',
+      rect: { x: 0, y: 100, width: 300, height: 48 },
+      depth: 3,
+      parentIndex: 2,
+    },
+    { index: 4, type: 'LinearLayout', depth: 2, parentIndex: 1 },
+    {
+      index: 5,
+      type: 'TextView',
+      identifier: 'android:id/title',
+      label: 'Apps',
+      rect: { x: 0, y: 148, width: 300, height: 48 },
+      depth: 3,
+      parentIndex: 4,
+    },
+    { index: 6, type: 'LinearLayout', depth: 2, parentIndex: 1 },
+    {
+      index: 7,
+      type: 'TextView',
+      identifier: 'android:id/title',
+      label: 'Battery',
+      rect: { x: 0, y: 196, width: 300, height: 48 },
+      depth: 3,
+      parentIndex: 6,
+    },
+  ]);
+  assertIdParityAcrossWriters(demoted, findByLabel(demoted, 'Network & internet'));
+
+  // Unique: a distinct id → both writers keep it.
+  const unique = buildNodes([
+    { index: 0, type: 'Window', depth: 0 },
+    {
+      index: 1,
+      type: 'Button',
+      identifier: 'save',
+      label: 'Save',
+      rect: { x: 10, y: 10, width: 40, height: 20 },
+      depth: 1,
+      parentIndex: 0,
+    },
+  ]);
+  assertIdParityAcrossWriters(unique, findByLabel(unique, 'Save'));
+
+  // Non-NFC edge: two ids equal under NFC but different raw bytes (decomposed
+  // vs precomposed "é"). The identity tuple always NFC-normalizes, so it sees
+  // one shared id and demotes; the pre-fix raw chain scan compared the raw
+  // bytes, saw two DIFFERENT ids, and kept the id in the chain — the exact
+  // half-demoted split this cross-invariant now forbids.
+  const decomposed = 'cafe\u0301'; // c a f e + U+0301 combining acute accent
+  const precomposed = 'caf\u00e9'; // c a f é (single U+00E9)
+  assert.notEqual(decomposed, precomposed); // distinct raw code-point sequences
+  assert.equal(decomposed.normalize('NFC'), precomposed.normalize('NFC')); // equal under NFC
+  const nonNfc = buildNodes([
+    { index: 0, type: 'RecyclerView', depth: 0 },
+    { index: 1, type: 'LinearLayout', depth: 1, parentIndex: 0 },
+    {
+      index: 2,
+      type: 'TextView',
+      identifier: decomposed,
+      label: 'Coffee',
+      rect: { x: 0, y: 100, width: 300, height: 48 },
+      depth: 2,
+      parentIndex: 1,
+    },
+    { index: 3, type: 'LinearLayout', depth: 1, parentIndex: 0 },
+    {
+      index: 4,
+      type: 'TextView',
+      identifier: precomposed,
+      label: 'Espresso',
+      rect: { x: 0, y: 148, width: 300, height: 48 },
+      depth: 2,
+      parentIndex: 3,
+    },
+  ]);
+  assertIdParityAcrossWriters(nonNfc, findByLabel(nonNfc, 'Coffee'));
+  // And concretely: the id is dropped from BOTH sides (not kept in the chain).
+  const evidence = computeTargetEvidence({
+    node: findByLabel(nonNfc, 'Coffee'),
+    preActionNodes: nonNfc,
+  });
+  assert.equal(evidence?.id, undefined);
+  const chain = buildSelectorChainForNode(findByLabel(nonNfc, 'Coffee'), 'android', {
+    action: 'get',
+    nodes: nonNfc,
+  });
+  assert.ok(!chain.some((entry) => entry.startsWith('id=')));
+});