Skip to content

Commit 10ff339

Browse files
thymikeeclaude
andauthored
refactor: declare selector resolution policy as data (#1649)
* refactor: declare selector resolution policy as data (#1630) Five native consumers of "resolve a selector against the screen" each hand-declared their ambiguity contract as inline requireUnique/ disambiguateAmbiguous literals, so the repo's real policy matrix was only discoverable by reading four files. SELECTOR_RESOLUTION_POLICIES (packages/selectors) now declares one row per caller — ambiguity kind plus the structural columns (rect, occlusion, off-screen guard, promotion, poll) — and selectorResolutionKnobs turns a row into the engine knobs it stands for. Callers consume rows; zero ambiguity literals remain in src. Semantics are unchanged by construction: each row was read off its call site. The matrix names what was previously implicit — act and get text disambiguate, is/get attrs fail closed, exists/find-reads and wait take the first match, mutating find rejects candidates unless narrowed (#1625). `reject-candidates` is declaration-only and rejected by selectorResolutionKnobs at the type level, because find enforces it through its own narrowing rather than engine knobs. resolution-policy-parity.test.ts gate-tests the matrix against the callers (ADR 0011's declared-plus-gate-tested pattern): knobs must match the named ambiguity contract, every claimed structural column must appear in the caller's source, the read/wait pipelines must genuinely lack the machinery they disclaim, and no caller may reintroduce an inline literal. Verified revert-sensitive: flipping readUnique to disambiguate and faking wait's occlusion column each fail it. Out of scope, unchanged, per the issue: the Maestro engine (ADR 0015) and the open click-implicit-wait product decision. * refactor: route wait and mutating find through the policy interface (#1649 review) P1 was right: the first head declared seven rows but genuinely routed five. selector-wait.ts never imported its row (it called listSelectorChainMatches directly), findAct consumed only requireRect while its ambiguity contract stayed bespoke, and the parity test sniffed marker strings in source files — so it stayed green across exactly that gap. Asserting about the layer I had edited instead of the behavior it produces. resolveSelectorChainWithPolicy is now the one policy-driven entry: it returns a discriminated outcome (none / resolved / ambiguous) because the rows genuinely disagree about what several matches mean, which is what previously forced each caller to re-derive its contract inline. wait and find's selector branch both route through it; find additionally asserts its row still says reject-candidates rather than assuming. The parity test is rebuilt on fixture trees driven through that interface — no source sniffing. Wiring verified revert-sensitive: flipping the wait row fails the policy tests, and flipping findAct fails REAL find handler tests (ambiguous-candidate listing), which is the proof the previous version could not produce. One behavior nuance the fixture work surfaced and now pins: disambiguation declines on genuinely indistinguishable candidates (the tiebreak is evidence, not a coin flip), so an acting row surfaces ambiguity there rather than binding one silently. * fix(test): let fallow see the host-process mock helper's real consumers Rebase onto main brought #1642's host-process-mock.ts into this PR's fallow scope, where its export reports as unused. It is not: three suites consume it, 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 it statically. Documented suppression rather than a restructure that would break the hoisting contract. Latent on main rather than introduced here: the audit gate is changed-files-only, so main sees the file in scope only from a PR whose diff contains it. * fix: keep every candidate when a policy resolves one winner (#1649 review P1) A real regression I introduced, not a test gap: routing wait through the policy interface collapsed the candidate set to the winner, and the #1349 landmark check is satisfied when SOME match carries the recorded identity. A first same-selector impostor therefore hid a later genuine landmark and timed the wait out. The resolved outcome now carries `matchedNodes` — the full candidate set of the alternative the winner came from — so a policy that picks one node no longer throws the rest away. wait passes that straight to the landmark check, restoring the original semantics. Regression test added at the within-one-poll shape the existing suite did not cover (both candidates in the SAME capture, impostor first); verified it goes red against the singleton reconstruction it replaces. * refactor: declare only the policy fields the matrix enforces (#1649 review) The occlusion / offscreenGuard / promotion / poll columns were never consumed by resolveSelectorChainWithPolicy or selectorResolutionKnobs: changing any of them left behavior and the suite green, so they were unverifiable claims that read as truth. (My earlier source-sniffing test "verified" them by grepping caller files for marker strings — which is why it also stayed green when a row was disconnected entirely.) The matrix now declares exactly what it enforces: the ambiguity contract and the rect requirement, both consumed by the resolution interface and pinned behaviorally. A new test asserts every row's field set, so an unenforceable column cannot reappear without coverage — verified by re-adding one and watching it fail. Routing the structural stages into typed behavior is tracked in #1656 with the constraint that each field must be consumed, not merely declared. * fix(selectors): flatten the policy outcome at the package boundary `PolicyResolutionOutcome.resolution` was typed as `AstSelectorResolution` and the root façade returned it unchanged, so the parser AST #1589 confined to `@agent-device/selectors/ast` came back through a nested field. `selector-wait.ts` reading `outcome.resolution.selector.raw` was the runtime proof. The existing boundary gate reads exported *names*, so it could not see this. The public outcome now lives beside `SelectorResolution` in public-resolution-types.ts with its selector as text; the parser-side shape is renamed `AstPolicyResolutionOutcome` and stays package-private, and the façade wrapper flattens on the way out — the same treatment `resolveSelectorChain` already gave `AstSelectorResolution`. Two new pins, both verified red against the shape they replace: a behavioral one asserting the façade returns selector text under every policy row, and a structural one asserting resolution shapes are re-exported from public-resolution-types.ts rather than from a parser-side module — which is what distinguishes the leak from a correct re-export in a name list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Rva4YGtSCAKJqH5PbpcCU --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0033002 commit 10ff339

13 files changed

Lines changed: 692 additions & 30 deletions

File tree

packages/selectors/src/index.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import type { SnapshotState } from '@agent-device/kernel/snapshot';
22
import type { Selector } from './internal/parse.ts';
33
import type {
4+
PolicyResolutionOutcome,
45
SelectorChainMatch,
56
SelectorChainMatchList,
67
SelectorMatchOptions,
78
SelectorResolution,
89
SelectorResolutionOptions,
910
} from './internal/public-resolution-types.ts';
11+
import { resolveSelectorChainWithPolicy as resolveSelectorChainWithPolicyAst } from './internal/resolve-with-policy.ts';
1012
import {
1113
checkElementTargetArgs,
1214
checkGetFormat,
@@ -60,6 +62,7 @@ import {
6062
export type { FindAction, FindLocator } from './internal/find.ts';
6163
export type { IsPredicate } from './internal/predicates.ts';
6264
export type {
65+
PolicyResolutionOutcome,
6366
SelectorChainMatchList,
6467
SelectorChainMatch,
6568
SelectorResolution,
@@ -259,3 +262,45 @@ function resolveSelectorChain(
259262
const result = resolveSelectorChainAst(nodes, parseSelectorChain(expression), options);
260263
return result ? { ...result, selector: result.selector.raw } : null;
261264
}
265+
export {
266+
SELECTOR_RESOLUTION_POLICIES,
267+
selectorResolutionKnobs,
268+
} from './internal/resolution-policy.ts';
269+
export type {
270+
KnobBackedSelectorAmbiguity,
271+
SelectorResolutionPolicy,
272+
} from './internal/resolution-policy.ts';
273+
import type { SelectorResolutionPolicy } from './internal/resolution-policy.ts';
274+
275+
/**
276+
* Public façade wrapper that accepts selector text and returns selector text —
277+
* never an AST, in either direction.
278+
*
279+
* The return leg is the half that is easy to miss: the parser-side outcome
280+
* carries the winning `Selector` node inside `resolution`, and returning it
281+
* unchanged would put a package-private parser object back in every caller's
282+
* hands through a nested field. The façade's own boundary gate reads exported
283+
* *names*, so it cannot see that; `selector-wait.ts` reading
284+
* `outcome.resolution.selector.raw` was the runtime proof it had happened.
285+
* Flattening here is the same treatment `resolveSelectorChain` above gives
286+
* `AstSelectorResolution` (#1589).
287+
*/
288+
function resolveSelectorChainWithPolicy(
289+
nodes: SnapshotState['nodes'],
290+
expression: string,
291+
policy: SelectorResolutionPolicy,
292+
options: SelectorMatchOptions,
293+
): PolicyResolutionOutcome {
294+
const outcome = resolveSelectorChainWithPolicyAst(
295+
nodes,
296+
parseSelectorChain(expression),
297+
policy,
298+
options,
299+
);
300+
if (outcome.kind !== 'resolved') return outcome;
301+
return {
302+
...outcome,
303+
resolution: { ...outcome.resolution, selector: outcome.resolution.selector.raw },
304+
};
305+
}
306+
export { resolveSelectorChainWithPolicy };

packages/selectors/src/internal/public-resolution-types.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,36 @@ export type SelectorResolution = {
2929
disambiguation?: SelectorDisambiguationDisclosure;
3030
};
3131

32+
/**
33+
* The façade twin of the parser-side `AstPolicyResolutionOutcome`: identical
34+
* except that the winning alternative is its raw selector text rather than the
35+
* `Selector` node, the same flattening `SelectorResolution` applies to
36+
* `AstSelectorResolution`.
37+
*
38+
* It exists as a separate declaration for the same reason that pair does
39+
* (#1589): the parser representation is package-private, and a nested return
40+
* type is a leak the façade's named-export gate cannot see — it filters export
41+
* *names*, so an `AstSelectorResolution` reached indirectly through
42+
* `outcome.resolution` would reopen the boundary silently.
43+
*/
44+
export type PolicyResolutionOutcome =
45+
/** No selector alternative matched anything. */
46+
| { kind: 'none' }
47+
/**
48+
* The node this policy authorizes acting on, plus the full candidate set of
49+
* the alternative it came from. Callers that verify identity across
50+
* candidates (wait's #1349 landmark check) need the whole set — a policy
51+
* that picks one winner must not throw the rest away, or a first impostor
52+
* would hide a later genuine match.
53+
*/
54+
| { kind: 'resolved'; resolution: SelectorResolution; matchedNodes: SnapshotNode[] }
55+
/**
56+
* Several matches and the policy refuses to choose. `fail-closed` returns
57+
* this instead of guessing; `reject-candidates` returns it so the caller can
58+
* narrow explicitly or surface the candidate list.
59+
*/
60+
| { kind: 'ambiguous'; selector: string; selectorIndex: number; matchedNodes: SnapshotNode[] };
61+
3262
/** The first matching selector alternative and its complete matched-node domain. */
3363
export type SelectorChainMatchList = {
3464
selector: string;
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
import type { SelectorResolutionOptions } from './public-resolution-types.ts';
2+
3+
/**
4+
* The per-caller selector-resolution policy matrix (#1630): every native
5+
* consumer of "resolve a selector against the screen" declares its ambiguity
6+
* contract here instead of passing `requireUnique`/`disambiguateAmbiguous`
7+
* literals at the call site. The engine stays policy-neutral; which row a
8+
* caller consumes IS the caller's documented contract, and changing a row is
9+
* a reviewable one-line policy change instead of a multi-file literal hunt.
10+
*
11+
* Ambiguity kinds:
12+
* - `disambiguate` — unique match required, but the engine's visible→deepest→
13+
* smallest-area tiebreak may pick a winner from an ambiguous set (acting
14+
* commands, `get text`).
15+
* - `fail-closed` — unique match required, ties reject (by design: `is`
16+
* predicates and `get attrs` must never guess).
17+
* - `first-match` — any match count accepted, first wins (existence reads and
18+
* the wait loop, where presence is the question).
19+
* - `reject-candidates` — multiple matches reject with the candidate list
20+
* unless the caller explicitly narrows (#1625's mutating-find contract).
21+
* Declaration-only: enforced by find's own narrowing logic, not by engine
22+
* knobs, so `selectorResolutionKnobs` rejects it at the type level.
23+
*
24+
* Scope, deliberately narrow: this matrix declares the **ambiguity contract
25+
* and the rect requirement**, and nothing else. Both are consumed by
26+
* `resolveSelectorChainWithPolicy` and pinned behaviorally in
27+
* resolution-policy-parity.test.ts, so a row that stops matching its
28+
* documented semantics fails a test.
29+
*
30+
* The surrounding pipeline stages — occlusion, the off-screen guard,
31+
* hittable-ancestor promotion, and the wait poll budget — still live in the
32+
* callers and are NOT declared here. An earlier revision listed them as
33+
* columns; nothing consumed them, so they were unverifiable claims that read
34+
* as truth while being free to drift (#1649 review). Routing them into typed
35+
* behavior is tracked in #1656.
36+
*/
37+
38+
export type KnobBackedSelectorAmbiguity = 'disambiguate' | 'fail-closed' | 'first-match';
39+
export type SelectorAmbiguityPolicy = KnobBackedSelectorAmbiguity | 'reject-candidates';
40+
41+
export type SelectorResolutionPolicy = {
42+
ambiguity: SelectorAmbiguityPolicy;
43+
/** Only nodes carrying a rect participate (acting paths need a tap point). */
44+
requireRect: boolean;
45+
};
46+
47+
export const SELECTOR_RESOLUTION_POLICIES = {
48+
/** click/press/fill/focus/longPress/drag/scroll targets (resolution.ts). */
49+
act: {
50+
ambiguity: 'disambiguate',
51+
requireRect: true,
52+
},
53+
/** The post-miss diagnosis probe deciding "no match" vs "matched but covered". */
54+
actCoveredDiagnosis: {
55+
ambiguity: 'first-match',
56+
requireRect: true,
57+
},
58+
/** `get text` — reads through the same tiebreak acting uses. */
59+
readText: {
60+
ambiguity: 'disambiguate',
61+
requireRect: false,
62+
},
63+
/** `is` non-exists predicates and `get attrs` — ties reject, never guess. */
64+
readUnique: {
65+
ambiguity: 'fail-closed',
66+
requireRect: false,
67+
},
68+
/** `exists` and find's read-only actions — presence is the question. */
69+
readAny: {
70+
ambiguity: 'first-match',
71+
requireRect: false,
72+
},
73+
/** `wait` — first match per poll, under the wait budget. */
74+
wait: {
75+
ambiguity: 'first-match',
76+
requireRect: false,
77+
},
78+
/** Mutating `find` (#1625): candidates reject unless explicitly narrowed. */
79+
findAct: {
80+
ambiguity: 'reject-candidates',
81+
requireRect: true,
82+
},
83+
} as const satisfies Record<string, SelectorResolutionPolicy>;
84+
85+
/**
86+
* The engine knobs a knob-backed policy row stands for. `reject-candidates`
87+
* rows are rejected at the type level — that contract is enforced by the
88+
* caller's narrowing logic, not by these knobs.
89+
*/
90+
export function selectorResolutionKnobs(
91+
policy: SelectorResolutionPolicy & { ambiguity: KnobBackedSelectorAmbiguity },
92+
): Pick<SelectorResolutionOptions, 'requireRect' | 'requireUnique' | 'disambiguateAmbiguous'> {
93+
if (policy.ambiguity === 'first-match') {
94+
return { requireRect: policy.requireRect, requireUnique: false };
95+
}
96+
return {
97+
requireRect: policy.requireRect,
98+
requireUnique: true,
99+
disambiguateAmbiguous: policy.ambiguity === 'disambiguate',
100+
};
101+
}
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import type { SnapshotState } from '@agent-device/kernel/snapshot';
2+
import type { SelectorChain } from './parse.ts';
3+
import type { SelectorMatchOptions } from './public-resolution-types.ts';
4+
import {
5+
listSelectorChainMatches,
6+
resolveSelectorChain,
7+
type AstSelectorResolution,
8+
} from './resolve.ts';
9+
import type { SelectorResolutionPolicy } from './resolution-policy.ts';
10+
11+
/**
12+
* The one policy-driven resolution entry every native caller routes through
13+
* (#1630). A caller passes the policy row that IS its documented contract;
14+
* this decides what "resolved" means for that row, so ambiguity semantics
15+
* live in the matrix rather than in each caller's local branching.
16+
*
17+
* The outcome is a discriminated union rather than a nullable node, because
18+
* the rows genuinely disagree about what to do with several matches:
19+
* `disambiguate` and `fail-closed` want one winner or nothing, `first-match`
20+
* wants the head of the list, and `reject-candidates` needs the whole
21+
* candidate set to refuse with (or to narrow, when the caller was given an
22+
* explicit index). Collapsing those into "node | null" is what previously
23+
* forced every caller to re-derive its own contract inline.
24+
*/
25+
26+
export type AstPolicyResolutionOutcome =
27+
/** No selector alternative matched anything. */
28+
| { kind: 'none' }
29+
/**
30+
* The node this policy authorizes acting on, plus the full candidate set
31+
* of the alternative it came from. Callers that verify identity across
32+
* candidates (wait's #1349 landmark check) need the whole set — a policy
33+
* that picks one winner must not throw the rest away, or a first impostor
34+
* would hide a later genuine match.
35+
*/
36+
| {
37+
kind: 'resolved';
38+
resolution: AstSelectorResolution;
39+
matchedNodes: SnapshotState['nodes'];
40+
}
41+
/**
42+
* Several matches and the policy refuses to choose. `fail-closed` returns
43+
* this instead of guessing; `reject-candidates` returns it so the caller
44+
* can narrow explicitly or surface the candidate list.
45+
*/
46+
| {
47+
kind: 'ambiguous';
48+
selector: string;
49+
selectorIndex: number;
50+
matchedNodes: SnapshotState['nodes'];
51+
};
52+
53+
export function resolveSelectorChainWithPolicy(
54+
nodes: SnapshotState['nodes'],
55+
chain: SelectorChain,
56+
policy: SelectorResolutionPolicy,
57+
options: SelectorMatchOptions,
58+
): AstPolicyResolutionOutcome {
59+
const matchOptions = { ...options, requireRect: policy.requireRect };
60+
61+
if (policy.ambiguity === 'reject-candidates') {
62+
const list = listSelectorChainMatches(nodes, chain, matchOptions);
63+
if (!list || list.matchedNodes.length === 0) return { kind: 'none' };
64+
if (list.matchedNodes.length > 1) {
65+
return {
66+
kind: 'ambiguous',
67+
selector: list.selector.raw,
68+
selectorIndex: list.selectorIndex,
69+
matchedNodes: list.matchedNodes,
70+
};
71+
}
72+
return resolvedFromList(list);
73+
}
74+
75+
if (policy.ambiguity === 'first-match') {
76+
const list = listSelectorChainMatches(nodes, chain, matchOptions);
77+
if (!list || list.matchedNodes.length === 0) return { kind: 'none' };
78+
return resolvedFromList(list);
79+
}
80+
81+
const resolution = resolveSelectorChain(nodes, chain, {
82+
...matchOptions,
83+
requireUnique: true,
84+
disambiguateAmbiguous: policy.ambiguity === 'disambiguate',
85+
});
86+
if (resolution) {
87+
const list = listSelectorChainMatches(nodes, chain, matchOptions);
88+
return {
89+
kind: 'resolved',
90+
resolution,
91+
matchedNodes: list?.matchedNodes ?? [resolution.node],
92+
};
93+
}
94+
95+
// Distinguish "nothing matched" from "matched but this policy will not
96+
// choose" — a fail-closed caller must report ambiguity, not absence.
97+
const list = listSelectorChainMatches(nodes, chain, matchOptions);
98+
if (!list || list.matchedNodes.length === 0) return { kind: 'none' };
99+
return {
100+
kind: 'ambiguous',
101+
selector: list.selector.raw,
102+
selectorIndex: list.selectorIndex,
103+
matchedNodes: list.matchedNodes,
104+
};
105+
}
106+
107+
function resolvedFromList(
108+
list: NonNullable<ReturnType<typeof listSelectorChainMatches>>,
109+
): AstPolicyResolutionOutcome {
110+
const node = list.matchedNodes[0];
111+
if (!node) return { kind: 'none' };
112+
return {
113+
kind: 'resolved',
114+
matchedNodes: list.matchedNodes,
115+
resolution: {
116+
node,
117+
selector: list.selector,
118+
selectorIndex: list.selectorIndex,
119+
matches: list.matchedNodes.length,
120+
diagnostics: [{ selector: list.selector.raw, matches: list.matchedNodes.length }],
121+
},
122+
};
123+
}

scripts/layering/facade-exports.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,3 +96,27 @@ export function readDirectNamedExports(source: string): string[] {
9696
}
9797
return [...names].sort();
9898
}
99+
100+
/**
101+
* Which module each name in `source` is re-exported FROM, for names that come
102+
* from a re-export rather than a local declaration.
103+
*
104+
* A façade's export *names* are only half its boundary: a type re-exported
105+
* from the right module and one re-exported from a package-private module read
106+
* identically in the name list, while only the second leaks. #1649 shipped
107+
* exactly that — a policy outcome re-exported from the parser-side module, so
108+
* its nested `resolution` field handed callers the private AST — and the
109+
* name-list gate stayed green throughout.
110+
*/
111+
export function readReExportSources(source: string): Map<string, string> {
112+
const parsed = parseSync('facade-reexport-source-scan.ts', source);
113+
const sources = new Map<string, string>();
114+
for (const staticExport of parsed.module.staticExports) {
115+
for (const entry of staticExport.entries) {
116+
if (entry.exportName.kind !== 'Name' || !entry.exportName.name) continue;
117+
if (!entry.moduleRequest) continue;
118+
sources.set(entry.exportName.name, entry.moduleRequest.value);
119+
}
120+
}
121+
return sources;
122+
}

scripts/layering/package-boundaries.test.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import fs from 'node:fs';
77
import path from 'node:path';
88
import { test } from 'node:test';
99
import { listSourceFiles } from './check.ts';
10-
import { readDirectNamedExports, readNamedExports } from './facade-exports.ts';
10+
import { readDirectNamedExports, readNamedExports, readReExportSources } from './facade-exports.ts';
1111
import {
1212
checkPackageBoundaries,
1313
checkPackageInternalSites,
@@ -398,6 +398,25 @@ test('the real tree parses, declares, and passes R11', () => {
398398
[],
399399
'selectors façade keeps AST and grammar internals private',
400400
);
401+
// Named exports are not the whole boundary. A parser-side type reached
402+
// through a NESTED field — `PolicyResolutionOutcome.resolution` typed as
403+
// `AstSelectorResolution` — leaks the same objects while exporting none of
404+
// their names, and the assertion above stays green on it (#1649). What
405+
// separates the two is which module the type is re-exported FROM:
406+
// `public-resolution-types.ts` holds the string-flattened shapes,
407+
// `resolve-with-policy.ts` and `resolve.ts` hold the parser-side ones. A
408+
// resolution type re-exported from either of the latter means a flattening
409+
// step at the façade was skipped.
410+
const selectorsReExports = readReExportSources(
411+
fs.readFileSync(path.join(repoRoot, 'packages/selectors/src/index.ts'), 'utf8'),
412+
);
413+
assert.deepEqual(
414+
['PolicyResolutionOutcome', 'SelectorResolution', 'SelectorChainMatchList'].filter(
415+
(name) => selectorsReExports.get(name) !== './internal/public-resolution-types.ts',
416+
),
417+
[],
418+
'selectors façade must publish resolution shapes from public-resolution-types.ts, not from the parser-side modules',
419+
);
401420
// The AST subpath's one in-repo consumer is the published SDK re-export.
402421
// Anything else importing it means the string-only façade was bypassed.
403422
assert.deepEqual(

0 commit comments

Comments
 (0)