mirror of https://github.com/callstack/agent-device.git synced 2026-09-14 20:06:34 +08:00

Files

T

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.

2026-08-04 19:06:39 +02:00

4.0 KiB

Raw Permalink Blame History

ADR 0010: Error system conventions

Status: accepted
Date: 2026-07-03

Context

The error system is centralized in src/kernel/errors.ts (AppError, normalizeError, defaultHintForCode, retriableForErrorCode) but has ~1,200 construction sites across the CLI, daemon, and platform layers. A July 2026 audit (code sweep + iOS/Android dogfooding) found the kernel sound while call-site quality drifted: internal validation thrown as bare Error (surfacing as UNKNOWN with a misleading hint), semantically wrong codes (COMMAND_FAILED for busy devices, INVALID_ARGS for expired server-side resources), missing hints on the most common agent failures (selector/ref misses), and consumers that drop fields (MCP tool errors carried only message). Nothing documented the conventions, so each new site re-decided them.

Decision

Every thrown error a user or agent can reach is an AppError. Bare throw new Error(...) is reserved for provably unreachable invariants. Input validation — including daemon-side decoding of command input — throws AppError('INVALID_ARGS', ...). Coercion of unknown caught errors uses asAppError(err, fallbackCode), which preserves the cause chain; do not hand-roll err instanceof AppError ? err : new AppError(...).
Code selection. Use the most specific KnownAppErrorCode; COMMAND_FAILED is for genuine runtime failures of a well-formed request, never a catch-all for capability gaps (UNSUPPORTED_OPERATION), contention (DEVICE_IN_USE, the only retriable code), or ambiguity (AMBIGUOUS_MATCH). New codes are added to the union deliberately; machine-dispatchable sub-classification rides in details.reason (the lease registry is the model).
Hints answer "what should the agent run next". A hint is required wherever the per-code default from defaultHintForCode would mislead; it is omitted where the default suffices — mass-adding boilerplate hints is worse than the default. Shared failure modes get shared hint constants next to the code that detects them (selectorFailureHint, STALE_REF_HINT in packages/selectors/src/internal/resolve.ts; resolveIosDevicectlHint; bootFailureHint), not copy-pasted strings. Re-wraps preserve an existing hint rather than clobbering it.
Wrapping external tool failures. Prefer exec.ts errors as-is. A hand-rolled wrap of an allowFailure result must carry { stdout, stderr, exitCode, processExitError: true } so normalizeError can surface the first meaningful stderr line instead of "tool exited with code N".
The wire error contract is code, message, hint, details (redacted), diagnosticId, logPath, plus optional typed signals retriable / supportedOn — the fields are absent unless confidently known. Every consumer surface (CLI human, CLI --json, SDK, MCP, events timeline) must carry code + message + hint at minimum; rehydration helpers (throwDaemonError, toDaemonHttpRpcError) copy all fields, and NormalizedError hoists the typed signals so --json consumers see them.
Observability. Failed requests always flush diagnostics: diagnosticId + ndjson logPath accompany every error (daemon-side under the session state dir, client-side under ~/.agent-device/logs). Messages must not embed raw stderr dumps or secrets — structured context belongs in details, which is redacted at normalize/write time.

Consequences

Agents can branch on code, retry on retriable, and follow hint without parsing prose; wording can improve without breaking consumers (except strings pinned by help-text guarantees — update those tests in lockstep).
The known-code union stays small and meaningful; the widened (string & {}) type still lets runner/daemon codes pass through, so SDK consumers keep a default branch.
New call sites have a documented bar: right code, contextual message naming the target, hint only when the default misleads, cause preserved.

4.0 KiB Raw Permalink Blame History

ADR 0010: Error system conventions

Context

Decision

Consequences

4.0 KiB

Raw Permalink Blame History