From 0de09657b2b8779e31cccd4b5aeb50ab91d89002 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 2 Aug 2026 09:12:23 +0200 Subject: [PATCH 1/2] test: pin selector-port behavior ahead of the P5 extraction (#1478) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins, at existing root seams, the eight behavior cells the approved P5 amendment (issue #1478 comment 5156017698) requires the future packages/ad-replay selector port (readSelectorExpression / resolveRecordedTarget / buildSelectorCandidates) to preserve. Test-only — no production code changes. --- .../interaction/runtime/selector-wait.test.ts | 123 +++++++++++- ...-replay-divergence-suggestion-port.test.ts | 87 +++++++++ ...-replay-target-classification-port.test.ts | 125 ++++++++++++ .../__tests__/target-identity-node.test.ts | 76 ++++++++ .../__tests__/selector-port-contract.test.ts | 184 ++++++++++++++++++ 5 files changed, 594 insertions(+), 1 deletion(-) create mode 100644 src/daemon/handlers/__tests__/session-replay-divergence-suggestion-port.test.ts create mode 100644 src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts create mode 100644 src/replay/__tests__/target-identity-node.test.ts create mode 100644 src/selectors/__tests__/selector-port-contract.test.ts diff --git a/src/commands/interaction/runtime/selector-wait.test.ts b/src/commands/interaction/runtime/selector-wait.test.ts index 1e2b2d2975..655b8be3eb 100644 --- a/src/commands/interaction/runtime/selector-wait.test.ts +++ b/src/commands/interaction/runtime/selector-wait.test.ts @@ -8,7 +8,12 @@ import { localCommandPolicy, } from '../../../runtime.ts'; import { makeSnapshotState } from '../../../__tests__/test-utils/index.ts'; -import { createSelectorDevice, selectorReadSnapshot } from './__tests__/test-utils/index.ts'; +import type { TargetAnnotationV1 } from '@agent-device/contracts/replay'; +import { + createFakeClock, + createSelectorDevice, + selectorReadSnapshot, +} from './__tests__/test-utils/index.ts'; test('runtime focused selector waits against a full snapshot', async () => { const snapshot = makeSnapshotState([ @@ -58,3 +63,119 @@ test('runtime wait can use backend text search', async () => { assert.deepEqual(result, { kind: 'text', text: 'Ready', waitedMs: 0 }); }); + +// --------------------------------------------------------------------------- +// #1478 P5 step 2, cell 7: wait-landmark identity mismatch (#1349's +// `recordedLandmark`). This is the root seam the future `resolveRecordedTarget` +// port must preserve — a selector match alone is not enough; the wait only +// reports success once SOME match carries the recorded landmark identity, and +// a deadline with only impostor matches must surface the +// `WAIT_LANDMARK_MISMATCH_REASON` refusal rather than either succeeding or an +// undifferentiated timeout. +// --------------------------------------------------------------------------- + +function screenNodes(parentLabel: string) { + return [ + { + index: 0, + depth: 0, + type: 'FrameLayout', + label: parentLabel, + rect: { x: 0, y: 0, width: 390, height: 844 }, + }, + { + index: 1, + depth: 1, + parentIndex: 0, + type: 'TextView', + label: 'Screen X', + rect: { x: 0, y: 100, width: 390, height: 40 }, + }, + ]; +} + +function recordedLandmark(): TargetAnnotationV1 { + return { + role: 'textview', + label: 'Screen X', + ancestry: [{ role: 'framelayout', label: 'Detail Screen' }], + sibling: 0, + viewportOrder: 0, + verification: 'verified', + }; +} + +test('P5 port cell 7: a landmark wait succeeds once a poll carries the recorded identity, not on the first same-selector match', async () => { + const clock = createFakeClock(); + let calls = 0; + const device = createAgentDevice({ + backend: { + platform: 'android', + captureSnapshot: async () => { + calls += 1; + // First poll: an impostor screen — same selector match ("Screen X"), + // wrong ancestor label, so the recorded landmark's ancestry prefix + // does not match. Second poll: the real destination screen. + const nodes = makeSnapshotState( + calls === 1 ? screenNodes('List Screen') : screenNodes('Detail Screen'), + ); + return { snapshot: nodes }; + }, + } satisfies AgentDeviceBackend, + artifacts: createLocalArtifactAdapter(), + sessions: createMemorySessionStore([{ name: 'default', snapshot: makeSnapshotState([]) }]), + policy: localCommandPolicy(), + clock, + }); + + const result = await device.selectors.wait({ + session: 'default', + target: { + kind: 'selector', + selector: 'label="Screen X"', + timeoutMs: 5000, + recordedLandmark: recordedLandmark(), + }, + }); + + assert.equal(result.kind, 'selector'); + if (result.kind !== 'selector') return; + assert.equal(result.node?.label, 'Screen X'); + assert.ok(calls >= 2, `must not report success on the impostor's own poll, got ${calls} polls`); + assert.ok(result.waitedMs > 0, 'must have advanced past the impostor poll'); +}); + +test('P5 port cell 7: a landmark wait refuses at the deadline with WAIT_LANDMARK_MISMATCH_REASON when every poll is an impostor', async () => { + const clock = createFakeClock(); + const device = createAgentDevice({ + backend: { + platform: 'android', + captureSnapshot: async () => ({ snapshot: makeSnapshotState(screenNodes('List Screen')) }), + } satisfies AgentDeviceBackend, + artifacts: createLocalArtifactAdapter(), + sessions: createMemorySessionStore([{ name: 'default', snapshot: makeSnapshotState([]) }]), + policy: localCommandPolicy(), + clock, + }); + + await assert.rejects( + () => + device.selectors.wait({ + session: 'default', + target: { + kind: 'selector', + selector: 'label="Screen X"', + timeoutMs: 250, + recordedLandmark: recordedLandmark(), + }, + }), + (error: unknown) => { + const details = (error as { details?: { reason?: string; matchCount?: number } }).details; + assert.equal(details?.reason, 'wait_landmark_identity_mismatch'); + // The selector itself matched (one impostor node) — this must not be + // reported as an ordinary selector-not-found timeout. + assert.equal(details?.matchCount, 1); + return true; + }, + ); +}); diff --git a/src/daemon/handlers/__tests__/session-replay-divergence-suggestion-port.test.ts b/src/daemon/handlers/__tests__/session-replay-divergence-suggestion-port.test.ts new file mode 100644 index 0000000000..2abb1b60bc --- /dev/null +++ b/src/daemon/handlers/__tests__/session-replay-divergence-suggestion-port.test.ts @@ -0,0 +1,87 @@ +/** + * #1478 P5 step 2: cell 8 — repair-suggestion ordering. + * + * `buildReplayDivergenceSuggestionForNode` (the divergence/repair-hint + * suggestion builder) joins `buildSelectorChainForNode`'s output with ` || ` + * into the suggestion's `selector` string verbatim — so the CHAIN's internal + * priority order (id, then role+label, then label, then value, then text) + * becomes the ORDER an agent sees candidate selector forms in. The future + * `buildSelectorCandidates` port operation ("replay repair/divergence + * suggestions via the existing root selector-chain builder", issue comment + * 5156017698) must preserve this order verbatim, not just the same set of + * candidates. + */ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { buildReplayDivergenceSuggestionForNode } from '../session-replay-divergence.ts'; +import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; +import { toSnapshotNodes } from './session-replay-target-classification-fixtures.ts'; +import type { ReplayReportAction } from '../session-replay-report-action.ts'; + +const identitySanitize = (value: string): string => value; + +test('P5 port cell 8: a node with id, role+label, label, and value all present suggests them in build.ts priority order', () => { + const nodes = toSnapshotNodes([ + { + index: 0, + type: 'Button', + identifier: 'save-btn', + label: 'Save Draft', + value: 'Draft', + rect: { x: 0, y: 0, width: 80, height: 30 }, + }, + ]); + const node = nodes[0]!; + const session = makeIosSession('default'); + const action: ReplayReportAction = { command: 'get', positionals: [], flags: {} }; + + const suggestion = buildReplayDivergenceSuggestionForNode({ + node, + nodes, + session, + action, + basis: 'id', + sanitize: identitySanitize, + }); + + assert.equal( + suggestion.selector, + 'id="save-btn" || role="button" label="Save Draft" || label="Save Draft" || value="Draft"', + ); +}); + +test('P5 port cell 8: a non-unique id (demoted per #1269) is never suggested, even first — role+label leads instead', () => { + const nodes = toSnapshotNodes([ + { + index: 0, + type: 'TextView', + identifier: 'android:id/title', + label: 'Network & internet', + rect: { x: 0, y: 100, width: 300, height: 48 }, + }, + { + index: 1, + type: 'TextView', + identifier: 'android:id/title', + label: 'Apps', + rect: { x: 0, y: 148, width: 300, height: 48 }, + }, + ]); + const node = nodes[0]!; + const session = makeIosSession('default'); + const action: ReplayReportAction = { command: 'get', positionals: [], flags: {} }; + + const suggestion = buildReplayDivergenceSuggestionForNode({ + node, + nodes, + session, + action, + basis: 'role-label', + sanitize: identitySanitize, + }); + + assert.equal( + suggestion.selector, + 'role="textview" label="Network & internet" || label="Network & internet"', + ); +}); diff --git a/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts b/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts new file mode 100644 index 0000000000..6a35ec2383 --- /dev/null +++ b/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts @@ -0,0 +1,125 @@ +/** + * #1478 P5 step 2: cell 5 — winner and matched-node domain from the same + * selector alternative. + * + * `classifyReplayTarget` composes `resolveSelectorChain` (the winner) with a + * matched-node domain it computes over `resolved.selector` — the SAME chain + * alternative resolution picked, not the first alternative with any match at + * all. The future `resolveRecordedTarget` port operation "internally composes + * parse/resolve/list-matches/match" (issue comment 5156017698) and must + * protect this exact invariant, since `matchCount`/the identity-set domain + * feed decision-3 classification (`classifyTargetBindingMatch`) directly. + * + * The fixture is deliberately hostile to the naive implementation: the first + * alternative is genuinely ambiguous (three-way tie, so resolution SKIPS it + * rather than picking a heuristic winner), and the second is unique. A domain + * computed from `listSelectorChainMatches` (which mirrors "first alternative + * with ANY match", independent of whether resolution could use it) would + * report matchCount 3 from the skipped first alternative instead of 1 from + * the alternative resolution actually used. + */ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import type { TargetAnnotationV1 } from '@agent-device/contracts/replay'; +import { classifyReplayTarget } from '../session-replay-target-classification.ts'; +import { toSnapshotNodes } from './session-replay-target-classification-fixtures.ts'; + +const PLATFORM = 'ios' as const; + +// Three "Decoy" buttons: the first two exactly tie (same depth/area), the +// third is uniquely deepest and smallest, so the deepest-then-smallest +// heuristic picks it decisively — an unresolvable tie would fall through to +// the second alternative regardless of `allowDisambiguation`, which is not +// what cell 5's contrast test needs. +function decoyAndSaveTree() { + return toSnapshotNodes([ + { + index: 0, + type: 'Button', + label: 'Decoy', + rect: { x: 0, y: 0, width: 40, height: 20 }, + depth: 1, + }, + { + index: 1, + type: 'Button', + label: 'Decoy', + rect: { x: 60, y: 0, width: 40, height: 20 }, + depth: 1, + }, + { + index: 2, + type: 'Button', + label: 'Decoy', + rect: { x: 120, y: 0, width: 20, height: 10 }, + depth: 3, + }, + { + index: 3, + type: 'Button', + identifier: 'save', + label: 'Save', + rect: { x: 0, y: 40, width: 40, height: 20 }, + depth: 1, + }, + ]); +} + +function saveRecorded(overrides: Partial = {}): TargetAnnotationV1 { + return { + role: 'button', + label: 'Save', + ancestry: [], + sibling: 3, + viewportOrder: 0, + verification: 'verified', + ...overrides, + }; +} + +test('P5 port cell 5: matchCount/domain come from the resolved alternative, not the first alternative with any match', () => { + const nodes = decoyAndSaveTree(); + const result = classifyReplayTarget({ + recorded: saveRecorded(), + token: 'label="Decoy" || id="save"', + nodes, + platform: PLATFORM, + refLabel: undefined, + requireRect: true, + // Disambiguation disabled: the first alternative's 3-way tie has no + // heuristic winner, so resolution must skip it for the unique second one. + allowDisambiguation: false, + }); + + assert.equal(result.verified, true); + if (!result.verified) throw new Error('unreachable'); + assert.equal(result.winnerNode.ref, 'e4'); + // The load-bearing assertion: 1 (the "id=save" domain), never 3 (the + // skipped "label=Decoy" domain). + assert.equal(result.matchCount, 1); +}); + +test('P5 port cell 5: with disambiguation enabled, a resolvable first alternative supplies its OWN full domain, not the second alternative it never tries', () => { + // Contrast case: the third "Decoy" is uniquely deepest, so disambiguation + // resolves the first alternative directly (resolution never reaches + // "id=save" at all). Classification's domain must be the first + // alternative's own 3-way match count, not 1 (what the second alternative + // would have produced). + const nodes = decoyAndSaveTree(); + const result = classifyReplayTarget({ + recorded: { ...saveRecorded(), role: 'button', label: 'Decoy', sibling: 2 }, + token: 'label="Decoy" || id="save"', + nodes, + platform: PLATFORM, + refLabel: undefined, + requireRect: true, + allowDisambiguation: true, + }); + + assert.equal(result.verified, true); + if (!result.verified) throw new Error('unreachable'); + assert.equal(result.winnerNode.ref, 'e3'); + // The load-bearing assertion: 3 (the "label=Decoy" domain resolution + // actually used), never 1 (the untried "id=save" domain). + assert.equal(result.matchCount, 3); +}); 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 0000000000..bc4e699355 --- /dev/null +++ b/src/replay/__tests__/target-identity-node.test.ts @@ -0,0 +1,76 @@ +/** + * #1478 P5 step 2: cell 6 — how the id identity tier is demoted. + * + * `idMatchCountInTree` / `demoteNonUniqueLocalIdentity` are the ONE shared + * uniqueness predicate behind both id-demotion sites (the `target-v1` + * identity tuple `session-target-evidence.ts` writes at record time, and the + * selector chain `buildSelectorChainForNode` builds — see + * `src/selectors/build.test.ts` for that consumer's own coverage). Neither of + * those consumer tests exercises this pair of functions directly; this file + * pins the shared predicate itself, so a future replay-port `resolveRecordedTarget` + * that re-derives identity demotion a different way (or drops it) fails here + * first. + */ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { buildNodes } from '../../__tests__/test-utils/snapshot-builders.ts'; +import { + demoteNonUniqueLocalIdentity, + idMatchCountInTree, + readNodeLocalIdentity, +} from '../target-identity-node.ts'; + +function sharedIdRows() { + return buildNodes([ + { + index: 0, + type: 'TextView', + identifier: 'android:id/title', + label: 'Network & internet', + rect: { x: 0, y: 100, width: 300, height: 48 }, + }, + { + index: 1, + type: 'TextView', + identifier: 'android:id/title', + label: 'Apps', + rect: { x: 0, y: 148, width: 300, height: 48 }, + }, + { + index: 2, + type: 'Button', + identifier: 'save', + label: 'Save', + rect: { x: 0, y: 196, width: 40, height: 20 }, + }, + ]); +} + +test('P5 port cell 6: idMatchCountInTree counts every node sharing the canonical id, independent of role/label/position', () => { + const nodes = sharedIdRows(); + assert.equal(idMatchCountInTree(nodes, 'android:id/title'), 2); + assert.equal(idMatchCountInTree(nodes, 'save'), 1); + assert.equal(idMatchCountInTree(nodes, 'does-not-exist'), 0); +}); + +test('P5 port cell 6: demoteNonUniqueLocalIdentity drops ONLY the id tier, and only when the id is shared', () => { + const nodes = sharedIdRows(); + + const sharedIdentity = readNodeLocalIdentity(nodes[0]!); + assert.equal(sharedIdentity.id, 'android:id/title'); + const demoted = demoteNonUniqueLocalIdentity(sharedIdentity, nodes); + assert.deepEqual(demoted, { role: 'textview', label: 'Network & internet' }); + assert.equal('id' in demoted, false, 'a non-unique id must not survive demotion'); + + const uniqueIdentity = readNodeLocalIdentity(nodes[2]!); + assert.equal(uniqueIdentity.id, 'save'); + const kept = demoteNonUniqueLocalIdentity(uniqueIdentity, nodes); + assert.deepEqual(kept, uniqueIdentity, 'a unique id must be preserved unchanged'); +}); + +test('P5 port cell 6: an identity with no recorded id is a pass-through — demotion never invents an id-based branch for it', () => { + const nodes = buildNodes([{ index: 0, type: 'Button', label: 'Unlabeled row' }]); + const identity = readNodeLocalIdentity(nodes[0]!); + assert.equal(identity.id, undefined); + assert.deepEqual(demoteNonUniqueLocalIdentity(identity, nodes), identity); +}); diff --git a/src/selectors/__tests__/selector-port-contract.test.ts b/src/selectors/__tests__/selector-port-contract.test.ts new file mode 100644 index 0000000000..be9382ea74 --- /dev/null +++ b/src/selectors/__tests__/selector-port-contract.test.ts @@ -0,0 +1,184 @@ +/** + * #1478 P5 step 2: behavior pins for the future `packages/ad-replay` selector + * port (approved amendment, issue comment 5156017698 — `readSelectorExpression` + * / `resolveRecordedTarget` / `buildSelectorCandidates`). These tests exercise + * today's root `src/selectors` implementation directly and are NOT the port + * itself — they exist so a port that changes any of these outcomes fails + * loudly instead of silently drifting. + * + * Four cells live here, all reachable through `resolveSelectorChain` / + * `listSelectorChainMatches` / `tryParseSelectorChain` — the primitives + * `resolveRecordedTarget` will internally compose: + * + * 1. invalid selector vs valid-but-no-match (distinguishable shapes) + * 2. fallback-alternative selection (first alternative has zero matches) + * 3. ambiguity with and without a disambiguation tiebreak + * 4. `requireRect` excluding an otherwise-matching, rect-less node + */ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import type { SnapshotState } from '@agent-device/kernel/snapshot'; +import { + listSelectorChainMatches, + parseSelectorChain, + resolveSelectorChain, + tryParseSelectorChain, +} from '../index.ts'; + +const saveButtonNodes: SnapshotState['nodes'] = [ + { + ref: 'e1', + index: 0, + type: 'Button', + label: 'Save', + rect: { x: 0, y: 0, width: 40, height: 20 }, + enabled: true, + hittable: true, + }, +]; + +// --------------------------------------------------------------------------- +// Cell 1: invalid selector vs valid-but-no-match +// --------------------------------------------------------------------------- + +test('P5 port cell 1: an invalid selector expression and a valid-but-unmatched one fail through different, non-interchangeable shapes', () => { + // Invalid: the parser itself refuses. `tryParseSelectorChain` never + // produces a chain to resolve against — there is no matched-node domain. + const invalid = tryParseSelectorChain('foo=bar'); + assert.equal(invalid, null); + assert.throws(() => parseSelectorChain('foo=bar'), /Unknown selector key/i); + + // Valid but unmatched: the parser succeeds (a real SelectorChain, with + // `.raw`/`.selectors`), and only `resolveSelectorChain` — a DIFFERENT + // function — reports the zero-match outcome, also as null. + const valid = tryParseSelectorChain('label="Ghost"'); + assert.notEqual(valid, null); + assert.equal(valid?.raw, 'label="Ghost"'); + assert.equal(valid?.selectors.length, 1); + assert.doesNotThrow(() => parseSelectorChain('label="Ghost"')); + + const resolved = resolveSelectorChain(saveButtonNodes, valid!, { + platform: 'ios', + requireUnique: true, + }); + assert.equal(resolved, null); + + // The two failures are reachable through DIFFERENT functions on the same + // input path: a caller (the future `readSelectorExpression`) must check + // parse validity before ever calling `resolveSelectorChain` + // (`resolveRecordedTarget`'s job) — collapsing them into one null would + // lose the "is this expression well-formed at all" signal callers like + // `session-replay-target-verification.ts` depend on. +}); + +// --------------------------------------------------------------------------- +// Cell 2: fallback-alternative selection +// --------------------------------------------------------------------------- + +test('P5 port cell 2: a later alternative wins when an earlier one has zero matches', () => { + const chain = parseSelectorChain('id="missing" || label="Save"'); + const resolved = resolveSelectorChain(saveButtonNodes, chain, { + platform: 'ios', + requireUnique: true, + }); + assert.ok(resolved); + assert.equal(resolved.selectorIndex, 1); + assert.equal(resolved.node.ref, 'e1'); + assert.deepEqual( + resolved.diagnostics.map((entry) => entry.matches), + [0, 1], + ); +}); + +// --------------------------------------------------------------------------- +// Cell 3: ambiguity with and without disambiguation +// --------------------------------------------------------------------------- + +function twoWayTieNodes(): SnapshotState['nodes'] { + return [ + { + ref: 'e1', + index: 0, + type: 'Button', + label: 'Press me', + rect: { x: 0, y: 0, width: 300, height: 300 }, + depth: 1, + enabled: true, + hittable: true, + }, + { + ref: 'e2', + index: 1, + type: 'Button', + label: 'Press me', + rect: { x: 10, y: 10, width: 100, height: 20 }, + depth: 2, + enabled: true, + hittable: true, + }, + ]; +} + +test('P5 port cell 3: the SAME ambiguous match is null without a tiebreak and a disclosed winner with one', () => { + const nodes = twoWayTieNodes(); + const chain = parseSelectorChain('label="Press me"'); + + const withoutTiebreak = resolveSelectorChain(nodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + }); + assert.equal(withoutTiebreak, null); + + const withTiebreak = resolveSelectorChain(nodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + disambiguateAmbiguous: true, + }); + assert.ok(withTiebreak); + assert.equal(withTiebreak.node.ref, 'e2'); + assert.equal(withTiebreak.matches, 2); + assert.equal(withTiebreak.disambiguation?.tiebreak, 'deepest'); + assert.equal(withTiebreak.disambiguation?.matchCount, 2); + assert.deepEqual( + withTiebreak.disambiguation?.alternatives.map((node) => node.ref), + ['e1'], + ); +}); + +// --------------------------------------------------------------------------- +// Cell 4: requireRect +// --------------------------------------------------------------------------- + +test('P5 port cell 4: requireRect excludes an otherwise-matching node with no usable rect, consistently across resolve and list-matches', () => { + const rectlessNodes: SnapshotState['nodes'] = [ + { ref: 'e1', index: 0, type: 'Button', label: 'Ghost row', enabled: true }, + ]; + const chain = parseSelectorChain('label="Ghost row"'); + + const withRectRequired = resolveSelectorChain(rectlessNodes, chain, { + platform: 'ios', + requireRect: true, + requireUnique: true, + }); + assert.equal(withRectRequired, null); + + const withoutRectRequired = resolveSelectorChain(rectlessNodes, chain, { + platform: 'ios', + requireRect: false, + requireUnique: true, + }); + assert.ok(withoutRectRequired); + assert.equal(withoutRectRequired.node.ref, 'e1'); + + // The future `resolveRecordedTarget` composes `resolveSelectorChain` (the + // winner) with `listSelectorChainMatches` (the domain, #1478 amendment's + // "same alternative" invariant) — both must apply the SAME rect policy, or + // the domain could disagree with a winner the policy already excluded. + const matchList = listSelectorChainMatches(rectlessNodes, chain, { + platform: 'ios', + requireRect: true, + }); + assert.equal(matchList, null); +}); From 73e0e73c4b672169a3056d0dc8ef495feaaa7d14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 2 Aug 2026 09:50:04 +0200 Subject: [PATCH 2/2] test: consolidate duplicated cell-5/cell-7 coverage per review Cell 7: relocate #1349's wait-landmark cases from selector-read.test.ts to selector-wait.test.ts (the 1:1 topology location for selector-wait.ts), replacing the weaker duplicate cell-7 cases added in the prior commit. The relocated tests keep the stronger assertions (real computeTargetEvidence- derived evidence, an initial no-match poll, observed-ancestry checks, and the plain-timeout-vs-landmark-mismatch distinction). Cell 5: the first case overlapped an existing later-alternative regression in session-replay-target-classification.test.ts. Sharpened it (rather than dropping it, since it is the only counterfactual-sensitive case for the allowDisambiguation=false skip path) to isolate the branch the existing regression's exact-tie fixture cannot reach, and paired it explicitly with the second case as a same-fixture, flag-flipped contrast. --- .../interaction/runtime/selector-read.test.ts | 140 +------------ .../interaction/runtime/selector-wait.test.ts | 186 +++++++++++------- ...-replay-target-classification-port.test.ts | 40 ++-- 3 files changed, 140 insertions(+), 226 deletions(-) diff --git a/src/commands/interaction/runtime/selector-read.test.ts b/src/commands/interaction/runtime/selector-read.test.ts index 2abffbdab9..00974c5a1f 100644 --- a/src/commands/interaction/runtime/selector-read.test.ts +++ b/src/commands/interaction/runtime/selector-read.test.ts @@ -15,8 +15,6 @@ import { createSelectorDevice, selectorReadSnapshot, } from './__tests__/test-utils/index.ts'; -import { computeTargetEvidence } from '../../../daemon/session-target-evidence.ts'; -import { WAIT_LANDMARK_MISMATCH_REASON } from '../../../replay/target-identity-node.ts'; import { AppError } from '@agent-device/kernel/errors'; test('runtime get reads text from a selector target', async () => { @@ -498,10 +496,12 @@ test('runtime selector convenience methods use explicit target helpers', async ( }); // --------------------------------------------------------------------------- -// #1349: wait's in-loop landmark identity verification (replay-only, -// threaded as `target.recordedLandmark`). Polling semantics are preserved — -// a same-selector impostor never aborts the wait; only the deadline turns -// rejected candidates into the fail-closed landmark refusal. +// Wait polls ride out captures that judged the screen unreadable (the +// mid-transition Android helper content verdicts) instead of aborting the +// wait — the live-validated destination-guard gap from #1349's PR review. +// (#1349's own in-loop landmark identity verification tests — the +// `target.recordedLandmark` cases — moved to `selector-wait.test.ts`, the +// 1:1 topology location for `selector-wait.ts`; #1478 P5 step 2 cell 7.) // --------------------------------------------------------------------------- function landmarkScreen(parentLabel: string) { @@ -518,134 +518,6 @@ function landmarkScreen(parentLabel: string) { ]); } -function recordedLandmarkFor(snapshot: ReturnType) { - const node = snapshot.nodes[1]!; - const evidence = computeTargetEvidence( - { node, preActionNodes: snapshot.nodes }, - { mode: 'landmark' }, - ); - assert.ok(evidence); - assert.equal(evidence.verification, 'verified'); - return evidence; -} - -function landmarkWaitDevice(captures: Array>) { - let call = 0; - const initial = captures[0]!; - const device = createAgentDevice({ - backend: { - platform: 'ios', - captureSnapshot: async () => { - const snapshot = captures[Math.min(call, captures.length - 1)]!; - call += 1; - return { snapshot }; - }, - } satisfies AgentDeviceBackend, - artifacts: createLocalArtifactAdapter(), - sessions: createMemorySessionStore([{ name: 'default', snapshot: initial }]), - policy: localCommandPolicy(), - clock: createFakeClock(), - }); - return device; -} - -test('runtime wait keeps polling past a same-selector impostor and succeeds on the recorded landmark', async () => { - const recordTime = landmarkScreen('Detail Screen'); - const recorded = recordedLandmarkFor(recordTime); - const impostor = landmarkScreen('List Screen'); - const empty = makeSnapshotState([{ index: 0, depth: 0, type: 'Other', label: 'Loading' }]); - const device = landmarkWaitDevice([empty, impostor, landmarkScreen('Detail Screen')]); - - const result = await device.selectors.wait({ - session: 'default', - target: { - kind: 'selector', - selector: 'label="Screen X"', - timeoutMs: 10_000, - recordedLandmark: recorded, - }, - }); - - assert.equal(result.kind, 'selector'); - if (result.kind !== 'selector') throw new Error('unreachable'); - // Two rejected polls (absent, then impostor) before the landmark appeared. - assert.equal(result.waitedMs >= 600, true); - assert.equal(result.node?.label, 'Screen X'); - assert.equal(result.preActionNodes?.length, 2); -}); - -test('runtime wait fails closed at the deadline when only impostors matched the selector', async () => { - const recorded = recordedLandmarkFor(landmarkScreen('Detail Screen')); - const device = landmarkWaitDevice([landmarkScreen('List Screen')]); - - const error = await device.selectors - .wait({ - session: 'default', - target: { - kind: 'selector', - selector: 'label="Screen X"', - timeoutMs: 1000, - recordedLandmark: recorded, - }, - }) - .then( - () => undefined, - (thrown: unknown) => thrown, - ); - - assert.ok(error instanceof AppError); - assert.equal(error.details?.reason, WAIT_LANDMARK_MISMATCH_REASON); - assert.equal(error.details?.matchCount, 1); - const observed = error.details?.observed as { role: string; label?: string }; - assert.equal(observed.label, 'Screen X'); - const ancestry = error.details?.observedAncestry as Array<{ role: string; label?: string }>; - assert.equal(ancestry[0]?.label, 'List Screen'); -}); - -test('runtime wait with a recorded landmark keeps the plain timeout when the selector never matched', async () => { - const recorded = recordedLandmarkFor(landmarkScreen('Detail Screen')); - const empty = makeSnapshotState([{ index: 0, depth: 0, type: 'Other', label: 'Loading' }]); - const device = landmarkWaitDevice([empty]); - - await assert.rejects( - device.selectors.wait({ - session: 'default', - target: { - kind: 'selector', - selector: 'label="Screen X"', - timeoutMs: 1000, - recordedLandmark: recorded, - }, - }), - (thrown: unknown) => { - assert.ok(thrown instanceof AppError); - assert.match(thrown.message, /wait timed out for selector/); - assert.equal(thrown.details?.reason, undefined); - return true; - }, - ); -}); - -test('runtime wait without a recorded landmark returns the satisfying match for record-time evidence', async () => { - const device = landmarkWaitDevice([landmarkScreen('Detail Screen')]); - - const result = await device.selectors.wait({ - session: 'default', - target: { kind: 'selector', selector: 'label="Screen X"', timeoutMs: 1000 }, - }); - - assert.equal(result.kind, 'selector'); - if (result.kind !== 'selector') throw new Error('unreachable'); - assert.equal(result.node?.label, 'Screen X'); - assert.equal(result.preActionNodes?.length, 2); -}); - -// --------------------------------------------------------------------------- -// Wait polls ride out captures that judged the screen unreadable (the -// mid-transition Android helper content verdicts) instead of aborting the -// wait — the live-validated destination-guard gap from #1349's PR review. -// --------------------------------------------------------------------------- - function unreadableCaptureError() { return new AppError( 'COMMAND_FAILED', diff --git a/src/commands/interaction/runtime/selector-wait.test.ts b/src/commands/interaction/runtime/selector-wait.test.ts index 655b8be3eb..052bcb233c 100644 --- a/src/commands/interaction/runtime/selector-wait.test.ts +++ b/src/commands/interaction/runtime/selector-wait.test.ts @@ -8,12 +8,14 @@ import { localCommandPolicy, } from '../../../runtime.ts'; import { makeSnapshotState } from '../../../__tests__/test-utils/index.ts'; -import type { TargetAnnotationV1 } from '@agent-device/contracts/replay'; import { createFakeClock, createSelectorDevice, selectorReadSnapshot, } from './__tests__/test-utils/index.ts'; +import { computeTargetEvidence } from '../../../daemon/session-target-evidence.ts'; +import { WAIT_LANDMARK_MISMATCH_REASON } from '../../../replay/target-identity-node.ts'; +import { AppError } from '@agent-device/kernel/errors'; test('runtime focused selector waits against a full snapshot', async () => { const snapshot = makeSnapshotState([ @@ -65,117 +67,149 @@ test('runtime wait can use backend text search', async () => { }); // --------------------------------------------------------------------------- -// #1478 P5 step 2, cell 7: wait-landmark identity mismatch (#1349's -// `recordedLandmark`). This is the root seam the future `resolveRecordedTarget` -// port must preserve — a selector match alone is not enough; the wait only -// reports success once SOME match carries the recorded landmark identity, and -// a deadline with only impostor matches must surface the -// `WAIT_LANDMARK_MISMATCH_REASON` refusal rather than either succeeding or an -// undifferentiated timeout. +// #1349 (relocated from `selector-read.test.ts` — this is the 1:1 topology +// location for `selector-wait.ts`, and #1478 P5 step 2 cell 7's pin): +// wait's in-loop landmark identity verification, threaded as +// `target.recordedLandmark`. Polling semantics are preserved — a +// same-selector impostor never aborts the wait; only the deadline turns +// rejected candidates into the fail-closed landmark refusal +// (`WAIT_LANDMARK_MISMATCH_REASON`), and a plain "the selector never matched +// at all" timeout stays undifferentiated. This is the root seam the future +// `resolveRecordedTarget` port operation must preserve. // --------------------------------------------------------------------------- -function screenNodes(parentLabel: string) { - return [ - { - index: 0, - depth: 0, - type: 'FrameLayout', - label: parentLabel, - rect: { x: 0, y: 0, width: 390, height: 844 }, - }, +function landmarkScreen(parentLabel: string) { + return makeSnapshotState([ + { index: 0, depth: 0, type: 'Other', label: parentLabel }, { index: 1, depth: 1, parentIndex: 0, - type: 'TextView', + type: 'StaticText', label: 'Screen X', - rect: { x: 0, y: 100, width: 390, height: 40 }, + rect: { x: 0, y: 0, width: 100, height: 20 }, }, - ]; + ]); } -function recordedLandmark(): TargetAnnotationV1 { - return { - role: 'textview', - label: 'Screen X', - ancestry: [{ role: 'framelayout', label: 'Detail Screen' }], - sibling: 0, - viewportOrder: 0, - verification: 'verified', - }; +function recordedLandmarkFor(snapshot: ReturnType) { + const node = snapshot.nodes[1]!; + const evidence = computeTargetEvidence( + { node, preActionNodes: snapshot.nodes }, + { mode: 'landmark' }, + ); + assert.ok(evidence); + assert.equal(evidence.verification, 'verified'); + return evidence; } -test('P5 port cell 7: a landmark wait succeeds once a poll carries the recorded identity, not on the first same-selector match', async () => { - const clock = createFakeClock(); - let calls = 0; +function landmarkWaitDevice(captures: Array>) { + let call = 0; + const initial = captures[0]!; const device = createAgentDevice({ backend: { - platform: 'android', + platform: 'ios', captureSnapshot: async () => { - calls += 1; - // First poll: an impostor screen — same selector match ("Screen X"), - // wrong ancestor label, so the recorded landmark's ancestry prefix - // does not match. Second poll: the real destination screen. - const nodes = makeSnapshotState( - calls === 1 ? screenNodes('List Screen') : screenNodes('Detail Screen'), - ); - return { snapshot: nodes }; + const snapshot = captures[Math.min(call, captures.length - 1)]!; + call += 1; + return { snapshot }; }, } satisfies AgentDeviceBackend, artifacts: createLocalArtifactAdapter(), - sessions: createMemorySessionStore([{ name: 'default', snapshot: makeSnapshotState([]) }]), + sessions: createMemorySessionStore([{ name: 'default', snapshot: initial }]), policy: localCommandPolicy(), - clock, + clock: createFakeClock(), }); + return device; +} + +test('runtime wait keeps polling past a same-selector impostor and succeeds on the recorded landmark', async () => { + const recordTime = landmarkScreen('Detail Screen'); + const recorded = recordedLandmarkFor(recordTime); + const impostor = landmarkScreen('List Screen'); + const empty = makeSnapshotState([{ index: 0, depth: 0, type: 'Other', label: 'Loading' }]); + const device = landmarkWaitDevice([empty, impostor, landmarkScreen('Detail Screen')]); const result = await device.selectors.wait({ session: 'default', target: { kind: 'selector', selector: 'label="Screen X"', - timeoutMs: 5000, - recordedLandmark: recordedLandmark(), + timeoutMs: 10_000, + recordedLandmark: recorded, }, }); assert.equal(result.kind, 'selector'); - if (result.kind !== 'selector') return; + if (result.kind !== 'selector') throw new Error('unreachable'); + // Two rejected polls (absent, then impostor) before the landmark appeared. + assert.equal(result.waitedMs >= 600, true); assert.equal(result.node?.label, 'Screen X'); - assert.ok(calls >= 2, `must not report success on the impostor's own poll, got ${calls} polls`); - assert.ok(result.waitedMs > 0, 'must have advanced past the impostor poll'); + assert.equal(result.preActionNodes?.length, 2); }); -test('P5 port cell 7: a landmark wait refuses at the deadline with WAIT_LANDMARK_MISMATCH_REASON when every poll is an impostor', async () => { - const clock = createFakeClock(); - const device = createAgentDevice({ - backend: { - platform: 'android', - captureSnapshot: async () => ({ snapshot: makeSnapshotState(screenNodes('List Screen')) }), - } satisfies AgentDeviceBackend, - artifacts: createLocalArtifactAdapter(), - sessions: createMemorySessionStore([{ name: 'default', snapshot: makeSnapshotState([]) }]), - policy: localCommandPolicy(), - clock, - }); +test('runtime wait fails closed at the deadline when only impostors matched the selector', async () => { + const recorded = recordedLandmarkFor(landmarkScreen('Detail Screen')); + const device = landmarkWaitDevice([landmarkScreen('List Screen')]); + + const error = await device.selectors + .wait({ + session: 'default', + target: { + kind: 'selector', + selector: 'label="Screen X"', + timeoutMs: 1000, + recordedLandmark: recorded, + }, + }) + .then( + () => undefined, + (thrown: unknown) => thrown, + ); + + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, WAIT_LANDMARK_MISMATCH_REASON); + assert.equal(error.details?.matchCount, 1); + const observed = error.details?.observed as { role: string; label?: string }; + assert.equal(observed.label, 'Screen X'); + const ancestry = error.details?.observedAncestry as Array<{ role: string; label?: string }>; + assert.equal(ancestry[0]?.label, 'List Screen'); +}); + +test('runtime wait with a recorded landmark keeps the plain timeout when the selector never matched', async () => { + const recorded = recordedLandmarkFor(landmarkScreen('Detail Screen')); + const empty = makeSnapshotState([{ index: 0, depth: 0, type: 'Other', label: 'Loading' }]); + const device = landmarkWaitDevice([empty]); await assert.rejects( - () => - device.selectors.wait({ - session: 'default', - target: { - kind: 'selector', - selector: 'label="Screen X"', - timeoutMs: 250, - recordedLandmark: recordedLandmark(), - }, - }), - (error: unknown) => { - const details = (error as { details?: { reason?: string; matchCount?: number } }).details; - assert.equal(details?.reason, 'wait_landmark_identity_mismatch'); - // The selector itself matched (one impostor node) — this must not be - // reported as an ordinary selector-not-found timeout. - assert.equal(details?.matchCount, 1); + device.selectors.wait({ + session: 'default', + target: { + kind: 'selector', + selector: 'label="Screen X"', + timeoutMs: 1000, + recordedLandmark: recorded, + }, + }), + (thrown: unknown) => { + assert.ok(thrown instanceof AppError); + assert.match(thrown.message, /wait timed out for selector/); + assert.equal(thrown.details?.reason, undefined); return true; }, ); }); + +test('runtime wait without a recorded landmark returns the satisfying match for record-time evidence', async () => { + const device = landmarkWaitDevice([landmarkScreen('Detail Screen')]); + + const result = await device.selectors.wait({ + session: 'default', + target: { kind: 'selector', selector: 'label="Screen X"', timeoutMs: 1000 }, + }); + + assert.equal(result.kind, 'selector'); + if (result.kind !== 'selector') throw new Error('unreachable'); + assert.equal(result.node?.label, 'Screen X'); + assert.equal(result.preActionNodes?.length, 2); +}); diff --git a/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts b/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts index 6a35ec2383..9d0e17eb4e 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-classification-port.test.ts @@ -10,13 +10,18 @@ * protect this exact invariant, since `matchCount`/the identity-set domain * feed decision-3 classification (`classifyTargetBindingMatch`) directly. * - * The fixture is deliberately hostile to the naive implementation: the first - * alternative is genuinely ambiguous (three-way tie, so resolution SKIPS it - * rather than picking a heuristic winner), and the second is unique. A domain - * computed from `listSelectorChainMatches` (which mirrors "first alternative - * with ANY match", independent of whether resolution could use it) would - * report matchCount 3 from the skipped first alternative instead of 1 from - * the alternative resolution actually used. + * `session-replay-target-classification.test.ts` already has a same-invariant + * regression ("uses the later chain alternative that resolution selected + * after an earlier tie"), but its first alternative is an EXACT tie — + * unresolvable regardless of `allowDisambiguation` (`summary.disambiguated` + * is null either way), so it only exercises the `!summary.disambiguated` + * branch of `resolveSelectorChain`'s skip condition. The two tests below + * instead hold ONE fixture fixed (a first alternative that IS resolvable — + * proven by the second test succeeding through it) and flip only + * `allowDisambiguation`, isolating the OTHER branch, + * `!options.disambiguateAmbiguous`: with it off, classification must still + * use the second alternative's OWN domain (1), never leak the first + * alternative's larger, unused domain (3) — and with it on, the reverse. */ import assert from 'node:assert/strict'; import { test } from 'vitest'; @@ -77,7 +82,7 @@ function saveRecorded(overrides: Partial = {}): TargetAnnota }; } -test('P5 port cell 5: matchCount/domain come from the resolved alternative, not the first alternative with any match', () => { +test("P5 port cell 5: allowDisambiguation off skips a RESOLVABLE (not just tied) first alternative, using the second alternative's own domain", () => { const nodes = decoyAndSaveTree(); const result = classifyReplayTarget({ recorded: saveRecorded(), @@ -86,8 +91,9 @@ test('P5 port cell 5: matchCount/domain come from the resolved alternative, not platform: PLATFORM, refLabel: undefined, requireRect: true, - // Disambiguation disabled: the first alternative's 3-way tie has no - // heuristic winner, so resolution must skip it for the unique second one. + // Off: `resolveSelectorChain`'s `!options.disambiguateAmbiguous` clause + // must skip the first alternative even though it is resolvable in + // principle (see the next test, same fixture, flag flipped). allowDisambiguation: false, }); @@ -99,12 +105,14 @@ test('P5 port cell 5: matchCount/domain come from the resolved alternative, not assert.equal(result.matchCount, 1); }); -test('P5 port cell 5: with disambiguation enabled, a resolvable first alternative supplies its OWN full domain, not the second alternative it never tries', () => { - // Contrast case: the third "Decoy" is uniquely deepest, so disambiguation - // resolves the first alternative directly (resolution never reaches - // "id=save" at all). Classification's domain must be the first - // alternative's own 3-way match count, not 1 (what the second alternative - // would have produced). +test('P5 port cell 5: the SAME fixture with allowDisambiguation on resolves through the first alternative and uses ITS domain instead', () => { + // Proof that the first alternative in the test above was genuinely + // resolvable (not an exact tie like the pre-existing + // session-replay-target-classification.test.ts regression): the uniquely + // deepest third "Decoy" lets disambiguation pick a winner directly, so + // resolution never reaches "id=save" at all. Classification's domain must + // be the first alternative's own 3-way match count, not 1 (what the second, + // untried alternative would have produced). const nodes = decoyAndSaveTree(); const result = classifyReplayTarget({ recorded: { ...saveRecorded(), role: 'button', label: 'Decoy', sibling: 2 },