mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
Michał Pierzchała
4f8dc3f31e
* 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.
18 KiB
18 KiB
AGENTS.md
agent-device is a CLI + daemon that automates Apple-platform (iOS/tvOS/macOS), Android, and web
targets for coding agents. A long-lived daemon owns device sessions; commands route through a
registry-derived command surface to per-platform backends.
This file carries the traps and invariants you cannot infer by reading the code. Everything situational lives one hop away — load it when the task calls for it.
| When the task involves | Read |
|---|---|
| Domain vocabulary, architecture language, capture-reliability contract | CONTEXT.md |
| Accepted architecture decisions | docs/adr/README.md (a "read when you touch…" index) |
| Which gates to run, test speed rules, shared fixtures | docs/agents/testing.md |
| Adding or changing a CLI flag | docs/agents/cli-flags.md |
| Opening a PR, or reviewing one | docs/agents/pull-requests.md |
| Running commands against a real device | docs/agents/device-verification.md |
| Issues, PRDs, triage labels | docs/agents/issue-tracker.md, docs/agents/triage-labels.md |
| Web automation backend setup/diagnostics | docs/agents/web-backend.md |
| Planning device automation commands | agent-device help workflow, then topic help (debugging, react-native, react-devtools, physical-device, macos, dogfood) |
Versioned CLI help is the agent-facing source of truth for command behavior — prefer it over any prose in this repo, including this file.
Principles (expensive lessons — each cost an incident)
- Guarantees erode at path boundaries. Any new dispatch path or fast path classifies its cells in
packages/contracts/src/interaction-guarantees.tsfirst; the typechecker forces completeness, you supply honesty. ADR 0011. - A registry claim is not a semantic check: never mark a cell
runnerwithout reading whether the Swift code implements the guarantee's definition, not just a similar-sounding behavior. - Delegation-on-error is not success-path parity. A fast path that falls back on failure can still succeed on a candidate the shared rules would refuse.
- Do not measure before confirming the code path can fire. An A/B whose B-arm cannot execute returns two green runs masquerading as evidence.
- A green check is evidence only once you have seen it red. Prove a new regression test against the pre-fix code (revert, run, quote the failing number), a moved test against its gates (planted type error, discovered-count delta), a structural gate against a planted violation. Three vacuous regression tests shipped in one day before this rule; review caught all three.
- Typed signals over message sniffing: key on structured details (
details.timeoutMs, reason codes), never on error text. Remaining sniffs are owned debt with in-code rationale — do not copy them. - Snapshot output is the token budget. Never add per-node bytes to the tree; response-level metadata rides once per response.
- Warnings compose, never clobber. Append through the shared response builder; two clobber bugs shipped before this rule.
- Unreleased API surface dies free. Before treating a field as wire-compat, check
git tag --contains <commit>; if it never shipped, delete it now. - Push only behind an
&&-chained affected gate:pnpm check:affected --run && git push. A push that can run after a failed gate eventually will; GitHub remains authoritative for reported device/toolchain lanes.
Derived registries — read the declaration site, not prose
Command identity, routing, capability, and request-policy traits are derived artifacts. Inspect the declaration site rather than any map someone wrote down:
- one
CommandDescriptorper command:src/core/command-descriptor/registry.ts(catalog, capabilities, MCP/CLI projection, batch policy, timeout policy — ADR 0008) - daemon route ownership + request-policy traits:
src/daemon/daemon-command-registry.ts(parity-tested) - interaction dispatch paths × guarantees:
packages/contracts/src/interaction-guarantees.ts(ADR 0011) - command names:
src/command-catalog.ts— never re-create command string sets in handlers - capabilities:
src/core/capabilities.tsis the only home for command/device support checks
src/daemon.ts stays a thin router and src/daemon/request-router.ts orchestration-only; command
logic belongs in handlers. New daemon handler-family commands update the daemon command registry.
Shared selector parsing/matching/resolution lives in @agent-device/selectors; request cancellation/progress
primitives in src/request; cross-layer platform and command data contracts in src/contracts. CLI
grammar owns flag declarations under src/commands/cli-grammar; cross-surface CLI schema composition
lives in src/cli-schema.
Enforcement gates (a failing gate located your incomplete change)
Invariants here are self-declaring gates. The correct response to a failure is to classify or cover the new thing — never to suppress or allowlist it.
- public CLI flags must be classified:
scripts/integration-progress-model.ts - guarantee matrix completeness + honesty:
src/__tests__/contracts/interaction-guarantees.test.ts(gap waivers need atrackingIssue; the pin list changes only in reviewed diffs) - every enforced/delegated matrix cell needs a contract scenario:
src/__tests__/contracts/interaction-contract-coverage.test.ts+test/integration/interaction-contract/ - interaction responses build only through
buildInteractionResponseData(construction-guard test) - every command declares a timeout policy on its descriptor (timeout-policy completeness test)
- TS/Swift rule parity: golden tables under
contracts/fixtures/, consumed by vitest and the gated XCTest — change the rule only via the table - cross-command apple-leak guard; folder DAG/import lint (zero value-import cycles, zero target-spine back-edges); fallow (dead code, duplication, complexity)
Hard Rules
- Process execution goes through
src/utils/exec.ts(runCmd,runCmdStreaming,runCmdSync,runCmdBackground,runCmdDetached). Do not import rawspawn/spawnSyncelsewhere — extend an exec helper instead. Plain.mjspackaging fixtures that cannot import TS helpers keep child-process usage local and preferexecFile/execFileSync. - Interactions use the daemon session flow:
openbefore,closeafter. keyboard dismissis the iOS keyboard dismissal path. It may tap safe native controls such asDone, but must not fall back to system back navigation.- Do not remove shared snapshot/session model behavior without full migration.
- Apple-family target changes keep
packages/kernel/src/device.ts,src/core/capabilities.ts,src/core/dispatch-resolve.ts,src/platforms/apple/core/devices.ts, andsrc/platforms/apple/core/runner/runner-xctestrun.tsin sync. - iOS simulator-set scoping is iOS-specific:
iosSimulatorDeviceSetmust not hide the host macOS desktop target when--platform macosor--target desktopis requested. - Use
inferFillText(src/daemon/action-utils.ts),uniqueStrings(@agent-device/kernel/collections), andevaluateIsPredicate(@agent-device/selectors) rather than reimplementing them. - Do not update
skills/**/SKILL.mdfor command behavior or workflow guidance unless the user asks. Skills are thin routers to versioned CLI help; they must not carry behavior details.
Scope & shape
- Keep changes to one command family or module group unless the task explicitly crosses boundaries. If scope expands, stop and confirm. Preserve daemon session semantics and platform behavior.
- Do not inspect both iOS and Android paths unless the task is explicitly cross-platform.
- Prefer composition at platform boundaries: public aliases normalize into shared primitives, and providers contribute transport/device bindings instead of cloning interaction runtimes.
- Use
unknownonly at trust boundaries — parsed JSON, daemon/runtime payloads, catch values, generic I/O, parser callbacks. Once validated, narrow to a domain type instead of carryingunknownthrough internal helper and formatter signatures. - When asked to make a change, do not unilaterally add a fallback. Complete the migration and remove superseded code and documentation rather than preserving an old path “just in case.” Get explicit approval before adding compatibility or fallback behavior.
- Before finalizing, do one tightening pass over touched and adjacent areas: drop obsolete code, redundant tests, stale helpers/fixtures, and duplication the change made unnecessary.
- Name durable module concepts with
CONTEXT.mdvocabulary. Do not coin parallel names across docs, tests, and code.
Module size is about agent context safety, and the unit is questions, not lines: a file should answer
one question so rg → read-whole-file stays one cheap bounded read.
- tripwires: target ≤300 LOC per implementation file; past 500, extract before adding behavior; past 1,000 is architecture debt unless it is generated data or a fixture snapshot. Tests are not exempt.
- name files by the domain concept they answer (
runner-cache.ts,interaction-touch-response.ts), not by layer leftovers (utils2.ts,common.tsaccretion). - colocate machine-readable claims with the code they describe — coverage manifests beside contract tests, registry cells beside enforcement pointers, decision comments at the decision site. Agents navigate by claims, not directory listings.
- test files mirror source topology 1:1; when a source module splits, split its test file in the same
PR. A 3,000-line family aggregation makes every fixture lookup a whole-file read.
interaction.test.tsand platformindex.test.tspredate this rule and shrink opportunistically — do not add to them. - shared fixtures are named exports in a sibling fixtures module (see
test/integration/interaction-contract/fixtures.ts), never inline literals repeated per test. - long guidance/data tables live behind focused modules, not beside parser/runtime logic.
- barrels only at package boundaries. Legacy internal barrels are gated for removal (
CONTEXT.md). - extract when it improves locality for a concept callers already need, not to hit a line count.
src/daemon/handlers/session.tsandsrc/platforms/apple/core/apps.tsare already over budget. Extract the Apple-family/macOS-specific helpers before adding behavior to either.
Toolchain gotchas
pnpmonly. Do not add or restorepackage-lock.json. ESLint/Prettier are gone — the lint/format stack is OXC (.oxlintrc.json,.oxfmtrc.json). Read.oxlintrc.jsonbefore treating lint output as a source-level bug.- Daemon state: packaged installs use
~/.agent-device; source checkouts use worktree-scoped dirs under~/.agent-device/dev/<basename-slug>-<hash>. Inspect withpnpm daemon:state-dir, override with--state-dir/AGENT_DEVICE_STATE_DIR, prune withpnpm clean:daemon --prune-dev. Daemons are isolated per worktree; devices are not — target different devices for concurrent worktrees. - Node ≥22. Prefer built-ins (
fetch, Web Streams,AbortSignal.timeout) over compatibility wrappers unless the surrounding code needs a lower-level transport. - Emit with
tsdown(Rolldown), typecheck with TypeScript 7 viatsc. Declaration generation uses the TS7 native executable and is stricter than a plain typecheck: if it fails, inspecttsconfig.lib.json(it needs an explicitrootDir: "./src") andtsdown.config.tsfirst, and runpnpm check:toolingfor any build-tooling edit. - Prefer the aggregate
package.jsonscripts; they encode the expected validation bundles better than ad hoc command lists.pnpm formatformats the whole repo (oxfmtwith no path list), and the only exclusion list is.oxfmtrc.jsonignorePatterns— Markdown, the Maestro conformance corpus, and generated baselines. Runpnpm format, neveroxfmt <path>: a path argument reformats a subset and hides whatever else drifted. - Before pushing, default to
pnpm check:affected --run. It selects the relevant local gates and reports native/device checks left to GitHub. Usepnpm check(check:tooling && check:fallow && check:unit) for broad refactors or an explicitly requested full deterministic core gate. Neither command claims to reproduce every GitHub job. Fallow's baselines are keyed by path, so a change that RENAMES a file must move that file's entry infallow-baselines/health.json— regenerating the baselines would silently accept every other outstanding finding too.
Apple runner seams
The OS-agnostic XCTest runner lives under src/platforms/apple/core/runner/. Keep dependency
direction clean: transport below client/session behavior, shared command/error contracts in the
runner contract module, xctestrun preparation/build/cache isolated from request execution. For
connect errors, retry policy, or command typing, start in
src/platforms/apple/core/runner/runner-contract.ts before touching client/transport files.
Diagnostics, errors, logs
- Diagnostics source of truth:
src/utils/diagnostics.ts(withDiagnosticsScope,updateDiagnosticsScope,emitDiagnostic,withDiagnosticTimer,flushDiagnosticsToSessionFile). No ad-hoc stderr/file logging where these apply; redaction stays centralized here. - Request diagnostics belong in
sessions/<effective-session>/requests/<request-id>.ndjson. The top-level daemon log is for lifecycle/startup and pre-session failures. Session artifact paths come fromsrc/daemon/session-store.ts— do not hand-build them in handlers. - Logs backend:
src/daemon/app-log.ts.session.tsorchestrates only (start/stop/path/doctor/mark) and must not duplicate backend logic. App/device logs stay inapp.log; Apple runner andxcodebuildsubprocess output belongs in the session-scopedrunner.log. Preserve the external grep/tail workflow documented in help/skills. - Normalize user-facing failures via
normalizeError(@agent-device/kernel/errors). Payload contract:code,message,hint,diagnosticId,logPath,details. Preservehint,diagnosticId, andlogPathwhen wrapping or rethrowing. Errors say what failed, why when known, and how to recover — recovery steps go inhintwhen the action is not obvious. --debugis canonical;--verboseis a backward-compatible alias.- An interaction that unexpectedly takes 5+ seconds is a daemon-log question, not an app question:
check the session
daemon.logor the failurelogPathfor runner restart, stale session recovery, AX failure, transport retry, or command timeout evidence. - Optional optimizations (cache/preflight/probe) are best-effort unless the feature contract says otherwise: on failure, timeout, non-OK, or unusable shape, fall back to the required command path. Keep their timeouts shorter than the operation they precede.
Selector system
- Interaction commands (
click,fill,get,is) andwaitaccept selectors and@ref. - Pipeline: parse → resolve → act → record selectorChain → re-resolve as a divergence suggestion on
replay failure. Call
buildSelectorChainForNodeafter resolving target nodes. - New element-targeting interactions must support selector +
@refand recordselectorChainsocollectReplaySelectorCandidates(src/daemon/handlers/session-replay-heal.ts) can rank it in a divergence report (session-replay-divergence.ts). ADR 0012 retired--update's silent rewrite-on-heal; there is no automated write path to hook into. - New selector keys stay centralized in the private parser under
packages/selectors; newispredicates belong inevaluateIsPredicate. - On macOS, snapshot rects are absolute in window space. Point-based runner interactions translate
through the interaction root frame — do not assume app-origin
(0,0). Prefer selector or@refover raw x/y in tests and docs, especially on macOS where window position varies across runs.
Known environment traps (do not debug these as regressions)
- The first
nodeexec right after the dev-signed Apple runner launches can block ~19s at 0% CPU (Gatekeeper re-verification). It poisons back-to-back CLI wall-clock timing; absorb it with a throwawaynode -e 0, or measure in-process/daemon-side. - A leftover session holding the device fails every subsequent command instantly with
DEVICE_IN_USEnaming the owner. The hint'sclose --sessionguidance is the fix, not daemon debugging. - Contention flakes:
request-handler-catalog("specialized daemon routes...") and the doctor provider scenario time out under host load. Before believing a regression, rerun in isolation AND reproduce on plainorigin/mainunder the same load. A changing failure set that passes in isolation is contention, not your change. The files CI may rerun once for a timeout-shaped failure are enumerated inscripts/lib/contention-retry.ts(docs/agents/testing.md, "Contention retry policy").
Docs & skills
- Before adding guidance, examples, schemas, or command metadata anywhere, decide which layer owns
it: the command surface, CLI grammar, CLI help, MCP projection, or daemon runtime. Picking the
layer after writing is how the same contract ends up duplicated across two of them.
docs/agents/cli-flags.mdwalks the layers for the flag case. - Decide docs impact with the change, not after. For behavior/CLI-surface changes: update
help/metadata, README or
website/docs/**when user-facing, and a help-conformance bench case (scripts/help-conformance-*.mjs) when command-planning guidance changes. - State in the final summary whether docs/skills were updated, and why not if they weren't.
When guidance conflicts, Hard Rules win, then scope, then testing, then style.