diff --git a/packages/selectors/src/index.ts b/packages/selectors/src/index.ts index ffabbe117..54a9191a7 100644 --- a/packages/selectors/src/index.ts +++ b/packages/selectors/src/index.ts @@ -1,12 +1,14 @@ 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 { checkElementTargetArgs, checkGetFormat, @@ -60,6 +62,7 @@ import { export type { FindAction, FindLocator } from './internal/find.ts'; export type { IsPredicate } from './internal/predicates.ts'; export type { + PolicyResolutionOutcome, SelectorChainMatchList, SelectorChainMatch, SelectorResolution, @@ -259,3 +262,45 @@ function resolveSelectorChain( 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'; +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 return leg is the half that is easy to miss: the parser-side outcome + * carries the winning `Selector` node inside `resolution`, and returning it + * unchanged would put a package-private parser object back in every caller's + * 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). + */ +function resolveSelectorChainWithPolicy( + nodes: SnapshotState['nodes'], + expression: string, + policy: SelectorResolutionPolicy, + options: SelectorMatchOptions, +): PolicyResolutionOutcome { + const outcome = resolveSelectorChainWithPolicyAst( + nodes, + parseSelectorChain(expression), + policy, + options, + ); + if (outcome.kind !== 'resolved') return outcome; + return { + ...outcome, + resolution: { ...outcome.resolution, selector: outcome.resolution.selector.raw }, + }; +} +export { resolveSelectorChainWithPolicy }; diff --git a/packages/selectors/src/internal/public-resolution-types.ts b/packages/selectors/src/internal/public-resolution-types.ts index f07dcafad..c1cf649dc 100644 --- a/packages/selectors/src/internal/public-resolution-types.ts +++ b/packages/selectors/src/internal/public-resolution-types.ts @@ -29,6 +29,36 @@ export type SelectorResolution = { disambiguation?: SelectorDisambiguationDisclosure; }; +/** + * The façade twin of the parser-side `AstPolicyResolutionOutcome`: identical + * except that the winning alternative is its raw selector text rather than the + * `Selector` node, the same flattening `SelectorResolution` applies to + * `AstSelectorResolution`. + * + * It exists as a separate declaration for the same reason that pair does + * (#1589): the parser representation is package-private, and a nested return + * type is a leak the façade's named-export gate cannot see — it filters export + * *names*, so an `AstSelectorResolution` reached indirectly through + * `outcome.resolution` would reopen the boundary silently. + */ +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. + */ + | { kind: 'resolved'; resolution: SelectorResolution; matchedNodes: SnapshotNode[] } + /** + * Several matches and the policy refuses to choose. `fail-closed` returns + * this instead of guessing; `reject-candidates` returns it so the caller can + * narrow explicitly or surface the candidate list. + */ + | { kind: 'ambiguous'; selector: string; selectorIndex: number; matchedNodes: SnapshotNode[] }; + /** The first matching selector alternative and its complete matched-node domain. */ export type SelectorChainMatchList = { selector: string; diff --git a/packages/selectors/src/internal/resolution-policy.ts b/packages/selectors/src/internal/resolution-policy.ts new file mode 100644 index 000000000..03880ad6d --- /dev/null +++ b/packages/selectors/src/internal/resolution-policy.ts @@ -0,0 +1,101 @@ +import type { SelectorResolutionOptions } from './public-resolution-types.ts'; + +/** + * The per-caller selector-resolution policy matrix (#1630): every native + * consumer of "resolve a selector against the screen" declares its ambiguity + * contract here instead of passing `requireUnique`/`disambiguateAmbiguous` + * literals at the call site. The engine stays policy-neutral; which row a + * caller consumes IS the caller's documented contract, and changing a row is + * a reviewable one-line policy change instead of a multi-file literal hunt. + * + * Ambiguity kinds: + * - `disambiguate` — unique match required, but the engine's visible→deepest→ + * smallest-area tiebreak may pick a winner from an ambiguous set (acting + * commands, `get text`). + * - `fail-closed` — unique match required, ties reject (by design: `is` + * predicates and `get attrs` must never guess). + * - `first-match` — any match count accepted, first wins (existence reads and + * the wait loop, where presence is the question). + * - `reject-candidates` — multiple matches reject with the candidate list + * unless the caller explicitly narrows (#1625's mutating-find contract). + * Declaration-only: enforced by find's own narrowing logic, not by engine + * knobs, so `selectorResolutionKnobs` rejects it at the type level. + * + * 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 + * documented semantics fails a test. + * + * The surrounding pipeline stages — occlusion, the off-screen guard, + * hittable-ancestor promotion, and the wait poll budget — still live in the + * callers and are NOT declared here. An earlier revision listed them as + * columns; nothing consumed them, so they were unverifiable claims that read + * as truth while being free to drift (#1649 review). Routing them into typed + * behavior is tracked in #1656. + */ + +export type KnobBackedSelectorAmbiguity = 'disambiguate' | 'fail-closed' | 'first-match'; +export type SelectorAmbiguityPolicy = KnobBackedSelectorAmbiguity | 'reject-candidates'; + +export type SelectorResolutionPolicy = { + ambiguity: SelectorAmbiguityPolicy; + /** Only nodes carrying a rect participate (acting paths need a tap point). */ + requireRect: boolean; +}; + +export const SELECTOR_RESOLUTION_POLICIES = { + /** click/press/fill/focus/longPress/drag/scroll targets (resolution.ts). */ + act: { + ambiguity: 'disambiguate', + requireRect: true, + }, + /** The post-miss diagnosis probe deciding "no match" vs "matched but covered". */ + actCoveredDiagnosis: { + ambiguity: 'first-match', + requireRect: true, + }, + /** `get text` — reads through the same tiebreak acting uses. */ + readText: { + ambiguity: 'disambiguate', + requireRect: false, + }, + /** `is` non-exists predicates and `get attrs` — ties reject, never guess. */ + readUnique: { + ambiguity: 'fail-closed', + requireRect: false, + }, + /** `exists` and find's read-only actions — presence is the question. */ + readAny: { + ambiguity: 'first-match', + requireRect: false, + }, + /** `wait` — first match per poll, under the wait budget. */ + wait: { + ambiguity: 'first-match', + requireRect: false, + }, + /** Mutating `find` (#1625): candidates reject unless explicitly narrowed. */ + findAct: { + ambiguity: 'reject-candidates', + requireRect: true, + }, +} 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 the + * caller's narrowing logic, not by these knobs. + */ +export function selectorResolutionKnobs( + policy: SelectorResolutionPolicy & { ambiguity: KnobBackedSelectorAmbiguity }, +): Pick { + if (policy.ambiguity === 'first-match') { + return { requireRect: policy.requireRect, requireUnique: false }; + } + return { + requireRect: policy.requireRect, + requireUnique: true, + disambiguateAmbiguous: policy.ambiguity === 'disambiguate', + }; +} diff --git a/packages/selectors/src/internal/resolve-with-policy.ts b/packages/selectors/src/internal/resolve-with-policy.ts new file mode 100644 index 000000000..2cccd7129 --- /dev/null +++ b/packages/selectors/src/internal/resolve-with-policy.ts @@ -0,0 +1,123 @@ +import type { SnapshotState } from '@agent-device/kernel/snapshot'; +import type { SelectorChain } from './parse.ts'; +import type { SelectorMatchOptions } from './public-resolution-types.ts'; +import { + listSelectorChainMatches, + resolveSelectorChain, + type AstSelectorResolution, +} from './resolve.ts'; +import type { SelectorResolutionPolicy } from './resolution-policy.ts'; + +/** + * The one policy-driven resolution entry every native caller routes through + * (#1630). A caller passes the policy row that IS its documented contract; + * this decides what "resolved" means for that row, so ambiguity semantics + * live in the matrix rather than in each caller's local branching. + * + * The outcome is a discriminated union rather than a nullable node, because + * the rows genuinely disagree about what to do with several matches: + * `disambiguate` and `fail-closed` want one winner or nothing, `first-match` + * wants the head of the list, and `reject-candidates` needs the whole + * candidate set to refuse with (or to narrow, when the caller was given an + * explicit index). Collapsing those into "node | null" is what previously + * forced every caller to re-derive its own contract inline. + */ + +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. + */ + | { + kind: 'resolved'; + resolution: AstSelectorResolution; + matchedNodes: SnapshotState['nodes']; + } + /** + * Several matches and the policy refuses to choose. `fail-closed` returns + * this instead of guessing; `reject-candidates` returns it so the caller + * can narrow explicitly or surface the candidate list. + */ + | { + kind: 'ambiguous'; + selector: string; + selectorIndex: number; + matchedNodes: SnapshotState['nodes']; + }; + +export function resolveSelectorChainWithPolicy( + nodes: SnapshotState['nodes'], + chain: SelectorChain, + 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); + } + + if (policy.ambiguity === 'first-match') { + const list = listSelectorChainMatches(nodes, chain, matchOptions); + if (!list || list.matchedNodes.length === 0) return { kind: 'none' }; + return resolvedFromList(list); + } + + const resolution = resolveSelectorChain(nodes, chain, { + ...matchOptions, + requireUnique: true, + disambiguateAmbiguous: policy.ambiguity === 'disambiguate', + }); + if (resolution) { + const list = listSelectorChainMatches(nodes, chain, matchOptions); + return { + kind: 'resolved', + resolution, + matchedNodes: list?.matchedNodes ?? [resolution.node], + }; + } + + // 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' }; + return { + kind: 'ambiguous', + selector: list.selector.raw, + selectorIndex: list.selectorIndex, + matchedNodes: list.matchedNodes, + }; +} + +function resolvedFromList( + list: NonNullable>, +): AstPolicyResolutionOutcome { + const node = list.matchedNodes[0]; + if (!node) return { kind: 'none' }; + return { + kind: 'resolved', + matchedNodes: list.matchedNodes, + resolution: { + node, + selector: list.selector, + selectorIndex: list.selectorIndex, + matches: list.matchedNodes.length, + diagnostics: [{ selector: list.selector.raw, matches: list.matchedNodes.length }], + }, + }; +} diff --git a/scripts/layering/facade-exports.ts b/scripts/layering/facade-exports.ts index 957666390..007ac99c1 100644 --- a/scripts/layering/facade-exports.ts +++ b/scripts/layering/facade-exports.ts @@ -96,3 +96,27 @@ export function readDirectNamedExports(source: string): string[] { } return [...names].sort(); } + +/** + * Which module each name in `source` is re-exported FROM, for names that come + * from a re-export rather than a local declaration. + * + * A façade's export *names* are only half its boundary: a type re-exported + * from the right module and one re-exported from a package-private module read + * identically in the name list, while only the second leaks. #1649 shipped + * exactly that — a policy outcome re-exported from the parser-side module, so + * its nested `resolution` field handed callers the private AST — and the + * name-list gate stayed green throughout. + */ +export function readReExportSources(source: string): Map { + const parsed = parseSync('facade-reexport-source-scan.ts', source); + const sources = new Map(); + for (const staticExport of parsed.module.staticExports) { + for (const entry of staticExport.entries) { + if (entry.exportName.kind !== 'Name' || !entry.exportName.name) continue; + if (!entry.moduleRequest) continue; + sources.set(entry.exportName.name, entry.moduleRequest.value); + } + } + return sources; +} diff --git a/scripts/layering/package-boundaries.test.ts b/scripts/layering/package-boundaries.test.ts index c1bbc69e4..345cab3c1 100644 --- a/scripts/layering/package-boundaries.test.ts +++ b/scripts/layering/package-boundaries.test.ts @@ -7,7 +7,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { test } from 'node:test'; import { listSourceFiles } from './check.ts'; -import { readDirectNamedExports, readNamedExports } from './facade-exports.ts'; +import { readDirectNamedExports, readNamedExports, readReExportSources } from './facade-exports.ts'; import { checkPackageBoundaries, checkPackageInternalSites, @@ -398,6 +398,25 @@ test('the real tree parses, declares, and passes R11', () => { [], 'selectors façade keeps AST and grammar internals private', ); + // Named exports are not the whole boundary. A parser-side type reached + // through a NESTED field — `PolicyResolutionOutcome.resolution` typed as + // `AstSelectorResolution` — leaks the same objects while exporting none of + // their names, and the assertion above stays green on it (#1649). What + // separates the two is which module the type is re-exported FROM: + // `public-resolution-types.ts` holds the string-flattened shapes, + // `resolve-with-policy.ts` and `resolve.ts` hold the parser-side ones. A + // resolution type re-exported from either of the latter means a flattening + // step at the façade was skipped. + const selectorsReExports = readReExportSources( + fs.readFileSync(path.join(repoRoot, 'packages/selectors/src/index.ts'), 'utf8'), + ); + assert.deepEqual( + ['PolicyResolutionOutcome', 'SelectorResolution', 'SelectorChainMatchList'].filter( + (name) => selectorsReExports.get(name) !== './internal/public-resolution-types.ts', + ), + [], + 'selectors façade must publish resolution shapes from public-resolution-types.ts, not from the parser-side modules', + ); // The AST subpath's one in-repo consumer is the published SDK re-export. // Anything else importing it means the string-only façade was bypassed. assert.deepEqual( diff --git a/src/__tests__/test-utils/host-process-mock.ts b/src/__tests__/test-utils/host-process-mock.ts index 8992d3926..795dca847 100644 --- a/src/__tests__/test-utils/host-process-mock.ts +++ b/src/__tests__/test-utils/host-process-mock.ts @@ -17,6 +17,10 @@ type HostProcessModule = typeof import('../../utils/host-process.ts'); * Usage: `vi.mock('/utils/host-process.ts', async (importOriginal) => * (await import('/test-utils/host-process-mock.ts')).pinOwnProcessStartTime(importOriginal))` */ +// Consumed by three suites, but only through `(await import(...)).pinOwnProcessStartTime` +// inside `vi.mock` factories — vitest hoists those above static imports, so the dynamic +// form is required and fallow cannot trace the consumers statically. +// fallow-ignore-next-line unused-export export async function pinOwnProcessStartTime( importOriginal: () => Promise, ): Promise { diff --git a/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts b/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts new file mode 100644 index 000000000..696d39631 --- /dev/null +++ b/src/commands/interaction/runtime/__tests__/resolution-policy-parity.test.ts @@ -0,0 +1,198 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +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. + */ + +function node(index: number, label: string, overrides: Partial = {}): SnapshotNode { + return { + ref: `e${index}`, + index, + depth: 1, + type: 'Button', + label, + rect: { x: 0, y: index * 40, width: 100, height: 30 }, + ...overrides, + } as SnapshotNode; +} + +/** Two nodes share a label: every ambiguity contract has to say something. */ +const AMBIGUOUS_TREE: SnapshotNode[] = [node(0, 'Save'), node(1, 'Save'), node(2, 'Cancel')]; +/** + * Same ambiguity, but the candidates differ in depth/area, so the engine's + * visible→deepest→smallest-area tiebreak CAN pick a winner. Kept separate + * from AMBIGUOUS_TREE because indistinguishable candidates are exactly the + * case where disambiguation must decline (below). + */ +const TIEBREAKABLE_TREE: SnapshotNode[] = [ + node(0, 'Save', { rect: { x: 0, y: 0, width: 300, height: 200 } }), + node(1, 'Save', { depth: 3, rect: { x: 10, y: 10, width: 80, height: 24 } }), + node(2, 'Cancel'), +]; +const UNIQUE_TREE: SnapshotNode[] = [node(0, 'Save'), node(1, 'Cancel')]; +/** Rectless nodes: only rect-requiring rows should reject these. */ +const RECTLESS_TREE: SnapshotNode[] = [node(0, 'Save', { rect: undefined })]; + +const OPTIONS = { platform: 'ios' as const }; + +function outcomeFor(policyName: keyof typeof SELECTOR_RESOLUTION_POLICIES, tree: SnapshotNode[]) { + return resolveSelectorChainWithPolicy( + tree, + 'label="Save"', + SELECTOR_RESOLUTION_POLICIES[policyName], + OPTIONS, + ); +} + +test('a unique match resolves under every policy', () => { + for (const name of Object.keys( + SELECTOR_RESOLUTION_POLICIES, + ) as (keyof typeof SELECTOR_RESOLUTION_POLICIES)[]) { + const outcome = outcomeFor(name, UNIQUE_TREE); + assert.equal(outcome.kind, 'resolved', name); + if (outcome.kind === 'resolved') assert.equal(outcome.resolution.node.label, 'Save'); + } +}); + +test('the façade returns selector TEXT under every policy, never a parser node', () => { + // #1589 made the root façade string-in/string-out and confined parser + // objects to `@agent-device/selectors/ast`. A nested return type reopens + // that boundary invisibly: the package-boundary gate filters exported + // *names*, so `PolicyResolutionOutcome.resolution` typed as the AST shape + // stayed green while production read `outcome.resolution.selector.raw`. + // Every row, and both branches that carry a selector. + for (const name of Object.keys( + SELECTOR_RESOLUTION_POLICIES, + ) as (keyof typeof SELECTOR_RESOLUTION_POLICIES)[]) { + const resolved = outcomeFor(name, UNIQUE_TREE); + assert.equal(resolved.kind, 'resolved', name); + if (resolved.kind === 'resolved') { + assert.equal(typeof resolved.resolution.selector, 'string', name); + assert.equal(resolved.resolution.selector, 'label="Save"', name); + } + const ambiguous = outcomeFor(name, AMBIGUOUS_TREE); + if (ambiguous.kind === 'ambiguous') { + assert.equal(typeof ambiguous.selector, 'string', name); + } else if (ambiguous.kind === 'resolved') { + assert.equal(typeof ambiguous.resolution.selector, 'string', name); + } + } +}); + +test('no match resolves to none under every policy', () => { + for (const name of Object.keys( + SELECTOR_RESOLUTION_POLICIES, + ) as (keyof typeof SELECTOR_RESOLUTION_POLICIES)[]) { + const outcome = resolveSelectorChainWithPolicy( + UNIQUE_TREE, + 'label="Absent"', + SELECTOR_RESOLUTION_POLICIES[name], + OPTIONS, + ); + assert.equal(outcome.kind, 'none', name); + } +}); + +test('disambiguating rows pick the tiebreak winner and disclose the match count', () => { + for (const name of ['act', 'readText'] as const) { + const outcome = outcomeFor(name, TIEBREAKABLE_TREE); + assert.equal(outcome.kind, 'resolved', name); + if (outcome.kind === 'resolved') { + assert.equal(outcome.resolution.matches, 2, `${name} discloses the real match count`); + assert.equal(outcome.resolution.node.index, 1, `${name} takes the deepest/smallest`); + } + } +}); + +test('disambiguation declines when candidates are genuinely indistinguishable', () => { + // The tiebreak is evidence, not a coin flip: identical candidates must not + // silently bind one. Acting rows surface the ambiguity instead. + for (const name of ['act', 'readText'] as const) { + assert.equal(outcomeFor(name, AMBIGUOUS_TREE).kind, 'ambiguous', name); + } +}); + +test('fail-closed rows refuse an ambiguous tree instead of guessing', () => { + const outcome = outcomeFor('readUnique', AMBIGUOUS_TREE); + assert.equal(outcome.kind, 'ambiguous'); + if (outcome.kind === 'ambiguous') assert.equal(outcome.matchedNodes.length, 2); +}); + +test('first-match rows take the head of an ambiguous tree', () => { + for (const name of ['readAny', 'wait', 'actCoveredDiagnosis'] as const) { + const outcome = outcomeFor(name, AMBIGUOUS_TREE); + assert.equal(outcome.kind, 'resolved', name); + if (outcome.kind === 'resolved') assert.equal(outcome.resolution.node.index, 0, name); + } +}); + +test('reject-candidates surfaces every candidate for the caller to narrow or refuse', () => { + const outcome = outcomeFor('findAct', AMBIGUOUS_TREE); + assert.equal(outcome.kind, 'ambiguous'); + if (outcome.kind === 'ambiguous') { + assert.deepEqual( + outcome.matchedNodes.map((n) => n.index), + [0, 1], + ); + } +}); + +test('rect-requiring rows skip rectless nodes; read and wait rows accept them', () => { + for (const name of ['act', 'findAct', 'actCoveredDiagnosis'] as const) { + assert.equal(outcomeFor(name, RECTLESS_TREE).kind, 'none', name); + } + for (const name of ['readUnique', 'readAny', 'readText', 'wait'] as const) { + assert.equal(outcomeFor(name, RECTLESS_TREE).kind, 'resolved', name); + } +}); + +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 + * code consumed, so changing them left both behavior and the suite green — + * an unverifiable claim reading as truth. This fails if such a field returns + * without behavioral coverage. + */ +test('policy rows declare only the fields this matrix actually enforces', () => { + for (const [name, policy] of Object.entries(SELECTOR_RESOLUTION_POLICIES)) { + assert.deepEqual( + Object.keys(policy).sort(), + ['ambiguity', 'requireRect'], + `${name} declares a field the matrix cannot enforce`, + ); + } +}); + +test('the documented per-caller contracts are the ones declared', () => { + assert.equal(SELECTOR_RESOLUTION_POLICIES.act.ambiguity, 'disambiguate'); + assert.equal(SELECTOR_RESOLUTION_POLICIES.readText.ambiguity, 'disambiguate'); + assert.equal(SELECTOR_RESOLUTION_POLICIES.readUnique.ambiguity, 'fail-closed'); + assert.equal(SELECTOR_RESOLUTION_POLICIES.readAny.ambiguity, 'first-match'); + assert.equal(SELECTOR_RESOLUTION_POLICIES.wait.ambiguity, 'first-match'); + assert.equal(SELECTOR_RESOLUTION_POLICIES.findAct.ambiguity, 'reject-candidates'); +}); diff --git a/src/commands/interaction/runtime/resolution.ts b/src/commands/interaction/runtime/resolution.ts index 1b4356c5f..b75f6f849 100644 --- a/src/commands/interaction/runtime/resolution.ts +++ b/src/commands/interaction/runtime/resolution.ts @@ -14,6 +14,8 @@ import { 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'; @@ -277,9 +279,7 @@ async function resolveSelectorInteractionTarget( selectorExpression, { platform: runtime.backend.platform, - requireRect: true, - requireUnique: true, - disambiguateAmbiguous: true, + ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.act), }, ); if ((!resolved || !resolved.node.rect) && params.requireInteractive) { @@ -289,17 +289,14 @@ async function resolveSelectorInteractionTarget( selectorExpression, { platform: runtime.backend.platform, - requireRect: true, - requireUnique: true, - disambiguateAmbiguous: true, + ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.act), }, ); } if (!resolved || !resolved.node.rect) { const covered = resolveSelectorChain(capture.snapshot.nodes, selectorExpression, { platform: runtime.backend.platform, - requireRect: true, - requireUnique: false, + ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.actCoveredDiagnosis), }); if (covered?.node && isSnapshotNodeInteractionBlocked(covered.node)) { throw buildCoveredInteractionError({ diff --git a/src/commands/interaction/runtime/selector-read.ts b/src/commands/interaction/runtime/selector-read.ts index 384499daa..5bae2a125 100644 --- a/src/commands/interaction/runtime/selector-read.ts +++ b/src/commands/interaction/runtime/selector-read.ts @@ -14,6 +14,10 @@ import { parseFindSelectorExpression, 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'; @@ -51,6 +55,10 @@ 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, @@ -209,7 +217,10 @@ export const getCommand: RuntimeCommand = a const resolved = await resolveSelectorNode(runtime, options, options.session ?? 'default', { selector: options.target.selector, - disambiguateAmbiguous: options.property === 'text', + policy: + options.property === 'text' + ? SELECTOR_RESOLUTION_POLICIES.readText + : SELECTOR_RESOLUTION_POLICIES.readUnique, }); assertExpectedResolvedTarget( resolved.node, @@ -317,9 +328,7 @@ export const isCommand: RuntimeCommand = asyn const resolved = resolveSelectorChain(capture.snapshot.nodes, selectorExpression, { platform: runtime.backend.platform, - requireRect: false, - requireUnique: true, - disambiguateAmbiguous: false, + ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.readUnique), }); if (!resolved) { throw new AppError( @@ -472,8 +481,7 @@ async function findFirstLocatorMatch( if (selectorExpression) { const resolved = resolveSelectorChain(capture.snapshot.nodes, selectorExpression, { platform: runtime.backend.platform, - requireRect: false, - requireUnique: false, + ...selectorResolutionKnobs(SELECTOR_RESOLUTION_POLICIES.readAny), }); return { capture, match: resolved?.node }; } @@ -487,7 +495,7 @@ async function resolveSelectorNode( runtime: AgentDeviceRuntime, options: GetCommandOptions, sessionName: string, - params: { selector: string; disambiguateAmbiguous: boolean }, + params: { selector: string; policy: KnobBackedResolutionPolicy }, ): Promise<{ capture: CapturedSnapshot; node: SnapshotNode; selector: string; ref: string }> { const capture = await captureSelectorSnapshot( runtime, @@ -499,9 +507,7 @@ async function resolveSelectorNode( ); const resolved = resolveSelectorChain(capture.snapshot.nodes, params.selector, { platform: runtime.backend.platform, - requireRect: false, - requireUnique: true, - disambiguateAmbiguous: params.disambiguateAmbiguous, + ...selectorResolutionKnobs(params.policy), }); if (!resolved) { throw new AppError( diff --git a/src/commands/interaction/runtime/selector-wait.test.ts b/src/commands/interaction/runtime/selector-wait.test.ts index 4faa01cd4..8b1b9175d 100644 --- a/src/commands/interaction/runtime/selector-wait.test.ts +++ b/src/commands/interaction/runtime/selector-wait.test.ts @@ -123,6 +123,57 @@ function landmarkWaitDevice(captures: Array>) return device; } +/** + * The within-one-poll twin of the test below (#1649 review P1): both + * candidates are on screen in the SAME capture, impostor first. The landmark + * check is satisfied when SOME match carries the recorded identity, so + * resolution must hand it every candidate — a policy that returns only its + * first-match winner would hide the genuine landmark behind the impostor and + * make this wait time out. + */ +function twoCandidateScreen(): ReturnType { + return makeSnapshotState([ + { index: 0, depth: 0, type: 'Other', label: 'List Screen' }, + { + index: 1, + depth: 1, + parentIndex: 0, + type: 'StaticText', + label: 'Screen X', + rect: { x: 0, y: 0, width: 100, height: 20 }, + }, + { index: 2, depth: 0, type: 'Other', label: 'Detail Screen' }, + { + index: 3, + depth: 1, + parentIndex: 2, + type: 'StaticText', + label: 'Screen X', + rect: { x: 0, y: 40, width: 100, height: 20 }, + }, + ]); +} + +test('runtime wait finds the recorded landmark behind a same-selector impostor in one capture', async () => { + const recorded = recordedLandmarkFor(landmarkScreen('Detail Screen')); + const device = landmarkWaitDevice([twoCandidateScreen()]); + + const result = await device.selectors.wait({ + session: 'default', + target: { + kind: 'selector', + selector: 'label="Screen X"', + timeoutMs: 2_000, + recordedLandmark: recorded, + }, + }); + + assert.equal(result.kind, 'selector'); + if (result.kind !== 'selector') throw new Error('unreachable'); + // The SECOND candidate is the one carrying the recorded ancestry. + assert.equal(result.node?.index, 3); +}); + 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); diff --git a/src/commands/interaction/runtime/selector-wait.ts b/src/commands/interaction/runtime/selector-wait.ts index 349ba6aca..423373d15 100644 --- a/src/commands/interaction/runtime/selector-wait.ts +++ b/src/commands/interaction/runtime/selector-wait.ts @@ -15,8 +15,10 @@ import { import type { PublicPlatform } from '@agent-device/kernel/device'; import { checkWaitText, - listSelectorChainMatches, type SelectorChainMatchList, + SELECTOR_RESOLUTION_POLICIES, + resolveSelectorChainWithPolicy, + type PolicyResolutionOutcome, } from '@agent-device/selectors'; import { deriveSelectorCapturePolicy } from './selector-capture-policy.ts'; import { findNodeByLabel, resolveRefLabel } from './selector-read-utils.ts'; @@ -27,6 +29,33 @@ import { waitTimeoutError, } from './wait-polling.ts'; +/** + * The landmark check (#1349) needs the full candidate set, which the policy + * outcome carries in either shape: a `first-match` resolution exposes the + * winner, and the ambiguous branch exposes all candidates. Wait's row never + * refuses, so this only ever adapts — it does not re-decide anything. + */ +function policyMatchList(outcome: PolicyResolutionOutcome): SelectorChainMatchList | undefined { + if (outcome.kind === 'ambiguous') { + return { + selector: outcome.selector, + selectorIndex: outcome.selectorIndex, + matchedNodes: outcome.matchedNodes, + }; + } + if (outcome.kind === 'resolved') { + return { + selector: outcome.resolution.selector, + selectorIndex: outcome.resolution.selectorIndex, + // Every candidate, not just the winner: the landmark check is satisfied + // when SOME match carries the recorded identity, so a first impostor + // must not hide a later genuine landmark (#1349). + matchedNodes: outcome.matchedNodes, + }; + } + return undefined; +} + type WaitCommandContext = { session?: string; requestId?: string; @@ -248,9 +277,15 @@ async function waitForSelector( const capture = poll.value; if (capture) { const nodes = capture.snapshot.nodes; - const matchList = listSelectorChainMatches(nodes, selectorExpression, { - platform: runtime.backend.platform, - }); + const outcome = resolveSelectorChainWithPolicy( + nodes, + selectorExpression, + SELECTOR_RESOLUTION_POLICIES.wait, + { platform: runtime.backend.platform }, + ); + // The wait row is `first-match`, so a multi-match screen resolves rather + // than refusing; the landmark check below is what decides satisfaction. + const matchList = policyMatchList(outcome); if (matchList) { const landmark = resolveLandmarkMatch(nodes, matchList, recordedLandmark); if (landmark.kind === 'satisfied') { diff --git a/src/daemon/handlers/find.ts b/src/daemon/handlers/find.ts index 1b1064c2b..b83e93fce 100644 --- a/src/daemon/handlers/find.ts +++ b/src/daemon/handlers/find.ts @@ -6,7 +6,10 @@ import { checkFindArgs, parseFindSelectorExpression, type FindLocator, - listSelectorChainMatches, + SELECTOR_RESOLUTION_POLICIES, + resolveSelectorChainWithPolicy, + type PolicyResolutionOutcome, + type SelectorResolutionPolicy, } from '@agent-device/selectors'; import { centerOfRect, @@ -32,6 +35,24 @@ import { stripInternalInteractionFlags } from '../interaction-outcome-policy.ts' import { dispatchFindReadOnlyViaRuntime } from '../selector-runtime.ts'; import { createSelectorCaptureRuntime } from '../selector-capture-runtime.ts'; import { isSparseSnapshotQualityVerdict } from '../../snapshot/snapshot-quality.ts'; + +/** + * Both branches of the `reject-candidates` contract produce a candidate set: + * a single resolved match, or the full ambiguous set find must refuse (or + * narrow) explicitly. + */ +function policyMatchedNodes(outcome: PolicyResolutionOutcome): SnapshotState['nodes'] { + if (outcome.kind === 'ambiguous') return outcome.matchedNodes; + if (outcome.kind === 'resolved') return [outcome.resolution.node]; + return []; +} + +function assertRejectsCandidates(policy: SelectorResolutionPolicy): void { + if (policy.ambiguity !== 'reject-candidates') { + throw new Error(`find's resolution policy must reject candidates, got "${policy.ambiguity}"`); + } +} + type FindContext = { req: DaemonRequest; sessionName: string; @@ -252,21 +273,29 @@ function resolveFindMatch(params: { // explicitly opts into positional narrowing. Selectors used to take the // first match silently, which was exactly the mis-binding path the error's // own recovery advice ("use a selector") pointed agents at. + const policy = SELECTOR_RESOLUTION_POLICIES.findAct; let matches: SnapshotState['nodes']; if (selectorExpression) { - matches = - listSelectorChainMatches(searchableNodes, selectorExpression, { - platform, - requireRect: true, - })?.matchedNodes ?? []; + // Selector-shaped queries resolve through the policy interface, so the + // `reject-candidates` contract is the matrix's decision rather than a + // local convention. The locator branch cannot: it matches by fuzzy text + // scoring, not by selector chains, so it produces its candidate set with + // its own matcher and joins the shared contract below. + matches = policyMatchedNodes( + resolveSelectorChainWithPolicy(searchableNodes, selectorExpression, policy, { platform }), + ); } else { matches = findBestMatchesByLocator(searchableNodes, locator, query, { - requireRect: true, + requireRect: policy.requireRect, }).matches; } matches = preferOnscreenMatches(matches, nodes); if (matches.length > 1) { + // The row says candidates reject unless the caller narrowed explicitly; + // assert that rather than assuming, so a future row edit cannot silently + // turn this into first-match. + assertRejectsCandidates(policy); const narrowed = narrowMultipleMatches(matches, flags); if (!narrowed) { return { ok: false, response: buildAmbiguousMatchError(matches, locator, query) };