diff --git a/examples/test-app/app.config.js b/examples/test-app/app.config.js index f5d38d3ce..b51fc3090 100644 --- a/examples/test-app/app.config.js +++ b/examples/test-app/app.config.js @@ -37,6 +37,7 @@ module.exports = { android: { package: 'com.callstack.agentdevicelab', predictiveBackGestureEnabled: false, + softwareKeyboardLayoutMode: 'pan', }, }, }; diff --git a/packages/ad-script/src/internal/__tests__/script.test.ts b/packages/ad-script/src/internal/__tests__/script.test.ts index 2fa055859..d4f30e9fd 100644 --- a/packages/ad-script/src/internal/__tests__/script.test.ts +++ b/packages/ad-script/src/internal/__tests__/script.test.ts @@ -50,6 +50,33 @@ test('formatPortableActionLine preserves inline open runtime hints', () => { ); }); +test('open replay script round-trips explicit Android test IME selection', () => { + const actions: SessionAction[] = [ + { + ts: Date.now(), + command: 'open', + positionals: ['Demo'], + flags: { testIme: false }, + }, + { + ts: Date.now(), + command: 'open', + positionals: ['Demo'], + flags: { testIme: true }, + }, + ]; + + const script = formatReplayScriptForTest(actions); + assert.match(script, /open "Demo" --no-test-ime/); + assert.match(script, /open "Demo" --test-ime/); + + const parsed = parseReplayScriptDetailed(script).actions; + assert.equal(parsed[0]?.flags.testIme, false); + assert.equal(parsed[1]?.flags.testIme, true); + assert.deepEqual(parsed[0]?.positionals, ['Demo']); + assert.deepEqual(parsed[1]?.positionals, ['Demo']); +}); + test('record replay script parses fps, quality, and hide-touches flags', () => { const script = 'record start "./capture.mp4" --fps 24 --quality high --hide-touches\n'; const parsed = parseReplayScriptDetailed(script).actions; diff --git a/packages/ad-script/src/internal/open-script.ts b/packages/ad-script/src/internal/open-script.ts index f8e2fd1ac..afb86dfa2 100644 --- a/packages/ad-script/src/internal/open-script.ts +++ b/packages/ad-script/src/internal/open-script.ts @@ -46,6 +46,11 @@ export function appendOpenActionScriptArgs( if (action.flags?.relaunch) { parts.push('--relaunch'); } + if (action.flags?.testIme === true) { + parts.push('--test-ime'); + } else if (action.flags?.testIme === false) { + parts.push('--no-test-ime'); + } appendRuntimeHintFlags(parts, action.runtime); } @@ -61,6 +66,14 @@ export function parseReplayOpenFlags(args: string[]): { flags.relaunch = true; continue; } + if (token === '--test-ime') { + flags.testIme = true; + continue; + } + if (token === '--no-test-ime') { + flags.testIme = false; + continue; + } argsWithoutRelaunch.push(token); } const parsedRuntime = parseReplayRuntimeFlags(argsWithoutRelaunch); diff --git a/packages/selectors/src/index.test.ts b/packages/selectors/src/index.test.ts index 857cd53fc..e1ce1b366 100644 --- a/packages/selectors/src/index.test.ts +++ b/packages/selectors/src/index.test.ts @@ -1,13 +1,15 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; import type { SnapshotNode } from '@agent-device/kernel/snapshot'; +import * as selectorsFacade from './index.ts'; import { buildSelectorCandidates, readReplaySelectorDisplayValue, readSelectorExpression, resolveRecordedTarget, resolveReplaySuggestionCandidate, - resolveSelectorChain, + resolveSelectorChainWithPolicy, + SELECTOR_RESOLUTION_POLICIES, } from './index.ts'; const saveNode: SnapshotNode = { @@ -177,11 +179,38 @@ test('replay suggestion resolution and display values stay string-only at the fa assert.equal(readReplaySelectorDisplayValue('label="Save"'), 'Save'); assert.equal(readReplaySelectorDisplayValue('label="Save" || label="Draft"'), undefined); - const resolved = resolveSelectorChain([saveNode], 'id="save"', { - platform: 'ios', - requireRect: true, - requireUnique: true, - }); - assert.equal(resolved?.selector, 'id="save"'); - assert.equal(typeof resolved?.selector, 'string'); + const resolved = resolveSelectorChainWithPolicy( + [saveNode], + 'id="save"', + SELECTOR_RESOLUTION_POLICIES.act, + { platform: 'ios' }, + ); + assert.equal(resolved.kind, 'resolved'); + assert.equal(resolved.kind === 'resolved' ? resolved.resolution.selector : null, 'id="save"'); +}); + +/** + * #1630 made `resolveSelectorChainWithPolicy` the façade's ONLY resolution + * entry: a native caller states its ambiguity contract by naming a policy row + * because there is no knob-taking or count-only resolver here to state it + * inline with instead. + * + * This guard is structural on purpose. Restoring either removed lookup is + * BEHAVIOURALLY invisible — `findSelectorChainMatch` is equivalent to the + * `readAny` row it was migrated to, which is exactly why that migration + * preserved semantics — so no fixture-tree assertion can catch a revert + * (#1715 review). The absence of the symbol is the only observable. + */ +test('the façade exposes no resolver that bypasses the policy matrix', () => { + const exported = Object.keys(selectorsFacade); + assert.ok(exported.includes('resolveSelectorChainWithPolicy')); + assert.ok(!exported.includes('resolveSelectorChain'), 'knob-taking resolver must stay private'); + assert.ok( + !exported.includes('findSelectorChainMatch'), + 'count-only existence lookup must stay private; `is exists` names the readAny row', + ); + assert.ok( + !exported.includes('selectorResolutionKnobs'), + 'knob derivation must stay private so a call site cannot rebuild a contract from knobs', + ); }); diff --git a/packages/selectors/src/index.ts b/packages/selectors/src/index.ts index 54a9191a7..03374a8dd 100644 --- a/packages/selectors/src/index.ts +++ b/packages/selectors/src/index.ts @@ -2,11 +2,8 @@ import type { SnapshotState } from '@agent-device/kernel/snapshot'; import type { Selector } from './internal/parse.ts'; import type { PolicyResolutionOutcome, - SelectorChainMatch, SelectorChainMatchList, SelectorMatchOptions, - SelectorResolution, - SelectorResolutionOptions, } from './internal/public-resolution-types.ts'; import { resolveSelectorChainWithPolicy as resolveSelectorChainWithPolicyAst } from './internal/resolve-with-policy.ts'; import { @@ -34,9 +31,7 @@ import { normalizeIsPositionals, } from './internal/predicates.ts'; import { - findSelectorChainMatch as findSelectorChainMatchAst, listSelectorChainMatches as listSelectorChainMatchesAst, - resolveSelectorChain as resolveSelectorChainAst, selectorFailureHint, STALE_REF_HINT, } from './internal/resolve.ts'; @@ -64,7 +59,6 @@ export type { IsPredicate } from './internal/predicates.ts'; export type { PolicyResolutionOutcome, SelectorChainMatchList, - SelectorChainMatch, SelectorResolution, } from './internal/public-resolution-types.ts'; export { formatSelectorFailure } from './internal/resolve.ts'; @@ -81,7 +75,6 @@ export { detectUnknownSelectorKeyToken, evaluateIsPredicate, findBestMatchesByLocator, - findSelectorChainMatch, isReadOnlyFindAction, normalizeFindActionToken, isRoleHintWord, @@ -99,7 +92,6 @@ export { readSelectorExpression, resolveRecordedTarget, resolveReplaySuggestionCandidate, - resolveSelectorChain, selectorFailureHint, selectorContainsValue, splitSelectorFromArgs, @@ -233,16 +225,6 @@ function validateSelectorExpression(expression: string): void { parseSelectorChain(expression); } -/** Public façade wrapper that accepts/returns selector text, never an AST. */ -function findSelectorChainMatch( - nodes: SnapshotState['nodes'], - expression: string, - options: SelectorMatchOptions, -): SelectorChainMatch | null { - const result = findSelectorChainMatchAst(nodes, parseSelectorChain(expression), options); - return result ? { ...result, selector: result.selector.raw } : null; -} - /** Public façade wrapper that accepts/returns selector text, never an AST. */ function listSelectorChainMatches( nodes: SnapshotState['nodes'], @@ -253,28 +235,16 @@ function listSelectorChainMatches( return result ? { ...result, selector: result.selector.raw } : null; } -/** Public façade wrapper that accepts/returns selector text, never an AST. */ -function resolveSelectorChain( - nodes: SnapshotState['nodes'], - expression: string, - options: SelectorResolutionOptions, -): SelectorResolution | null { - const result = resolveSelectorChainAst(nodes, parseSelectorChain(expression), options); - return result ? { ...result, selector: result.selector.raw } : null; -} -export { - SELECTOR_RESOLUTION_POLICIES, - selectorResolutionKnobs, -} from './internal/resolution-policy.ts'; -export type { - KnobBackedSelectorAmbiguity, - SelectorResolutionPolicy, -} from './internal/resolution-policy.ts'; +export { SELECTOR_RESOLUTION_POLICIES } from './internal/resolution-policy.ts'; +export type { SelectorResolutionPolicy } from './internal/resolution-policy.ts'; import type { SelectorResolutionPolicy } from './internal/resolution-policy.ts'; /** - * Public façade wrapper that accepts selector text and returns selector text — - * never an AST, in either direction. + * The façade's ONLY selector-resolution entry (#1630): every native consumer + * of "resolve a selector against the screen" states its contract by naming a + * `SELECTOR_RESOLUTION_POLICIES` row, because there is no knob-taking resolver + * here to state it inline with instead. Accepts selector text and returns + * selector text — never an AST, in either direction. * * The return leg is the half that is easy to miss: the parser-side outcome * carries the winning `Selector` node inside `resolution`, and returning it @@ -282,8 +252,8 @@ import type { SelectorResolutionPolicy } from './internal/resolution-policy.ts'; * hands through a nested field. The façade's own boundary gate reads exported * *names*, so it cannot see that; `selector-wait.ts` reading * `outcome.resolution.selector.raw` was the runtime proof it had happened. - * Flattening here is the same treatment `resolveSelectorChain` above gives - * `AstSelectorResolution` (#1589). + * Flattening here is the same treatment `listSelectorChainMatches` above gives + * its own selector node (#1589). */ function resolveSelectorChainWithPolicy( nodes: SnapshotState['nodes'], diff --git a/packages/selectors/src/internal/public-resolution-types.ts b/packages/selectors/src/internal/public-resolution-types.ts index c1cf649dc..c7a365ca0 100644 --- a/packages/selectors/src/internal/public-resolution-types.ts +++ b/packages/selectors/src/internal/public-resolution-types.ts @@ -45,11 +45,14 @@ export type PolicyResolutionOutcome = /** No selector alternative matched anything. */ | { kind: 'none' } /** - * The node this policy authorizes acting on, plus the full candidate set of - * the alternative it came from. Callers that verify identity across - * candidates (wait's #1349 landmark check) need the whole set — a policy - * that picks one winner must not throw the rest away, or a first impostor - * would hide a later genuine match. + * The node this policy authorizes acting on, plus the candidate set of the + * first matching alternative. Callers that verify identity across candidates + * (wait's #1349 landmark check) need the whole set — a policy that picks one + * winner must not throw the rest away, or a first impostor would hide a + * later genuine match. Those callers use `first-match` rows, where the two + * fields describe the same alternative by construction; a uniqueness row can + * resolve from a LATER alternative, so pair `matchedNodes` with + * `resolution.selector` before reading them as one set. */ | { kind: 'resolved'; resolution: SelectorResolution; matchedNodes: SnapshotNode[] } /** @@ -66,14 +69,6 @@ export type SelectorChainMatchList = { matchedNodes: SnapshotNode[]; }; -/** A first-match lookup used by existence checks. */ -export type SelectorChainMatch = { - selectorIndex: number; - selector: string; - matches: number; - diagnostics: SelectorDiagnostics[]; -}; - /** * The options every selector lookup takes. Stated once here rather than inline * per function so a façade wrapper and the parser-side function it forwards to diff --git a/packages/selectors/src/internal/replay.ts b/packages/selectors/src/internal/replay.ts index b84b002a7..eb677d497 100644 --- a/packages/selectors/src/internal/replay.ts +++ b/packages/selectors/src/internal/replay.ts @@ -6,6 +6,7 @@ import { splitIsSelectorArgs, splitSelectorFromArgs } from './arguments.ts'; import { buildSelectorChainForNode } from './build.ts'; import { matchesSelector } from './match.ts'; import { tryParseSelectorChain } from './parse.ts'; +import { selectorResolutionKnobs } from './resolution-policy.ts'; import { listSelectorChainMatches, resolveSelectorChain } from './resolve.ts'; /** @@ -78,6 +79,23 @@ export function readSelectorExpression( return { kind: 'expression', expression: split.selectorExpression, rest: split.rest }; } +/** + * A replay policy states its ambiguity contract per request rather than per + * caller, so it names an ambiguity KIND from the same vocabulary the static + * matrix uses and derives its engine knobs through the same function (#1630) — + * `selectorResolutionKnobs` stays the only place `requireUnique` and + * `disambiguateAmbiguous` are named. + */ +function recordedTargetResolutionOptions(policy: ReplayRecordedTargetPolicy) { + return { + platform: policy.platform, + ...selectorResolutionKnobs({ + ambiguity: policy.allowDisambiguation ? 'disambiguate' : 'fail-closed', + requireRect: policy.requireRect, + }), + }; +} + /** Resolve a recorded target and return the winning node plus its same-alternative domain. */ export function resolveRecordedTarget( expression: string, @@ -86,12 +104,7 @@ export function resolveRecordedTarget( ): ReplayRecordedTargetResolution { const chain = tryParseSelectorChain(expression); if (!chain) return { kind: 'unresolved', reason: 'parse-invalid', matchedNodes: [] }; - const resolved = resolveSelectorChain(nodes, chain, { - platform: policy.platform, - requireRect: policy.requireRect, - requireUnique: true, - disambiguateAmbiguous: policy.allowDisambiguation, - }); + const resolved = resolveSelectorChain(nodes, chain, recordedTargetResolutionOptions(policy)); if (resolved) { const matchedNodes = nodes.filter((node) => { if (policy.requireRect && !node.rect) return false; @@ -147,12 +160,7 @@ export function resolveReplaySuggestionCandidate( ): ReplaySuggestionCandidateMatch | undefined { const chain = tryParseSelectorChain(candidate); if (!chain) return undefined; - const resolved = resolveSelectorChain(nodes, chain, { - platform: policy.platform, - requireRect: policy.requireRect, - requireUnique: true, - disambiguateAmbiguous: policy.allowDisambiguation, - }); + const resolved = resolveSelectorChain(nodes, chain, recordedTargetResolutionOptions(policy)); if (!resolved) return undefined; return { node: resolved.node, basis: classifySuggestionBasis(resolved.selector) }; } diff --git a/packages/selectors/src/internal/resolution-policy.test.ts b/packages/selectors/src/internal/resolution-policy.test.ts new file mode 100644 index 000000000..0207d437b --- /dev/null +++ b/packages/selectors/src/internal/resolution-policy.test.ts @@ -0,0 +1,25 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { SELECTOR_RESOLUTION_POLICIES, selectorResolutionKnobs } from './resolution-policy.ts'; + +/** + * `selectorResolutionKnobs` is package-private (#1630): the façade exports no + * resolver that accepts `requireUnique`/`disambiguateAmbiguous`, so this is the + * only place in the repo that names them and the only place a knob-vs-row + * mismatch can be introduced. The row-level BEHAVIOR each mapping produces is + * pinned separately, through the public interface, in + * src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts. + */ +test('knobs stay consistent with the ambiguity each knob-backed row names', () => { + for (const [name, policy] of Object.entries(SELECTOR_RESOLUTION_POLICIES)) { + if (policy.ambiguity === 'reject-candidates') continue; + const knobs = selectorResolutionKnobs(policy); + assert.equal(knobs.requireRect, policy.requireRect, name); + if (policy.ambiguity === 'first-match') { + assert.equal(knobs.requireUnique, false, name); + } else { + assert.equal(knobs.requireUnique, true, name); + assert.equal(knobs.disambiguateAmbiguous, policy.ambiguity === 'disambiguate', name); + } + } +}); diff --git a/packages/selectors/src/internal/resolution-policy.ts b/packages/selectors/src/internal/resolution-policy.ts index 0d46bdcda..e0e61ef44 100644 --- a/packages/selectors/src/internal/resolution-policy.ts +++ b/packages/selectors/src/internal/resolution-policy.ts @@ -22,8 +22,9 @@ import type { SelectorResolutionOptions } from './public-resolution-types.ts'; * * Scope, deliberately narrow: this matrix declares the **ambiguity contract * and the rect requirement**, and nothing else. Both are consumed by - * `resolveSelectorChainWithPolicy` and pinned behaviorally in - * resolution-policy-parity.test.ts, so a row that stops matching its + * `resolveSelectorChainWithPolicy` — the one door native callers have, since + * the package façade exports no knob-taking resolver — and pinned behaviorally + * in resolution-policy-parity.test.ts, so a row that stops matching its * documented semantics fails a test. * * The surrounding pipeline stages — occlusion, the off-screen guard, @@ -82,8 +83,11 @@ export const SELECTOR_RESOLUTION_POLICIES = { } as const satisfies Record; /** - * The engine knobs a knob-backed policy row stands for. `reject-candidates` - * rows are rejected at the type level — that contract is enforced by caller + * The engine knobs a knob-backed policy row stands for, and the only place in + * the repo that names `requireUnique`/`disambiguateAmbiguous` (#1630): the + * façade exports no resolver that accepts them, so a caller cannot restate an + * ambiguity contract as knobs even by accident. `reject-candidates` rows are + * rejected at the type level — that contract is enforced by caller * classification, not by these knobs. */ export function selectorResolutionKnobs( diff --git a/packages/selectors/src/internal/resolve-with-policy.ts b/packages/selectors/src/internal/resolve-with-policy.ts index 2cccd7129..c60114734 100644 --- a/packages/selectors/src/internal/resolve-with-policy.ts +++ b/packages/selectors/src/internal/resolve-with-policy.ts @@ -6,7 +6,7 @@ import { resolveSelectorChain, type AstSelectorResolution, } from './resolve.ts'; -import type { SelectorResolutionPolicy } from './resolution-policy.ts'; +import { selectorResolutionKnobs, type SelectorResolutionPolicy } from './resolution-policy.ts'; /** * The one policy-driven resolution entry every native caller routes through @@ -27,11 +27,14 @@ export type AstPolicyResolutionOutcome = /** No selector alternative matched anything. */ | { kind: 'none' } /** - * The node this policy authorizes acting on, plus the full candidate set - * of the alternative it came from. Callers that verify identity across - * candidates (wait's #1349 landmark check) need the whole set — a policy - * that picks one winner must not throw the rest away, or a first impostor - * would hide a later genuine match. + * The node this policy authorizes acting on, plus the candidate set of the + * first matching alternative. Callers that verify identity across candidates + * (wait's #1349 landmark check) need the whole set — a policy that picks one + * winner must not throw the rest away, or a first impostor would hide a + * later genuine match. Those callers use `first-match` rows, where the two + * fields describe the same alternative by construction; a uniqueness row can + * resolve from a LATER alternative, so pair `matchedNodes` with + * `resolution.selector` before reading them as one set. */ | { kind: 'resolved'; @@ -56,46 +59,39 @@ export function resolveSelectorChainWithPolicy( policy: SelectorResolutionPolicy, options: SelectorMatchOptions, ): AstPolicyResolutionOutcome { - const matchOptions = { ...options, requireRect: policy.requireRect }; - - if (policy.ambiguity === 'reject-candidates') { - const list = listSelectorChainMatches(nodes, chain, matchOptions); - if (!list || list.matchedNodes.length === 0) return { kind: 'none' }; - if (list.matchedNodes.length > 1) { - return { - kind: 'ambiguous', - selector: list.selector.raw, - selectorIndex: list.selectorIndex, - matchedNodes: list.matchedNodes, - }; - } - return resolvedFromList(list); - } + const ambiguity = policy.ambiguity; + // One matching pass serves every row. It also settles "nothing matched" + // once, up front: no alternative matching under this row's rect requirement + // is the only way any row reaches `none`, uniqueness included — a + // fail-closed row that finds candidates it will not choose between reports + // ambiguity, never absence. + const list = listSelectorChainMatches(nodes, chain, { + ...options, + requireRect: policy.requireRect, + }); + if (!list || list.matchedNodes.length === 0) return { kind: 'none' }; - if (policy.ambiguity === 'first-match') { - const list = listSelectorChainMatches(nodes, chain, matchOptions); - if (!list || list.matchedNodes.length === 0) return { kind: 'none' }; - return resolvedFromList(list); + if (ambiguity === 'first-match') return resolvedFromList(list); + if (ambiguity === 'reject-candidates') { + return list.matchedNodes.length > 1 ? ambiguousFromList(list) : resolvedFromList(list); } + // The uniqueness knobs are named in exactly one place (#1630); the rows + // handled above leave only the two that map onto them. A resolution here may + // come from a LATER alternative than `list`, since uniqueness skips an + // ambiguous alternative to try the next one. const resolution = resolveSelectorChain(nodes, chain, { - ...matchOptions, - requireUnique: true, - disambiguateAmbiguous: policy.ambiguity === 'disambiguate', + ...options, + ...selectorResolutionKnobs({ ambiguity, requireRect: policy.requireRect }), }); - if (resolution) { - const list = listSelectorChainMatches(nodes, chain, matchOptions); - return { - kind: 'resolved', - resolution, - matchedNodes: list?.matchedNodes ?? [resolution.node], - }; - } + return resolution + ? { kind: 'resolved', resolution, matchedNodes: list.matchedNodes } + : ambiguousFromList(list); +} - // Distinguish "nothing matched" from "matched but this policy will not - // choose" — a fail-closed caller must report ambiguity, not absence. - const list = listSelectorChainMatches(nodes, chain, matchOptions); - if (!list || list.matchedNodes.length === 0) return { kind: 'none' }; +function ambiguousFromList( + list: NonNullable>, +): AstPolicyResolutionOutcome { return { kind: 'ambiguous', selector: list.selector.raw, diff --git a/packages/selectors/src/internal/resolve.ts b/packages/selectors/src/internal/resolve.ts index 6b1394e60..b30e3f6d9 100644 --- a/packages/selectors/src/internal/resolve.ts +++ b/packages/selectors/src/internal/resolve.ts @@ -99,7 +99,11 @@ export function listSelectorChainMatches( return null; } -/** The parser-side twin of the façade's `SelectorChainMatch`. */ +/** + * A first-match lookup used by existence checks. No façade twin: the root + * façade resolves through the policy interface only, so this shape reaches + * consumers via the published `./ast` surface alone (#1630). + */ export type AstSelectorChainMatch = { selectorIndex: number; selector: Selector; diff --git a/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts b/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts index e7c1a1bf9..6933a9f87 100644 --- a/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts +++ b/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts @@ -4,15 +4,18 @@ import type { SnapshotNode } from '@agent-device/kernel/snapshot'; import { SELECTOR_RESOLUTION_POLICIES, resolveSelectorChainWithPolicy, - selectorResolutionKnobs, } from '@agent-device/selectors'; /** * The matrix is exercised through the interface callers actually use - * (`resolveSelectorChainWithPolicy`) against fixture trees, so each row's - * ambiguity contract is proven behaviorally rather than asserted about - * source text. A row that stops matching its documented semantics fails - * here even though the declaration still reads plausibly. + * (`resolveSelectorChainWithPolicy` — since #1630 the façade's only + * resolution entry) against fixture trees, so each row's ambiguity contract + * is proven behaviorally rather than asserted about source text. A row that + * stops matching its documented semantics fails here even though the + * declaration still reads plausibly. + * + * Which row each CALLER consumes is a separate question, pinned end to end in + * selector-read-policy.test.ts. */ function node(index: number, label: string, overrides: Partial = {}): SnapshotNode { @@ -159,20 +162,6 @@ test('rect-requiring rows skip rectless nodes; read and wait rows accept them', } }); -test('knobs stay consistent with the ambiguity each knob-backed row names', () => { - for (const [name, policy] of Object.entries(SELECTOR_RESOLUTION_POLICIES)) { - if (policy.ambiguity === 'reject-candidates') continue; - const knobs = selectorResolutionKnobs(policy); - assert.equal(knobs.requireRect, policy.requireRect, name); - if (policy.ambiguity === 'first-match') { - assert.equal(knobs.requireUnique, false, name); - } else { - assert.equal(knobs.requireUnique, true, name); - assert.equal(knobs.disambiguateAmbiguous, policy.ambiguity === 'disambiguate', name); - } - } -}); - /** * The matrix may only declare what it can enforce (#1649 review). An earlier * revision carried occlusion / off-screen / promotion / poll columns that no diff --git a/src/commands/interaction/runtime/__tests__/selector-read-policy.test.ts b/src/commands/interaction/runtime/__tests__/selector-read-policy.test.ts new file mode 100644 index 000000000..e5e9fc9b4 --- /dev/null +++ b/src/commands/interaction/runtime/__tests__/selector-read-policy.test.ts @@ -0,0 +1,131 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { selector } from '../selector-read-utils.ts'; +import { + ambiguousSelectorReadSnapshot, + createSelectorDevice, + skippedAlternativeSelectorSnapshot, +} from './test-utils/index.ts'; + +/** + * #1630: the read commands share one resolution interface and differ ONLY by + * the `SELECTOR_RESOLUTION_POLICIES` row each names. resolution-policy-parity + * proves what a row means to the engine; these prove which row each command + * actually consumes, end to end, so re-pointing a caller at a different row + * fails here instead of silently changing what `is` or `get` binds to. + * + * One ambiguous fixture drives all four, because that is the only screen on + * which the rows disagree: a unique match resolves identically under every + * one of them. + */ + +const AMBIGUOUS_SELECTOR = 'label="Save"'; + +/** The tiebreak winner: deeper and smaller than its same-label ancestor. */ +const DISAMBIGUATED_REF = '@e3'; +/** Document order head, which the first-match rows take instead. */ +const FIRST_MATCH_REF = '@e2'; + +test('get text disambiguates an ambiguous selector (readText row)', async () => { + const device = createSelectorDevice(ambiguousSelectorReadSnapshot(), { + readText: 'Save', + }); + + const result = await device.selectors.getText(selector(AMBIGUOUS_SELECTOR), { + session: 'default', + }); + + assert.equal(result.kind, 'text'); + assert.equal(`@${result.node.ref}`, DISAMBIGUATED_REF); +}); + +test('get attrs fails closed on the same ambiguous selector (readUnique row)', async () => { + const device = createSelectorDevice(ambiguousSelectorReadSnapshot()); + + const error = await device.selectors + .getAttrs(selector(AMBIGUOUS_SELECTOR), { session: 'default' }) + .then( + () => null, + (thrown: unknown) => thrown, + ); + + assert.ok(error instanceof AppError, 'get attrs must refuse rather than guess a duplicate'); + assert.equal(error.code, 'COMMAND_FAILED'); +}); + +test('is fails closed on the same ambiguous selector (readUnique row)', async () => { + const device = createSelectorDevice(ambiguousSelectorReadSnapshot()); + + const error = await device.selectors + .is({ session: 'default', predicate: 'visible', selector: AMBIGUOUS_SELECTOR }) + .then( + () => null, + (thrown: unknown) => thrown, + ); + + assert.ok(error instanceof AppError, 'is must refuse rather than answer about one duplicate'); + assert.equal(error.code, 'COMMAND_FAILED'); + assert.equal((error.details as { reason?: string } | undefined)?.reason, 'selector_not_found'); +}); + +test('is exists answers from the first matching alternative (readAny row)', async () => { + const device = createSelectorDevice(skippedAlternativeSelectorSnapshot()); + + // `exists` exposes no node ref, so the row has to be read off WHICH + // alternative answered. On a fixture whose first alternative is merely + // tiebreakable, `pass: true` + the same count come out of first-match, + // disambiguation, and the pre-#1630 raw lookup alike — which is why the + // first alternative here is an undecidable tie: only a first-match row + // answers from it (#1715 review). + const result = await device.selectors.is({ + session: 'default', + predicate: 'exists', + selector: 'label="Save" || id="save-unique"', + }); + + assert.equal(result.pass, true); + assert.equal(result.selector, 'label="Save"'); + assert.equal(result.matches, 2); +}); + +test('is exists tolerates the ambiguity its sibling predicates refuse (readAny row)', async () => { + const device = createSelectorDevice(ambiguousSelectorReadSnapshot()); + + // Caller-facing contrast on the shared fixture: the same screen and selector + // the fail-closed `is` above refuses, answered here because presence is a + // different question from "which one". + const result = await device.selectors.is({ + session: 'default', + predicate: 'exists', + selector: AMBIGUOUS_SELECTOR, + }); + + assert.equal(result.pass, true); + assert.equal(result.matches, 2); +}); + +test('find takes the document-order head on the same tree (readAny row)', async () => { + const device = createSelectorDevice(ambiguousSelectorReadSnapshot()); + + // `get_attrs`, not `exists`: WHICH node the row selected has to be + // observable, or the assertion cannot separate `readAny` from the + // disambiguating row — `found: true` holds either way while the selection + // silently moves to the tiebreak winner (#1715 review). + const attrs = await device.selectors.find({ + session: 'default', + query: AMBIGUOUS_SELECTOR, + action: 'get_attrs', + }); + assert.equal(attrs.kind, 'attrs'); + assert.equal(attrs.kind === 'attrs' ? attrs.ref : undefined, FIRST_MATCH_REF); + + // The presence contract rides the same resolution, so a row that refuses an + // ambiguous screen would fail here rather than answering `found: true`. + const exists = await device.selectors.find({ + session: 'default', + query: AMBIGUOUS_SELECTOR, + action: 'exists', + }); + assert.deepEqual(exists, { kind: 'found', found: true }); +}); diff --git a/src/commands/interaction/runtime/__tests__/test-utils/index.ts b/src/commands/interaction/runtime/__tests__/test-utils/index.ts index 56d56ee2f..75d4629fe 100644 --- a/src/commands/interaction/runtime/__tests__/test-utils/index.ts +++ b/src/commands/interaction/runtime/__tests__/test-utils/index.ts @@ -406,6 +406,93 @@ export function selectorReadSnapshot(): SnapshotState { ]); } +/** + * Two nodes share the label `Save` but differ in depth and area, so the + * engine's visible→deepest→smallest-area tiebreak CAN pick a winner. That is + * exactly the tree on which the read rows disagree (#1630): `get text` + * disambiguates to the inner node, `is` and `get attrs` fail closed on the + * same screen, and `find exists` takes the first match. + */ +export function ambiguousSelectorReadSnapshot(): SnapshotState { + return makeSnapshotState([ + { + index: 0, + depth: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 400, height: 800 }, + }, + { + index: 1, + depth: 1, + parentIndex: 0, + type: 'Button', + label: 'Save', + rect: { x: 0, y: 0, width: 300, height: 200 }, + hittable: true, + }, + { + index: 2, + depth: 2, + parentIndex: 1, + type: 'Button', + label: 'Save', + rect: { x: 10, y: 10, width: 80, height: 24 }, + hittable: true, + }, + ]); +} + +/** + * The tree that separates first-match from uniqueness rows (#1715 review). + * Alternative one (`label="Save"`) matches two nodes that are genuinely + * indistinguishable — same depth, same area, both on screen — so the engine's + * tiebreak DECLINES. Alternative two (`id="save-unique"`) matches exactly one. + * + * `first-match` therefore answers from alternative one (2 matches), while every + * uniqueness row skips the undecidable alternative and answers from alternative + * two (1 match). A fixture whose first alternative is merely *tiebreakable* + * cannot tell those apart: disambiguation succeeds there and reports the same + * count first-match would. + */ +export function skippedAlternativeSelectorSnapshot(): SnapshotState { + return makeSnapshotState([ + { + index: 0, + depth: 0, + type: 'Application', + rect: { x: 0, y: 0, width: 400, height: 800 }, + }, + { + index: 1, + depth: 1, + parentIndex: 0, + type: 'Button', + label: 'Save', + rect: { x: 0, y: 40, width: 100, height: 30 }, + hittable: true, + }, + { + index: 2, + depth: 1, + parentIndex: 0, + type: 'Button', + label: 'Save', + rect: { x: 0, y: 120, width: 100, height: 30 }, + hittable: true, + }, + { + index: 3, + depth: 1, + parentIndex: 0, + type: 'Button', + identifier: 'save-unique', + label: 'Confirm', + rect: { x: 0, y: 200, width: 100, height: 30 }, + hittable: true, + }, + ]); +} + export function createSelectorDevice( snapshot: SnapshotState, options: { diff --git a/src/commands/interaction/runtime/resolution.test.ts b/src/commands/interaction/runtime/resolution.test.ts index 4ab25db39..c404af06d 100644 --- a/src/commands/interaction/runtime/resolution.test.ts +++ b/src/commands/interaction/runtime/resolution.test.ts @@ -7,7 +7,7 @@ import { throwIfOffscreenInteractionTarget, tryResolveRefNode, } from './resolution.ts'; -import { resolveSelectorChain } from '@agent-device/selectors'; +import { resolveRecordedTarget } from '@agent-device/selectors'; import { makeSnapshotState } from '../../../__tests__/test-utils/index.ts'; import type { Point } from '@agent-device/kernel/snapshot'; import { @@ -375,12 +375,12 @@ test('runtime fill #1280: fill is excluded from retargeting — the chain stays assert.deepEqual(result.selectorChain, ['role="edittext" editable=true']); // ...and it resolves back to the editable container on the record-time // tree — the saved script stays replayable. - const resolved = resolveSelectorChain(snapshot.nodes, result.selectorChain!.join(' || '), { + const resolved = resolveRecordedTarget(result.selectorChain!.join(' || '), snapshot.nodes, { platform: 'android', requireRect: true, - requireUnique: true, + allowDisambiguation: false, }); - assert.equal(resolved?.node.type, 'EditText'); + assert.equal(resolved.kind === 'resolved' ? resolved.winner.type : undefined, 'EditText'); }); test('runtime fill surfaces targetHittable and a hint for a non-hittable selector match (Maps pin case, #1037)', async () => { diff --git a/src/commands/interaction/runtime/resolution.ts b/src/commands/interaction/runtime/resolution.ts index 975eb8b82..fe245a3f5 100644 --- a/src/commands/interaction/runtime/resolution.ts +++ b/src/commands/interaction/runtime/resolution.ts @@ -9,13 +9,12 @@ import type { } from '../../../runtime-contract.ts'; import { formatSelectorFailure, - resolveSelectorChain, + resolveSelectorChainWithPolicy, selectorFailureHint, STALE_REF_HINT, type SelectorResolution, buildSelectorChainForNode, SELECTOR_RESOLUTION_POLICIES, - selectorResolutionKnobs, } from '@agent-device/selectors'; import { resolvePressRecordingTarget } from '../../../core/press-retarget.ts'; import { requireSnapshotSession } from './selector-read-shared.ts'; @@ -316,7 +315,6 @@ async function resolveRefInteractionTarget( }; } -// fallow-ignore-next-line complexity async function resolveSelectorInteractionTarget( runtime: AgentDeviceRuntime, options: CommandContext, @@ -339,23 +337,13 @@ async function resolveSelectorInteractionTarget( ); } if (!resolved || !resolved.node.rect) { - const covered = resolveSelectorChain(capture.snapshot.nodes, selectorExpression, { - platform: runtime.backend.platform, - ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.actCoveredDiagnosis), + throw selectorInteractionFailure({ + runtime, + nodes: capture.snapshot.nodes, + selectorExpression, + action: params.action, + resolved, }); - if (covered?.node && isSnapshotNodeInteractionBlocked(covered.node)) { - throw buildCoveredInteractionError({ - label: `Selector ${covered.selector}`, - node: covered.node, - action: params.action, - selector: covered.selector, - }); - } - throw new AppError( - 'COMMAND_FAILED', - formatSelectorFailure(selectorExpression, resolved?.diagnostics ?? [], { unique: true }), - { hint: selectorFailureHint(resolved?.diagnostics ?? []) }, - ); } assertReplayTargetResolution(resolved.node, capture.snapshot.nodes, params); const node = params.promoteToHittableAncestor @@ -392,6 +380,44 @@ async function resolveSelectorInteractionTarget( }; } +/** + * No usable acting target. Before reporting "did not match", re-probe the same + * tree through the diagnosis row: a selector that DOES match but landed on a + * covered node is a different failure with a different recovery, and the + * acting row — rect-required, candidates rejected — cannot tell the caller + * that. Both probes name a policy row, so the two contracts stay visible side + * by side instead of as two sets of engine knobs (#1630). + */ +function selectorInteractionFailure(params: { + runtime: AgentDeviceRuntime; + nodes: SnapshotState['nodes']; + selectorExpression: string; + action: InteractionAction; + resolved: SelectorResolution | null; +}): AppError { + const { runtime, nodes, selectorExpression, action, resolved } = params; + const covered = resolveSelectorChainWithPolicy( + nodes, + selectorExpression, + SELECTOR_RESOLUTION_POLICIES.actCoveredDiagnosis, + { platform: runtime.backend.platform }, + ); + if (covered.kind === 'resolved' && isSnapshotNodeInteractionBlocked(covered.resolution.node)) { + return buildCoveredInteractionError({ + label: `Selector ${covered.resolution.selector}`, + node: covered.resolution.node, + action, + selector: covered.resolution.selector, + }); + } + const diagnostics = resolved?.diagnostics ?? []; + return new AppError( + 'COMMAND_FAILED', + formatSelectorFailure(selectorExpression, diagnostics, { unique: true }), + { hint: selectorFailureHint(diagnostics) }, + ); +} + function assertReplayTargetResolution( node: SnapshotNode, nodes: SnapshotState['nodes'], diff --git a/src/commands/interaction/runtime/selector-read.ts b/src/commands/interaction/runtime/selector-read.ts index 5bae2a125..108816e39 100644 --- a/src/commands/interaction/runtime/selector-read.ts +++ b/src/commands/interaction/runtime/selector-read.ts @@ -1,9 +1,8 @@ import { FIND_VALUE_REQUIRED_MESSAGE, findBestMatchesByLocator, - findSelectorChainMatch, formatSelectorFailure, - resolveSelectorChain, + resolveSelectorChainWithPolicy, selectorFailureHint, buildSelectorChainForNode, checkIsPredicate, @@ -15,9 +14,6 @@ import { type FindAction, type FindLocator, SELECTOR_RESOLUTION_POLICIES, - selectorResolutionKnobs, - type KnobBackedSelectorAmbiguity, - type SelectorResolutionPolicy, } from '@agent-device/selectors'; import type { SnapshotNode } from '@agent-device/kernel/snapshot'; import { isSparseSnapshotQualityVerdict } from '../../../snapshot/snapshot-quality.ts'; @@ -55,10 +51,6 @@ import { TINY_STABLE_TREE_NODE_COUNT, } from './stable-capture.ts'; -type KnobBackedResolutionPolicy = SelectorResolutionPolicy & { - ambiguity: KnobBackedSelectorAmbiguity; -}; - export type { SelectorSnapshotOptions } from './selector-read-shared.ts'; export type { WaitCommandOptions, @@ -305,10 +297,18 @@ export const isCommand: RuntimeCommand = asyn }); if (predicate === 'exists') { - const matched = findSelectorChainMatch(capture.snapshot.nodes, selectorExpression, { - platform: runtime.backend.platform, - }); - if (!matched) { + // `readAny`, the same row find's read actions use: presence is the + // question, so any match count passes and the first one answers. The row + // already documented itself as serving `exists`, but this branch used to + // reach the engine directly — the claim was true of the docs and not of + // the code (#1630). + const matched = resolveSelectorChainWithPolicy( + capture.snapshot.nodes, + selectorExpression, + SELECTOR_RESOLUTION_POLICIES.readAny, + { platform: runtime.backend.platform }, + ); + if (matched.kind !== 'resolved') { throw new AppError( 'COMMAND_FAILED', formatSelectorFailure(selectorExpression, [], { unique: false }), @@ -320,17 +320,22 @@ export const isCommand: RuntimeCommand = asyn return { predicate: predicate, pass: true, - selector: matched.selector, - matches: matched.matches, + selector: matched.resolution.selector, + matches: matched.resolution.matches, selectorChain: readSelectorAlternatives(selectorExpression), }; } - const resolved = resolveSelectorChain(capture.snapshot.nodes, selectorExpression, { - platform: runtime.backend.platform, - ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.readUnique), - }); - if (!resolved) { + // `readUnique` is the fail-closed row: an ambiguous screen reports the same + // refusal as no match at all, because `is` must never guess which duplicate + // it answered about. + const outcome = resolveSelectorChainWithPolicy( + capture.snapshot.nodes, + selectorExpression, + SELECTOR_RESOLUTION_POLICIES.readUnique, + { platform: runtime.backend.platform }, + ); + if (outcome.kind !== 'resolved') { throw new AppError( 'COMMAND_FAILED', formatSelectorFailure(selectorExpression, [], { unique: true }), @@ -343,6 +348,7 @@ export const isCommand: RuntimeCommand = asyn }, ); } + const resolved = outcome.resolution; assertExpectedResolvedTarget( resolved.node, capture.snapshot.nodes, @@ -479,11 +485,13 @@ async function findFirstLocatorMatch( throw sparseSelectorSnapshotError(capture.snapshot.snapshotQuality); } if (selectorExpression) { - const resolved = resolveSelectorChain(capture.snapshot.nodes, selectorExpression, { - platform: runtime.backend.platform, - ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.readAny), - }); - return { capture, match: resolved?.node }; + const outcome = resolveSelectorChainWithPolicy( + capture.snapshot.nodes, + selectorExpression, + SELECTOR_RESOLUTION_POLICIES.readAny, + { platform: runtime.backend.platform }, + ); + return { capture, match: outcome.kind === 'resolved' ? outcome.resolution.node : undefined }; } const match = findBestMatchesByLocator(capture.snapshot.nodes, locator, options.query, { requireRect: false, @@ -491,11 +499,19 @@ async function findFirstLocatorMatch( return { capture, match }; } +/** + * `get` names the two rows it may consume by type: `readText` disambiguates + * through the same tiebreak acting uses, `readUnique` fails closed. A row with + * any other ambiguity contract is a compile error here rather than a silent + * change to what `get` will bind to. + */ +type GetResolutionPolicy = (typeof SELECTOR_RESOLUTION_POLICIES)['readText' | 'readUnique']; + async function resolveSelectorNode( runtime: AgentDeviceRuntime, options: GetCommandOptions, sessionName: string, - params: { selector: string; policy: KnobBackedResolutionPolicy }, + params: { selector: string; policy: GetResolutionPolicy }, ): Promise<{ capture: CapturedSnapshot; node: SnapshotNode; selector: string; ref: string }> { const capture = await captureSelectorSnapshot( runtime, @@ -505,11 +521,13 @@ async function resolveSelectorNode( ...deriveSelectorCapturePolicy(), }, ); - const resolved = resolveSelectorChain(capture.snapshot.nodes, params.selector, { - platform: runtime.backend.platform, - ...selectorResolutionKnobs(params.policy), - }); - if (!resolved) { + const outcome = resolveSelectorChainWithPolicy( + capture.snapshot.nodes, + params.selector, + params.policy, + { platform: runtime.backend.platform }, + ); + if (outcome.kind !== 'resolved') { throw new AppError( 'COMMAND_FAILED', formatSelectorFailure(params.selector, [], { unique: true }), @@ -520,8 +538,8 @@ async function resolveSelectorNode( } return { capture, - node: resolved.node, - selector: resolved.selector, - ref: `@${resolved.node.ref}`, + node: outcome.resolution.node, + selector: outcome.resolution.selector, + ref: `@${outcome.resolution.node.ref}`, }; } diff --git a/src/core/press-retarget.test.ts b/src/core/press-retarget.test.ts index 0f6271f77..5eba30fbd 100644 --- a/src/core/press-retarget.test.ts +++ b/src/core/press-retarget.test.ts @@ -3,7 +3,7 @@ import assert from 'node:assert/strict'; import type { SnapshotNode } from '@agent-device/kernel/snapshot'; import { buildNodes } from '../__tests__/test-utils/snapshot-builders.ts'; import { computeTargetEvidence } from '../daemon/session-target-evidence.ts'; -import { buildSelectorChainForNode, resolveSelectorChain } from '@agent-device/selectors'; +import { buildSelectorChainForNode, resolveRecordedTarget } from '@agent-device/selectors'; import { readNodeLocalIdentity } from '@agent-device/ad-script'; import { resolvePressRecordingTarget } from './press-retarget.ts'; @@ -284,14 +284,18 @@ function assertChainAndEvidenceAgreeOnRecordedNode(params: { assert.equal(evidence.role, readNodeLocalIdentity(recordedNode).role); const chain = buildSelectorChainForNode(recordedNode, platform, { action: 'click', nodes }); - const resolved = resolveSelectorChain(nodes, chain.join(' || '), { + const resolved = resolveRecordedTarget(chain.join(' || '), nodes, { platform, requireRect: true, - requireUnique: true, + allowDisambiguation: false, }); - assert.ok(resolved, `chain ${JSON.stringify(chain)} failed to resolve uniquely`); assert.equal( - resolved.node.ref, + resolved.kind, + 'resolved', + `chain ${JSON.stringify(chain)} failed to resolve uniquely`, + ); + assert.equal( + resolved.kind === 'resolved' ? resolved.winner.ref : undefined, recordedNode.ref, 'the recorded chain must resolve back to the exact node evidence was computed for', ); diff --git a/src/daemon/handlers/__tests__/session-replay-selector-routes.test.ts b/src/daemon/handlers/__tests__/session-replay-selector-routes.test.ts new file mode 100644 index 000000000..83abc7e0d --- /dev/null +++ b/src/daemon/handlers/__tests__/session-replay-selector-routes.test.ts @@ -0,0 +1,68 @@ +import path from 'node:path'; +import { beforeEach, expect, test, vi } from 'vitest'; + +import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; +import { mkdtempForTestSync } from '../../../__tests__/test-utils/tmp-dir.ts'; +import { dispatchCommand } from '../../../core/dispatch.ts'; +import { SessionStore } from '../../session-store.ts'; +import { runReplayScriptFile } from '../session-replay-runtime.ts'; +import { baseReplayRequest, writeReplayFile } from './session-replay-runtime.fixtures.ts'; + +vi.mock('../../../core/dispatch.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, dispatchCommand: vi.fn(), resolveTargetDevice: vi.fn() }; +}); + +const mockDispatchCommand = vi.mocked(dispatchCommand); + +beforeEach(() => { + mockDispatchCommand.mockReset(); + mockDispatchCommand.mockRejectedValue(new Error('no device runner available')); +}); + +test('replay executes selector reads before reporting a covered-target divergence', async () => { + const root = mkdtempForTestSync('agent-device-replay-selector-routes-'); + const sessionName = 'default'; + const sessionStore = new SessionStore(path.join(root, 'sessions')); + sessionStore.set(sessionName, makeIosSession(sessionName)); + const filePath = writeReplayFile(root, [ + 'open "Demo"', + 'get text "id=\\"field-name\\""', + 'get attrs "id=\\"field-name\\""', + 'is visible "id=\\"field-name\\""', + 'find id "field-name" get attrs', + 'click "id=\\"field-name\\""', + ]); + const invoked: string[] = []; + + const response = await runReplayScriptFile({ + req: baseReplayRequest({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (request) => { + invoked.push(request.command); + if (request.command !== 'click') return { ok: true, data: {} }; + return { + ok: false, + error: { + code: 'COMMAND_FAILED', + message: 'Target is covered by another visible element', + }, + }; + }, + }); + + expect(invoked).toEqual(['open', 'get', 'get', 'is', 'find', 'click']); + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error.code).toBe('REPLAY_DIVERGENCE'); + expect(response.error.details?.divergence).toMatchObject({ + kind: 'action-failure', + step: { index: 6 }, + cause: { + code: 'COMMAND_FAILED', + message: 'Target is covered by another visible element', + }, + }); +}); 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 3f74d5e7e..6e1a27cc3 100644 --- a/src/daemon/handlers/__tests__/session-replay-target-classification.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-target-classification.test.ts @@ -3,7 +3,7 @@ import assert from 'node:assert/strict'; import type { RawSnapshotNode, SnapshotNode } from '@agent-device/kernel/snapshot'; import type { TargetAnnotationV1 } from '@agent-device/contracts/replay'; import { computeTargetEvidence } from '../../session-target-evidence.ts'; -import { buildSelectorChainForNode, resolveSelectorChain } from '@agent-device/selectors'; +import { buildSelectorChainForNode, resolveRecordedTarget } from '@agent-device/selectors'; import { resolvePressRecordingTarget } from '../../../core/press-retarget.ts'; import { classifyReplayTarget } from '../session-replay-target-classification.ts'; import { @@ -548,18 +548,20 @@ test('#1269 e2e: a demoted shared-id row rebinds by role+label after the shared- // 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, 'id="android:id/title"', { + const idResolved = resolveRecordedTarget('id="android:id/title"', replayNodes, { platform: ANDROID, requireRect: true, - requireUnique: true, + allowDisambiguation: false, }); - assert.equal(idResolved, null); // non-unique: refuses to bind - const labelResolved = resolveSelectorChain( - replayNodes, + // Non-unique, not absent: the reason is what distinguishes "matched four + // rows and refused" from "matched nothing". + assert.equal(idResolved.kind === 'unresolved' ? idResolved.reason : null, 'ambiguous'); + const labelResolved = resolveRecordedTarget( 'role="textview" label="Connected devices"', - { platform: ANDROID, requireRect: true, requireUnique: true }, + replayNodes, + { platform: ANDROID, requireRect: true, allowDisambiguation: false }, ); - assert.equal(labelResolved?.node.ref, expected.ref); + assert.equal(labelResolved.kind === 'resolved' ? labelResolved.winner.ref : null, expected.ref); }); // --------------------------------------------------------------------------- @@ -623,22 +625,22 @@ test('#1280 e2e: a retargeted press on a row container rebinds its labeled desce assertVerified(result, { winnerRef: expected.ref, matchCount: 1 }); // Contrast: recording the CONTAINER itself (no retarget) leaves a - // role-only identity — every row's wrapper shares it — that a - // requireUnique resolve refuses to bind under the same reorder. This is - // the FDR the retarget removes. + // role-only identity — every row's wrapper shares it — that a fail-closed + // replay resolve refuses to bind under the same reorder. This is the FDR + // the retarget removes. const containerChain = buildSelectorChainForNode(container, ANDROID, { action: 'click', nodes: recordNodes, }); assert.deepEqual(containerChain, ['role="linearlayout"']); - const containerResolved = resolveSelectorChain(replayNodes, containerChain.join(' || '), { + const containerResolved = resolveRecordedTarget(containerChain.join(' || '), replayNodes, { platform: ANDROID, requireRect: true, - requireUnique: true, + allowDisambiguation: false, }); assert.equal( - containerResolved, - null, + containerResolved.kind === 'unresolved' ? containerResolved.reason : null, + 'ambiguous', 'the un-retargeted container selector refuses to bind uniquely', ); }); diff --git a/src/platforms/web/agent-browser-provider.test.ts b/src/platforms/web/agent-browser-provider.test.ts index 55ff1da00..db4cfc0e9 100644 --- a/src/platforms/web/agent-browser-provider.test.ts +++ b/src/platforms/web/agent-browser-provider.test.ts @@ -20,7 +20,7 @@ import { createAgentBrowserWebProvider } from './agent-browser-provider.ts'; import type { WebSnapshotResult } from './provider.ts'; import { withCommandExecutorOverride, type ExecResult } from '../../utils/exec.ts'; import { AppError } from '@agent-device/kernel/errors'; -import { buildSelectorChainForNode, resolveSelectorChain } from '@agent-device/selectors'; +import { buildSelectorChainForNode, resolveRecordedTarget } from '@agent-device/selectors'; import { attachRefs } from '@agent-device/kernel/snapshot'; import { installFakeManagedAgentBrowser } from './__tests__/test-utils.ts'; @@ -554,10 +554,12 @@ function assertRoleSelectorResolves(snapshot: WebSnapshotResult): void { const nodesWithRefs = attachRefs(snapshot.nodes); const selectorChain = buildSelectorChainForNode(nodesWithRefs[2]!, 'web'); assert.deepEqual(selectorChain, ['role="button" label="Save"', 'label="Save"']); - const resolved = resolveSelectorChain(nodesWithRefs, selectorChain[0]!, { + const resolved = resolveRecordedTarget(selectorChain[0]!, nodesWithRefs, { platform: 'web', + requireRect: false, + allowDisambiguation: false, }); - assert.equal(resolved?.node.label, 'Save'); + assert.equal(resolved.kind === 'resolved' ? resolved.winner.label : undefined, 'Save'); } function expectedNode( diff --git a/src/snapshot/__tests__/android-input-method-overlays.test.ts b/src/snapshot/__tests__/android-input-method-overlays.test.ts new file mode 100644 index 000000000..f863943a4 --- /dev/null +++ b/src/snapshot/__tests__/android-input-method-overlays.test.ts @@ -0,0 +1,37 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; + +import type { RawSnapshotNode } from '@agent-device/kernel/snapshot'; +import { isAndroidInputMethodSnapshotNode } from '../android-input-method-overlays.ts'; +import { annotateCoveredSnapshotNodes } from '../snapshot-occlusion.ts'; + +test('an Android input-method window covers app targets emitted after it', () => { + const nodes: RawSnapshotNode[] = [ + { + index: 0, + type: 'android.widget.FrameLayout', + label: 'Q', + identifier: 'com.google.android.inputmethod.latin:id/key_pos_0_0', + bundleId: 'com.google.android.inputmethod.latin', + rect: { x: 50, y: 500, width: 100, height: 100 }, + hittable: true, + }, + { + index: 1, + type: 'android.widget.Button', + label: 'Covered app target', + identifier: 'covered-app-target', + bundleId: 'com.example.app', + rect: { x: 0, y: 450, width: 200, height: 200 }, + hittable: true, + }, + ]; + + const annotated = annotateCoveredSnapshotNodes(nodes, { + isAdditionalOverlayNode: isAndroidInputMethodSnapshotNode, + }); + + assert.equal(annotated[0]?.interactionBlocked, undefined); + assert.equal(annotated[1]?.interactionBlocked, 'covered'); + assert.equal(annotated[1]?.hittable, false); +}); diff --git a/src/snapshot/snapshot-occlusion.ts b/src/snapshot/snapshot-occlusion.ts index 6f0d150ef..585afba0c 100644 --- a/src/snapshot/snapshot-occlusion.ts +++ b/src/snapshot/snapshot-occlusion.ts @@ -131,28 +131,25 @@ function findCoveringNode( ): RawSnapshotNode | null { const cached = scan.coverCache.get(targetPosition); if (cached !== undefined) return cached; - // Reentrancy guard: `visibleCoverRect` recurses into `findCoveringNode` for - // the SAME targetPosition only through a cycle in `overlayPositions` - // ordering, which cannot happen (positions strictly increase along any - // recursive path — see the `position <= targetPosition` filter below) — - // but seed `null` before recursing regardless, so a future edit that - // breaks that invariant fails closed (no cover) instead of re-entering. + // Reentrancy guard: an additional Android overlay may precede an app target + // in the cross-window traversal, while that target may itself be overlay-like. + // Seed `null` before following either direction so such a cycle fails closed + // instead of re-entering. scan.coverCache.set(targetPosition, null); const targetRect = positiveRect(target.rect); if (!targetRect) return finishFindCoveringNode(scan, targetPosition, null); const center = centerOfRect(targetRect); + const targetIsAdditionalOverlay = options.isAdditionalOverlayNode?.(target) === true; - // Mutation-lane note: relaxing `<=` to `<` here would only change behavior - // if `position === targetPosition` were reachable — a node covering - // itself. That case is already excluded one line below regardless: a - // self-candidate has `candidateRect === targetRect` (same node, same - // object), so `areRectsApproximatelyEqual` in `visibleCoverRect` always - // excludes it. for (const position of scan.overlayPositions) { - if (position <= targetPosition) continue; - const candidate = scan.nodes[position]; - if (candidate && canCoverPoint(scan, position, target, targetRect, center, options)) { + if ( + !canPositionCoverTarget(scan, position, targetPosition, targetIsAdditionalOverlay, options) + ) { + continue; + } + const candidate = scan.nodes[position]!; + if (canCoverPoint(scan, position, target, targetRect, center, options)) { return finishFindCoveringNode(scan, targetPosition, candidate); } } @@ -160,6 +157,20 @@ function findCoveringNode( return finishFindCoveringNode(scan, targetPosition, null); } +function canPositionCoverTarget( + scan: OcclusionScan, + candidatePosition: number, + targetPosition: number, + targetIsAdditionalOverlay: boolean, + options: SnapshotOcclusionOptions, +): boolean { + const candidate = scan.nodes[candidatePosition]; + if (!candidate) return false; + if (candidatePosition > targetPosition) return true; + if (candidatePosition === targetPosition || targetIsAdditionalOverlay) return false; + return options.isAdditionalOverlayNode?.(candidate) === true; +} + function finishFindCoveringNode( scan: OcclusionScan, targetPosition: number, diff --git a/test/integration/replays/android/fixture/02-selector-routes-covered-diagnosis.ad b/test/integration/replays/android/fixture/02-selector-routes-covered-diagnosis.ad new file mode 100644 index 000000000..58f2cfa58 --- /dev/null +++ b/test/integration/replays/android/fixture/02-selector-routes-covered-diagnosis.ad @@ -0,0 +1,14 @@ +# Focused selector reads followed by a deliberate covered-target refusal. +context platform=android kind=emulator timeout=120000 + +env APP_TARGET="com.callstack.agentdevicelab" + +open "${APP_TARGET}" --no-test-ime --launch-url "agent-device-test-app:///form" +wait "id=\"field-name\"" 30000 +get text "id=\"field-name\"" +get attrs "id=\"field-name\"" +is visible "id=\"field-name\"" +find id "field-name" get attrs +orientation landscape-left +click "id=\"field-name\"" +click "id=\"field-name\"" diff --git a/test/integration/replays/ios/fixture/03-selector-routes-covered-diagnosis.ad b/test/integration/replays/ios/fixture/03-selector-routes-covered-diagnosis.ad new file mode 100644 index 000000000..26f7f309b --- /dev/null +++ b/test/integration/replays/ios/fixture/03-selector-routes-covered-diagnosis.ad @@ -0,0 +1,14 @@ +# Focused selector reads followed by a deliberate covered-target refusal. +context platform=ios kind=simulator timeout=120000 + +env APP_TARGET="Agent Device Tester" + +open "${APP_TARGET}" --launch-url "agent-device-test-app:///automation?event=selector.routes&payload=%7B%22source%22%3A%22replay%22%7D" +wait "id=\"automation-title\"" 30000 +get text "id=\"automation-title\"" +get attrs "id=\"automation-title\"" +is visible "id=\"automation-title\"" +find id "automation-title" get attrs +app-switcher +snapshot -i +click "id=\"gearshape.fill\""