mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
main
25 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michał Pierzchała
|
de8703b6a0 |
fix(selectors): resolve a wrapper chain's control for uniqueness reads (#2501)
A control reported through its own accessibility wrapper answers a selector twice, and a regular iOS snapshot omits unverified hittability, so the ladder that relates a wrapper to its control cannot fire. #2482 collapsed that chain for mutating resolution only: `press` tapped the toolbar button while `is visible` and `get attrs` reported "Selector did not match" and `screenshot --crop-on` refused the same screen as two nodes. Export the collapse beside the classification that asks for it and apply it where a read row's answer was a refusal. Rows that resolve before any refusal are untouched, and a candidate set the rule does not recognize as one control - a cell and the button inside it, or matches in distinct subtrees - still refuses. Replay verifies a recorded target by resolving its recorded selector again under the same row's refusal rules, so a step whose screen had not changed verified as an identity mismatch on its first replay. Verification names the collapsed control too, which is the node dispatch acted on and the node the recorded identity carries. |
||
|
Prateek Ranka
|
c94e66e9b8 |
fix(selectors): collapse an unverified-hittability wrapper chain to its control (#2482)
* fix(selectors): collapse an unverified-hittability wrapper chain to its control A SwiftUI toolbar wrapper and its control share one identifier, and regular iOS snapshots omit hittability evidence. findPreferredActionableDescendant requires verified hittability, and the wrapper's rect differs by under a point per edge, so press/fill saw two distinct actionable elements for one control and refused with AMBIGUOUS_MATCH. Resolve the deepest semantic touch target when every candidate lacks hittability evidence and all rects agree within sub-pixel slack. Candidates carrying any hittability fact keep the existing rules. * fix(selectors): keep the wrapper-collapse fallback to non-actionable wrappers Review follow-up on #2482. The unverified-hittability collapse accepted any ancestry chain whose rects agreed within a point, so a cell and the button inside it (both actionable, no hittability evidence) collapsed to the descendant: a silent wrong-control press where the previous rules refused as ambiguous. The fallback now requires every candidate above the control to be a non-actionable wrapper, and a negative regression covers the semantic-ancestor case next to the captured Other/Button success case. Gate: pnpm check:affected --run - 304 files / 2011 tests, all runnable checks passed. * perf(selectors): keep the wrapper collapse inside its budgeted closure The extracted module grew the eager closure of three budgeted entries by one module each -- interaction-targeting.ts 13 -> 14, selector-pipeline.ts 25 -> 26, absence-observation-resolution.ts likewise -- and the eager-closure gate ratchets that closure against the merge-base: an entry surface that drags more of the repo onto the import path is a loading-shape regression whatever the reason. The rule has exactly one consumer, so it now lives beside the classification that asks it and is no longer an exported surface. Its tests move to the owning module's test file and exercise `classifyActionableTouchCandidates`, the boundary the command actually calls. Each of the four refusals fails when its own guard is mutated: the hittability condition, the 1 pt slack, the non-actionable-wrapper condition, and the semantic-control condition. --------- Co-authored-by: Michał Pierzchała <thymikee@gmail.com> |
||
|
Michał Pierzchała
|
6d08de4609 |
feat(scroll): find off-screen targets in one command with --until (#2436)
* refactor(interaction): extract the scroll command runtime out of gestures.ts * feat(scroll): add --until <selector>, report honored travel, fix web amount units * test(scroll): cover --until through the provider-backed integration path * perf(selectors): keep the scroll-until predicate off the eager import path * fix(scroll): refuse an unreadable capture instead of reporting end-of-content * fix(selectors): keep the capture-readability check off the eager import path * test(selectors): use a declared snapshot quality state in the capture fixtures * fix(scroll): read the capture quality verdict under the spelling the backend uses * refactor(scroll): collapse --until onto the one route that runs it * refactor(scroll): drop unexported until types and duplicated guidance prose * test(scroll): fix the climbing fixture and drop duplicated route-level cases * refactor(scroll): delete the dead command-runtime executor and reuse canonical predicates * refactor(interaction): keep requireResolvedPoint local to the gesture runtime |
||
|
Michał Pierzchała
|
fef0b12cc5 |
chore: hoist shared snapshot/selector test fixtures into a single canonical location (#2419)
* chore: hoist shared snapshot/selector test fixtures into @agent-device/selectors PR #2397 left two copies of the snapshot-state builder and duplicated geometry/touch-point arbitraries (root's src/__tests__/test-utils/ and the package's internal/__tests__/), because packages cannot import root src/. Move the canonical versions into a new @agent-device/selectors/test-fixtures subpath and have both root and the selectors package import from it, leaving buildNodes and the root-only replay/gesture arbitraries in place. Fixes #2402 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USabYbQjD16A2UkkvMpf5x * chore: exempt test-fixtures.ts's test-only arbitraries from dead-code check PROPERTY_RUNS, scrollingContainerTypeArb, distinctRectPairArb, and interactionTouchPointScenarioArb are consumed only by *.test.ts files, which Fallow's --production analysis does not see, matching the existing pattern for other workspace-package symbols reached only from the test tree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USabYbQjD16A2UkkvMpf5x * chore: consolidate makeSnapshotState into capture-kit, rename fixtures file An adversarial review of the #2402 fixture-hoisting change found a third copy of makeSnapshotState in packages/capture-kit/src/snapshot-state.fixtures.ts, predating PR #2397. Since @agent-device/selectors already depends on capture-kit, make capture-kit's copy canonical (exported as ./snapshot-state-fixtures) and have the selectors package's fixtures module re-export it instead of duplicating it a third time. Also rename packages/selectors/src/test-fixtures.ts to snapshot-geometry.fixtures.ts (subpath ./snapshot-geometry-fixtures) to match every other test-fixture module's *.fixtures.ts convention in this repo, which lets it fall under .fallowrc.json's existing blanket **/*.fixtures.ts dead-code exemption instead of needing a bespoke per-symbol entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USabYbQjD16A2UkkvMpf5x --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
8d5ca680c0 |
refactor(move): move the selector pipeline and interaction targeting into @agent-device/selectors (#2397)
* refactor(move): move the selector pipeline and interaction targeting into @agent-device/selectors The 11 pipeline modules (selector-pipeline, selector-pipeline-policy, interaction-targeting, touch-semantics, interaction-positionals, press-retarget, interaction-touch-point, absence-observation and its errors/resolution companions, and the interaction-error vocabulary) are exposed as per-file subpaths. The two test-utils files the moved tests share with root tests are copied into the package, following the existing package-local test-utility pattern. * chore(gates): point the layering pins at the moved selector pipeline R19's owner constant now names the pipeline in packages/selectors, and the rule additionally refuses in-package relative routes to the engine file so the co-location cannot widen the door. The package-boundaries export/dependency pins and the fallow health baseline key follow the files. * fix: drop two unused exports flagged by fallow * test: point press-retarget comment at the relocated touch-semantics module |
||
|
Michał Pierzchała
|
1f9d940bff |
refactor(capture-kit): complete ADR 0019 end state — relocate snapshot and recording zones (#2385)
* refactor(capture-kit): relocate snapshot and recording zones into capture-kit
Move the ADR 0019 end-state capture zones into @agent-device/capture-kit:
- src/snapshot/** -> packages/capture-kit/src/snapshot/** (presentation,
freshness, scroll-edge-state, ios-snapshot-runtime, android occlusion)
- src/recording/** -> packages/capture-kit/src/recording/**
- src/core/snapshot-{chrome,state,tree-ingestion,node-lookup}.ts ->
packages/capture-kit/src/
- src/snapshot-quality/ test -> capture-kit presentation tree (directory
retires with its last file)
Pure renames: import re-pointing and gate updates follow in the next commit.
The snapshot-desktop-surface test parks in src/__tests__/ because it pins
the root eager-import-closure walker.
* refactor(capture-kit): re-point capture and recording consumers to the new subpaths
Rewires every consumer of the relocated snapshot/recording modules to the new @agent-device/capture-kit subpath exports, adds the 23 subpath entries to the capture-kit exports map, fixes the moved recording-scripts test's __dirname-relative paths for the deeper location, and records the completed migration in ADR 0019's end state.
* chore(gates): align layering, mutation, fallow and CI gates with the capture-kit relocation
Moves the executable-policy roots, presentation-owner constant, zone ranks, authority fixture, mutation sharding globs, stryker aliases, fallow baselines and the iOS workflow's android-owned paths-ignore entry onto the new packages/capture-kit paths, and extends the planted-red coverage to the new presentation-owner subpath.
* chore: point capture-domain source-of-truth comments at the relocated capture-kit modules
* test: point shutdown recording mock at capture-kit and cover interactor acquisition presentation
* test(capture-kit): update upstream presentation test imports
* chore(gates): follow relocated snapshot assembly in R74
* test(daemon): freeze prewarm deadline assertion clocks
|
||
|
Michał Pierzchała
|
b4ebd778cc |
refactor(daemon): move four pure leaves to their kits (#2347)
* refactor(selectors): own the parameterized recorded fill leaf `parameterized-recorded-fill.ts` has no value dependency on the daemon: it reads a `TargetAnnotationV1` type from contracts and calls `selectorContainsValue`, so its whole value graph already sits inside `@agent-device/selectors`. Move it there behind its own subpath and let the two daemon consumers reach it by specifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK * refactor(host-kit): own the daemon code signature leaves `code-signature.ts` fingerprints a checkout from `node:crypto`/`fs`/`path` and `findProjectRoot`; `code-signature-cache.ts` adds a stat-validated cache over it through `publishFileSync`. Neither reaches the daemon, and both questions — what does this source tree hash to, and can that hash be replayed from stat alone — are host mechanics. Move both into host-kit behind their own subpaths, carrying `code-signature-cache.test.ts` unchanged apart from its specifiers, and let the launch spec and server lifecycle reach them by specifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK * refactor(capture-kit): own the screenshot overlay cluster `screenshot-overlay.ts` decides which snapshot nodes earn a ref and where the ref lands on a screenshot; `screenshot-overlay-draw.ts` paints them. Both read kernel snapshot vocabulary, contracts snapshot predicates, and capture-kit's own PNG and rect-projection mechanics — nothing from the daemon. The two `src/snapshot/screenshot-overlay/` helpers had no other importer, and `react-native-overlay.ts` sits on kernel plus its contracts vocabulary alone. Move the cluster into capture-kit as flat siblings of the PNG and projection modules it already used, exposing `./screenshot-overlay` and `./react-native-overlay`; the draw, rects, and android halves stay package internals with no subpath of their own. The moved tests carry over unchanged apart from their specifiers, over a package-local snapshot-state fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK * refactor(capture-kit): own the post-gesture stability loop `post-gesture-stability.ts` polls a caller-supplied snapshot function until a surface settles. It is generic over its snapshot and signature types and reads only host-kit diagnostics and `sleep`, so the loop is capture mechanics with no daemon knowledge; the daemon keeps the pending record, the comparator, and the verdict wiring it hands in. The verdict test stays in `src/daemon` because it composes the loop with the daemon's own `interaction-outcome-policy.ts`; only its specifier changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK * chore(gates): pin the four leaf subpaths in the R11 export lists R11 pins every workspace package's exact subpath set, so the four moves need their new specifiers named: `@agent-device/selectors/parameterized-recorded-fill`, `@agent-device/host-kit/code-signature{,-cache}`, and `@agent-device/capture-kit/{screenshot-overlay,react-native-overlay,post-gesture-stability}`. The selectors comment counted its subpaths in prose; it now counts four and says what the fourth is. No eager-closure row is needed: every new entry is a rename the merge-base reader follows, and each closure is unchanged (56/5/19/32/4/8), so all six fall under no-growth rather than the new-entry ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK * refactor(capture-kit): drop the needless duplication suppression The `fallow-ignore-next-line code-duplication` on the package-local snapshot-state fixture suppressed nothing: `fallow dupes` reports three clone groups on this tree and the fixture is in none of them, with or without the comment. A suppression that matches no finding is dead weight at best and a stale-suppression failure at worst. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK * fix(host-kit): follow workspace subpaths when fingerprinting daemon source `walkDaemonCodeGraph` followed relative specifiers only, so a source checkout's signature covered whatever the daemon still imported by relative path. That was already lossy and the leaf moves made it wrong: the walker itself, the overlay, the recorded-fill and the stability loop all left the graph, so editing them no longer changed the signature a client compares a running daemon against, and the cache's format guard lost the "the walk invalidates every document" property its comment rests on. Measured from `src/daemon.ts`: 619 modules on main with the walker stamped, 611 after the moves with it gone. Resolve a scoped specifier through the owning workspace package's `exports` map and walk into the file it names. The manifest is stamped, not merely probed, so an `exports` retarget invalidates without either endpoint changing; an uninstalled package is recorded as an absent path. Installed dependencies are still not followed — they change on install, not on edit — and the test is structural rather than a name pattern. The graph is now 1459 modules and ~112ms cold, which is what the stat-validated cache exists to absorb. Regression: five of the six new walker tests fail against the previous walker, including one that stamps the real daemon graph and asserts the walker is in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BPFjVXwbDrXPqp5W6K2iDK --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
172ee149cf |
feat(screenshot): add --crop-on to crop captures to a selector frame (#2276)
* feat(screenshot): add crop-on geometry core and cropTarget selector rows
* feat(screenshot): declare crop-on flag, script round-trip, and snapshot runtime plan
* feat(screenshot): run the crop leaf after the platform write and before scale
* feat(screenshot): expose --crop-on in the CLI and surface crop warnings
* chore(gates): declare crop-on capture-kit subpaths and scope the crop scenario exemption
* refactor(screenshot): split crop target/policy module and trim redundant coverage
Address review comments at
|
||
|
Michał Pierzchała
|
2371ba9bff |
feat: add strict native absence assertion (#2245)
* feat: add strict native absence assertion * fix: address absence assertion review feedback |
||
|
Michał Pierzchała
|
db08548026 | refactor: enforce src/utils retirement (#2149) (#2229) | ||
|
Michał Pierzchała
|
947582a3cc | refactor(daemon): move interaction and find routes behind facade (#2178) (#2228) | ||
|
Michał Pierzchała
|
70fb783729 |
fix: reuse canonical Maestro visibility context (#2156)
* fix: reuse canonical Maestro visibility context * perf: skip empty Maestro visibility filtering * refactor: centralize snapshot visibility context |
||
|
Michał Pierzchała
|
af6f12e391 |
chore: adopt shared oxlint config (#2115)
* chore: adopt shared oxlint config * fix: preserve project lint boundaries * fix: remove redundant oxlint config |
||
|
Nicolas Bataille
|
4b8bcaca60 |
feat(interaction): accept fill <target> "" as the clear-field primitive (#2066)
* feat(interaction): accept fill <target> "" as the clear-field primitive
Emptying an input was not expressible: `fill` refused the empty string
("Expected text to be a non-empty string"), `type` only appends, and `keyboard`
has no delete verb. Clearing a field before typing is a routine QA step, so the
only route was the app's own clear button or N locale-dependent keyboard delete
presses read out of a snapshot.
`fill <target> ""` now means "replace with nothing". Both platforms already own
the clear half of replace, so this is the validation and reporting that stood in
front of it, not a new interaction:
- `stringField` takes an opt-in `allowEmpty`, used only by `fill`'s `text`.
`requiredField` still refuses a MISSING text, so `fill @e57` stays an error
rather than silently erasing the field — `readFillTargetFromPositionals` now
reports `undefined` for "no text argument" instead of collapsing it to `''`.
`type` keeps refusing an empty text: appending nothing is not a clear.
- The Apple runner's empty-text early return skipped the clear while reporting
"typed". For a replacement it now runs `clearTextInput` and verifies the field
came back empty (secure fields stay unverifiable, as elsewhere).
- Android already clears before typing and skips an empty shell/IME write, but
its verifier read a cleared field's absent `text` attribute as a mismatch
against `''`. An empty expectation now accepts null or "".
Whitespace-only text keeps its established per-shape rules; only `''` is new.
Closes #2063
* fix(interaction): fail the empty-fill clear closed on every backend
Addresses the P1 review on #2066, then closes the same fail-open class
on the backends the PR did not reach:
- Android: an empty expectation no longer matches when the verification
scan observed NO input node at all — actual is null both for a cleared
field and for a wrong point/lost focus, and three empty samples of
nothing were a stable success for a clear that never touched a field.
- Apple runner: when the empty-replacement path cannot resolve a clear
target (including the synthesized first-responder route, whose target
carries no element), it returns the typed TEXT_INPUT_NOT_FOCUSED
failure instead of falling through to the vacuous-typing
verified-success return. Regression runs in the ios.yml XCTest lane.
- webdriver: fill is tap + sendKeys and owns no clear mechanism, so an
empty fill refuses as UNSUPPORTED_OPERATION before touching the
device, instead of reporting a clear it cannot perform.
- linux + web coordinate fill: typing zero characters over the
select-all selection left the old value intact; the empty fill now
deletes the selection.
- recording: an empty --record-as literal matches inside every string;
it now parameterizes only the fill's own text field instead of
rewriting every empty field and empty evidence label in the entry.
(The session-wide echo registry already excluded empty literals.)
- help: the text-entry topic taught agents that fill "" is not a
clear-field command; it now states the new contract.
Each new test was observed red against the pre-fix code.
* fix(android): read hint-showing from the helper so a cleared field verifies
Live Pixel 9 emulator, adb-shell channel: clearing the Settings search
field succeeded on the device but reported 'Android fill verification
failed', because a cleared EditText dumps its HINT as text — getText()
returns the hint for an empty field on modern Android, so 'Search
settings' read back as a residual value. This is the same
placeholder-as-value trap the Apple runner already handles with
treatingPlaceholderAsEmpty.
The helper now emits hint-showing (isShowingHintText, API 26+), the
hierarchy parser carries it, and fill verification matches against the
field's VALUE — hint-only text is an empty value, for empty and
non-empty expectations alike. A field whose real value equals its hint
string keeps failing the clear check: only the authoritative flag, never
the text, says it is a hint. Raw uiautomator dumps carry no such fact
and keep the fail-closed behavior.
Live evidence, both admission channels, after this fix: test-ime and
adb-shell clears both report Filled 0 chars with the field back on its
placeholder; the pre-fix adb-shell run failed closed (never a false
success).
* fix(interaction): close the adversarial-review findings on the empty-fill clear
- android adb-shell: the delete burst is sized from the value being
REMOVED (pre-mutation read; the attempt's cap when unreadable), not
from the empty incoming text, which sent the 12/24-delete minimums and
could never empty a field longer than 36 characters.
- android: the unconfirmed soft-success no longer applies to an empty
expectation — nothing app-formats the empty value, so residue after a
clear is a failed clear, and the soft-success also skipped the second,
bigger delete burst.
- android masked fields: an empty expectation accepts an observed masked
node with no dump text (a masked field WITH content dumps its bullet
run), so clearing a password field no longer fails after the clear
worked — matching iOS, where a secure-field clear succeeds unverified.
- find: 'find <q> fill ""' now reaches the fill leaf as the clear
request on both the CLI reader and the daemon positional parse; a
MISSING value keeps its refusal at each producer, so the typed
value: string contract is unchanged.
- maestro export: a recorded clear exports as tapOn + eraseText instead
of a vacuous inputText: "" (with the 50-character-default warning).
- the missing-text refusals teach the clear form: (use "" to clear
the field).
Full unit suite green (1061 files); each behavioral fix carries a test
observed red against the prior code.
* refactor(interaction,android): extract the fill parse and shell-attempt branches
The review commits pushed parseFillTarget and fillAndroid over the
complexity gate (13 cyclomatic each). Each fill target shape parses in
its own function sharing one missing-text response, and the adb-shell
attempt (clear sizing + clear + type + verify) moves out of the fill
loop. Behavior-preserving; the existing tests cover every branch.
* refactor(interaction,android): one owner per empty-fill fact
Design pass after review: the missing-vs-empty rule and the observed-
value rule each had several owners; now each has one.
- parseFillTarget decodes ONCE through readFillTargetFromPositionals —
which already owns shape detection and documents the undefined-vs-''
contract on DecodedFillTarget — and keeps only what the wire owns:
versioned-ref admission, the selector whitespace rule, and the daemon
responses. This deletes the point branch's duplicated slicing, the
hasFillText guard, and the three per-shape parse functions.
- observedAndroidValue() is the single statement of Android's value
rule (absent attribute and hint-only text are the empty value); the
text branch, the match rule, and the masked branch all consume it.
The masked branch thereby gains the hint-showing collapse it was
missing, and isAcceptableAndroidFillMatch narrows to plain strings.
- The empty-text-is-clear contract is stated once, on Interactor.fill
in contracts, instead of implied per backend.
Behavior-preserving except the masked+hint gain; the existing tests
cover every branch (494 Android, 15 fill-target).
---------
Co-authored-by: Michał Pierzchała <thymikee@gmail.com>
|
||
|
Michał Pierzchała
|
942a310fb3 |
perf(selectors): collapse the double tree scan for disambiguate/fail-closed rows (#2009)
resolveSelectorChainWithPolicy ran listSelectorChainMatches (one full scan per alternative) and then resolveSelectorChain (another full scan per alternative) for the disambiguate/fail-closed rows readText/readUnique use — the same defect class #1690 removed from replay's resolveRecordedTarget. resolveSelectorChainDomain now tracks the first alternative that matched anything (`firstMatch`) unconditionally, in the same pass that already decides the winning resolution, so resolveSelectorChainWithPolicy needs only one call for these rows. matchedNodes keeps naming the first alternative that matched (not the winner) when they differ, preserving the existing contract wait's landmark check and the ambiguous outcome rely on. Also fixes analyzeSelectorMatches's lazy isVisible: it now builds the viewport-root rect list once per alternative (via the newly extracted collectViewportRects) alongside the existing lazily-built byIndex map, instead of isNodeVisibleOnScreen re-deriving it on every ambiguous candidate. Closes #1970 Claude-Session: https://claude.ai/code/session_01YJoiggu7utUNDBmdzSBK2h Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
66458c7916 |
perf(selectors): resolve a recorded replay target in one matching pass (#1988)
* perf(selectors): resolve a recorded replay target in one matching pass resolveRecordedTarget ran two full scans of the snapshot tree per call: resolveSelectorChain visited every node to pick a winner, then the caller either re-filtered every node with the winning selector or handed the chain to listSelectorChainMatches to re-derive the same domain. resolveSelectorChainDomain returns the matched-node set the deciding pass already collected — the winning alternative's when one resolved, the first matching alternative's when none did, which is the set listSelectorChainMatches reports. resolveSelectorChain now delegates to it, so its shape and the published ./ast surface are unchanged, and listSelectorChainMatches stays exported for its three other callers. Winner, matchedNodes, matchCount, disambiguation disclosure, and the ambiguous-vs-no-match classification are unchanged. Observed red first: the new traversal-count regression reported 1 redundant filter scan on both the resolved and the unresolved leg. * test(selectors): count the full array-scan surface in the replay traversal gate Review F1: `observeTreeTraversals` counted only `Symbol.iterator`, `filter` and `map`, so a regression that reintroduced a whole-tree pass through `flatMap`/`forEach`/`reduce`/`some` kept the assertion green. Widen it to the whole scan surface and pin the true counts. That exposes eight `flatMap` scans on the ambiguous leg — four whole-tree viewport-rect lookups per ambiguous candidate, charged by `isNodeVisibleOnScreen` with no precomputed viewport rects. They predate this branch and are left alone here; pinning them keeps the number from growing unnoticed. Rename the test to what it proves: no SECOND matching pass per alternative, not "reads the tree once". The review's F4 note on `ActionableTouchTopology.viewportRootRects` moved to the find-ranking branch this one stacks on, since it documents that branch's type. * test(selectors): count find/every/indexOf-class scans in the traversal gate The replay traversal counter only watched filter/map/flatMap/forEach/reduce/ some/iterator, so a redundant full-tree scan expressed via find, every, findIndex/findLast(Index), includes, indexOf, or reduceRight stayed green. Expand SCAN_METHODS to the full scan surface; planted-red demonstrated by injecting a single nodes.find() into resolveRecordedTarget (gate fails with find: 1) and reverting. |
||
|
Michał Pierzchała
|
f065e6aeb4 |
fix(maestro): align label metadata ownership (#1909)
* fix(maestro): align label metadata ownership * fix(maestro): centralize command label parsing * test(maestro): cover runFlow label ownership |
||
|
Michał Pierzchała
|
d072819632 |
refactor: reuse kernel uniqueStrings instead of local copies (#1892)
Three modules hand-rolled local uniqueStrings helpers despite the AGENTS.md reuse rule and the canonical export in @agent-device/kernel/collections: - src/platforms/apple/core/perf-frame.ts (identical semantics) - src/platforms/web/agent-browser-lifecycle.ts (empty-string filter now composed at the one call site that receives caller input; the home marker call site passes path.join results that are never empty) - packages/selectors/src/internal/build.ts (identical semantics) All three files' packages already depend on @agent-device/kernel and siblings import the same subpath. |
||
|
Michał Pierzchała
|
74eab2a554 |
refactor: route selector-resolution structural stages into typed policy (#1744)
* refactor: route selector structural stages into typed policy #1649 landed the per-caller ambiguity matrix and deliberately left four structural columns out: occlusion, off-screen, hittable-ancestor promotion, and the poll budget were per-caller pipeline code, so declaring them would have been an unverifiable claim (nothing consumed them; flipping one left the suite green). This adds the missing half as a table with runners. `SELECTOR_PIPELINE_POLICIES` (src/core/selector-pipeline-policy.ts) gives each caller ONE row naming its ambiguity contract plus its four stages, and every stage is reached only through a runner that reads the row: - occlusion -> selectorPipelineCandidates (candidacy) and resolveSelectorPipelineTarget (refusal). Acting rows exclude covered nodes and refuse covered targets; `find` and the diagnosis probe keep them as candidates and refuse at the target; reads and `wait` ignore them. - promotion -> resolveSelectorPipelineTarget. The per-call-site `promoteToHittableAncestor: boolean` is gone: click/press/longpress name `promotedTarget`, fill/focus/scroll/drag endpoints and the native-ref preflight name `resolvedTarget`. `find`'s below-the-root variant is a declared value rather than a second local helper. - off-screen -> throwIfOffscreenInteractionTarget, which now takes the row and returns the node untouched (no iOS rescue round trip) for observation rows. - poll -> selectorPollBudget, which createWaitPolling derives its deadline and inter-poll delay from; the two wait loops carry a budget, every other row carries none and cannot be polled. Behavior is byte-identical. The acting refusal keeps its exact node, label and details in every branch (promotion declines to retarget away from a covered node, so the "both covered" case names the same node it always did), and `find` carries the occlusion verdict to the focus/type seam rather than raising it early, because find click/fill still delegate that refusal to the interaction leaf's own error shape. selector-pipeline-policy.test.ts drives EVERY row through EVERY runner, including the rows whose answer is "skip" — the half that used to be an absence of code, and an absence cannot fail. Each stage was proven red by flipping its cell (occlusion, promotion, off-screen, poll, plus the declare-only-what-is-enforced guard). The ADR 0011 occlusion/nonHittable `via` pointers for the runtime tree paths now name the runner that makes the decision, not the predicate it applies. Closes #1656; prework for #1739 (waves 4-5). * docs: state constraints instead of narrating the refactor Comment pass over #1656: drop the "used to be per-caller code" / "not module constants" / "rather than an omission" narration — a comment should say what a future edit must respect, not what the previous shape was — and compress the find occlusion-verdict and poll-budget notes to the constraint they actually carry. * refactor: make the selector pipeline the only door to the engine Review of #1744: the structural rows were declared but bypassable. Read and wait routes composed `selectorPipelineCandidates(row, nodes)` with the raw `resolveSelectorChainWithPolicy(..., row.resolution)` and never entered the promotion or off-screen stages, so flipping a read row's `promotion` or `offscreen` changed only the policy unit tests — production `get`/`is`/`wait` were unaffected, which is the unverifiable-column failure #1656 exists to remove. Callers could also pair one row's candidate set with another row's ambiguity contract, and `find list` reached the engine directly. The owning interface (src/core/selector-pipeline.ts) now runs every stage a row declares, skips included, and the stage functions are private to it: - `resolveSelectorPipeline` — single-target rows: candidacy, ambiguity, the replay-guard hook, promotion, occlusion, off-screen. - `listSelectorPipelineMatches` — `reject-candidates` rows, returning the candidate set AND the tree the row sees, so ranking and equivalence classification judge the same nodes candidacy produced. - `runNodePipelineStages` — the node stages for a target from a non-chain matcher (`@ref`, find's fuzzy locator) or a narrowed candidate set. A row whose off-screen stage refuses must supply a refusal shape, so flipping an observation row to `refuse` fails on its real route instead of silently observing. `find list` now names a `readList` row (the new `reject-candidates`/no-rect ambiguity row) instead of calling the engine. R17 selector-pipeline-ownership (scripts/layering/) makes the bypass structurally inexpressible: only the owner may import the engine entry points. Proven against a planted import in selector-read.ts, which the repo-wide scan rejects with the entry points that replace it. Flips now fail through REAL command routes, verified one at a time: readUnique.occlusion/offscreen/promotion and wait.occlusion via get attrs / is / wait; readAny.offscreen via is exists and find; readList.occlusion via find list; promotedTarget.promotion via runtime click. The wait route test needed an advancing clock first — with the frozen one a refused wait spun instead of failing, so the flip hung rather than asserting. * refactor: drop find's dead candidate binding The selector branch bound the row's candidate set and never read it: only the acting classification needs that tree, and find's locator branch brings its own matcher. Names what actually governs the locator target — the shared node stages below, not a candidate set it never had. * refactor: reserve the selector engine behind the pipeline owner Review of #1744 (three blockers). **Listing rows no longer claim stages they cannot run.** `find <q> list` resolves to a candidate SET, so promotion, the off-screen guard and a poll budget have nothing to apply to — a listing has no single element to retarget, keep on screen, or wait for. `readList` now declares only the two stages a listing executes (`SelectorListPolicy`: resolution + occlusion), and the narrower shape is load-bearing: `runNodePipelineStages` and `selectorPollBudget` take the full row, so handing them a listing row is a compile error rather than a silently skipped stage. Pinned with `@ts-expect-error` — widening `readList` makes the directives unused and fails the typecheck. **The engine door is a specifier, not a symbol.** R17's regex could not see a namespace import, a re-export, or a deferred `import()`, none of which mention the symbol it matched. The two engine entries moved to `@agent-device/selectors/engine`, and R19 enforces over the resolved import graph, where every one of those forms is the same edge. Proven on the repo-wide scan by planting each form into a shipped route: namespace import, dynamic import, and `export *` laundering all come back red. `resolveImportEdges` drops an edge whose specifier resolves to nothing, so a specifier rule goes quiet — not red — if the subpath is ever retired. The gate now says that out loud instead of scanning clean. **R19, not R17.** #1750 allocates R17/R18. Verified free against origin/main and that PR's diff, then validated by real merges in both directions: the uniqueness gate passes either way and the three ids stay distinct. The gate itself is new (`scripts/layering/rule-ids.ts`): two branches taking one free number do not conflict in git, so nothing caught R17 twice. Matching whole string literals is what separates a declaration from prose that names a rule, and it is what let the gate see #1750's `const RULE = '…'` shape — the first version missed it and would have been vacuous. `main`'s two pre-existing collisions (R11, R13) are listed as known, not pinned by equality, so #1750 lands in either order without breaking this. Also: the root façade now exposes no resolver at all, and its surface test pins both doors. * fix(layering): make each rule-id allowance expire with its collision Review of #1744: `KNOWN_RULE_ID_COLLISIONS` filtered the exact R11/R13 collision strings, so once #1750 renames those rules apart the entries would keep waving those very collisions through if anyone reintroduced them. "Inert" was wrong — a stale allowance fails open, permanently. `ruleIdCollisionFailures` now checks the transition from both sides: a collision nobody allowed fails, AND an allowance whose collision is absent from the scan fails as a stale allowance. The entry therefore has to be deleted in the same change that removes the collision, and the list burns down to empty, which admits nothing. #1750 is still open, so the transitional entries stay for now (option (b)). Verified against a scratch tree carrying that PR's rename: leaving the list untouched reports both entries as stale; deleting them is clean; and reintroducing `R11 names contracts-implementation-authority and package-boundaries` afterwards is rejected. The last of those is also a unit regression, so the post-transition guarantee is pinned rather than argued. |
||
|
Michał Pierzchała
|
338aa2a0d5 |
refactor: route every native selector resolution through the policy interface (#1715)
* refactor: route every native selector resolution through the policy interface #1649 declared the per-caller ambiguity matrix; four native call sites still bypassed it, spreading `selectorResolutionKnobs(row)` into a raw `resolveSelectorChain` instead of naming the row. That left the "one interface" claim aspirational: a caller could restate its contract as engine knobs and nothing would notice. - `is` non-exists, `get text`/`get attrs`, find's read actions, and the covered-selector diagnosis probe now call `resolveSelectorChainWithPolicy` with their existing row. Semantics are byte-identical: the knob-backed branch of that interface forwards to the same engine call the call sites built by hand. - The façade drops `resolveSelectorChain` and `selectorResolutionKnobs`, so no knob-taking resolver is reachable from outside the package and a call site cannot re-acquire the knobs even by accident. `requireUnique`/`disambiguateAmbiguous` are now named in exactly one function, which `resolve-with-policy.ts` and the replay resolver both derive through. - `get` names the two rows it may consume as a type, so pointing it at any other ambiguity contract is a compile error. Tests: selector-read-policy.test.ts pins which row each read command consumes, end to end, on one ambiguous fixture — the only tree the rows disagree on. Each assertion was proven red by re-pointing its caller at a neighbouring row. The knob-consistency check moves into the package beside the now-private helper. Test call sites that used the raw resolver move to `resolveRecordedTarget`, the same knobs and the path that actually replays a recorded chain. Extracting the failure branch drops `resolveSelectorInteractionTarget` below the complexity threshold; its `fallow-ignore` waiver is removed (verified load-bearing before the extraction, unnecessary after). Closes #1630. Structural stages (occlusion, off-screen, promotion, poll budget) stay per-caller pipeline code, tracked in #1656. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD * test: observe which node find's row selected, not just that one existed #1715 review, P2: the find row assertion was only half a pin. `find exists` returns `found: true` for any resolved node, and the `list` call it leaned on goes through listFindMatches — a path that consumes no policy row at all. So repointing findFirstLocatorMatch at `readText` left both assertions green while selection silently moved from the document-order head to the tiebreak winner. Assert through `find get_attrs`, which returns the ref of the node the row actually selected. Both neighbouring rows are now red: `readText` fails '@e3' !== '@e2' (the move the old test missed), `readUnique` fails by refusing the ambiguous screen. `exists` stays as a second, weaker assertion on the same resolution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD * refactor: route is exists through the matrix, collapse the double match pass Follow-up tightening on the same seam. `is exists` reached findSelectorChainMatch directly while the `readAny` row's own doc claimed to serve "`exists` and find's read-only actions" — true of the docs, not of the code, which is the unverifiable-claim shape #1656's review called out. It now names `readAny`, the row it always described. Equivalent by construction: both take the first alternative with any match under requireRect: false, and disclose that alternative's count. That leaves the root façade with no consumer for findSelectorChainMatch, so it goes the way of resolveSelectorChain — dropped from the string-only façade, kept on the published ./ast surface. Its façade-twin type SelectorChainMatch dies with it (fallow caught it). resolveSelectorChainWithPolicy matched twice on the uniqueness path: once via resolveSelectorChain, then again to fill matchedNodes. Hoisting the single list call above the row switch removes that second pass, collapses two duplicated ambiguous literals into one helper, and drops a `?? [resolution.node]` fallback that was unreachable — a resolution implies its alternative matched, so the list is never null there. While hoisting: the resolved arm's matchedNodes can describe a different alternative than resolution.selector, because uniqueness skips an ambiguous alternative to try the next one. Unreachable today (only first-match callers read it, where both come from one list), and left as-is rather than silently changed — but the doc claimed "the alternative it came from", so it now says what is actually true. Tests: is exists gets a caller-level pin on the shared ambiguous fixture — passes with matches: 2 where its fail-closed siblings refuse — proven red by pointing it at readUnique. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD * test: discriminate is exists's row by alternative, guard the façade structurally #1715 review, second regression-validity gap. The `is exists` pin observed only `pass: true` and `matches: 2` on a fixture whose first alternative was merely TIEBREAKABLE — so disambiguation succeeded there and reported the same count first-match would. `readAny`, `readText`, and the pre-migration raw lookup all produced that, and only the readUnique swap I had checked went red. One mutation proven is not the same as the row being pinned. `exists` exposes no node ref, so the row has to be read off WHICH alternative answered. New fixture: alternative one matches two nodes that are genuinely indistinguishable (same depth, same area, both on screen) so the tiebreak declines; alternative two matches exactly one. First-match answers from alternative one; every uniqueness row skips the undecidable alternative and answers from alternative two. Asserting the selector now separates them — readText and readUnique both fail with `id="save-unique"` where `label="Save"` is expected. Restoring the raw lookup stays behaviourally invisible, though: findSelectorChainMatch is equivalent to the readAny row it migrated to, which is precisely why that migration preserved semantics. No fixture assertion can catch that revert, so the guard is structural — the façade's export list must not carry resolveSelectorChain, findSelectorChainMatch, or selectorResolutionKnobs. Follows the packages/maestro index.test.ts absence-assertion precedent. Verified red by re-exporting the lookup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuKzQWn6WQcMYaAZVvJzdD * fix: cover selector routes in device replays * test: simplify selector replay regression --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
6c0fcb64a1 |
fix: reject distinct ambiguous mutation targets (#1667)
* fix: reject distinct ambiguous mutation targets * fix(ios): scope the raw-match rejection to mutating dispatches `RunnerTests+Interaction.findElement` applied the new fail-closed classification to `querySelector` as well as press/type, because the read call site takes the default `allowNonHittableFallback: false`. With one visible/hittable match and one non-hittable same-selector duplicate the query started returning AMBIGUOUS_MATCH where it previously selected the hittable element, and `queryDirectIosSelectorOrFallback` preserves that error for read callers — so `get`, `is`, and `wait` surfaced an error instead of their prior answer. `classifyDirectSelectorCandidates` now takes a `rawMatchPolicy`. Mutations keep `.rejectDistinctMatches` (the default, so no mutation call site changes); `queryElement` passes `.preferHittableMatch`, restoring the prior read rule: prefer the single hittable match, ambiguous only when hittable matches compete, and never adopt the Maestro coordinate fallback. The Maestro expected-point path is untouched. Covers the one-hittable + one-non-hittable read, competing hittable reads, and the non-hittable-only read. ADR 0011's amendment now states the scope. * test(ios): execute selector read ambiguity regression --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
10ff339d14 |
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> |
||
|
Michał Pierzchała
|
870d12c406 |
fix: honest find contract — press/tap aliases, read-only list action, selector uniqueness (#1637)
* fix: honest find contract — press/tap aliases, read-only list, selector uniqueness (#1625) Three defects in find's contract, fixed together because they are one vocabulary: press/tap are the same action as click everywhere else in this CLI, yet find rejected them — agents using the vocabulary the tool itself established burned a tool call per attempt (four in one bench run). Both parsers now normalize press/tap to click; longpress/swipe stay real exclusions. The #1602 recovery hint told agents to run bare find to 'list matches', but bare find CLICKS a unique match — inspection guidance pointing at a mutation (the #1625 report: 'find Dictionary' navigated into Dictionary). find <q> list is the read-only surface that guidance needed: every match with its @ref, unique match included, never a tap. Captured UNSCOPED (the label-scope optimization narrows to the first match, exactly wrong for listing), published as an ADR 0014 partial frame authorizing every listed ref. Selector-shaped queries skipped the ambiguity check and took the first match silently — the mis-binding path the AMBIGUOUS_MATCH recovery advice itself pointed agents at, while --first/--last were documented as explicit opt-ins. Selector and text queries now share one contract: multiple matches reject with the #1597 candidates listing unless --first/--last narrows explicitly. The hint is rewritten around the new contract; docs and the MCP find output schema follow. Regressions at every layer: both parsers (alias, list token, unsupported-action hint shape), the daemon handler (selector ambiguity with candidates, --first opt-out, list returns all matches with zero action dispatches, unique-match list does not tap). * refactor: single-home the find read result and flatten parseFindArgs (fallow) The daemon's DaemonFindResult had drifted into an identical structural twin of the engine's FindReadCommandResult — the two grew the list variant in parallel and crossed the clone threshold. The shape now lives in contracts as FindReadResult (below both zones, per R2's own remedy) with the engine and daemon both aliasing it. parseFindArgs collapses the four bare single-token actions into one membership check and extracts the get sub-action parser, bringing it back under the complexity threshold instead of waiving it. * style: merge duplicate contracts import (lint) * fix: accept list on the MCP input surface and pin every listed ref (review) - FIND_ACTION_VALUES gains 'list' so field-metadata/MCP input no longer rejects the action the CLI parser accepts - FindCommandResponseData types 'matches' (public client response) - MCP mergeFindRefPins learns every matches[] ref, so a plain @eN press after find-list forwards pinned and the partial frame admits it - CLI/MCP text renders every listed match as its own pinned line via the snapshot-line role/label normalizers - regressions: daemon partial-frame scope, pin store, CLI output, MCP schema + find-list->press chain, typed client list response |
||
|
Michał Pierzchała
|
8ba5f9b8de |
fix: surface AMBIGUOUS_MATCH candidates and name find's supported actions (#1602)
* fix: surface AMBIGUOUS_MATCH candidates and name find's supported actions (#1597) AMBIGUOUS_MATCH errors now list the matching candidates (ref, role, label/identifier) rendered the same way as snapshot -i lines, capped at 5 with a "+N more" marker. buildAmbiguousMatchError (the single producer, src/daemon/handlers/find.ts) reuses formatSnapshotLine to build the list; formatAmbiguousMatchCandidateLines (src/utils/output.ts) renders it unconditionally on both text surfaces an agent actually reads (CLI printHumanError and MCP formatToolErrorText) — previously the candidates lived only in details, which neither surface printed. find's "Unsupported find action: X" (e.g. from `find <text> press`) now attaches a hint naming every action find actually supports and the two-step recovery shape: run find "<text>" to resolve the ref, then dispatch the gesture as its own command (press @eNN). The hint is a single exported constant (UNSUPPORTED_FIND_ACTION_HINT) shared by both throw sites — packages/selectors' raw-token parser and the CLI's typed reader (src/commands/interaction/selectors.ts) — so they can't drift. Matching semantics are unchanged; ambiguous rejection stays by-design. The help-conformance corpus's AMBIGUOUS_MATCH quiz is updated: its premise ("candidate refs were not shown") no longer holds, but with 3 identically-labeled candidates the lesson (don't guess a specific ref) still holds. * fix: guard the AMBIGUOUS_MATCH candidate renderer against device-domain shapes Review on #1602 (P2): formatAmbiguousMatchCandidateLines ran for every normalized error and stringified details.candidates unconditionally, but device-domain AMBIGUOUS_MATCH/APP_NOT_INSTALLED errors (findBootedAppleSimulatorWithApp, src/core/dispatch-resolve.ts) reuse that key for { id, name } device objects with no `matches` field — CLI and MCP would have printed "Candidates: [object Object]" for those. The renderer now requires numeric details.matches AND every candidate to be a string before rendering anything, restricting it to buildAmbiguousMatchError's element-match shape; unrecognized shapes render nothing, same as before this feature existed. Added regression tests against the exact device-error shape on both text surfaces. Also unexports AMBIGUOUS_MATCH_CANDIDATE_LIMIT (fallow flagged it as an unused production export) — it has no consumer outside find.ts. |
||
|
Michał Pierzchała
|
4f8dc3f31e |
refactor: move selector engine into workspace package (#1589)
* refactor: move selector engine into workspace package
* refactor(selectors): trim the package façade to its real consumers
Follow-up to the selector-package cutover, from a structural review of it.
- Drop 15 façade symbols with no consumer anywhere in the repo:
selectorUsesKey (added by the cutover, never called), isNodeVisible /
isNodeEditable (the real helpers are contracts/snapshot's), normalizeText,
splitIsSelectorArgs, IS_PREDICATE_REQUIRED_MESSAGE, four nested Replay
types, SelectorDisambiguationDisclosure, and the four kernel type
re-exports every consumer already imports from kernel directly.
- Delete SelectorCapturePolicyInput.selectorExpression, which
deriveSelectorCapturePolicy never read; the policy varies only by
predicate, so it takes one now. Two of the four tests asserted that the
unread parameter had no effect and could not fail; they go with it.
- Return the Maestro export vocabulary to the maestro package. The cutover
inlined MAESTRO_TEXT/STATE_SELECTOR_KEYS' values into the CLI call site,
leaving both constants dead in the package that owns the concept and no
gate over the two copies. MAESTRO_SELECTOR_PROJECTION is now the one
statement of it.
- Dedupe SelectorDiagnostics and SelectorDisambiguationDisclosure, declared
character-for-character twice across the AST/string seam, and name the two
shared option shapes once instead of five inline copies. The parser-side
resolution types take an Ast prefix so the twins read as twins.
- Delete three identity wrappers: parsePrivateSelector,
selectorExpressionToMaestro, and the formatSelectorFailure forwarder —
nothing passes it a chain any more, so the SelectorChain | string union
and its branch go too.
- Delete internal/index.ts, an AST barrel whose only consumer was one test
in the same directory (renamed to engine.test.ts), and the match.ts
pass-through that existed to feed it.
- ReplaySelectorGrammar had three variants for two behaviors; 'wait' and
'ordinary' were the same path. It is 'is' | 'positional' now.
- Drop the deleted src/sdk/selectors.ts from .fallowrc.json's entry list.
Behavior unchanged. pnpm check green: 598 unit files / 5278 tests, smoke
35 passed / 3 live skipped, layering 71/71, depgraph 22/22, mutation config
45/45, fallow clean, package smoke sound. Counterfactual: pointing
MAESTRO_SELECTOR_PROJECTION.textKeys at the state keys turns three
replay-maestro-export cells red; restored before commit.
* test(selectors): split the engine aggregation test by source concept
`internal/index.test.ts` (renamed `engine.test.ts` when its barrel went away)
was a 708-line aggregation over the whole engine — past the 500-line tripwire
and mirroring no source module, so it also ran as one serial unit.
It becomes five files that each mirror what they test, plus the parser cells
folded into the existing parse test:
resolve.test.ts alternative fallback, strict uniqueness,
first-match existence
resolve-disambiguation.test.ts ADR 0012 ranking: deepest, smallest-area,
winner-vs-challenger disclosure, tie fallback
resolve-viewport.test.ts the visibility half: on-screen beats
off-screen, including inside an off-screen
scroll container
match.test.ts per-key matching semantics (text, role,
focused, appname/windowtitle, decoded
newline labels)
arguments.test.ts where the selector ends and the command's
positionals begin, both grammars
parse.test.ts +6 grammar/escape cells beside the existing
property tests
The login-form tree shared by resolve.test.ts and match.test.ts moves to
`__tests__/login-form-nodes.ts` rather than being copied into both.
All 27 cells are carried over unchanged and still pass; no file now exceeds
224 lines. pnpm check green: 602 unit files / 5278 tests, layering 71/71,
depgraph 22/22, mutation config 45/45, fallow clean over 127 changed files.
* revert(selectors): keep agent-device/selectors public, behind one AST subpath
The cutover removed the `agent-device/selectors` public subpath as part of
tightening the API. It is in use, so the removal is reverted: the subpath ships
the same ten symbols v0.20.5 shipped, with the same signatures.
That has to coexist with the reason the package façade is string-only, so the
AST leaves through one named door instead of the main one:
@agent-device/selectors string-in/string-out; every in-repo consumer
@agent-device/selectors/ast the published parser surface; one consumer,
src/sdk/selectors.ts
`packages/selectors/src/ast.ts` re-exports parseSelectorChain,
tryParseSelectorChain, isSelectorToken, the AST-taking findSelectorChainMatch
and resolveSelectorChain, isNodeVisible, isNodeEditable, and types
SelectorChain / SelectorDiagnostics. `formatSelectorFailure` keeps its
published `SelectorChain | string` first parameter as a shim here rather than
widening internal/resolve.ts back to a union — the compatibility obligation
sits at the boundary that owes it.
This is strictly narrower than main, where the AST was reachable from anywhere
in src/ via src/selectors/*. Two gates hold it there: facade-symbols.ts pins
./ast to exactly the v0.20.5 list, and package-boundaries.test.ts asserts
src/sdk/selectors.ts is the only file outside the package that imports it.
Restored alongside: the ./selectors export and tsdown entry/chunk group, the
.fallowrc.json entry, the package-exports supported-subpath list, and both
client-api.md sections. No CHANGELOG entry — nothing is removed any more.
pnpm check green: 602 unit files / 5278 tests, smoke 35 passed / 3 live
skipped, layering 71/71 (10 packages, 32 subpaths), depgraph 22/22, mutation
config 45/45, fallow clean over 129 changed files, package smoke imported all
12 published entry points with publint and attw passing. Verified functionally
against the built dist: the doc's parse -> findSelectorChainMatch example
returns the same shapes as before, resolveSelectorChain still returns an AST
`selector`, and formatSelectorFailure still accepts a chain.
* fix(selectors): correct the two expectations that still assume the removal
Review P1s on a792415a: restoring the public subpath left two gates asserting
it was gone.
- installed-package-metro.test.ts moved `agent-device/selectors` into the
blocked-specifier list. It goes back to the subpath smoke set, running the
same `isSelectorToken('||')` + `parseSelectorChain` check it ran before the
removal, so the file's only remaining delta from main is a formatter reflow.
- owner-files-no-leak.test.ts asserted `dist/src/sdk-selectors.js` was absent.
It requires the stable named chunk again, and still rejects an auto-numbered
`selectors2.js` fallback — the pair is what proves the restored tsdown chunk
group is doing its job, verified against a clean build.
PR body corrected: the removal is no longer described as intentional API
tightening.
* refactor(selectors): satisfy the widened fallow scope after rebase
main's #1591 (the follow-up filed from this review) removed `packages/**` from
.fallowrc.json's ignorePatterns, so the new package is audited for the first
time. Everything below is a finding fallow could not previously see.
Dead surface, all confirmed consumer-free:
- 12 type re-exports from the `.` façade whose shapes consumers only ever
reach structurally.
- MAESTRO_TEXT_SELECTOR_KEYS / MAESTRO_STATE_SELECTOR_KEYS, orphaned by this
branch's own MAESTRO_SELECTOR_PROJECTION change, and the test-util
SELECTOR_VALUE_HAZARDS. All three are module-local now.
- IS_PREDICATE_USAGE_HINT fails --production because its only consumer is the
is-argument-surface parity test. It gets a commented `ignoreExports` entry
rather than deletion: the constant is what makes the daemon and CLI raise
ONE hint instead of two copied strings (ADR 0010), so the test asserting
that is the point, not an accident.
`fast-check` is now declared by the package that imports it.
Duplication, split by what could be proven:
- `isUsefulVisibilityAnchor` existed character-for-character in both
packages/selectors and packages/maestro. Moved to
@agent-device/contracts/snapshot, which both already depend on and which
already owns this vocabulary. Safe because the `normalizeType` each copy
called is itself character-identical to the contracts one — checked before
moving, since a different normalizer would have silently changed which
nodes anchor.
- maestro additionally reimplemented `normalizeType`, `buildSnapshotNodeMap`
(as `buildSnapshotNodeByIndex`) and `findSnapshotAncestor`, all
character-identical to contracts'. Deleted in favour of the shared ones.
- The three scroll-ancestor walks are NOT deduped. They are structurally the
same walk but each uses a different scrollable predicate, and I have no
evidence the three agree; collapsing them would be a Maestro-conformance
change, not a cleanup. Both maestro sites now say so, and the work is filed
separately.
`projectSelectorExpression` (15 cyclomatic / 22 cognitive, written by the
cutover) splits into a dispatcher plus `readAgreedTextValue` and
`projectSelectorTerms`; all three are under threshold.
Rebase note: the one conflict, in package-boundaries.test.ts, resolved to
NEITHER side — #1591 had already deleted `AdReplayVerifiedTargetGuard` as an
unused export, and this branch deletes the seven ReplaySelectorPort names, so
the conflicting block is empty.
* build: record fast-check for packages/selectors in the lockfile
Declaring the dependency in packages/selectors/package.json without
regenerating pnpm-lock.yaml made every CI job fail in its install step with
ERR_PNPM_OUTDATED_LOCKFILE. My local `pnpm install --frozen-lockfile` printed
"+ 1 dependencies were added: fast-check@^4.9.0" and exited 0, which read as
success but was the same mismatch CI refuses.
Regenerated with the pinned pnpm 11.17.0, not the 11.5.3 on this machine:
11.5.3 rewrites peer-dependency resolution keys repo-wide (dropping
`(supports-color@7.2.0)` suffixes) and produced a 222-line diff. With the
pinned version the diff is the 4 lines this change actually needs, plus
pnpm's alphabetical re-sort of the root selectors entry.
|