diff --git a/src/daemon/__tests__/session-target-evidence.test.ts b/src/daemon/__tests__/session-target-evidence.test.ts index 81d278db7..c447859c6 100644 --- a/src/daemon/__tests__/session-target-evidence.test.ts +++ b/src/daemon/__tests__/session-target-evidence.test.ts @@ -466,3 +466,112 @@ test('computeTargetEvidence: a parent cycle records unverifiable, never verified assert.ok(evidence); assert.equal(evidence.verification, 'unverifiable'); }); + +// --------------------------------------------------------------------------- +// #1269: Android list-row false divergences. `android:id/title` is Android's +// OWN framework namespace, shared by every titled row on a screen (11 matches +// on the Settings root in the reported repro); the label-less +// linearlayout/recyclerview/framelayout ancestry gives disambiguation nothing +// to work with but sibling/viewportOrder position, so a shifted row binds the +// wrong element and the identity verifier correctly (but falsely) diverges. +// The fix demotes a shared framework id below a discriminating label at +// record time instead of loosening replay-time verification. +// --------------------------------------------------------------------------- + +/** 11 sibling rows under label-less generic containers, each row's title sharing `android:id/title` — the reported Settings-root shape. */ +function androidSettingsRowsFixture(labels: readonly string[]): SnapshotNode[] { + const raw: RawSnapshotNode[] = [ + { index: 0, type: 'FrameLayout', depth: 0 }, + { index: 1, type: 'RecyclerView', depth: 1, parentIndex: 0 }, + ]; + labels.forEach((label, position) => { + const rowIndex = 2 + position * 2; + const titleIndex = rowIndex + 1; + raw.push({ index: rowIndex, type: 'LinearLayout', depth: 2, parentIndex: 1 }); + raw.push({ + index: titleIndex, + type: 'TextView', + identifier: 'android:id/title', + label, + rect: { x: 0, y: position * 60, width: 400, height: 56 }, + depth: 3, + parentIndex: rowIndex, + }); + }); + return toSnapshotNodes(raw); +} + +const SETTINGS_ROOT_ROW_LABELS = [ + 'Network & internet', + 'Connected devices', + 'Apps', + 'Notifications', + 'Battery', + 'Storage', + 'Sound', + 'Display', + 'Accessibility', + 'Security', + 'Privacy', +]; + +test('computeTargetEvidence (#1269): a shared android:id/* framework id (matchCount 11) is demoted below a discriminating label', () => { + assert.equal(SETTINGS_ROOT_ROW_LABELS.length, 11); + const nodes = androidSettingsRowsFixture(SETTINGS_ROOT_ROW_LABELS); + const winner = findByLabel(nodes, 'Network & internet'); + const evidence = computeTargetEvidence({ node: winner, preActionNodes: nodes }); + assert.ok(evidence); + // The shared framework id must not become primary identity now that the + // label can discriminate this row from the other 10. + assert.equal(evidence.id, undefined); + assert.equal(evidence.role, 'textview'); + assert.equal(evidence.label, 'Network & internet'); + // Demoting the id narrows record-time self-check's identity set from all + // 11 rows down to the single row sharing this label — verified cleanly, + // no sibling/viewportOrder position guessing required. + assert.equal(evidence.verification, 'verified'); +}); + +test('computeTargetEvidence (#1269): a unique (non-framework) resource id is still preferred as primary identity', () => { + const nodes = toSnapshotNodes([ + { 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: 'com.android.settings:id/network_and_internet_row', + label: 'Network & internet', + rect: { x: 0, y: 0, width: 400, height: 56 }, + depth: 3, + parentIndex: 2, + }, + ]); + const winner = nodes[3]!; + const evidence = computeTargetEvidence({ node: winner, preActionNodes: nodes }); + assert.ok(evidence); + assert.equal(evidence.id, 'com.android.settings:id/network_and_internet_row'); + assert.equal(evidence.label, 'Network & internet'); + assert.equal(evidence.verification, 'verified'); +}); + +test('computeTargetEvidence (#1269): a shared framework id with NO label is kept — nothing else can discriminate', () => { + const nodes = toSnapshotNodes([ + { 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', + rect: { x: 0, y: 0, width: 400, height: 56 }, + depth: 3, + parentIndex: 2, + }, + ]); + const winner = nodes[3]!; + const evidence = computeTargetEvidence({ node: winner, preActionNodes: nodes }); + assert.ok(evidence); + assert.equal(evidence.id, 'android:id/title'); + assert.equal(evidence.label, undefined); +}); diff --git a/src/daemon/handlers/__tests__/session-replay-target-classification.test.ts b/src/daemon/handlers/__tests__/session-replay-target-classification.test.ts index 69e96e8f1..44c5a353b 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-classification.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-classification.test.ts @@ -1,8 +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 { classifyReplayTarget } from '../session-replay-target-classification.ts'; +import { computeTargetEvidence } from '../../session-target-evidence.ts'; +import { buildSelectorChainForNode } from '../../../selectors/build.ts'; import { bottomTabsRealCaptureFixture, recordArticleEvidence, @@ -464,3 +466,98 @@ test('classifyReplayTarget: document-order determinism for equal rect centers', }); // --------------------------------------------------------------------------- +// #1269: Android list-row false divergences. Reproduces the wave-3 E1 +// Settings-root repro end to end — record the recorded selector-chain +// EXPRESSION (the same "id=... || role=... label=..." text a real `.ad` +// script stores) against a tree where a row shift moved the recorded row, +// exactly what the reported divergence's `targetBinding.observed` showed. +// Before the fix the id clause led the chain, matched all 11 rows, and its +// own silent position-based disambiguation would resolve some OTHER row +// (identity-mismatch, "safe" but false). After the fix the id is demoted +// below the label clause, so the chain resolves via the label — uniquely and +// correctly — even after the shift. +// --------------------------------------------------------------------------- + +function androidSettingsRowsFixture(labels: readonly string[]): SnapshotNode[] { + const raw: RawSnapshotNode[] = [ + { index: 0, type: 'FrameLayout', depth: 0 }, + { index: 1, type: 'RecyclerView', depth: 1, parentIndex: 0 }, + ]; + labels.forEach((label, position) => { + const rowIndex = 2 + position * 2; + const titleIndex = rowIndex + 1; + raw.push({ index: rowIndex, type: 'LinearLayout', depth: 2, parentIndex: 1 }); + raw.push({ + index: titleIndex, + type: 'TextView', + identifier: 'android:id/title', + label, + rect: { x: 0, y: position * 60, width: 400, height: 56 }, + depth: 3, + parentIndex: rowIndex, + }); + }); + return toSnapshotNodes(raw); +} + +test('classifyReplayTarget (#1269): recorded selector-chain expression still resolves the correct row after a shift, with the shared id demoted', () => { + const recordNodes = androidSettingsRowsFixture([ + 'Network & internet', + 'Connected devices', + 'Apps', + 'Notifications', + 'Battery', + 'Storage', + 'Sound', + 'Display', + 'Accessibility', + 'Security', + 'Privacy', + ]); + const recordedWinner = recordNodes.find((node) => node.label === 'Network & internet'); + assert.ok(recordedWinner); + const recorded = computeTargetEvidence({ node: recordedWinner, preActionNodes: recordNodes }); + assert.ok(recorded); + assert.equal(recorded.id, undefined); // demoted: the shared framework id is not primary identity + + const recordedChain = buildSelectorChainForNode(recordedWinner, 'android', { action: 'get' }); + // The label clause leads; the demoted framework id survives only as the + // very last fallback entry. + assert.equal(recordedChain[0], 'role="textview" label="Network & internet"'); + assert.equal(recordedChain.at(-1), 'id="android:id/title"'); + const token = recordedChain.join(' || '); + + // Replay tree: a conditional row ("A new conditional row") was inserted + // ahead of "Apps", shifting sibling/viewportOrder for every row after it — + // the same class of shift the issue's divergence payload showed. + const replayNodes = androidSettingsRowsFixture([ + 'Network & internet', + 'Connected devices', + 'A new conditional row', + 'Apps', + 'Notifications', + 'Battery', + 'Storage', + 'Sound', + 'Display', + 'Accessibility', + 'Security', + 'Privacy', + ]); + + const result = classifyReplayTarget({ + recorded, + token, + nodes: replayNodes, + platform: 'android', + refLabel: undefined, + requireRect: false, + allowDisambiguation: true, + }); + assertVerified(result, { + winnerRef: replayNodes.find((node) => node.label === 'Network & internet')!.ref, + matchCount: 1, + }); +}); + +// --------------------------------------------------------------------------- diff --git a/src/replay/__tests__/target-identity-node.test.ts b/src/replay/__tests__/target-identity-node.test.ts new file mode 100644 index 000000000..f291f2834 --- /dev/null +++ b/src/replay/__tests__/target-identity-node.test.ts @@ -0,0 +1,47 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { + isNonDiscriminatingAndroidId, + readNodeLocalIdentity, +} from '../target-identity-node.ts'; + +// --------------------------------------------------------------------------- +// #1269: `android:id/*` is Android's OWN framework namespace (list-row +// titles, generic containers, ...), so unrelated sibling rows routinely share +// the exact same id — it must never win outright over a discriminating +// label the way an app-owned id does (decision 3's "Local identity"). +// --------------------------------------------------------------------------- + +test('isNonDiscriminatingAndroidId: true only for the android:id/* framework namespace', () => { + assert.equal(isNonDiscriminatingAndroidId('android:id/title'), true); + assert.equal(isNonDiscriminatingAndroidId('com.android.settings:id/network_and_internet_row'), false); + assert.equal(isNonDiscriminatingAndroidId('save'), false); + assert.equal(isNonDiscriminatingAndroidId(undefined), false); +}); + +test('readNodeLocalIdentity (#1269): a shared framework id is dropped when a label can discriminate instead', () => { + const identity = readNodeLocalIdentity({ + type: 'TextView', + identifier: 'android:id/title', + label: 'Network & internet', + }); + assert.deepEqual(identity, { role: 'textview', label: 'Network & internet' }); +}); + +test('readNodeLocalIdentity (#1269): a unique (non-framework) resource id is still primary identity', () => { + const identity = readNodeLocalIdentity({ + type: 'TextView', + identifier: 'com.android.settings:id/network_and_internet_row', + label: 'Network & internet', + }); + assert.deepEqual(identity, { + id: 'com.android.settings:id/network_and_internet_row', + role: 'textview', + label: 'Network & internet', + }); +}); + +test('readNodeLocalIdentity (#1269): a shared framework id with no label is kept — nothing else can discriminate', () => { + const identity = readNodeLocalIdentity({ type: 'TextView', identifier: 'android:id/title' }); + assert.deepEqual(identity, { id: 'android:id/title', role: 'textview' }); +}); diff --git a/src/replay/target-identity-node.ts b/src/replay/target-identity-node.ts index 148b16655..5f87fb145 100644 --- a/src/replay/target-identity-node.ts +++ b/src/replay/target-identity-node.ts @@ -21,14 +21,36 @@ import { type LocalIdentity, } from './target-identity.ts'; +/** + * #1269: `android:id/*` is Android's OWN framework namespace, assigned by the + * OS to the widget roles it supplies (a list row's title/summary/icon, generic + * containers, ...) rather than by the app — so unrelated sibling rows + * routinely share the exact same id (`android:id/title` matches every titled + * row on a Settings screen). A recorded id in this namespace is not + * selective; it must never outrank a recorded label as the winning identity. + */ +const ANDROID_FRAMEWORK_ID_PREFIX = 'android:id/'; + +/** True for a shared, OS-assigned Android resource-id that cannot discriminate one sibling from another. */ +export function isNonDiscriminatingAndroidId(id: string | undefined): boolean { + return id !== undefined && id.startsWith(ANDROID_FRAMEWORK_ID_PREFIX); +} + export function readNodeLocalIdentity( node: Pick, ): LocalIdentity { const role = normalizeRoleField(normalizeType(node.type ?? '')); const id = normalizeIdentifierField(node.identifier); const label = normalizeLabelField(node.label); + // #1269: demote a non-discriminating framework id below a discriminating + // label — decision 3's "Local identity" only lets `id` win outright when it + // actually IS the recording's discriminator, so a shared `android:id/*` id + // is dropped whenever a label can take over instead. + const primaryId = isNonDiscriminatingAndroidId(id) && label !== undefined ? undefined : id; return { - ...(id !== undefined ? { id: truncateToUtf8Bytes(id, TARGET_ANNOTATION_MAX_FIELD_BYTES) } : {}), + ...(primaryId !== undefined + ? { id: truncateToUtf8Bytes(primaryId, TARGET_ANNOTATION_MAX_FIELD_BYTES) } + : {}), role: truncateToUtf8Bytes(role, TARGET_ANNOTATION_MAX_FIELD_BYTES), ...(label !== undefined ? { label: truncateToUtf8Bytes(label, TARGET_ANNOTATION_MAX_FIELD_BYTES) } diff --git a/src/selectors/build.test.ts b/src/selectors/build.test.ts new file mode 100644 index 000000000..109bf52db --- /dev/null +++ b/src/selectors/build.test.ts @@ -0,0 +1,41 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import type { SnapshotNode } from '../kernel/snapshot.ts'; +import { buildSelectorChainForNode } from './build.ts'; + +function node(overrides: Partial): SnapshotNode { + return { ref: 'e1', index: 0, type: 'TextView', ...overrides }; +} + +// --------------------------------------------------------------------------- +// #1269: a shared `android:id/*` framework id (e.g. every Settings row's +// `android:id/title`) must not lead the recorded selector chain — leading +// with it lets an ambiguous match resolve via silent position-based +// disambiguation before the chain ever reaches the (unique) label clause. +// --------------------------------------------------------------------------- + +test('buildSelectorChainForNode (#1269): a shared android:id/* id is demoted below the label clause', () => { + const chain = buildSelectorChainForNode( + node({ identifier: 'android:id/title', label: 'Network & internet' }), + 'android', + ); + assert.equal(chain[0], 'role="textview" label="Network & internet"'); + // The demoted id survives only as the very last fallback entry. + assert.equal(chain.at(-1), 'id="android:id/title"'); +}); + +test('buildSelectorChainForNode (#1269): a unique (non-framework) resource id still leads the chain', () => { + const chain = buildSelectorChainForNode( + node({ + identifier: 'com.android.settings:id/network_and_internet_row', + label: 'Network & internet', + }), + 'android', + ); + assert.equal(chain[0], 'id="com.android.settings:id/network_and_internet_row"'); +}); + +test('buildSelectorChainForNode (#1269): a shared framework id with no label is kept — nothing else can discriminate', () => { + const chain = buildSelectorChainForNode(node({ identifier: 'android:id/title' }), 'android'); + assert.equal(chain[0], 'id="android:id/title"'); +}); diff --git a/src/selectors/build.ts b/src/selectors/build.ts index 2511b2ad0..4fe69313d 100644 --- a/src/selectors/build.ts +++ b/src/selectors/build.ts @@ -2,6 +2,7 @@ import type { Platform, PublicPlatform } from '../kernel/device.ts'; import type { SnapshotNode } from '../kernel/snapshot.ts'; import { isNodeVisible } from './node.ts'; import { extractNodeText, normalizeType } from '../snapshot/snapshot-processing.ts'; +import { isNonDiscriminatingAndroidId } from '../replay/target-identity-node.ts'; export function buildSelectorChainForNode( node: SnapshotNode, @@ -15,8 +16,15 @@ export function buildSelectorChainForNode( const value = normalizeSelectorText(node.value); const text = normalizeSelectorText(extractNodeText(node)); const requireEditable = options.action === 'fill'; + // #1269: a shared `android:id/*` framework id (e.g. every list row's + // `android:id/title`) is not selective. When a label can discriminate + // instead, demote the id clause below every discriminating clause rather + // than leading the chain with it — the id would otherwise resolve + // ambiguously first and never even reach the label clause that resolves + // uniquely. + const idIsDemoted = Boolean(id) && isNonDiscriminatingAndroidId(id ?? undefined) && Boolean(label); - if (id) { + if (id && !idIsDemoted) { chain.push(`id=${quoteSelectorValue(id)}`); } if (role && label) { @@ -50,6 +58,12 @@ export function buildSelectorChainForNode( if (role && requireEditable && !chain.some((entry) => entry.includes('editable=true'))) { chain.push(`role=${quoteSelectorValue(role)} editable=true`); } + if (idIsDemoted && id) { + // Last-resort fallback only: every discriminating clause above already + // had first refusal, so this id only ever helps when they've all stopped + // matching (e.g. the label itself changed). + chain.push(`id=${quoteSelectorValue(id)}`); + } const deduped = uniqueStrings(chain); if (deduped.length === 0 && role) {