Files
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
..

Dependency graph report

pnpm depgraph                       # -> .tmp/depgraph/graph.json + a text summary
pnpm depgraph --out /tmp/graph.json
pnpm depgraph:test

Emits the dependency graph of every production file under src/ (tests excluded) as JSON, plus a short summary of what the layering gate does not enforce. There is no renderer: the productive artifact is the JSON, queried directly.

Blast radius of one file

pnpm depgraph affected src/utils/exec.ts        # bounded text
pnpm depgraph affected src/daemon/ref-frame.ts --json --limit 25

Reverse reachability over the value-edge graph, plus the three lookups that used to follow it: the scripts/check-affected/ gate plan for the dependent set, the public commands whose handler chain reaches the file (value + dynamic edges, because handlers load through import()) with their owning live iOS scenarios when that manifest is in the tree, and the ADR 0011 guarantee-matrix cells the file implements. See docs/agents/testing.md § "Before editing a shared module".

When to reach for this

It pays for itself on three questions, and misleads on a fourth.

"What am I about to break?" nodes[].in is the dependent count — blast radius. Size the nodes by dependents and the files you should touch carefully are the big ones. Faster than grepping, and it counts type-only and dynamic edges that a grep for from '...' misses.

"Where is the debt actually concentrated?" Zone-level counts (zoneEdges) answer "which boundary carries the most traffic" in one query. The pass that produced ADR-adjacent findings started here.

"What is wrong that the gate does not enforce?" This is the part CI cannot give you. The gate rejects value-import cycles (R4) and spine back-edges (R5); the graph additionally reports:

  • value edges whose target is also reachable at distance >= 2 — static module reachability, and only that. It is not a removability claim, and the obvious reading is wrong: reachability does not carry bindings (if a imports { c } while b only re-exports it as { c as b }, the path exists and deleting a -> c still breaks a), it does not preserve when a module's side effects run, and a direct import is often deliberately clearer than reaching through a barrel. Deciding whether any given edge can go needs symbol-level analysis this does not attempt. ~1300 of them: a place to look, never a work list.
  • type-only and dynamic cycles — 8 of them, all outside R4 by design (a type-only import is free at runtime, a dynamic one is a deliberate cold-start seam). Worth reading when a module feels hard to reason about.

Where it misleads: a cluster's size is not its difficulty. This is worth stating plainly because it already cost a day. The commands -> client cluster looked like the obvious win — 28 type-only inversions, all pointing at one file. Moving that file down took the gate from 42 to 48, because the vocabulary it holds depends on commands/, metro/, core/ and remote/; declaring it in contracts/ made the foundation depend on the layers above it. The picture shows you an edge's weight, not whether it can be reversed.

So: use the render to find a candidate, then answer "can this move?" numerically before planning anything. The question is always what does the target itself import, and what rank is that?

pnpm depgraph
# Zone pairs that invert the ranked spine. Read `typeInversions` rather than deriving it from
# `zoneEdges`: those counts come from the COLLAPSED edge list, where one edge per file pair
# survives and `dynamic` outranks `type`, so a module imported both lazily and for its types
# would drop out. `typeInversions` is counted by the gate's own rule and is what CI compares
# against TYPE_INVERSION_BASELINE.
node -e "const j=require('./.tmp/depgraph/graph.json');
  Object.entries(j.typeInversions)
    .sort((a, b) => b[1] - a[1])
    .forEach(([pair, n]) => console.log(String(n).padStart(4), pair));"

Note zoneEdges[].backEdge flags R5 value back-edges only, and there are none — filtering on it returns an empty list, which is the gate passing, not a broken query.

What is authoritative

pnpm check:layering is. The viewer reads the same model, so the numbers should agree — and that agreement is now enforced rather than hoped for: the Layering Guard job runs scripts/depgraph/model.test.ts, whose last test asserts this report's inversion count reproduces TYPE_INVERSION_BASELINE. If the tree changes and only one side is updated, CI fails and names the difference. The two cannot be green independently.

What that check proves precisely: the report's graph build, over the real tree, agrees with the gate's baseline. It is a cross-check of the extraction and the baseline against reality, not two independent algorithms — typeInversionsByPair deliberately applies the gate's counting rule (once per file pair, over raw edges) so the numbers cannot diverge for a reason unrelated to layering. In particular it does NOT count from the collapsed edge list, where dynamic outranks type and a module imported both lazily and for its types would drop out.

If they ever disagree, the gate is right and the baseline or the tree is wrong.

Why it reuses the layering gate

The graph is extracted with scripts/layering/model.ts, the same module scripts/layering/check.ts uses in CI. File set, zone partition, edge kinds (value / type-only / dynamic), and cycle definition are therefore identical to the rules the gate enforces — a separate extractor with its own resolution behaviour would draw a graph nobody is enforcing. Cross-checked once against dependency-cruiser 3.1.1 (at the commit it was written): same modules and edges, plus 88 dynamic/type-only edges dependency-cruiser fails to resolve.

What the JSON carries

  • zones[] — id, spine rank (null when intentionally unranked), classification, file count, LOC.
  • zoneEdges[] — per zone pair: total count, valueCount, and backEdge (R5 value back-edges only — see the note above).
  • nodes[] — per file: zone index, LOC, in/out degree, lvl (longest path to a sink over value edges; R4 guarantees that subgraph is a DAG), and cyc (index into cycles, or -1).
  • edges[] — index-addressed [from, to, kind, flags]. Kind: 0 value, 1 type-only, 2 dynamic. Flags bitfield: 1 spine back-edge, 2 target also reachable at distance >= 2, 4 type-only inversion.
  • cycles[] — each with kind (value / type / dynamic) and its node path.

Bit 2 means the target is reachable from the source at distance >= 2 over value edges. That is module reachability, not removability — see the caveats above. Treat it as a question ("why is this imported directly as well?"), never as an instruction.