mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
t3code/fix-2458-quality-refactor
68 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
527a56a6e7 |
refactor(move): move the batch runner and batch policy into @agent-device/command-registry (#2388)
* refactor(command-registry): move the batch runner and batch policy into @agent-device/command-registry * chore(gates): re-point the sdk-batch chunk groups and fallow baseline at the command-registry batch module |
||
|
Michał Pierzchała
|
51ed6217cc |
refactor(daemon): relocate the daemon client out of src/daemon (#2360)
* refactor(daemon): extract the repair-tombstone reader below store and client `findUnrecoveredRepairCommitFailure` reads session artifacts off disk and is reached from the daemon client, which had to import `session-store.ts` — the daemon's largest server module — for it. Move the tombstone shape, its file reader and the unrecovered-commit scan into `session-repair-tombstone.ts`, a leaf below both, and give the tombstone file name a single owner. No behavior change; both consumers keep their existing tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CZkJjeEhLmyGpGtcwY8pqc * refactor(daemon): relocate the daemon client out of src/daemon `src/daemon/client/` is the daemon's client, not the daemon: no daemon file imports it, and its consumers are the CLI, the Node client, the proxy command and the injected dispatch type. Move it to `src/daemon-client/` as renames so `src/daemon` is server code plus the shared kernel the client still needs — `config.ts`, `daemon-process.ts`, `request-progress-protocol.ts`, `daemon-request.ts` and the extracted `session-repair-tombstone.ts`. Zone name and rank are unchanged (`daemon-client`, 5); the zone now falls out of the folder instead of a `src/daemon/client/` prefix. Tests move unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CZkJjeEhLmyGpGtcwY8pqc * refactor(daemon): move the session artifact path helpers out of session-store `src/cli.ts` and `src/remote/remote-request-diagnostics.ts` reach into `session-store.ts` for one pure path function, `resolveRemoteRequestDiagnosticsPath`, which made every CLI process eagerly evaluate the daemon's session store and its whole subtree — the script writer, the event log, the action recorder and the replay transaction vocabulary. The four artifact path helpers name files; they hold no store state. Move them to `src/daemon/session-artifact-paths.ts`, a leaf over `session-paths.ts`, and point all ten consumers at it. `src/cli.ts`'s eager closure drops from 379 modules to 365 and no longer contains `session-store.ts`; the store itself is 464 -> 341 lines. AGENTS.md's declaration-site pointer follows. No behavior change: the helpers are unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CZkJjeEhLmyGpGtcwY8pqc * chore(gates): re-key the daemon-client gate paths onto src/daemon-client Path-keyed enforcement follows the relocated files: the fallow health baseline entries, the oxlint per-file override, the wire-compat surface/ledger/mutation paths, and the layering zone derivation (the `src/daemon/client/` prefix is dead now that the folder itself names the zone). R10's external daemon request/session-state importer list gains the five client modules. The edges are unchanged by this PR — the client has always built `DaemonRequest` and read `DaemonResponse`; it sat inside `src/daemon/` and so fell under the prefix skip. Naming the files keeps the dependency enumerated and shrink-only, so a new `src/daemon-client/` module reaching `session-state` still fails. Its size assertion now reads the recorded list instead of a literal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CZkJjeEhLmyGpGtcwY8pqc --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
1f0eedff89 |
refactor(daemon): move shared snapshot execution out of handlers (#2232)
* refactor(daemon): move shared snapshot execution out of handlers * fix: remove retired snapshot health baseline |
||
|
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
|
544a804965 | refactor(daemon): move session observability behind facade (#2216) | ||
|
Michał Pierzchała
|
b042045522 |
refactor(output): split presentation owners (#2202)
* refactor(output): split presentation owners * fix(output): keep candidate rendering in surface owners |
||
|
Michał Pierzchała
|
010f09bf0d | refactor(daemon): move open lifecycle behind session facade (#2201) | ||
|
Michał Pierzchała
|
28f80ddb40 | refactor(cli): move update-check policy (#2194) | ||
|
Michał Pierzchała
|
8591f47dd3 |
refactor: extract daemon session lifecycle inventory facade (#2183)
* refactor: extract session lifecycle inventory facade * test: cover session inventory failure response |
||
|
Michał Pierzchała
|
1d5ddb54a3 |
refactor(snapshot): move snapshot policy utilities (#2135) (#2181)
* refactor(snapshot): move snapshot policy utilities (#2135) * fix(snapshot): preserve CLI eager closure |
||
|
Michał Pierzchała
|
f513b1d4ae |
refactor: extract daemon replay behind one application facade (#2166)
* refactor: extract daemon replay behind application facade * fix: address replay facade review findings * test: close replay ownership import scan gap * fix: tighten replay capability boundaries |
||
|
Michał Pierzchała
|
caa3dc23f9 |
refactor: dissolve caller-side src/replay into command and CLI owners (#2151)
* refactor: dissolve caller-side replay ownership * fix: remove replay test-only export * fix: restore replay loader promise boundary |
||
|
Michał Pierzchała
|
f152827447 | refactor(snapshot): move text-surface into snapshot presentation (#2152) | ||
|
Michał Pierzchała
|
a6232e51cf | refactor: prune platform split residue (#2123) | ||
|
Michał Pierzchała
|
9abcd7fe03 |
refactor: move Apple platform family into package (#2118)
* refactor: move Apple platform family into package * fix: preserve Apple facade sync contracts * fix: complete Apple W4 rebase review fixes |
||
|
Michał Pierzchała
|
c7f42ccedc |
refactor: move Android family behind package exports (#2117)
* refactor: move Android family behind package exports * fix: address Android W5 review feedback * fix: update relocated routing fixture assertion |
||
|
Michał Pierzchała
|
838ed223b5 |
refactor: move W6 platform families behind package facades (#2116)
* refactor: move W6 platform families behind package facades * fix: address W6 loading and composition review |
||
|
Michał Pierzchała
|
72cae2bc72 |
refactor(apple): colocate the XCUITest runner client into packages/platform-apple (#2040) (#2050)
* refactor(apple): colocate the XCUITest runner client into packages/platform-apple (#2040) Moves src/platforms/apple/core/runner/ (34 modules + apple-runner-platform.ts and the 30 runner test suites) into packages/platform-apple/src/runner/ — Apple mechanics live in the Apple package. Host capabilities (exec, diagnostics, retry, process probes, locks, Apple tooling, physical-device control) enter through the package-owned AppleRunnerHost port; the root composition module src/platforms/apple/core/runner-client.ts constructs the client exactly once and re-exposes the bound operations under their historical names. R13 admits the transitional state deliberately: the family exports its root façade plus exactly the enumerated ./runner, ./runner/client, and ./runner/test-host subpaths; the ./runner façade subpath is the recorded #1983 seam for unmigrated root consumers; ./runner/client has one composition root and ./runner/test-host one vitest installer; the runner subtree may own its cache files and sockets while raw process primitives stay banned. When #1983 completes, the subpaths and every subtree exemption are deleted and the family returns to a single implementation-lazy façade export. * docs(adr): model the runner subtree as a durable platform-owned facet Review correction on #2050: the sunset story attributed the runner-consumer migration to #1983, which owns snapshot/presentation vocabulary — not the runner's daemon/root consumers — so that event cannot delete the ./runner subpaths or the subtree exemptions. Reword ADR-0019, R13, and the gate comments: the facet is the intended ownership model, its seam is enumerated and pinned (exact export list, one client composition root, one test-host installer, raw-process ban, eager-closure pins), and the seam narrows only if a real runner-consumer migration retires the direct consumers. The declaration mechanism stays apple-specific until another family needs a mechanics facet. No behavior change; identifiers and comments only. |
||
|
Michał Pierzchała
|
5b6feafe92 |
Extract snapshot policy from daemon to host-side facet (#1983) (#2014)
* refactor(snapshot): give the Wave 4 policies neutral host seams (#1983)
#2005 established the presentation ownership boundary and moved the iOS
presentation policies out of `src/daemon/`. It left the three remaining Wave 4
policies behind their existing daemon adapters. This closes that gap, so
`src/snapshot/` owns host-side snapshot policy generally rather than
presentation alone.
Freshness recovery: the window vocabulary, the Android staleness classification
and its thresholds, and the retry loop move to `src/snapshot/snapshot-freshness/`.
The loop is parameterized by a classifier and a retry schedule, so how long a
backend may lag behind a real transition is a policy input rather than a
constant the loop owns. `src/daemon/session-snapshot-freshness.ts` keeps only
what needs a session — reading and retiring the window on store-owned
`SessionState`, and choosing the comparison baseline from snapshot lineage — and
remains the declared R7 owner of `androidSnapshotFreshness`. The two call sites
#1739 named as the Wave 5 blockers, `selector-capture-runtime.ts` and
`deferred-interaction-outcome.ts`, now reach freshness through the seam.
Timeout evidence: whether a failure is the accessibility-timeout shape becomes a
policy in `src/snapshot/snapshot-timeout-policy.ts`. The published
`details.androidSnapshotTimeoutScreenshot` payload becomes vocabulary in
`@agent-device/contracts/snapshot-timeout-evidence`, built through constructors
so an assembly site cannot publish a fifth, undeclared arm. It gets its own
subpath rather than riding the shared capture facade, which keeps it out of the
CLI cold-start closure. Typed details, diagnostics and screenshot evidence are
unchanged.
Screenshot-overlay policy: which Android nodes earn an overlay ref, and what
rectangle an overlay covers, move to `src/snapshot/screenshot-overlay/`. The
daemon keeps approved artifact and ref assembly only — ranking, projection to
screenshot pixels, drawing and PNG IO.
The boundary test generalizes from the presentation subtree to the whole facet:
nothing under `src/snapshot/` may import `src/daemon/`. It gains a positive
control, because a filter that stopped matching would look identical to a
boundary being obeyed.
The residual call sites #1983 also named are audited and deliberately left in
place. `direct-ios-selector.ts` carries no presentation policy; its two pure
exports are selector derivation and ADR 0011 delegation-on-error, whose owner
would be the selector pipeline governed by R19, not this facet. ADR 0004 records
the finding so it does not have to be re-derived.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GLYhmt5ZNHQATG8T8ZFo7R
* refactor(snapshot): address adversarial review of the Wave 4 seams
Three findings from an adversarial pass over
|
||
|
Michał Pierzchała
|
50f460cce4 |
refactor(snapshot): establish presentation ownership boundary (#2005)
* refactor(snapshot): establish presentation ownership boundary * docs: keep context glossary within budget * fix(snapshot): address presentation boundary review * test(snapshot): ratchet eager closure budgets |
||
|
Emmanuel Chucks 🇬🇭
|
856ff3886f |
fix(ios): preserve final-probe xcodebuild diagnostics (#1776)
* fix(ios): preserve final-probe xcodebuild diagnostics * refactor(ios): split runner startup transport |
||
|
Michał Pierzchała
|
c7565cb1f8 |
refactor(snapshot): clean snapshot ownership (#1754)
* refactor(snapshot): clean snapshot ownership * fix(snapshot): address ownership review feedback |
||
|
Michał Pierzchała
|
52402aec4d |
refactor(daemon): split touch interaction orchestration into semantic modules (#1748)
* refactor(daemon): split touch interaction orchestration Closes part of #1691: interaction-touch.ts becomes a router; press, fill, direct-iOS, shared runtime, Android readiness, and response projection each own one module. Behavior is unchanged. * test(daemon): split touch interaction coverage by module Redistributes all 87 discovered cases across the new module topology and re-keys the two touch-family fallow baseline entries to the paths that now hold the same (net one fewer) findings. * docs: point ADR 0014 at the merged Android readiness regression file * refactor(daemon): give targeted-touch admission its own module Keeps interaction-touch-press.ts inside the 300-line budget after the complexity decomposition: admission (surface/capability/button policy, target parsing, @ref staleness and mutation admission) answers its own question. * refactor(daemon): drop the redundant targeted-touch label alias * test(daemon): split touch suites along the new production seams Adds the press-admission suite the production module was missing and splits the four over-budget suites along new production seams (direct-iOS eligibility, Android ref freshness, touch payload). Every suite installs the full device mock set: three Android-session cases regressed to TOOL_MISSING on a runner without adb when the mock set was trimmed per file. |
||
|
Michał Pierzchała
|
b15c502318 |
refactor: extract platform network runtime (#1702)
* refactor: extract platform network runtime * fix: preserve platform network recovery routes * test: guard network parser placement |
||
|
Michał Pierzchała
|
b1ed5353d1 |
refactor: extract platform log runtime (#1701)
* refactor: extract platform log runtime * fix: clear terminal app log recovery markers * fix: preserve scoped app log tooling * fix: preserve app log cancellation * fix: handle large changed coverage diffs * fix: harden Limrun runtime identity * refactor: tighten platform log runtime * fix: close app log trust gaps * fix: accept canonical session path aliases * refactor: extract durable capture kit * fix: refresh retained log marker admission * fix: rotate app logs after process relaunch |
||
|
Michał Pierzchała
|
c06bed9f77 |
refactor: extract platform device inventory runtime (#1699)
* refactor: extract platform inventory runtime * fix: preserve scoped Apple inventory tooling * fix: preserve Apple tool cancellation * refactor: tighten platform inventory boundaries |
||
|
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.
|
||
|
Michał Pierzchała
|
761317deb7 |
refactor(daemon): extract native .ad replay to packages/ad-replay (#1478 P5) (#1555)
* refactor(replay): move the dependency-free engine leaves into packages/ad-replay Stage A of the #1478 P5 extraction: vars, plan-digest (+canonical-json, sole consumer), the target-identity classification core, report-action, and suggestion-ranking move verbatim; imports updated. The package facade temporarily re-exports the moved symbols so root consumers keep compiling; a later stage narrows it to inspectAdReplay/runAdReplay only. * chore(layering): register packages/ad-replay in the workspace and DAG * refactor(replay): define the three-operation replay selector port with dual adapters (#1478 P5) * refactor(daemon): route replay handlers through the selector port (#1478 P5) * refactor(replay): split target verification into engine policy and daemon authority (#1478 P5) * refactor(replay): move the .ad step loop behind inspectAdReplay/runAdReplay (#1478 P5) * refactor(replay): lock the ad-replay façade to its real consumers (#1478 P5) * test(replay): prove shared-id demotion on both selector-port adapters (#1555 review) * fix(replay): restore invalid replayBackend rejection on the native path (#1555 review) * refactor(replay): move shared .ad vocabulary to its owner, packages/ad-script (#1555 review) * refactor(replay): neutral step/run outcomes and digest/resume behind inspectAdReplay (#1555 review) P1 "do not smuggle daemon wire failures through a generic": drop the TResponse generic from AdReplayStepRuntime/runAdReplay. executeStep and handleActionFailure now return neutral tagged AdReplayStepOutcome/ AdReplayStepFailure values (kind/message/artifactPaths only); runAdReplay returns a neutral completed/failed AdReplayRunOutcome. The engine never holds or returns a DaemonResponse. The daemon adapter (createAdReplayStepRuntime, session-replay-runtime.ts) keeps its real wire response in a local side-map as it builds each neutral outcome, and runReplayScriptFile reads it back once runAdReplay reports which step failed, so the final response is byte-identical to before this split. P1 "parsing/planning/digest/resume must also occur behind runAdReplay": relocate computeReplayPlanDigest's call site and the --from/--plan-digest resume-point math (resolveReplayEntryIndex) behind inspectAdReplay's manifest as planDigest and a resolveEntryIndex closure. Neither is a new top-level export -- inspectAdReplay/runAdReplay stay the only two. Timing is preserved exactly (still called eagerly in prepareReplayPlan, before prepareReplaySession's coordinator-mutating side effects) since moving resume validation to run inside runAdReplay itself would let a rejected --from request mutate coordinator/session state first -- a real ordering hazard, not just a cosmetic one. computeReplayPlanDigest/ReplayPlanDigestMetadata/resolveReplayEntryIndex leave the ad-replay façade; request-router-repair-expired.test.ts and prepareReplayPlan read the digest/resume result off the manifest instead. * refactor(replay): relocate classifyTargetBindingMatch and pin the ad-replay façade (#1555 review) P1 "complete the binding façade instead of documenting deviations": classifyTargetBindingMatch never had a real consumer reachable through inspectAdReplay/runAdReplay -- both its callers (the daemon's record-time self-check in session-target-evidence.ts and its replay-time classification wrapper in session-replay-target-classification.ts) are daemon files that imported it directly. It interprets TargetAnnotationV1 evidence semantics shared beyond the engine, so it moves to packages/ad-script alongside target-annotation-identity.ts (new target-annotation-classification.ts + its test), and both daemon call sites now import it from there instead of @agent-device/ad-replay. One deviation remains and is reported rather than papered over per the review's own instruction: the four target-verification policy functions (planPreDispatchTargetVerification, planPostResolutionTargetVerification, deriveReplayTargetGuardMismatchEvidence, deriveWaitLandmarkMismatchEvidence) and the ReplaySelectorPort type family stay exported. Their sole caller, session-replay-target-verification.ts, interleaves these pure decisions with daemon-only async work (capture, SessionStore, coordinator/resume stamping, wire shaping) that must stay outside the engine by design; moving their call sites to live only behind runAdReplay would require restructuring that whole orchestration into new fine-grained AdReplayStepRuntime capabilities, which is out of scope for this pass. See packages/ad-replay/src/index.ts's header comment for the full reasoning. P1 "add the reviewer-required exact exported-symbol gate": adds readNamedExports (scripts/layering/package-boundaries.ts), a small parser over a façade's `export { .. } from`, `export type { .. } from`, and direct-declaration forms, and pins @agent-device/ad-replay's exact 21-symbol export list in package-boundaries.test.ts. Plant-verified: a stray `export const` addition failed the assertion; removed it and the gate went green again. * refactor(replay): drive target verification from the engine step loop (#1555 review) Moves the verify-then-dispatch decision flow into packages/ad-replay's step loop so the four target-verification policy functions (plan{PostResolution,PreDispatch}TargetVerification, derive{ReplayTargetGuardMismatch,WaitLandmark}MismatchEvidence) become engine-private and leave the ad-replay façade. The daemon (session-replay-target-verification.ts) shrinks to the narrow AdReplayStepRuntime capabilities the engine drives: routing (beginTargetVerification), capture (captureObservation), classification (classifyTarget), dispatch (dispatchStep), and wire-building (buildRecordedUnverifiableFailure, buildTargetBindingFailure, buildPostDispatchTargetBindingFailure). Wire output and replay-compat stay byte-identical; the exact-symbol façade gate is updated to the shrunken export list. * refactor(daemon): decompose the replay adapter's two over-threshold functions (#1555) * refactor(replay): fold #1554's keep-session terminal-lifecycle policy into the ad-replay engine Rebasing p5/extract-ad-replay onto main pulled in #1554's --keep-session feature, which had grown its own daemon-side terminal-close-suppression predicate (session-replay-terminal-lifecycle.ts's resolveSuppressedTerminalCloseIndex/countExecutedReplayActions) independently of this branch's own engine-side one (step-loop.ts's isRepairArmedTerminalCloseAction). Both are the same decision family — replay --keep-session and an active --save-script repair now share ONE structural resolution (resolveSuppressedTerminalCloseIndex, generalized to "terminal among EXECUTABLE actions" rather than the old physical-last-index check) and one suppression check inside runAdReplay, gated on keepSession OR runtime.isRepairArmed(). AdReplayRunRequest grew a keepSession field; the neutral 'replayed' count in AdReplayRunOutcome is now computed inline in the loop instead of the daemon's old actions.length - entryIndex approximation. requireLiveSessionForKeepSession (the --keep-session live-session postcondition) stays daemon-side, inlined into session-replay-runtime.ts, since it inspects SessionStore state the engine never sees. The daemon-only session-replay-terminal-lifecycle.ts this arrived with is deleted entirely — its isExecutableReplayAction was a duplicate of the engine's own. runReplayScriptFile's Maestro-format routing (including the new --keep-session Maestro rejection) was extracted into routeMaestroReplay to keep the function under fallow's complexity threshold after re-threading keepSession through it. Added packages/ad-replay/src/internal/__tests__/step-loop.test.ts covering the unified suppression decision (both keepSession and repair-armed) directly against runAdReplay, including the terminal-among-executable-actions case with a trailing nested replay marker. The daemon-level integration tests (6 tests in session-replay-terminal-lifecycle.test.ts, exercising the same behavior through runReplayScriptFile) and the SDK provider-scenario test (active-session-script-publication.test.ts) needed no changes and pass unmodified. * refactor(daemon): decompose session-replay-runtime.ts into three modules (#1555) Splits the ~1096-line replay runtime into cohesive pieces, keeping session-replay-runtime.ts as thin orchestration (~240 LOC): - session-replay-runtime-engine-adapter.ts: the AdReplayStepRuntime adapter (createAdReplayStepRuntime, the build*Failure capability implementations, and the lastResponse/lastObservation side-map mechanics), extracted verbatim. - session-replay-runtime-plan.ts: extended with the plan-side helpers (validateReplayBackendFlag, inspectReplayPlanManifest, resolveReplayPlanEntryIndex, prepareReplayPlan, routeMaestroReplay) alongside the buildReplayMetadataFlags helper already there — buildReplayMetadataFlags is now module-private since its one caller moved into the same file. Also introduces ReplayScriptFileParams, named here (instead of derived via Parameters<typeof runReplayScriptFile>) so routeMaestroReplay can reference the shape without importing back from session-replay-runtime.ts. - session-replay-runtime-session.ts (new): session preparation (prepareReplaySession and its coordinator arming/repair-preflight helpers), extracted verbatim. Coordinator ownership is unchanged: createReplayCoordinator is still constructed only in session-replay-runtime.ts, matching replay-coordinator-ownership.test.ts's allowlist as-is — every extracted module receives the already-constructed ReplayCoordinator as a parameter. Pure move; no behavior change. * test(replay): cover pre-step artifact ordering and resume-before-mutation (#1555) Two invariants found during the P5 decomposition pass now have direct counterfactual-verified coverage: - packages/ad-replay/src/internal/__tests__/step-loop.test.ts: a post-dispatch target-binding mismatch (dispatchWithGuard) must report the accumulated PRE-step artifact snapshot it was called with, never the artifacts the failed dispatch itself produced. Verified red by swapping the buildPostDispatchTargetBindingFailure call to outcome.artifactPaths. - src/daemon/handlers/__tests__/session-replay-runtime-plan.test.ts: a rejected --from/--plan-digest resume must never reach prepareReplaySession's coordinator-mutating writes (the R2 ordering invariant) — a pre-armed repair transaction and corrective-resume watermark are asserted byte-for-byte unchanged after rejection. Verified red by calling prepareReplaySession before honoring the plan-validation rejection. * fix(ad-replay): enforce the exact two-entrypoint facade (#1555 review P1) packages/ad-replay/src/index.ts now exports exactly two value symbols, inspectAdReplay and runAdReplay, and zero types — formatReplaySuccessMessage (presentation) moves beside its one caller in session-replay-runtime.ts, and every type a root daemon file needs is derived structurally off the two entrypoints in the one new src/daemon/ad-replay-facade-types.ts module instead of being named off the façade. scripts/layering/package-boundaries.ts's readNamedExports is rewritten on oxc-parser's own static-export table instead of a regex, so it can no longer silently miss a widening export form: a bare `export *` re-export or an `export default` now throws (an un-enumerable, and therefore un-pinnable, export), while `export * as ns` and every other enumerable form is still counted. The pinned exact-symbol assertion in package-boundaries.test.ts is narrowed to ['inspectAdReplay', 'runAdReplay']. * fix(ad-replay): translate wire failures before the engine boundary (#1555 review P1) AdReplayDispatchOutcome's guard-mismatch/landmark-mismatch variants carried a generic `details: Record<string, unknown> | undefined` bag straight off the wire response — a daemon wire projection crossing into the engine even though the outcome itself was already a neutral type. The daemon adapter (session-replay-runtime-engine-adapter.ts) now narrows that bag into the typed AdReplayGuardMismatchEvidence/AdReplayLandmarkMismatchEvidence shapes (observed identity, expected/observed structural denotation, ancestry entries, match count) before returning the outcome; the unknown-parsing readers move there with the wire-reading responsibility they always were. target-verification.ts's deriveReplayTargetGuardMismatchEvidence/ deriveWaitLandmarkMismatchEvidence now consume only the typed values — no `unknown`-valued record type remains on any engine-crossing signature. * fix(ad-replay): move variable semantics/planning behind runAdReplay (#1555 review P1) The daemon assembled the `${VAR}` scope (buildPreparedReplayScope) and interpolated actions at two independent call sites: dispatch's own (invokeReplayAction) and target verification's separate one (resolveTargetVerificationEntry) — duplicated orchestration the P5 design assigns to the engine. runAdReplay's request now carries the raw scope INPUTS (varSources: plain builtins/file/shell/cli-env data, plus actionLines/actionSourcePaths/ resolvedPath for interpolation-error location) instead of a built scope; the engine builds the scope and resolves each action exactly once per step, handing the RESOLVED action to dispatchStep/beginTargetVerification while every other capability still receives the ORIGINAL recorded action (a target-binding divergence reports the recorded selector, never an expanded ${VAR}). This is the one resolution site now — session-replay-action-runtime.ts's invokeReplayAction and session-replay-target-verification.ts's resolveTargetVerificationEntry no longer hold a scope or call resolveReplayAction themselves. Scrub-value collection (collectReplayScrubbableVarValues, for divergence-report redaction) is kept single-sourced in the engine too: it's computed from the engine's own live scope and threaded to each build-failure/handleActionFailure capability as an explicit scrubVars argument, rather than the daemon recomputing it from a second scope object (which would have gone stale, since expandedBuiltinNames tracking now only happens engine-side). The Maestro replay path's own daemon-side vars usage is unrelated (a different engine) and is out of scope here. * fix(ad-script): make ${VAR} interpolation a linear scanner CodeQL flagged the interpolation regex's fallback group as js/polynomial-redos once vars.ts moved into packages/ (library-input classification): every ${NAME:- prefix of an unclosed input rescanned to end-of-string, quadratic overall — 1,857 ms measured on 20k repetitions of '${A:-['. Replaced with a single-pass scanner; failed fallback scans emit their span verbatim and resume after it (escape-pair alignment is identical from every candidate start inside the span, so no later candidate can terminate where the failed scan could not). Equivalence: 200k-trial differential fuzz against the retired regex over the adversarial alphabet, zero mismatches; both adversarial shapes now resolve in 1-2 ms. * refactor(ad-replay): typed façade replaces the zero-type rule (#1555 structural-quality review) Reverses the exact-two-value zero-type export shape #1555's second review pass established: it forced every root type derivation through one shim (src/daemon/ad-replay-facade-types.ts) and left four daemon-side twin types (TargetVerificationEntry, TargetClassificationOutcome, TargetBindingFailureEvidence, ReplayVerifiedTargetGuard) plus a toDaemonEvidence copy translator shadowing the engine's own shapes. packages/ad-replay/src/index.ts now exports inspectAdReplay/runAdReplay (unchanged, still the only two values) plus the neutral vocabulary their signatures are built from, by name — following packages/maestro's façade precedent. The exact-symbol gate in scripts/layering/package-boundaries.test.ts is widened to pin the full sorted list (values + types). The four daemon twins are deleted; session-replay-target-verification.ts and session-replay-runtime-engine-adapter.ts now use the engine's own AdReplayVerificationEntry/AdReplayTargetClassification/ AdReplayTargetBindingEvidence/AdReplayVerifiedTargetGuard directly. TargetBindingDivergenceBuilt's array fields are now readonly-compatible, so toDaemonEvidence's copy is gone — evidence flows through unchanged. * fix(ad-replay): honor the selector port's own contract in the parse gate target-verification.ts's planPreDispatchTargetVerification used resolveRecordedTarget (operation 2, resolve) over an empty node tree purely to read its parse-invalid reason — a resolve call standing in for a parse call, even though readSelectorExpression (operation 1, parse) exists to answer exactly that question and was already unused inside the engine. Replaced with port.readSelectorExpression('ordinary', [token]). The mapping is not 'invalid' -> skip: production's 'ordinary'/'wait' grammars only ever record a boundary once it has already parsed, so a single malformed token can only come back 'not-applicable' there ('invalid' is unreachable from this call site on the production adapter). Both non-'expression' outcomes map to skip, matching the historical behavior (a single parse-invalid reason covered both cases). platform dropped from the function's params — it was only ever threaded to the resolve call this replaces. Added a contract-suite cell pinning the exact (diverging) discriminant each adapter reports for a selector-shaped-but-malformed bare token, and why the divergence is harmless for the one real consumer. * refactor(ad-replay): split step-loop.ts and shrink the daemon adapter (#1555 structural-quality review) step-loop.ts (810 LOC) splits three ways, following packages/maestro's own precedent: - internal/runtime-port-types.ts: the AdReplayStepRuntime boundary vocabulary (all the neutral types the engine/daemon exchange). - internal/verify-dispatch.ts: verifyAndDispatchStep + its dispatchNoGuard/ dispatchWithGuard helpers. - internal/step-loop.ts: runAdReplay itself plus the terminal-close/ executable-action structural logic (isExecutableReplayAction, resolveSuppressedTerminalCloseIndex). packages/ad-replay/src/index.ts's type exports now source from runtime-port-types.ts. step-loop.test.ts's AdReplayStepRuntime import moves to the new path (no assertion changes). src/daemon/handlers/session-replay-runtime-engine-adapter.ts (553 LOC after item 1's twin removal) shrinks to 294 via two further extractions: - session-replay-dispatch-narrowing.ts: the wire `details` bag -> typed evidence narrowing and dispatch-failure classification. - session-replay-runtime-step-support.ts: ReplayStepContext (moved here to avoid a cycle with the adapter, which re-exports it by name) plus the failure-wrapping/diagnostics-support helpers. Final LOC: adapter 294, dispatch-narrowing 148, step-support 153, step-loop 225, verify-dispatch 246, runtime-port-types 374. * test(ad-replay): package-local tests for resume.ts/target-verification.ts + terminal-lifecycle test rename resume.test.ts covers resolveReplayEntryIndex directly (previously only exercised transitively through the daemon's session-replay-runtime-plan tests): no --from/--plan-digest, the paired-flags requirement, in-range --from, out-of-range rejection, stale-digest rejection, the authorized empty-tail boundary (actionCount + 1) gated on a matching watermark, and the unperformed-record-and-heal growth check. Counterfactual run and restored: widening describeOutOfRangeResumeFrom's bound turns the out-of-range/ empty-tail-without-watermark assertions red (2 failures observed). target-verification.test.ts covers all four engine policy functions directly: the two plan* pre-capture gates and the two derive* post-dispatch evidence builders, including item 2's own new decision surface (a fake ReplaySelectorPort proving both non-'expression' readSelectorExpression outcomes map to skip). Counterfactual run and restored: narrowing the check to the literal `'invalid' -> skip` reading turns the 'not-applicable' case red (reports recorded-unverifiable instead of skip). session-replay-terminal-lifecycle.test.ts renamed to session-replay-runtime-keep-session.test.ts: its production module (session-replay-terminal-lifecycle.ts) was already deleted by the #1554 fold-in, and its six cases drive the full runReplayScriptFile round trip against a real SessionStore (including daemon-only postconditions the engine's step loop never reaches) rather than testing engine policy through the façade in isolation — the engine's own terminal-close-suppression decision already has direct, cheaper coverage in step-loop.test.ts. No assertion changes; both files' header comments cross-reference the split. * refactor(ad-replay): compute scrub values once per step, one name end to end collectReplayScrubbableVarValues(scope) was called fresh at 5 separate return points inside one verifyAndDispatchStep invocation plus once more in handleActionFailure — always the same result, since nothing between them mutates scope. step-loop.ts's runAdReplay now computes scrubVars ONCE per step, right after resolveReplayAction (the one call that can grow the scope's expanded-builtins set), and threads it as a plain readonly AdReplayScrubValue[] value; verify-dispatch.ts no longer imports ReplayVarScope or collectReplayScrubbableVarValues at all. "One name" end to end: the daemon's TargetBindingDivergenceContext.scrubVars and withReplayFailureDiagnostics's scrubVars param used a separately-derived ReturnType<typeof collectReplayScrubbableVarValues> (mutable array) instead of the engine's own AdReplayScrubValue, requiring a [...scrubVars] copy at every daemon call site to satisfy the mutable-array type. Both now use readonly AdReplayScrubValue[]/readonly ReplayVarScrubEntry[] (structurally identical, already readonly-safe downstream — scrubReplayVarValues and createReplayDivergenceSanitizer already accepted readonly arrays), so the four [...scrubVars] copies in session-replay-runtime-engine-adapter.ts are gone. * fix(daemon): make lastObservation genuinely per-step, not per-run createAdReplayStepRuntime's lastObservation closure lives for the whole replay run (one factory call covers every step), but was never reset between steps. Every current buildTargetBindingFailure call site happens to be preceded by this same step's own captureObservation, so the `lastObservation ?? { reason: 'observation-missing' }` fallback could never actually fire — but if it ever did (a future call path reaching buildTargetBindingFailure without capturing first), it would silently attach the PREVIOUS step's screen instead of reporting the missing-capture condition the fallback message claims. armStep runs exactly once per step, before any of that step's other capabilities (verified against step-loop.ts's runAdReplay loop order) — the natural per-step boundary. It now clears lastObservation first. No behavior change on any reachable path today (full daemon + ad-replay suite: 1766/1766 green); an unrelated device-claim-prune contention flake was observed once and did not reproduce on isolated or full-suite reruns. * docs(ad-replay): fix decayed review-changelog comments naming defunct symbols Four comments named symbols/paths that no longer exist, left behind by earlier review passes describing PR history rather than the current constraint: - session-replay-runtime-step-support.ts / session-replay-runtime.ts (2 sites): referenced a function called executeStep, which was never reintroduced under that name after the P5 split — the actual mechanism is the runtime's dispatch/build-failure capabilities recording into the lastResponse side-map. - session-replay-runtime.ts: referenced an engine collectArtifactPaths capability that does not exist — artifactPaths is a daemon-side Set the adapter mutates via collectReplayActionArtifactPaths. - packages/ad-replay/src/internal/selector-port.ts: pointed at ./testing/in-memory-selector-port.ts, the in-memory adapter's pre-stage-D location — it has lived at src/__tests__/test-utils/in-memory-replay-selector-port.ts since. - session-replay-repair-hint.ts / session-replay-runtime-step-support.ts (2 sites): named target-identity.ts, which does not exist (the real file is target-identity-node.ts); the second site additionally mislabeled classifyReplayTarget as engine-side when it is daemon-side (session-replay-target-classification.ts). Comment-only; no behavior change. * refactor(ad-script): move declaredScriptPlatform to its natural shared owner packages/ad-replay/src/internal/inspect.ts's declaredScriptPlatform and src/daemon/replay-device-selection.ts's readScriptReplaySelection each kept their own copy of the same "platform declared before the first open" scan over runtime/open actions — .ad script semantics, not engine or daemon policy, needed independently by ad-replay's plan-digest precedence and the daemon's device-selection platform resolution. Verified this was a genuine duplicate (not the single-sourced state I initially reported): readScriptReplaySelection's platform-tracking loop computes the identical result via a differently-shaped traversal fused with its own app-target scan. resolveDeclaredScriptPlatform now lives in packages/ad-script (its natural owner: the one package both ad-replay and the daemon already depend on, avoiding the R11 issue that justified the original duplication). The daemon's app-target scan stays its own separate pass; fusing it back into the shared function would smuggle a daemon-only concern into ad-script for no measurable cost (the actions array is small, and the shared function already stops at the same point the app-target scan needs to look). * docs(ad-replay): fix package.json description to match the current façade Described "target-identity, variable substitution, plan-digest, and report primitives" — the wide pre-#1555-review façade shape. Vars/identity/report vocabulary moved to ad-script/daemon across the P5 and #1555 review passes; the package now exports exactly inspectAdReplay/runAdReplay plus the neutral AdReplayStepRuntime vocabulary. Description updated to match. * refactor(daemon): fold the step-support fragment back into the engine adapter A simplicity audit judged session-replay-runtime-step-support.ts a size-target fragment, not a concern boundary: four unrelated concerns, one consumer, and a header comment admitting it existed to satisfy the <300 LOC metric. Folded back; the previously-exported helpers are module-private again; the adapter's honest size is renegotiated from the plan metric (dispatch-narrowing stays extracted — it has one nameable job). |
||
|
Michał Pierzchała
|
b125435989 |
refactor: extract WebDriver provider package (#1504)
* refactor: extract webdriver provider package * refactor: consolidate shared XML codec |
||
|
Michał Pierzchała
|
0e51007b04 |
refactor: isolate maestro engine package (#1506)
* refactor: isolate maestro engine package * perf: deepen maestro facade boundaries |
||
|
Michał Pierzchała
|
0ee2a86129 |
refactor: extract contracts workspace package (#1499)
* refactor: extract contracts workspace package * fix: preserve screenshot diff result contract * test: stabilize Android keyboard smoke |
||
|
Michał Pierzchała
|
53e4be5f86 |
Remove SkillGym suite and repo-health snapshot infrastructure (#1480)
* chore: drop SkillGym and the repo-health aggregator (#1412 descope) Remove the SkillGym harness (test/skillgym/), its check-affected lane, package scripts, and devDependency — the help-conformance bench is now the single non-gating small-model oracle. skills/ markdown classifies as docs in the affected-check selector instead of failing open. Remove scripts/repo-health: its only gating assertion duplicated the Layering Guard job, its case-count metric imported the deleted SkillGym suite, and its sole planned consumer (#1424 / PR #1477) was closed with the Track C descope on #1412. Verified: check-affected node --test suites, oxfmt, oxlint, tsc, check:layering, fallow audit vs origin/main, and the full unit suite (unit-core + subprocess-stub) all pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep * fix(scripts): fold slow-test budgets into the reporter for production-exports The Fallow production-exports gate flagged all three budget exports: their in-file consumer (SLOW_TEST_RATCHET) and the repo-health entry point that kept the module reachable were both removed in the descope, leaving the config-loaded reporter as the only consumer — invisible to --production analysis. The data-only module's second consumer is gone, so per the boundaries-are-earned norm the constants move into the reporter instead of gaining a suppression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep * docs: align skills/ format policy and purge last SkillGym mention Address both P2 review findings on #1480: the testing-matrix row and the selector's formatGate both still claimed oxfmt covers skills/, while selectChecks classifies skills/*.md docs-only (oxfmt ignores **/*.md, so the claim was a no-op even before). The matrix now states the docs-only policy and formatGate drops the dead underSkills fact. The merged examples/README.md index (from #1469) loses its skillgym mention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
56b72c5cf7 |
refactor(boundaries): put shared contracts below their consumers, gate the result (#1405)
* refactor(boundaries): move shared contracts below their consumers Acts on the depgraph findings: type-only edges are invisible to R5, so vocabulary that everything depends on had drifted above the zones that use it. - contracts/: the four platform-plugin facet tags (LogBackend, RecordingBackendTag, PerfMetricsSamplerTag, PlatformGatedProviderResolverKey) now live beside the plugin contract itself, which also moves out of core/; NetworkEntry moves next to the command surface that renders it; and the click-button, recording-export-quality, interactor-types and runner-lease-context vocabularies move down out of core/. - (root) drops from 29 files to 13: the internal *-contract/output/annotation modules move into contracts/, kernel/ (daemon-error, observability-redaction beside kernel/redaction), core/ (batch-policy, an ADR 0008 projection), commands/ (cli-command-aliases) and remote/ (upload-progress, upload-stream). What remains is entrypoints and the composition roots that R2 requires to sit outside the spine. - utils/ joins the ranked spine at rank 1 after its only two upward files move to the zones they were reaching for (cli/resolve-cli-options, cli-schema/cli-config), putting ~336 value edges under the gate. - Internal imports that routed types through the client-types re-export hub now name their real source. Type-only spine inversions drop from 61 to 35; the remainder is two clusters (client/client-types.ts and the ADR 0003 daemon facet). No behaviour change: 4470 unit tests and the layering gate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * style: merge the duplicate contract imports the tag moves created Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * refactor(imports): name the declaring module, share find's argument rules Two follow-ups from re-measuring the graph after the boundary moves. 1. 89 type imports across 79 files routed through a re-export hub in another zone: `CliFlags` reached through commands/cli-grammar/flag-types.ts (52) when it is declared in contracts/cli-flags.ts, the replay suite result types reached through daemon/types.ts when they are declared in contracts/replay.ts, the doctor types through a daemon handler module, and so on. Each hop invented a cross-zone edge the architecture never asked for — including every apparent replay -> daemon and utils -> commands dependency. They now name the module that declares them. Within-zone hops are left alone; those are a local style choice, not a boundary claim. 2. `find`'s three positional/flag checks existed in both daemon entry points with hand-repeated messages, and the copy in dispatchFindReadOnlyViaRuntime was unreachable — its only caller validates first. Both now call checkFindArgs in selectors/find.ts, beside parseFindArgs and isReadOnlyFindAction, for the reason that module's own comment already gives: so the two paths cannot disagree. The refusal is returned rather than thrown, because the two mechanisms are not observationally identical in the session event log. Type-only spine inversions: 61 -> 35. 4470 unit tests and every gate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * feat(layering): ratchet type-only spine inversions (R6) R5 ignores type-only edges by design — they cost nothing at runtime and do not affect cold start — so nothing was watching the direction they point. Ranking them the same way found 61 inversions, including contracts/ and utils/ declared in terms of rank-4 zones. 26 are fixed by the preceding commits; R6 pins the rest per zone pair so they can only shrink, and a new pair fails outright rather than being added to the baseline. The two remaining clusters each need their own change, and the baseline says so: the per-command Options/Result vocabulary declared inside the public Node-client surface, and the ADR 0003 daemon facet shape that core's descriptor registry composes. Both ratchet directions are covered: growth fails, and shrinking without lowering the number fails too, so the baseline cannot quietly stop describing the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs: record the import-graph findings behind this refactor A dated snapshot, not a normative document: when it disagrees with scripts/layering/, the gate wins. The graph tool that produced it lives on the claude/depgraph-viewer branch, deliberately out of this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * refactor(selectors): state the shared selector argument rules once R2 (commands-floor) forbids the daemon from importing commands/, and that is the right call: commands/ is the client-side surface — its only consumers are cli/, cli-schema/, mcp/, client/ and the composition roots — while the daemon is the executor on the other side of the wire. ADR 0008 protects exactly that seam. Relaxing R2 would let the executor depend on a client projection and pull CLI grammar and output formatting into the daemon's bundle. But the rule does force duplication: the daemon must validate independently because it accepts requests from any client, so 10 refusal messages existed in both zones. The only place a shared rule can live is below both, and selectors/ already held the parsers (splitIsSelectorArgs, splitSelectorFromArgs, isSupportedPredicate) and even the `is` predicate message — just not the checks that use them. Three drifts had already appeared in the `is` predicate rule alone: - commands/interaction/selectors.ts re-implemented the predicate list as an inlined seven-way `!==` chain while importing the message and hint from selectors/predicates.ts, so adding a predicate to the shared list would not have reached the CLI grammar. - That inlined chain compared the raw token, so the CLI rejected `is TEXT ...` while the daemon it hands the command to accepts it. The CLI now matches the executor; this is an intentional alignment, not an accident. - isCommand raised the same refusal without IS_PREDICATE_USAGE_HINT, so whether an agent got recovery guidance depended on which layer noticed first — the failure mode ADR 0010's audit calls out. checkIsPredicate, checkIsArgs, checkGetFormat, checkElementTargetArgs and checkWaitText now hold those rules, each beside the parser it wraps, and report a refusal rather than choosing how to raise it: the daemon returns a response, the command surface throws. Those mechanisms are not interchangeable — they write different session events — so the shared check stays out of that decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * feat(daemon): give ADR 0014's ref frame one transition, pin SessionState owners `SessionStore.get()` returns the live record out of a private Map and `set()` re-puts the same reference, so every `session.<field> = …` in the daemon is a durable write to store-owned state: 57 of them across 17 files, against 26 `set()` calls that are therefore ceremonial. Nothing at the store boundary can check what those writes are supposed to keep true. Measuring which module writes which field showed the problem is narrower than the raw count suggests — 16 of 27 fields already have exactly one writer. The sharp case is ADR 0014's ref frame: `refFrameState`, `refFrameScope`, `refFrameTree` and `refFrameGeneration` must move together or the frame is incoherent (an `active` state with a stale tree resolves refs against a namespace nobody authorized), yet complete issuance wrote them in ref-frame.ts and partial issuance wrote the same four in session-snapshot.ts. ref-frame.ts's own header claims to be "the single owner of the frame's transitions", and session-snapshot.ts documented itself as the exception. Both forms now go through `activateRefFrame`; they differ only in scope. `recordSession` deliberately moves alone in two paths (recording without arming a publication), so the save-script cluster gets no invented abstraction — it gets ownership instead. R7 records every field's owner and stops the set from growing quietly: a new SessionState field must declare one, a foreign write fails naming the owner to call, and an owner that stops writing must be removed so the table cannot drift into fiction. Field names are read out of the `SessionState` declaration, so a daemon module with an unrelated local named `session` — a provider or runner session — cannot trip it. 4475 unit tests and every gate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs: record the reference semantics and refresh the findings SessionStore.get/set now document that the record is handed out live, since that is the fact behind R7. The findings snapshot picks up the resolved R2 question, the ref-frame consolidation and the two new gate scopes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * refactor(boundaries): rank every satellite zone, extract the provider port Second-order effect of the earlier rounds. With `utils` on the spine and `(root)` emptied of shared contracts, the eleven zones that were unranked "because ranking them would invent an order the architecture had not committed to" turned out to have a consistent rank already — the order was there, unasserted. Solving the constraint system showed one blocker: `utils/remote-config.ts` projected a remote-config profile into `CliFlags` while reaching up into `remote/`, and its only three consumers were in `cli/`. It moves there as `cli/remote-config-flags.ts`, and every satellite zone joins the spine. Ranked coverage goes from 730/895 files to 882/895. Only `(root)` stays out, and now for one stated reason: R2 forbids `daemon/` from importing `commands/`, so the files that wire them compose the spine from above. Ranking them exposed 22 type-only inversions R6 had never been able to see, and they were concentrated rather than scattered: - The device-provider port. `providers/` and `cloud-webdriver/` implement what the daemon calls, so both sides name `DeviceLease`, `LeaseLifecycleProvider`, `LeaseLifecycleContext` and `DeviceInventoryProvider` — now declared in contracts/device-provider.ts, below both. The adapters also imported the daemon's NARROWED `DaemonRequest` while only ever reading `req.flags`; they now name the public one from kernel/contracts. - `MetroPrepareKind` and the remote-config profile field groups move to contracts/ for the same reason: the command surface validates them and contracts/cli-flags.ts is composed from them. Two clusters remain, ratcheted with their reasons in TYPE_INVERSION_BASELINE: the client-types vocabulary, and `SessionAction`, which needs `CommandFlags` and `DaemonBatchStep` to move with it. Also fixes two things CI caught: the eight type re-exports my earlier import redirection orphaned (none published through any src/sdk/* entrypoint, so no public surface changes) and `isSupportedPredicate`, now module-private since `checkIsPredicate` is the admission API. `fallow-baselines/health.json` is keyed by path, so the moved cli-config entry moves with the file rather than being regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * fix(selectors): use the admitted predicate, not the raw option Review finding. `isCommand` called `checkIsPredicate` and then kept reading `options.predicate` for the capture policy, the `exists` branch, `evaluateIsPredicate`, the failure message and the returned result. Admission normalizes case, so an upper-case predicate was let past the gate and then evaluated against lower-case branches: `EXISTS` skipped its own branch and fell through to the generic path, and the result echoed the raw token. I widened admission at that surface without threading the normalized value through it — the CLI-grammar surface in the same change does use the admitted value. Every decision after admission now reads it. Two tests, both verified to fail without the fix: - a production-route regression driving `device.selectors.is` with `EXISTS`/`TEXT`, plus one pinning that an unknown predicate is still refused WITH the ADR 0010 usage hint; - a surface parity gate (selectors/__tests__/is-argument-surface-parity.test.ts) in the repo's existing parity style, asserting the daemon and CLI-grammar surfaces reach the same verdict and hand the same normalized predicate downstream across an input table. A helper-only test cannot catch a surface that admits correctly and then discards the result, which is what happened here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs: name the pre-push gate, and the formatter's path allowlist Both misses in this PR's review were process, not judgement, and the docs pointed the wrong way for both. AGENTS.md said "prefer the aggregate package.json scripts" without naming which aggregate, and CONTRIBUTING listed `pnpm test` and the targeted checks but never `pnpm check`. `check:tooling` looks like the gate and is a subset of it: it stops before the Fallow audit, so the dead exports this PR introduced passed a clean `check:tooling` and failed CI. Both files now name `pnpm check`, say what it covers, and say what it cannot (the device matrix). The same gap produced a second mistake twice: `oxfmt <path>` reformats whatever you point it at, while the repo's `format` script is an allowlist that excludes `scripts/` and every `.md`. One run reformatted 50 unrelated script files into a commit; the next nearly did it to AGENTS.md. AGENTS.md now says to run `pnpm format`, never `oxfmt <path>`. It also records the rule that cost a CI cycle: Fallow's baselines are keyed by path, so a renamed file needs its baseline entry moved, not the baselines regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * revert: undo stray formatter output across docs and scripts Three separate `oxfmt <path>` runs in this branch reformatted files the repo's `format` script deliberately excludes: 55 files under scripts/maestro-conformance plus scripts/perf, sync-mcp-metadata and the slow-test reporter, and 12 markdown files including six ADRs and docs/agents/. All of it was whitespace, quote style and markdown table padding — no content — but it inflated the diff a reviewer has to read and would have rewritten prose ownership across files this change has no business touching. All 70 are back to their origin/main content, so the diff outside src/ is now exactly this change's scope: three docs, scripts/layering, the Fallow baseline, and five provider integration tests. The rule this violated is now in AGENTS.md: run `pnpm format`, never `oxfmt <path>`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * style: reformat two provider tests with the repo's pinned oxfmt `pnpm format:check` failed in CI on the two files whose imports I merged by hand. The repo pins oxfmt 0.42.0 as a devDependency and both `format` scripts invoke `./node_modules/oxfmt/bin/oxfmt`; I had reformatted with `npx oxfmt`, which resolved 0.60.0, and the two versions disagree about wrapping a 100-column import. This is the rule AGENTS.md already states — run `pnpm format`, never oxfmt directly — so there is nothing to add to the docs, only to do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * fix(ci): install deps for the layering guard, and gate the zero-dep contract The Layering Guard job failed with ERR_MODULE_NOT_FOUND on `oxc-parser`. The job ran with `install-deps: false` — no `pnpm install`, so no `node_modules` — and R7 had started parsing the daemon with oxc-parser instead of matching assignment operators with a regex. `pnpm check:layering` passed on every local run, because locally `node_modules` is always there. The job now installs dependencies. The alternative was to put R7 back on a regex, which cannot see `??=` or a computed `session[key] =` write, so it would trade a correct rule for a fast job. That leaves the interesting part: the zero-dep contract is real for the jobs that keep it, and it is invisible to every local run, which is the worst combination a constraint can have. R8 makes it checkable. It reads the zero-dep job list out of `.github/workflows/` rather than restating it — declaring a job zero-dep is what puts it under the rule — walks each job's entry scripts and their whole relative-import closure, and requires every specifier to be a Node builtin or another repo file. A zero-dep job whose entry scripts the scan cannot identify fails too, so the rule cannot be escaped by changing how the job invokes them. Specifiers come from oxc-parser's module record, not a line scan. The closures include `--test` files, and a test about imports naturally embeds import syntax in a fixture string; the line scanner reported two such phantom violations in model.test.ts before the switch, which is how a gate stops being trusted. Verified by re-running the real gate against three injected regressions: the layering job back on `install-deps: false` (reproduces the exact CI failure, pointing at session-state.ts:24), a package import added to the still-zero-dep affected-selector closure, and a zero-dep job whose run step names no script. Also corrects the CONTEXT.md spine paragraph, which still described the satellite zones as deliberately unranked after they had all joined the ranked spine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * fix(layering): make R7 exhaustive, and follow session records through aliases Review finding: `SESSION_STATE_FIELD_OWNERS` covered 27 of `SessionState`'s 42 fields and nothing asserted parity, so a new field could be added and pass the gate by being invisible to it. R7's advertised claim — "every SessionState write is inside its declared owner" — was broader than what it checked. Investigating that turned up a second, larger gap the finding did not name: the scan only recognized a binding literally named `session`. The daemon names these records by role, so `nextSession`, `provisionalSession`, `completedSession`, `preRunSession` and `preEntrySession` were all invisible — and three of those writes were genuine violations R7 existed to catch: src/daemon/snapshot-runtime.ts:256 nextSession.snapshotScopeSource src/daemon/snapshot-runtime.ts:265 nextSession.snapshotGeneration src/daemon/handlers/session-replay-runtime.ts:707 preEntrySession.pendingRecordAndHeal The first two are the #1076 versioned-ref invariant: the generation advances exactly when the stored tree is replaced. That rule lived in `setSessionSnapshot` and had acquired a second statement of itself in snapshot-runtime.ts, whose own comment admitted the bypass. It now goes through `setSnapshotLineage` in the owning module. The third clears a watermark stamped by session-replay-resume.ts; `clearPendingRecordAndHealWatermark` puts the clear beside the stamp. Gate changes: - Binding detection accepts aliases, paired with the existing declared-field filter so an unrelated `…Session` local only registers if it also writes a field SessionState owns — where the remedy is the same anyway. - `fieldClassificationDrift` asserts parity in all three directions: unclassified, in-both, and naming a field SessionState no longer declares. - `STORE_OWNED_SESSION_STATE_FIELDS` classifies the 11 fields the store establishes at construction. It is a positive claim, so a direct write to one fails and names both remedies. - Four fields the widened scan made visible (`lease`, `deviceClaim`, `appName`, `saveScriptComplete`) got real owners. `nextSnapshotGeneration` is now module-private: replacing its only external call site orphaned the export, which `pnpm check` caught via Fallow. Verified against three injected regressions: a new SessionState field with no direct write (the reviewer's exact scenario), a foreign write through an alias binding, and a direct write to a store-established field. All three rejected. `pnpm check` green, 4486 unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs(daemon): correct the snapshot-lineage claim, and pin the real contract Device verification of the snapshot-lineage route found that a ref pinned before a `diff` keeps resolving with no pinned-ref warning. That is the designed ADR 0014 behaviour, not a regression — the comment describing it was wrong, and I propagated it. `main`'s comment in snapshot-runtime.ts said a diff "leaves client refs pinned to the previous generation, which is exactly what the pinned warning diagnoses". The counter and the authorization epoch are different clocks: - `diff` passes `issuesRefsToClient: false`, so it never reactivates the frame; - `resolveRefStalenessWarning` compares a pin against the frame EPOCH, not the observation counter, and its own comment says why — a capture that bumped the counter must not make a valid pin from the issuing frame look stale. So advancing the counter is not the same as invalidating client refs, and the observable the comment promised does not exist. I carried the sentence into `setSnapshotLineage`'s doc when the transition moved, and then into a hardware verification request, which cost a reviewer a device run against a false claim. `setSnapshotLineage` itself is unchanged and was a pure move: same expressions, same inputs as the inline assignments it replaced, so this route behaves exactly as it does on main. A comment that contradicts the code should be an assertion instead, so the contract is now pinned in session-snapshot.test.ts: the diff advances the counter, preserves the epoch, leaves the pre-diff pin resolving without a warning, and still warns for a pin from a different frame. Verified to fail when the epoch comparison is swapped for the counter. A second test covers the keep-current branch, which had no coverage. `pnpm check` green, 4488 unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
devin-ai-integration[bot]
|
8246362999 |
chore: baseline-free production-exports cleanup (#1276) (#1282)
* chore: baseline-free production-exports cleanup (#1276) Classify and burn down the 32 baseline-tolerated unused production exports. - Live seams: annotate with @internal JSDoc visibility tags (test hooks, introspection helpers, public install-source constant) so fallow no longer treats them as dead production exports. - Wrappers: collapse re-export wrappers in commands/index.ts (ref/selector) and daemon/lease-context.ts (buildLeaseDiagnosticsContext); update all importers to pull directly from the source module. - Stale baseline entry: remove the non-existent resetAndroidMultiTouchHelperInstallCache entry. - Empty fallow-baselines/production-unused-exports.json so check:production-exports now fails loudly on any new dead export. Fixes #1276 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore: address review feedback on production-exports cleanup (#1276) - CONTRIBUTING.md: document that intentional non-production exports should use JSDoc @internal with a short justification, treated as a reviewed baseline entry. - isPlatform: fix JSDoc tag to "@internal" and remove conflicting "public" wording. - ARCHIVE_EXTENSIONS: re-export from src/sdk/install-source.ts so the public install-source subpath has a real consumer story for the constant. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore: make production-exports check truly baseline-free (#1276) - Drop --baseline from pnpm check:production-exports and remove the check:production-exports:baseline generation script. - Delete fallow-baselines/production-unused-exports.json. - Update CONTRIBUTING.md to describe the baseline-free behavior and remove references to reviewed baseline entries for production unused exports. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Michał Pierzchała <thymikee@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
devin-ai-integration[bot]
|
0a8ea3a57b |
refactor: consolidate architecture ownership and client results (#1210)
* refactor: consolidate architecture ownership and client results Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix: keep selector parse chunk grouping current Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix: update moved architecture breadcrumbs Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix: enforce moved selector architecture Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix: keep selector guarantee ownership current Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * docs: update selector ownership references Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Michał Pierzchała <thymikee@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
Michał Pierzchała
|
d4146c7f1b |
feat: add Android test IME helper for deterministic text entry (#1198) (#1201)
* feat: add Android test IME helper for deterministic text entry (#1198) Ships a headless InputMethodService (android-ime-helper) as a third Android helper APK, replacing the visible system keyboard during automated sessions. Renders zero accessibility nodes and accepts Unicode/CJK/emoji text over a base64-encoded broadcast channel, fixing both the settle-diff IME-chrome flood and the ASCII-only adb-shell text entry limit in one structural fix. - android-ime-helper/: InputMethodService + build/package scripts on the existing helper-APK toolchain (javac+d8+aapt2+zipalign+apksigner). - src/platforms/android/ime-helper.ts, ime-lifecycle.ts: install/version lifecycle (shared with the other two helpers via the new helper-package-install.ts), activation on session open, and on-device restore-hygiene (previous IME persisted to a device settings key so any daemon/state-dir can recover it; restored on close, daemon teardown, and daemon startup for orphans left by a crashed run). - input-actions.ts: fill/type route through the helper's broadcast channel when active, unicode-safe; unchanged ASCII-shell fallback otherwise. - doctor: new android-test-ime check flags a stuck helper IME with a copy-pasteable `adb shell ime set` remediation command. - Gating: default-on for emulators, opt-in via `open --test-ime` on real devices. - Dead-weight: rewrote the manual ADBKeyBoard workaround doc, dropped the now-provably-live skillgym non-ASCII eval case, updated the ASCII fallback's error message to point at the helper instead of dead-ending. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * fix(#1201 review): permission-gate the IME receiver, fix CI, add opt-out Addresses the independent review's blockers and should-fixes. SECURITY (blocker 1): the text-injection receiver was RECEIVER_EXPORTED with no gate — any co-installed app could inject text into the focused field while the test IME was active. Fixed by requiring the WRITE_SECURE_SETTINGS sender permission on the (in-process, dynamically-registered) receiver: adb shell holds it, third-party apps cannot. The reviewer's suggested exported=false + explicit-component approach was tried first but empirically breaks delivery on API 36 (adb shell cannot reach a non-exported receiver there) — documented in the helper README. Live-verified: a purpose-built rogue APK's broadcasts (implicit and package-scoped, no permission) are silently dropped, field unchanged; adb shell's bare broadcast still injects. Added ime-helper-security.test.ts asserting the permission gate and that no permissionless exported registration returns. CI (blocker 2): (a) added `testIme` to integration-progress-model flag buckets (Integration Tests was red on the unclassified flag). (b) mocked resolveAndroidImeHelperArtifact in session-doctor-android / ime-lifecycle / input-actions-test-ime tests so they no longer depend on android-ime-helper/dist existing on disk (Coverage was red on a fresh checkout); verified by running them with dist removed. Should-fixes: added `--no-test-ime` to opt out on emulators (tri-state gating, parser-tested); PR body's "byte-identical" claim corrected to size/CRC-match. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * docs(#1201): pin the API-36 exported-receiver constraint in a comment The RECEIVER_EXPORTED flag cannot express why it must stay exported. Add a one-line note so a future hardening pass doesn't switch to RECEIVER_NOT_EXPORTED and silently break the CLI (adb shell can't deliver explicit broadcasts to non-exported components on API 36+; WRITE_SECURE_SETTINGS is the actual gate). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * fix(#1201 review): harden IME restore lifecycle (blockers 1 & 2) Blocker 1 — a failed restore no longer deletes the recovery value. restore now reads back default_input_method after `ime set` and only clears the persisted previous-IME record on a confirmed-successful restore; a failed set keeps the value so a later retry / startup recovery / doctor remediation can still un-strand the user off the helper IME. Blocker 2 — startup orphan-recovery no longer overwrites/races user state. It only restores when the device's CURRENT default IME is still our helper (so a user who legitimately switched away is left alone), and skips any device a live session in this process owns (the fire-and-forget startup vs. concurrent `open` race — activate now marks the device active BEFORE the `ime set`, so any recovery pass that could observe the helper active also observes the flag and skips). Never persists the helper itself as the previous IME. activate also verifies its own switch via read-back. Exported ANDROID_IME_HELPER_SERVICE_COMPONENT so restore compares the active IME without reading the packaged artifact from disk. Tests: failed-restore keeps the value (+ later recovery succeeds), startup no-op when current != helper, startup skips a live-owned device. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * chore(#1201): delete unused ACTION_ENTER path, baseline test-only export seams Rebased onto main (#1202 production-unused-exports gate). Two follow-ups: - Deleted the unused ACTION_ENTER broadcast end-to-end (TS sendAndroidImeHelperEnter + its test, Java handler, README): nothing routes through it — `keyboard enter` uses the keyevent ENTER path — so the new production-exports gate flagged it as dead production code. Removed rather than grandfathered. - Added the three legitimate test-only seams (resetAndroidImeHelperInstallCache, resetAndroidTestImeActivationCacheForTests, setAndroidTestImeActiveForTests) to fallow-baselines/production-unused-exports.json, matching how the sibling helper reset functions (resetAndroidMultiTouchHelperInstallCache, ...) are already grandfathered there. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * fix(#1201): stop daemon-startup adb spawn on non-Android hosts (macOS Smoke) Root cause of the red macOS Smoke shard (proven, not hand-waved): the fire-and-forget restoreOrphanedAndroidTestImeOnDaemonStartup ran `adb devices` at EVERY daemon startup, on every platform. GitHub macOS runners ship the Android SDK, so this cold-started the adb server mid-replay and destabilized the macOS System Settings replay timing — the failed job's cleanup shows "Terminate orphan process: pid (N) (adb)"; main's green runs spawn no adb. Fix: gate the startup orphan scan behind a host-side marker written in the daemon state dir when a session activates the test IME (mirrors the managed-web-browser orphan-cleanup `installed` gate). A host that never uses the Android test IME — the macOS CI runner included — never writes the marker and so never spawns adb at startup. The marker is cleared once nothing is left stuck. Adds SessionStore.resolveStateDir(); tests: startup recovery does not scan adb when no marker exists (+ marker cleared after a clean scan). 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * chore(#1201): suppress fallow class-member false-positive on state-dir accessor CI's Fallow audit flags SessionStore.resolveDaemonStateDir as an unused class member, but it is called via sessionStore.resolveDaemonStateDir() in session-open.ts — fallow's class-member tracer just doesn't resolve a method call sited inside a call argument. Renamed for clarity (avoids the collision with config.ts's free resolveStateDir) and added the localized fallow-ignore-next-line unused-class-member suppression. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> * fix(#1201 review): durable persist before switch + device-scoped recovery markers Addresses devin-ai-integration's two P1 restore-safety blockers on 19cbce79d. P1.1 — durably persist the restore target BEFORE the global IME switch. writePersistedPreviousIme now checks the `settings put` exit code AND reads the value back, returning a boolean. activate persists first and, if it cannot be persisted, fails open to the existing input path WITHOUT switching — a rejected `settings put` can no longer strand the user on the helper with no restore target. Regression test added. P1.2 — close the marker crash/offline blind spot. Recovery intent is now recorded per device, BEFORE the switch (ordering: durable record -> marker -> ime set), eliminating the post-switch/pre-marker crash window. Markers are device-scoped and each is retained until that device is actually observed clean: an offline/disconnected-but-stuck device keeps its marker and is recovered on reconnect instead of being cleared because the current `adb devices` scan saw no set-failed. Close-time restore clears only that device's marker (stateDir plumbed through teardown/close). Tests cover the persist-failure, post-switch/pre-marker crash, offline-then-reconnect, live-session-owned, and user-switched-away cases. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
46d2931bf0 | refactor: remove redundant facade exports (#1204) | ||
|
Michał Pierzchała
|
6e21fedc08 | refactor: remove production-unused exports (#1203) | ||
|
Michał Pierzchała
|
fb1117f229 |
ci: ratchet against production-unused exports (#1202)
* ci: ratchet against test-only exports Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass. * fix: harden test-only-exports ratchet per review Addresses the two should-fixes and all five minors from the independent review of #1202: - Replace the regex own-file occurrence count with an oxc-parser AST walk (typescript@7 ships no JS scanner API, so the review's fallback tool suggestion is the primary): identifiers are counted as AST nodes deduped by source span, so mentions in JSDoc/block comments, strings, and template-literal text no longer masquerade as call sites (review finding 1, both constructed cases re-verified fixed), and a `//` inside a string no longer hides real usages (finding 6). Span dedupe keeps barrel re-exports (`export { x } from`) counting once. The sharper count surfaced one organic false negative on main: `selector` in src/commands/index.ts was previously exempted because the regex matched "selector" inside the './...selector-read.ts' import path string; it is now baselined alongside its sibling `ref` (same re-export line). - Make the baseline shrink-only (finding 2): --update-baseline refuses new findings with the same wire/delete/annotate message, so the `// test-seam:` annotation in the reviewed source diff is the only acceptance path; CONTRIBUTING no longer documents baseline regeneration as an acceptance option and now describes baseline growth as a deliberate manual edit. - Stale baseline entries now emit a `::warning` CI annotation (finding 3). - Commit a re-runnable fixture test (finding 4): check.test.ts mirrors scripts/layering/model.test.ts, builds a synthetic package with a clearMetroSessionHints-shaped export (JSDoc self-mention included), asserts it is flagged, and asserts the annotated twin passes; wired before the check in pnpm check:test-only-exports. - Mark the unreadable/unparseable-file fallbacks CONSERVATIVE: per CONTRIBUTING's convention (finding 5). - Document the dynamic property access (obj[name]) blind spot in the script header and CONTRIBUTING (finding 7). * fix: harden test-only export ratchet * refactor: use native Fallow export gate * chore: refresh production export baseline |
||
|
Michał Pierzchała
|
54f6d45b32 | refactor: extract host process primitives (#1134) | ||
|
Michał Pierzchała
|
e833de474d |
refactor: split oversized session and apple app files (#1061)
* refactor: split oversized session and apple app files * fix: satisfy ci after file split |
||
|
Michał Pierzchała
|
e229957602 |
refactor: split daemon server runtime into daemon/server/ — Phase 5 (#985)
Phase-5 §5.5 folder move (server side; the daemon/client/ split shipped in #962). Extracts the process-bootstrap / server-runtime cluster into src/daemon/server/ as a pure, behaviorless path codemod — no logic changes. Moved (server bootstrap/runtime — the layer that spins up the daemon and owns the platform graph; each imported only by the bootstrap layer + each other): src/daemon-runtime.ts -> src/daemon/server/daemon-runtime.ts src/daemon/http-server.ts -> src/daemon/server/http-server.ts src/daemon/transport.ts -> src/daemon/server/transport.ts src/daemon/server-lifecycle.ts -> src/daemon/server/server-lifecycle.ts src/daemon/server-shutdown.ts -> src/daemon/server/server-shutdown.ts Left in src/daemon/ root (request core / shared wire helpers, out of scope): request-router.ts, handlers/, session-store.ts, lease-registry.ts, context.ts (the daemon's request layer) and http-contract.ts / http-health.ts / http-errors.ts / config.ts (HTTP wire contract + daemon config shared across client, remote, and cli — not server-only). Left: src/daemon.ts (the thin process entry) stays at src/ with the other package entrypoints; it is coupled to its physical path by four non-import string references (rslib entry, config dev-mode sentinel, process-identity detection regex, daemon-client launch srcPath), so moving it is beyond a pure import codemod. Rewrote every from/import/import()/type-only specifier per importer (resolve-based path.relative recompute) across src and test, and renamed the fallow health-baseline key for http-server.ts. daemon-runtime's static platforms/ import is now inside the daemon-server seam the layering lint (#984 R3) allows. Verification: tsc --noEmit 0; layering check (branch script) unchanged (3 pre-existing R3 violations, 0 new); oxfmt clean; oxlint --deny-warnings 0; fallow audit --base origin/main clean (14 files); rslib build 0 (internal/daemon entry still emits); vitest 17 passed (daemon-entrypoint, http-server-rpc-validation, server-shutdown + 3 provider-integration). |
||
|
Michał Pierzchała
|
26ac865c63 | refactor: consolidate Apple platform internals (#968) | ||
|
Michał Pierzchała
|
bcf910a2bb |
refactor: split daemon client driver into daemon/client/ — Phase 5 (#962)
Move the daemon CLIENT driver (the in-process side that sends requests to a
running daemon) out of the src/ root into src/daemon/client/, per
plans/perfect-shape.md §5.5 ('daemon/client/ <- daemon-client*.ts'; the
daemon- prefix co-located client driver + server bootstrap at src root).
Files moved (7): daemon-client{,-lifecycle,-metadata,-progress,-rpc,-timeout,
-transport}.
- git renames; 19 importers repointed via the resolve-based codemod
(intra-set stays ./, kernel -> ../../, daemon/remote deps recomputed)
- Layering Guard verified: none import src/commands/* (safe under src/daemon/)
- not a public export; no rslib impact
- update fallow-baselines/health.json keys
Behaviorless path codemod; typecheck/lint/format/build/tests green.
|
||
|
Michał Pierzchała
|
189b062519 |
refactor: extract client/ folder — Phase 5 (#960)
Move the SDK client + companion-tunnel cluster out of the src/ root into a dedicated src/client/ folder, per plans/perfect-shape.md §5.5 (~8k LOC client/remote unfoldered at src root; remote/ already extracted in #951). Files moved (9): client, client-types, client-shared, client-normalizers, client-companion-tunnel{,-contract,-worker}, client-react-devtools-companion, companion-tunnel. - git renames; 68 importers repointed via the resolve-based codemod (intra-client stays ./, staying deps recomputed) - companion-tunnel.ts keeps rslib key 'internal/companion-tunnel' so dist output stays dist/src/internal/companion-tunnel.js (public subpath byte-identical; verified by build) - update non-src importers (3 integration tests, vitest coverage include), .fallowrc.json entrypoint, fallow-baselines/health.json keys backend.ts and daemon-client*.ts are intentionally left for follow-up (daemon/client split). Behaviorless path codemod; all gates green. |
||
|
Michał Pierzchała
|
65227c6719 |
refactor: absorb CLI parser into cli/parser/ — Phase 5 (#958)
Move the CLI argument/flag/help parser out of utils/ into a dedicated src/cli/parser/ folder, per plans/perfect-shape.md §5.5 (utils/ hosts a 3k CLI parser among its buried subsystems). Files moved (3): args, cli-flags, cli-help (args->cli-help intra-set import stays relative). - git renames; importers repointed via the resolve-based codemod (64 importers; staying-utils/kernel deps recomputed to ../../) - no public-export/rslib impact - update scripts/integration-progress-model.ts import + fallow-baselines/ health.json keys (args incl. :high impact variant) Behaviorless path codemod. typecheck/lint/format/build/tests green; integration-progress model still runs. |
||
|
Michał Pierzchała
|
f1248970f2 |
refactor: extract snapshot/ folder — Phase 5 (#953)
Move the AX-snapshot processing domain out of utils/ into a dedicated
src/snapshot/ intent folder, per plans/perfect-shape.md §5.5 (utils hosts
the AX-snapshot domain among 3 subsystems).
Files moved (9): snapshot-{diff,label-signals,lines,occlusion,processing,
quality,tree,visibility} + mobile-snapshot-semantics (processes SnapshotNode,
depends on snapshot-tree). android-helper-snapshot-presentation stays in
utils/ with its android-helper-presentation/ cluster.
- git renames; imports repointed via the resolve-based codemod
(staying-utils -> ../utils/, intra-snapshot -> ./, kernel unchanged)
- no public-export/rslib impact; update fallow-baselines/health.json keys
- tests stay in their domain __tests__/ dirs, imports repointed
Behaviorless path codemod. typecheck/lint/format/build/tests green.
|