mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
v0.20.9
64 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michał Pierzchała
|
d8a7d03faf |
refactor: route application lifecycle through runtime facts (#1759)
* refactor: route application lifecycle through runtime facts Moves the canonical `open`, `prepare`, `close` and internal `runtime` descriptors behind package-owned lifecycle bindings admitted from device runtime facts, while daemon request/session policy and public response construction stay put. Based on main, which already carries the boot unit, the parametrized cutover gate and the apps unit. Readiness is package-owned there, so the Apple and Android bindings call ensureAppleReady/ensureAndroidReady rather than a root readiness bag; ensureAppleReady gained an onColdBootStart hook so open keeps warming the runner cache in parallel with a cold boot, and a narrow markBooted port publishes readiness' fresh observation so a flow still makes one simctl listing. Cutover rows take R24-R27, clear of the accepted catalog and the sibling install stack, and cutoverTableDefects rejects a duplicate rule id. Two defects this unit introduced are fixed here rather than shipped: `open <app> <url>` dropped the URL on a first open, and test-IME activation was first fatal on an unobtainable helper and then over-caught. Helper unavailability is a typed non-activation outcome now; fence, lock and post-record failures propagate. The duplication the unit had accumulated is gone: one runtime-admission module instead of five per-command copies, one direct-lifecycle binding factory instead of six hand-rolled packages, one transport-hint predicate, one session finalization path, and no identity-wrapper module. * fix: allocate lifecycle cutover rows after deployment * chore: preserve lifecycle union reconstruction * fix: reconcile lifecycle runtime stack * refactor: tighten lifecycle runtime topology * refactor: remove superseded runtime adapters * fix: preserve stacked runtime cutovers * test: preserve migrated runtime ownership * test: move Android deployment retry ownership * test: extract runtime hint fixtures * fix: preserve lifecycle stack invariants * fix: complete lifecycle runtime cutover * fix: remove lifecycle cutover residue |
||
|
Michał Pierzchała
|
66cca1a5b8 |
refactor: route install commands through platform runtime (#1758)
* refactor: route install commands through platform runtime * fix: preserve stacked runtime facts * fix: align deployment facts with shutdown runtime * refactor: simplify capability facts projection * style: format capability facts projection * fix: preserve migrated capability ownership * fix: propagate deployment artifact cancellation * refactor: move Harmony deployment mechanics into package * refactor: move Apple deployment tools into package * refactor: move Android deployment tools into package * refactor: inject deployment temporary storage * refactor: remove superseded deployment helpers * fix: preserve provider deployment transport |
||
|
Michał Pierzchała
|
5855dfc2e0 |
refactor: route shutdown through device runtime (#1757)
* refactor: route shutdown through device runtime * fix: cover shutdown cutover review gaps * fix: propagate shutdown cancellation * fix: move shutdown mechanics to platform owners * fix: pass device to shutdown fact fixture * test: cover shutdown facts in session state fixtures * test: simplify Android shutdown assertions * fix: preserve Apple shutdown cancellation |
||
|
Michał Pierzchała
|
39cd4d346a |
refactor: route appstate through platform runtime (#1755)
* refactor: route appstate through platform runtime * test: keep appstate capability fixture below complexity limit * test: cover appstate required readiness fact * fix: align appstate facts with boot readiness * fix: keep appstate use declaration minimal * fix: close appstate parity and ownership gaps * docs: record final appstate size accounting * fix: merge neutral runtime imports * docs: align final appstate size totals * fix: remove stale runtime dependency edges * docs: correct appstate size accounting * refactor: keep runtime-use factory internal * docs: itemize runtime-use relocation * fix: move appstate queries into runtime packages * refactor: retire root foreground query paths * refactor: share Android foreground parser ownership * fix: preserve Android appstate parser precedence * docs: keep appstate evidence in review artifacts * fix: keep appstate runtime loading lazy * fix: fail closed for stale limrun appstate * fix: preserve limrun recovery and abort appstate * fix: narrow limrun exact-owner recovery * fix: allocate appstate cutover rule * fix: reconcile appstate with merged main * style: format harmony runtime test * fix: allocate appstate rule id * fix: allocate appstate layering rule * fix: remove stale app command admissions * fix: close appstate layering regressions * fix: align Harmony capability parity with runtime facts * test: cover limrun recovery-only readiness * fix: keep Limrun recovery binding app-log only * fix: parse Android app state in linear time |
||
|
Michał Pierzchała
|
c7565cb1f8 |
refactor(snapshot): clean snapshot ownership (#1754)
* refactor(snapshot): clean snapshot ownership * fix(snapshot): address ownership review feedback |
||
|
Michał Pierzchała
|
eabc936a0f |
refactor: route apps through request runtime (#1756)
* refactor: route apps through request runtime * test: remove stale apps adapter mock * fix: clean apps runtime replay artifacts * fix: remove stale runtime test exports * refactor: simplify apps runtime admission * test: exercise runtime use through facade * fix: close apps runtime admission gaps * fix: disambiguate doctor app inventory callback * fix: keep capability fixture below complexity limit * fix: keep HarmonyOS app inventory fail-closed * test: type HarmonyOS app admission fixture * fix: restore HarmonyOS app inventory parity * test: align HarmonyOS readiness fixture * fix: preserve HarmonyOS doctor app parity * test: type HarmonyOS doctor fixture * fix: isolate HarmonyOS doctor policy * fix: align apps cutover with shared rule catalog |
||
|
Michał Pierzchała
|
b13c06338e |
refactor: route boot through readiness runtime (#1747)
* refactor: route boot through readiness runtime * fix: separate boot admission from readiness * fix: register boot cutover policy * refactor(runtime): move readiness into platform owners * fix(test): tolerate provider temp cleanup races |
||
|
Michał Pierzchała
|
ba48146520 |
refactor(layering): one parametrized runtime-command-cutover gate (ADR 0019 §8) (#1745)
* refactor(layering): fold the four cutover policies into one parametrized gate ADR 0019 §8: the per-command cutover gates consolidate into one parametrized runtime-command-cutover gate driven by a table of migrated commands. Adding a migrated command adds a row; the mechanism carries one planted-red proof instead of one per command. Part of #1739 (wave 0) * fix(layering): scope cutover calls to lexical owners * chore: format cutover ownership model |
||
|
Michał Pierzchała
|
8f98d23f14 |
refactor(layering): give each colliding rule id its own number (#1750)
* refactor(layering): give each colliding rule id its own number
R11 and R13 each named two unrelated rules. report() groups violations by the
rule string and titles every annotation `Layering drift (${rule})`, so a shared
number made the guard's output ambiguous about which rule fired.
Reference counts decided which rule keeps its number. R11 package-boundaries is
named in ~30 places (CONTEXT.md, ADR 0019, testing.md, the mutation and
affected-check configs, four package source comments, its own tests) against one
for the contracts rule; R13 platform-package-substrate is the RULE in three
policy files plus CONTEXT.md, ADR 0019 and model.ts against two for the devices
cutover. Both keepers stay put and the two newest rules move up:
R11 contracts-implementation-authority -> R18
R13 device-inventory-cutover -> R17
R17/R18 follow the namespace's order-of-addition convention (R14 #1701 < R15
#1702 < R16 #1724): device-inventory-cutover landed in #1699 and
contracts-implementation-authority in #1701. #1656 took R19 for
selector-pipeline-ownership on the same reading.
The rule-map header in check.ts is renumbered and reordered back into numeric
order, and gains the R18 entry the contracts rule never had -- without it a
reader looking up an R18 violation finds nothing where they used to find the
wrong rule. deviceInventoryCutoverSummary() was also the only OK-line summary
not leading with its rule number, which is what made the number unreadable from
the success line in the first place.
Also corrects a normative ADR reference. ADR 0019's platform-package import
rules -- contracts-to-platform, sibling-platform, root/daemon, raw-process --
are R13's, as CONTEXT.md:420 already says. The R11 attribution predates
platform-package-policy (#1697, a day before #1699), when R11 was the only
package rule.
* chore(layering): retire the expired R11/R13 collision allowances
KNOWN_RULE_ID_COLLISIONS was opened for exactly the two collisions the previous
commit renames apart, and ruleIdCollisionFailures expires an allowance on
contact: once the collision is gone the entry fails as stale, because a list
still naming it would wave it back through if anyone reintroduced it.
Both entries are therefore deleted in the change that removes the collisions,
leaving the empty list that admits nothing. The namespace is now one-to-one
across R2-R19.
|
||
|
Michał Pierzchała
|
74eab2a554 |
refactor: route selector-resolution structural stages into typed policy (#1744)
* refactor: route selector structural stages into typed policy #1649 landed the per-caller ambiguity matrix and deliberately left four structural columns out: occlusion, off-screen, hittable-ancestor promotion, and the poll budget were per-caller pipeline code, so declaring them would have been an unverifiable claim (nothing consumed them; flipping one left the suite green). This adds the missing half as a table with runners. `SELECTOR_PIPELINE_POLICIES` (src/core/selector-pipeline-policy.ts) gives each caller ONE row naming its ambiguity contract plus its four stages, and every stage is reached only through a runner that reads the row: - occlusion -> selectorPipelineCandidates (candidacy) and resolveSelectorPipelineTarget (refusal). Acting rows exclude covered nodes and refuse covered targets; `find` and the diagnosis probe keep them as candidates and refuse at the target; reads and `wait` ignore them. - promotion -> resolveSelectorPipelineTarget. The per-call-site `promoteToHittableAncestor: boolean` is gone: click/press/longpress name `promotedTarget`, fill/focus/scroll/drag endpoints and the native-ref preflight name `resolvedTarget`. `find`'s below-the-root variant is a declared value rather than a second local helper. - off-screen -> throwIfOffscreenInteractionTarget, which now takes the row and returns the node untouched (no iOS rescue round trip) for observation rows. - poll -> selectorPollBudget, which createWaitPolling derives its deadline and inter-poll delay from; the two wait loops carry a budget, every other row carries none and cannot be polled. Behavior is byte-identical. The acting refusal keeps its exact node, label and details in every branch (promotion declines to retarget away from a covered node, so the "both covered" case names the same node it always did), and `find` carries the occlusion verdict to the focus/type seam rather than raising it early, because find click/fill still delegate that refusal to the interaction leaf's own error shape. selector-pipeline-policy.test.ts drives EVERY row through EVERY runner, including the rows whose answer is "skip" — the half that used to be an absence of code, and an absence cannot fail. Each stage was proven red by flipping its cell (occlusion, promotion, off-screen, poll, plus the declare-only-what-is-enforced guard). The ADR 0011 occlusion/nonHittable `via` pointers for the runtime tree paths now name the runner that makes the decision, not the predicate it applies. Closes #1656; prework for #1739 (waves 4-5). * docs: state constraints instead of narrating the refactor Comment pass over #1656: drop the "used to be per-caller code" / "not module constants" / "rather than an omission" narration — a comment should say what a future edit must respect, not what the previous shape was — and compress the find occlusion-verdict and poll-budget notes to the constraint they actually carry. * refactor: make the selector pipeline the only door to the engine Review of #1744: the structural rows were declared but bypassable. Read and wait routes composed `selectorPipelineCandidates(row, nodes)` with the raw `resolveSelectorChainWithPolicy(..., row.resolution)` and never entered the promotion or off-screen stages, so flipping a read row's `promotion` or `offscreen` changed only the policy unit tests — production `get`/`is`/`wait` were unaffected, which is the unverifiable-column failure #1656 exists to remove. Callers could also pair one row's candidate set with another row's ambiguity contract, and `find list` reached the engine directly. The owning interface (src/core/selector-pipeline.ts) now runs every stage a row declares, skips included, and the stage functions are private to it: - `resolveSelectorPipeline` — single-target rows: candidacy, ambiguity, the replay-guard hook, promotion, occlusion, off-screen. - `listSelectorPipelineMatches` — `reject-candidates` rows, returning the candidate set AND the tree the row sees, so ranking and equivalence classification judge the same nodes candidacy produced. - `runNodePipelineStages` — the node stages for a target from a non-chain matcher (`@ref`, find's fuzzy locator) or a narrowed candidate set. A row whose off-screen stage refuses must supply a refusal shape, so flipping an observation row to `refuse` fails on its real route instead of silently observing. `find list` now names a `readList` row (the new `reject-candidates`/no-rect ambiguity row) instead of calling the engine. R17 selector-pipeline-ownership (scripts/layering/) makes the bypass structurally inexpressible: only the owner may import the engine entry points. Proven against a planted import in selector-read.ts, which the repo-wide scan rejects with the entry points that replace it. Flips now fail through REAL command routes, verified one at a time: readUnique.occlusion/offscreen/promotion and wait.occlusion via get attrs / is / wait; readAny.offscreen via is exists and find; readList.occlusion via find list; promotedTarget.promotion via runtime click. The wait route test needed an advancing clock first — with the frozen one a refused wait spun instead of failing, so the flip hung rather than asserting. * refactor: drop find's dead candidate binding The selector branch bound the row's candidate set and never read it: only the acting classification needs that tree, and find's locator branch brings its own matcher. Names what actually governs the locator target — the shared node stages below, not a candidate set it never had. * refactor: reserve the selector engine behind the pipeline owner Review of #1744 (three blockers). **Listing rows no longer claim stages they cannot run.** `find <q> list` resolves to a candidate SET, so promotion, the off-screen guard and a poll budget have nothing to apply to — a listing has no single element to retarget, keep on screen, or wait for. `readList` now declares only the two stages a listing executes (`SelectorListPolicy`: resolution + occlusion), and the narrower shape is load-bearing: `runNodePipelineStages` and `selectorPollBudget` take the full row, so handing them a listing row is a compile error rather than a silently skipped stage. Pinned with `@ts-expect-error` — widening `readList` makes the directives unused and fails the typecheck. **The engine door is a specifier, not a symbol.** R17's regex could not see a namespace import, a re-export, or a deferred `import()`, none of which mention the symbol it matched. The two engine entries moved to `@agent-device/selectors/engine`, and R19 enforces over the resolved import graph, where every one of those forms is the same edge. Proven on the repo-wide scan by planting each form into a shipped route: namespace import, dynamic import, and `export *` laundering all come back red. `resolveImportEdges` drops an edge whose specifier resolves to nothing, so a specifier rule goes quiet — not red — if the subpath is ever retired. The gate now says that out loud instead of scanning clean. **R19, not R17.** #1750 allocates R17/R18. Verified free against origin/main and that PR's diff, then validated by real merges in both directions: the uniqueness gate passes either way and the three ids stay distinct. The gate itself is new (`scripts/layering/rule-ids.ts`): two branches taking one free number do not conflict in git, so nothing caught R17 twice. Matching whole string literals is what separates a declaration from prose that names a rule, and it is what let the gate see #1750's `const RULE = '…'` shape — the first version missed it and would have been vacuous. `main`'s two pre-existing collisions (R11, R13) are listed as known, not pinned by equality, so #1750 lands in either order without breaking this. Also: the root façade now exposes no resolver at all, and its surface test pins both doors. * fix(layering): make each rule-id allowance expire with its collision Review of #1744: `KNOWN_RULE_ID_COLLISIONS` filtered the exact R11/R13 collision strings, so once #1750 renames those rules apart the entries would keep waving those very collisions through if anyone reintroduced them. "Inert" was wrong — a stale allowance fails open, permanently. `ruleIdCollisionFailures` now checks the transition from both sides: a collision nobody allowed fails, AND an allowance whose collision is absent from the scan fails as a stale allowance. The entry therefore has to be deleted in the same change that removes the collision, and the list burns down to empty, which admits nothing. #1750 is still open, so the transitional entries stay for now (option (b)). Verified against a scratch tree carrying that PR's rename: leaving the list untouched reports both entries as stale; deleting them is clean; and reintroducing `R11 names contracts-implementation-authority and package-boundaries` afterwards is rejected. The last of those is also a unit regression, so the post-transition guarantee is pinned rather than argued. |
||
|
Michał Pierzchała
|
057ab1c82d |
fix(layering): stop double-reporting contracts-authority violations (#1746)
* fix(layering): stop double-reporting contracts-authority violations main()'s violation list spread checkContractsImplementationAuthority(sources) twice, so every R11 contracts-implementation-authority finding was printed and ::error-annotated twice on a red run — inflating the headline violation count and producing duplicate GitHub annotations on the same file:line. Verified by planting a `setTimeout` call in a contracts production source: the rule reported 2 identical violations before and 1 after, with the extra annotation gone. `pnpm check:layering` stays green (136/136 policy tests). Nothing in the suite covers main()'s assembly of the violation list — the policy tests all call their rule functions directly — so neither a duplicated nor a dropped entry there is currently detectable. * test(layering): hold main() to wiring every rule exactly once The duplicate this branch removed survived because nothing enumerates the guard's rules: main()'s violation list is hand-written, and the per-policy tests call their rule functions directly, never seeing the wiring. A lost spread is the dangerous version of the same gap — the rule stops being enforced and the run still prints OK. Make the file's own bindings the oracle: every in-scope `check*` value, local or imported, must be spread into main()'s violation list exactly once. That covers both directions plus a third case — a policy written and never wired in. Fails closed if main() or the array is renamed, so the instrument cannot pass by finding nothing. Test-only rather than an R17 inside the guard: a self-referential rule is defeated by dropping its own spread, which is exactly the failure it exists to catch. Verified by mutating the real check.ts in both directions (re-planting the duplicate, then dropping checkZeroDepJobs) — each turns the run red, and the restored file is green at 143/143. * test(layering): discover layering suites by glob instead of by hand check:layering named its 14 test files one by one, so adding a policy test meant remembering to register it — and twice nobody did. Both halves of the R16 record cutover shipped with tests that have never run: scripts/layering/record-runtime-mechanics-policy.test.ts (2 tests) scripts/layering/record-runtime-registry-policy.test.ts (1 test) Their policies are live in the guard; only the tests were dormant. All three pass, so nothing had rotted — the coverage was simply never being collected. Glob the directory the way mutation:test already globs its own, which makes the filesystem the enumeration and retires the registration step. 143 -> 146 tests, still green. This is the same defect as the duplicate spread this branch opened with, one level up: a hand-maintained list with nothing checking it against reality. * refactor(layering): register guard rules in a keyed table Replaces the AST wiring guard with a construction that cannot express the defect, per review on #1746. The parser was the wrong instrument: it reconstructed one array's shape from TypeScript syntax, so it only recognised top-level function declarations and imports whose local name matched /^check[A-Z]/. A const-defined or aliased rule was invisible to it, a helper named checkX was a false positive, and naming and syntax became part of the interface — all to detect a mistake rather than prevent it. Rules now live in a keyed table over a shared context, executed once via Object.values. An object cannot hold a key twice, so double registration is unrepresentable rather than merely detected, and oxlint's no-dupe-keys rejects the attempt at the source. LayeringRuleId makes a missing key a type error, and LAYERING_RULE_IDS gives the catalog to check exhaustiveness against. Call sites and order are unchanged, so grouped output and the success line are byte-identical. One regression test remains, through the production interface: scripts/ is outside tsconfig.json's `include`, so the Record's exhaustiveness is an editor signal rather than a CI gate, and the catalog assertion is what fails the build when wiring goes missing. Verified by mutation: dropping an entry and registering an uncatalogued one both fail the test, a duplicated key fails oxlint, and re-planting the original contracts violation reports it exactly once. Net -133 LOC. |
||
|
Michał Pierzchała
|
62001cf210 |
refactor(record): derive session recording from the publication lifecycle (#1719)
* refactor(record): derive session recording from the publication lifecycle `SessionState.recordSession` stored an answer the script-publication aggregate already contained. Every writer set both, but nothing made them agree, and #1533 was the consequence: a `--save-script` ingress re-armed the flag behind an ABORTED status, and a bare `close` published a recording the caller had been told was aborted. That fix routed every write through one rule, which made the two agree without making disagreement unrepresentable. The field remained a second source of truth, and its doc comments had to carry the invariant that a type could enforce. Remove the field and derive the answer. `isRecordingPublication` reads recording off the lifecycle: ordinary authoring records only while ARMED; a repair transaction records for its whole lifetime, terminal statuses included. That last clause is deliberately exact rather than merely safe — `armRepairStep` armed the old flag and neither `abortRepair` nor `commitRepair` ever cleared it, so narrowing it would silently stop evidence capture for a committed repair. Whether it should is a real question, and a behavior change, so it is left alone here. What this buys, beyond one less field: - `buildNextOpenSession` and `finalizeOrdinaryCloseScript` make no recording decision at all now, so no surface can arm recording without moving the lifecycle that authorizes it. - The writer's publication gate is answered entirely by the aggregate. Its separate ABORTED check is gone: a terminal authoring lifecycle is already not recording, so one question replaces two that could disagree. - The R7 ownership ratchet drops from 23 writer-owned fields / 29 owner claims to 22 / 26, and the layering manifest loses the entry whose comment documented the smell ("deliberately set on its own by paths that record without arming a publication"). Behavior-preserving: the derivation reproduces what the flag held at every transition. The test fixtures that armed `recordSession` with no publication state described a shape production stopped producing at #1478; they now carry the lifecycle that causes recording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PFW9gJqz1wEHoowkdd1nFW * test(close-script): flush queued event-log writes before removing the tmp root CI failed the Coverage lane with ENOTEMPTY removing the test's tmp root, in `afterEach` rather than in an assertion. `SessionStore.recordAction` QUEUES its event-log append (`queueEventLogWrite`) instead of writing it, and every close path in this file records an action. Nothing awaited that write, so `fs.rmSync(root, {recursive: true})` could race it: the pending append recreates `<root>/sessions/<name>/` while rmSync is walking, and the final rmdir fails ENOTEMPTY. It needs CI's parallel load to lose the race — the file passes 12/12 in isolation locally. Await `flushSessionEventLogWrites()` before removing. The hazard is latent in any test that records actions and then removes its tmp root; this fixes the file that failed rather than sweeping the pattern, which deserves its own change. Not added to the #1419 contention-retry list: that list requires a concrete spawn/wait mechanism named per entry, and this file has none. The race was a real teardown bug, not lane contention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PFW9gJqz1wEHoowkdd1nFW * docs: correct ADR 0016 on recording vs publication for repair Review caught a real overstatement. The amendment claimed evidence capture and publication authorization are "the same question asked of the same state". That holds for ordinary authoring — ARMED both records and publishes, ABORTED and PUBLISHED do neither — but not for repair: `isRecordingPublication` is true for every repair status including `committed` and `aborted`, while the writer additionally applies `isRepairArmedWriteBlocked`, refusing a committed transaction and one that is not yet committable. State it as it is: both decisions derive from the same aggregate, but they remain distinct predicates, and collapsing them would republish a committed repair or commit an incomplete prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PFW9gJqz1wEHoowkdd1nFW --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
1b2e786128 | refactor: move screen recording onto platform runtime (#1724) | ||
|
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
|
10ff339d14 |
refactor: declare selector resolution policy as data (#1649)
* refactor: declare selector resolution policy as data (#1630) Five native consumers of "resolve a selector against the screen" each hand-declared their ambiguity contract as inline requireUnique/ disambiguateAmbiguous literals, so the repo's real policy matrix was only discoverable by reading four files. SELECTOR_RESOLUTION_POLICIES (packages/selectors) now declares one row per caller — ambiguity kind plus the structural columns (rect, occlusion, off-screen guard, promotion, poll) — and selectorResolutionKnobs turns a row into the engine knobs it stands for. Callers consume rows; zero ambiguity literals remain in src. Semantics are unchanged by construction: each row was read off its call site. The matrix names what was previously implicit — act and get text disambiguate, is/get attrs fail closed, exists/find-reads and wait take the first match, mutating find rejects candidates unless narrowed (#1625). `reject-candidates` is declaration-only and rejected by selectorResolutionKnobs at the type level, because find enforces it through its own narrowing rather than engine knobs. resolution-policy-parity.test.ts gate-tests the matrix against the callers (ADR 0011's declared-plus-gate-tested pattern): knobs must match the named ambiguity contract, every claimed structural column must appear in the caller's source, the read/wait pipelines must genuinely lack the machinery they disclaim, and no caller may reintroduce an inline literal. Verified revert-sensitive: flipping readUnique to disambiguate and faking wait's occlusion column each fail it. Out of scope, unchanged, per the issue: the Maestro engine (ADR 0015) and the open click-implicit-wait product decision. * refactor: route wait and mutating find through the policy interface (#1649 review) P1 was right: the first head declared seven rows but genuinely routed five. selector-wait.ts never imported its row (it called listSelectorChainMatches directly), findAct consumed only requireRect while its ambiguity contract stayed bespoke, and the parity test sniffed marker strings in source files — so it stayed green across exactly that gap. Asserting about the layer I had edited instead of the behavior it produces. resolveSelectorChainWithPolicy is now the one policy-driven entry: it returns a discriminated outcome (none / resolved / ambiguous) because the rows genuinely disagree about what several matches mean, which is what previously forced each caller to re-derive its contract inline. wait and find's selector branch both route through it; find additionally asserts its row still says reject-candidates rather than assuming. The parity test is rebuilt on fixture trees driven through that interface — no source sniffing. Wiring verified revert-sensitive: flipping the wait row fails the policy tests, and flipping findAct fails REAL find handler tests (ambiguous-candidate listing), which is the proof the previous version could not produce. One behavior nuance the fixture work surfaced and now pins: disambiguation declines on genuinely indistinguishable candidates (the tiebreak is evidence, not a coin flip), so an acting row surfaces ambiguity there rather than binding one silently. * fix(test): let fallow see the host-process mock helper's real consumers Rebase onto main brought #1642's host-process-mock.ts into this PR's fallow scope, where its export reports as unused. It is not: three suites consume it, but only through `(await import(...)).pinOwnProcessStartTime` inside vi.mock factories — vitest hoists those above static imports, so the dynamic form is required and fallow cannot trace it statically. Documented suppression rather than a restructure that would break the hoisting contract. Latent on main rather than introduced here: the audit gate is changed-files-only, so main sees the file in scope only from a PR whose diff contains it. * fix: keep every candidate when a policy resolves one winner (#1649 review P1) A real regression I introduced, not a test gap: routing wait through the policy interface collapsed the candidate set to the winner, and the #1349 landmark check is satisfied when SOME match carries the recorded identity. A first same-selector impostor therefore hid a later genuine landmark and timed the wait out. The resolved outcome now carries `matchedNodes` — the full candidate set of the alternative the winner came from — so a policy that picks one node no longer throws the rest away. wait passes that straight to the landmark check, restoring the original semantics. Regression test added at the within-one-poll shape the existing suite did not cover (both candidates in the SAME capture, impostor first); verified it goes red against the singleton reconstruction it replaces. * refactor: declare only the policy fields the matrix enforces (#1649 review) The occlusion / offscreenGuard / promotion / poll columns were never consumed by resolveSelectorChainWithPolicy or selectorResolutionKnobs: changing any of them left behavior and the suite green, so they were unverifiable claims that read as truth. (My earlier source-sniffing test "verified" them by grepping caller files for marker strings — which is why it also stayed green when a row was disconnected entirely.) The matrix now declares exactly what it enforces: the ambiguity contract and the rect requirement, both consumed by the resolution interface and pinned behaviorally. A new test asserts every row's field set, so an unenforceable column cannot reappear without coverage — verified by re-adding one and watching it fail. Routing the structural stages into typed behavior is tracked in #1656 with the constraint that each field must be consumed, not merely declared. * fix(selectors): flatten the policy outcome at the package boundary `PolicyResolutionOutcome.resolution` was typed as `AstSelectorResolution` and the root façade returned it unchanged, so the parser AST #1589 confined to `@agent-device/selectors/ast` came back through a nested field. `selector-wait.ts` reading `outcome.resolution.selector.raw` was the runtime proof. The existing boundary gate reads exported *names*, so it could not see this. The public outcome now lives beside `SelectorResolution` in public-resolution-types.ts with its selector as text; the parser-side shape is renamed `AstPolicyResolutionOutcome` and stays package-private, and the façade wrapper flattens on the way out — the same treatment `resolveSelectorChain` already gave `AstSelectorResolution`. Two new pins, both verified red against the shape they replace: a behavioral one asserting the façade returns selector text under every policy row, and a structural one asserting resolution shapes are re-exported from public-resolution-types.ts rather than from a parser-side module — which is what distinguishes the leak from a correct re-export in a name list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Rva4YGtSCAKJqH5PbpcCU --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
d85072d935 |
perf(cli): route command aliases through the help fast path (#1641)
* perf(cli): route command aliases through the help fast path bin.ts's `--help` fast path resolved aliases through a hand-written two-entry table that had drifted out of sync with the real CLI_COMMAND_ALIASES registry (five entries). `tap`, `launch`, and `relaunch` missed the table and silently fell through to a full runCli() bootstrap just to print static help text (~150-165ms vs ~45-50ms for aliases already in the table). Delegate to the shared normalizeCliCommandAlias registry instead of the stale local table, so every alias the registry knows about gets the fast path automatically. * test(cli): add R12 layering guard for bin.ts's alias delegation The unit test added for the alias fast-path fix (cli-help-alias-fast-path.test.ts) calls normalizeCliCommandAlias directly, so it stays green even if bin.ts itself reverts to a hand-rolled table — it pins the registry composition, not bin.ts's own wiring, and bin.ts cannot be safely unit-imported (it runs unguarded top-level dispatch on import and is deliberately excluded from coverage). Add an AST-based structural guard instead, in the style already established by scripts/layering/session-state.ts, facade-exports.ts, and zero-dep-jobs.ts (oxc-parser's module/program records, not a line scan, so a fixture's string literal can't produce a false hit). R12 asserts two facts about src/bin.ts: it holds a value import of normalizeCliCommandAlias from commands/cli-command-aliases.ts, and it contains none of the registry's own alias tokens as string literals. The token list is read out of the registry's own source (CLI_COMMAND_ALIASES's `alias:` property values), not hard-coded, so a future sixth alias is covered automatically. Both facts were false on the pre-fix bin.ts, verified by reverting locally and capturing the failure before restoring the fix. Wired into the existing check:layering chain (already part of check:tooling), next to R7's session-state ownership rule, which pins the same "delegate to your single owner" shape. * test(cli): pin the alias-resolver call into buildCommandUsageText (R12 P2) Maintainer review of R12 (PR #1641): import-presence and literal-absence alone let bin.ts regress to buildCommandUsageText(helpTarget) while the normalizeCliCommandAlias import stays in place, used harmlessly elsewhere (or not at all) — the real-tree gate stayed green through that exact regression. Add a third fact: bin.ts's call to buildCommandUsageText must receive, as its argument, a call to the LOCAL binding the resolver was imported as (aliasResolverLocalName + usageTextCallsResolver, both AST-based). Binding by local name rather than the literal export name means a renamed import (`as resolveAlias`) still verifies, and an unrelated same-named local cannot be mistaken for it. Verified by reverting locally to exactly the missed regression — import left in place, call reverted to buildCommandUsageText(helpTarget) — and confirming R12 now fails where the two-fact version passed; restored after. Two negative fixtures pin the scenario going forward: import present but unused, and import present but used only unrelated to the call. * test(cli): make R12's delegation fact universal and value-bound The previous fact 3 asked whether *any* `buildCommandUsageText(resolver(...))` existed in bin.ts. That quantifier is satisfied by a decoy call while the line that actually ships resolves nothing: void buildCommandUsageText(normalizeCliCommandAlias('open')); const commandHelp = buildCommandUsageText(helpTarget); Fact 3 now requires EVERY `buildCommandUsageText` call to receive the imported resolver applied to the fast path's own help-target binding, which rejects both lines above independently. The help-target name is read from bin.ts (the variable initialized by `resolveSimpleHelpTarget`), so renaming it re-points the guard instead of disarming it. Because fact 3 claims binding identity by name, it also now rejects a local shadow of the resolver and an ambiguous second help-target declaration — a same-named local would otherwise let the composition read as delegation while calling something that resolves nothing. The predicate returns the reason rather than a boolean, so the gate names which of the several distinct failures happened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Rva4YGtSCAKJqH5PbpcCU --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
d5f11f6e2f |
refactor: import package types directly — no internal re-export laundering (#1640)
* refactor: import package types directly instead of re-exporting from internal modules
Post-#1636 review feedback: internal src modules were re-exporting package
types (export type { X } from '@agent-device/...'), giving one declaration
several import paths and hiding its provenance. New rule applied repo-wide:
internal modules import directly from the owning package; only published
entry surfaces (src/sdk/* entries, client-types, finders, metro composition,
remote-config-schema) may re-export.
Eleven internal re-exports removed and ~110 import sites redirected to the
packages, the big two being CommandFlags (core/dispatch chain, 39 sites) and
SessionAction (daemon/types.ts, 19 sites). Two were already dead
(RefFrameEffect via daemon-command-registry, DiffSnapshotCommandResult via
capture/runtime/snapshot). Entry-surface chains now re-export from the
package rather than laundering through a second internal module
(client-types/client-metro MetroBridgeScope).
Side effect: the R9 type cycle shrinks again, 49 -> 47 (daemon-server
19 -> 17); ceilings lowered to match.
* refactor: drop command-schema's CliFlags re-export (#1640 review P2)
The one consumer (cli/parser/args.ts, a multi-line import the sweep's
single-line scan missed) now imports CliFlags from contracts/command;
FlagDefinition/FlagKey stay — they are src-declared types, not package
laundering.
|
||
|
Michał Pierzchała
|
d5f99bab1c |
refactor: sink backend.ts's cycle-closing types below both zones (#1632) (#1636)
backend.ts imported RepeatedInput up from commands/command-input.ts and ScreenshotResultData up from utils/screenshot-result.ts — the interface hub typed in terms of the zones that depend on it, R6's textbook inversion shape. - RepeatedInput now lives in @agent-device/contracts/interaction; command-input.ts re-exports it for its existing importers. - ScreenshotResultData already had a byte-identical canonical declaration in contracts/snapshot-types.ts (exported via contracts/capture); the utils copy is now a re-export of it, deleting the duplicate outright. Measured member-by-member: the R9 type cycle collapses 76 -> 49 files. backend.ts, runtime-contract.ts, commands/runtime-types.ts, and commands/runtime-common.ts all leave the component (27 files stranded out at once); zone ceilings lowered to the measured values (commands 33 -> 14, platforms 7 -> 2, root 5 -> 3, daemon-server 20 -> 19) and CONTEXT.md's hub list recomputed (core/dispatch.ts 8, command-catalog.ts 7, resolution.ts 6, command-descriptor/registry.ts 6). No TYPE_INVERSION_BASELINE additions. |
||
|
Michał Pierzchała
|
d919876cb0 |
refactor(daemon): one interface for the deferred interaction outcome (#1633)
* refactor(daemon): one interface for the deferred interaction outcome (#1629) The machinery answering "did that mutation actually take effect?" was three modules coordinated only through raw SessionState fields: two independent marking sites (finalizeTouchInteraction vs dispatchGenericCommand, plus a third in session-open), a resolve side buried as private functions in snapshot-capture.ts with no direct tests, freshness heuristics split across the seam, and two raw reads of session.postGestureStabilization outside the owning module. - src/daemon/deferred-interaction-outcome.ts is now the one interface: markDeferredInteractionOutcome (every mutating route, one ordering) and resolveDeferredInteractionOutcome (every snapshot capture, parameterized over the capture primitive so it is directly testable). - getAndroidFreshnessReason moves beside its state machine in android-snapshot-freshness.ts, with the module's first direct test file. - isPostGestureStabilizationPending replaces the raw field reads in direct-ios-selector.ts and selector-capture-runtime.ts. - snapshot-capture.ts shrinks 617 -> 359 lines and keeps only capture, state building, and scope resolution. - No behavior change. The R9 type cycle drops 76 -> 74 files; zone ceiling lowered accordingly. CONTEXT.md gains the "deferred interaction outcome" term. * style: format android-snapshot-freshness.test.ts * fix(layering): record the honest R9 delta — the new module joins the cycle (+1 node) The earlier 76 -> 74 measurement was an artifact: the layering scan reads tracked files, and deferred-interaction-outcome.ts was still untracked. The real delta vs main is 76 -> 77 / daemon-server 20 -> 21: the choke point sits inline on value paths that already ran member-to-member, so the cycle gains one node and zero new edges. Ceiling raised explicitly with the rationale at the baseline, per the R9 rule's own escape hatch. * refactor(daemon): host the deferred-outcome seam in the stabilization owner (#1633 review) Zero R9 growth, per review: a NEW aggregator file cannot stay out of the cycle by shedding type imports — its value imports of the two member owners close the loop regardless. The unique zero-growth host is an existing cycle node, and the stabilization owner is the only legal one (the policy module cannot value-import stabilization back, R4; the freshness module would join as a new member). So deferred-interaction-outcome.ts absorbs the post-gesture-stabilization implementation and becomes the R7 owner of postGestureStabilization: the seam lives in a node that was already on the member-to-member paths it concentrates. - markPostGestureStabilization is now module-private behind markDeferredInteractionOutcome; stabilization marking tests drive the public interface. - The freshness retry loop moves beside its classifier in android-snapshot-freshness.ts; gesture-no-effect helpers move to a leaf file. Both stay outside the cycle. - Ratchet reverted to main's exact values (76 files, daemon-server 20) — measured member-by-member vs main: the diff is empty. * refactor(daemon): extract the pure stability loop into a leaf (#1633 review) The deferred-interaction-outcome owner keeps the seam, the pending-record ownership, and the R7 clear; the quiet-window polling loop and the baseline-distrust verdict move to post-gesture-stability.ts, parameterized by hooks (capture, surface reader, the three signature comparators) so the leaf imports no cycle owners and no SessionState — verified outside the R9 cycle, which stays at main's exact 76/20. Owner drops 539 -> 374 lines. The no-effect corroboration keeps comparing against the ORIGINAL pre-gesture baseline (never a mid-loop rebased one), now stated in the leaf's doc. The stabilization loop suite drives the unchanged public adapter; the verdict suite wires the real classifier through the leaf's hook. |
||
|
Michał Pierzchała
|
d8b309c6db |
refactor(contracts): name façade exports explicitly and retire the pin table (#1614)
* refactor(contracts): name façade exports explicitly and retire the pin table Thirteen of the fourteen `@agent-device/contracts` façades were bare `export *` barrels. `facades/snapshot.ts`, added by #1582, was the one exception — explicit named re-exports — and that is now the rule. Everything #1574 built to cope with `export *` goes with them: scripts/layering/facade-symbols.ts -980 (816 pinned names) scripts/layering/facade-exports.ts -192 (readFacadeExports) scripts/layering/facade-exports.test.ts -234 (star semantics) scripts/layering/package-boundaries.test.ts -55 `readFacadeExports` re-implemented ESM `GetExportedNames`/`ResolveExport` — star-chain resolution, ambiguity rejection, diamond binding identity, cycle guards, spec-accurate `default` filtering at the star rather than the source. All of it existed to enumerate what `export *` hides. 523 of the 816 pinned names belonged to contracts, i.e. to those thirteen files. Once a façade names its exports, the façade file IS the pin, and it is visible in the diff of the file that widened rather than in a separate table a reviewer has to cross-check. `readNamedExports` (20 lines) stays and is enough: it already throws on bare `export *` and on `export default`. The pin is replaced by one structural gate — no façade may contain a bare star — which reuses that rejection rather than adding a regex. Surface equivalence verified independently, not asserted: main's own `readFacadeExports` run over the new façades, compared against main's own `FACADE_SYMBOLS` table — 31 subpaths, 0 added, 0 removed. Red evidence for the new gate: planting `export * from '../request-progress.ts'` back into facades/progress.ts fails it with the file named and the reason quoted; 12 pass / 0 fail once reverted. Not included: the `lowerAndroidTouchPlan` tuple-assertion drive-by. It needs `sampleGestureOffsets` to carry a min-arity tuple through `.map()`, which TypeScript will not infer without a typed helper — a real change to the gesture-plan contract rather than a drive-by, so it stays out. * test(layering): assert façades stay exhaustive over their sources Review on #1614 caught this conversion silently narrowing the public surface. The explicit lists were generated against the surface at fork time; #1567 landed 13 exports meanwhile — `DragOptions`, the drag-gesture vocabulary (`COORDINATE_GESTURE_KINDS`, `CoordinateGesturePayload`, the three `DEFAULT_DRAG_*` constants, `DragGestureInput`, `DragGesturePayload`, `GestureCommandInput`, `buildDragGesturePlan`, `dragGesturePayloadFromPositionals`, `normalizeGestureCommandInput`) and `MultiTargetAnnotationV1`. The `export *` barrels had been forwarding all 13 automatically; the rebase dropped every one, and only a human diff caught it. The star-rejection gate could not: it only proves a façade does not WIDEN invisibly. Narrowing is the failure an explicit list newly makes possible, because `export *` could not narrow by construction. So the property the stars gave for free is now asserted directly — every name a re-exported source declares must appear in the façade. Scoped to `packages/*/src/facades/`, the barrels this PR converted. A hand-curated package `index.ts` is a different thing: `ad-replay` deliberately publishes two values out of a much larger `internal/`, and forcing exhaustiveness there would widen a surface its owner narrowed on purpose (#1555). A source that itself carries a bare `export *` is skipped — unknowable from that file alone, and reachable because the façade re-exports the starred module directly too, which IS checked. Red evidence: dropping `MultiTargetAnnotationV1` from facades/replay.ts — one of the 13 the old gate was blind to — fails with the file, the source and the symbol named. 13 pass / 0 fail once restored. * fix(layering): close the exhaustiveness gate's starred-source hole Two review findings, plus a third the gate caught on itself. P1 — the three `DEFAULT_DRAG_*` constants join the existing public-façade suppression, alongside `COORDINATE_GESTURE_KINDS` and `normalizePublicGesture` which the same conversion surfaced. All five are #1567's drag vocabulary, made individually visible to `--production` analysis for the first time because a bare star used to hide them from that exact check. Kept rather than narrowed, for the reason the existing entry already states: the façade's surface stays byte-identical to what the retired pin table asserted, and narrowing is a follow-up with its own review. P2 — the exhaustiveness gate skipped any source carrying a bare `export *`, which dropped that module's DIRECT exports from the check too. `gesture-plan.ts` stars `gesture-plan-types.ts`, so removing `buildDragGesturePlan` from the façade narrowed the public surface and still passed. `readDirectNamedExports` now reads exactly the names a module declares or re-exports BY NAME and ignores the star, so direct exports are checked while the starred set stays covered by the façade's own direct re-export of that module. Red evidence: removing `buildDragGesturePlan` from facades/interaction.ts now fails naming file, source and symbol; 13 pass / 0 fail restored. Third, and the reason the gate is worth having: rebasing onto main after #1612 merged silently dropped `TEXT_ENTRY_ROUTES`, `TextEntryRoute` and `TypeTextBackendResult` from the interaction façade — the same narrowing class as the #1567 one review caught by hand, one merge later. The gate failed on it before CI did. Restored. |
||
|
Michał Pierzchała
|
ee473b6adc |
refactor(daemon): give the Maestro fallback and ambiguous-match details real types (#1612)
Three places smuggled structured data through untyped bags and re-read it
with runtime guards. Each gets an explicit typed boundary.
A. The resolution-suppression rule was encoded twice in
interaction-touch-response.ts — a spread ternary in the runner-payload
branch and an unconditional destructure used conditionally in the runtime
branch, with the ADR 0012 rationale living on only one source variant.
Both branches now read one `suppressesResolutionDisclosure(source)`
predicate through one `applyResolutionDisclosurePolicy` helper, where the
reason is stated once. The union field is renamed
`maestroCoordinateFallbackDispatched` (the dispatch path that ran) and
hoisted into a shared base. handleFillCommand's two-arm interactor.fill
call collapses to one.
B. `Interactor.type` narrows from `Record<string, unknown> | void` to
`TypeTextBackendResult | void`; the Apple runner boundary is the single
place the wire payload becomes that type. `maestroFallbackDetails` returns
a typed `{ used, extra }` instead of a bag both call sites re-read.
C. `details.candidates` meant two incompatible things. The device-domain
resolvers now key their list `devices`, so the shared renderer drops its
shape-disambiguation guards and the device list actually renders.
|
||
|
Thiago Brezinski
|
a13a6832ee |
feat: add selector-targeted drag gestures (#1567)
* feat: add selector-targeted drag gestures * fix: address drag gesture review feedback * fix: satisfy drag review quality gates * fix(android): lower drag trajectories piecewise * test(replay): validate drag fixture selectors * fix(ios): ignore full-viewport chrome containers * test(drag): prove destination on live devices |
||
|
Michał Pierzchała
|
8ba5f9b8de |
fix: surface AMBIGUOUS_MATCH candidates and name find's supported actions (#1602)
* fix: surface AMBIGUOUS_MATCH candidates and name find's supported actions (#1597) AMBIGUOUS_MATCH errors now list the matching candidates (ref, role, label/identifier) rendered the same way as snapshot -i lines, capped at 5 with a "+N more" marker. buildAmbiguousMatchError (the single producer, src/daemon/handlers/find.ts) reuses formatSnapshotLine to build the list; formatAmbiguousMatchCandidateLines (src/utils/output.ts) renders it unconditionally on both text surfaces an agent actually reads (CLI printHumanError and MCP formatToolErrorText) — previously the candidates lived only in details, which neither surface printed. find's "Unsupported find action: X" (e.g. from `find <text> press`) now attaches a hint naming every action find actually supports and the two-step recovery shape: run find "<text>" to resolve the ref, then dispatch the gesture as its own command (press @eNN). The hint is a single exported constant (UNSUPPORTED_FIND_ACTION_HINT) shared by both throw sites — packages/selectors' raw-token parser and the CLI's typed reader (src/commands/interaction/selectors.ts) — so they can't drift. Matching semantics are unchanged; ambiguous rejection stays by-design. The help-conformance corpus's AMBIGUOUS_MATCH quiz is updated: its premise ("candidate refs were not shown") no longer holds, but with 3 identically-labeled candidates the lesson (don't guess a specific ref) still holds. * fix: guard the AMBIGUOUS_MATCH candidate renderer against device-domain shapes Review on #1602 (P2): formatAmbiguousMatchCandidateLines ran for every normalized error and stringified details.candidates unconditionally, but device-domain AMBIGUOUS_MATCH/APP_NOT_INSTALLED errors (findBootedAppleSimulatorWithApp, src/core/dispatch-resolve.ts) reuse that key for { id, name } device objects with no `matches` field — CLI and MCP would have printed "Candidates: [object Object]" for those. The renderer now requires numeric details.matches AND every candidate to be a string before rendering anything, restricting it to buildAmbiguousMatchError's element-match shape; unrecognized shapes render nothing, same as before this feature existed. Added regression tests against the exact device-error shape on both text surfaces. Also unexports AMBIGUOUS_MATCH_CANDIDATE_LIMIT (fallow flagged it as an unused production export) — it has no consumer outside find.ts. |
||
|
Michał Pierzchała
|
74efcbbd84 |
fix(snapshot): one-shot recovered warning for internally armed penalties (#1590)
* fix(snapshot): one-shot recovered warning for internally armed penalties The deferred-capture suppression assumed the capture that armed the XCTest-channel penalty already rendered the full 'overly complex or slow accessibility tree' warning. Internal captures (selector resolution, settle observation loops, system-modal probes) can arm the penalty without any user-facing render, leaving the next public snapshot with only the structured verdict and no CLI warning line. The runner cannot tell user-facing from internal captures, so the daemon now holds a per-session one-shot latch (snapshot-quality-latch.ts) applied at the snapshot/diff response seam: a genuine recovered render sets it silently, the first public 'deferred' verdict without the latch re-renders the full warning once and sets it, a healthy public verdict clears it (the penalty window is over), and an app switch supersedes it. Internal observation responses (observationOnly) neither consume nor clear the latch. Follow-up to PR #1587 review (non-blocking hardening). * chore(layering): declare the deferred-warning latch owner and ratchet baseline The R7 session-state gate requires every SessionState field to have a declared writer owner: recoveredSnapshotWarningLatch is owned solely by snapshot-quality-latch.ts (matching the field's 'managed only through' contract), and the R10 pressure baseline grows deliberately to 23 writer-owned fields / 29 owner claims. Also oxfmt-formats the new latch test. * fix(snapshot): latch on the captured verdict, not the retained session snapshot Review P2 on #1590: the latch seam read a diff capture's verdict back from session.snapshot, but an empty ref-scoped capture deliberately retains the previous stored snapshot (shouldKeepCurrentSnapshot) — so a deferred capture could consult a retained healthy verdict, clearing the latch and omitting the one-shot warning. The daemon snapshot backend now fills a per-request CapturedSnapshotQuality slot on every capture, and the seam latches on that just-captured verdict for both snapshot and diff. New production-path regression: an empty ref-scoped diff over a retained healthy snapshot with a deferred capture warns once (verified red against the previous seam). * test(snapshot): pin app-switch latch supersession through the dispatch seam Cross-vendor review follow-up: the app-switch transition was pinned only at the pure-function level; a regression in how the seam keys the latch by the session's appBundleId would not have been caught. Two-dispatch integration test: latch held for app A, bundle switched, app B's first deferred verdict warns once and rekeys the latch. |
||
|
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
|
eb3fc5b28d |
chore: scan packages/** with fallow instead of ignoring it (#1591)
`ignorePatterns: ["packages/**"]` landed in #1494 W0 with the recorded reason "its resolver cannot follow workspace specifiers". That was either wrong at the time or never re-checked: the fallow version has not moved (^2.95.0 then and now) and it resolves @agent-device/* through each package's exports map today. packages/kernel alone exposes 8 subpaths and ~110 exports reachable only via workspace specifiers, and scanning it reports zero findings — a resolver that could not follow the specifier would report all of them. The cost of the ignore is that every package extraction silently removes its code from dead-code analysis. #1589 moved the selector engine into packages/selectors/ and shipped a façade with 15 zero-consumer exports, including `selectorUsesKey`, written in that PR and never called. A follow-up commit removed them by hand; nothing would have caught them. Removing the pattern surfaced 43 findings, driven to zero by deleting the dead code rather than by baselining or excluding it (fallow-baselines/*.json are empty on purpose — the posture is fix-or-document-the-exemption, so a first baseline entry would be a policy change): - 38 are deleted. 24 façade type re-exports whose only claim was that a consumer might one day want to name them — typecheck is green without every one, so the claim was theoretical; 5 façade value re-exports; 9 `export` keywords on symbols used only inside their own file. Every deleted façade symbol comes off scripts/layering/facade-symbols.ts (and ad-replay's inline pin in package-boundaries.test.ts) in the same change, so R11 is narrowed with the façade, never weakened around it. - 4 stale suppressions in src/provider-limrun-runtime.ts existed only because packages/ was invisible. - 5 have consumers analysis genuinely cannot see, and get an `ignoreExports` entry naming the consumer per the existing `comment` convention: four test-tree importers that --production does not walk, and `LimrunIosCommandExecution`, which src/sdk/limrun.ts republishes as agent-device/limrun — its only importer compiles in a temp checkout, so no static edge reaches it. test/integration/limrun-public-types.test.ts is the standing proof that one is real API. Three doc comments named types their façade no longer exports and are corrected rather than left asserting something false — including #1555's claim in session-replay-target-verification.ts that the daemon imports `AdReplayVerifiedTargetGuard` directly. It does not; it reaches that shape through `AdReplayTargetClassification`/`AdReplayDispatchGuard`, which is why the name read as dead. `scripts/maestro-conformance/**` was ignored wholesale to cover its corpus data. Narrowed to `corpus/**`, which un-hides the tooling beside it and turned up one more file-local export (`buildManifest`); regenerate.mjs's importer of `fixtureContentHash` becomes visible, so that needs no exemption at all. scripts/check-affected/model.ts deliberately did not select the `fallow` check for packages/*/src/**, carrying the same stale rationale as a comment. Without that selection the new scope would never run in the affected-driven lane, so the ignore removal would have bought nothing. model.test.ts now pins the selection. Verified: check:fallow and check:production-exports green with packages in scope; full-repo `fallow dead-code` back to its one pre-existing finding; typecheck, layering (R11), lint, format, build, check:package, and the limrun published-types integration test all pass. Probed by adding a fresh zero-consumer export to the xml façade — check:production-exports reports it, so the #1589 case now fails the gate. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
bcaa106845 |
refactor: extract snapshot and replay identity semantics (#1582)
* refactor: extract snapshot and replay identity semantics * refactor: move pure rect primitives from contracts to kernel/rect containsPoint, pickLargestRect, and isRectVisibleInViewport are raw rectangle arithmetic with no snapshot awareness, so they belong beside rectContains/rectArea in @agent-device/kernel/rect rather than in the snapshot-semantics vocabulary. The node-aware resolveViewportRect folds into contracts/snapshot-visibility.ts, retiring snapshot-geometry.ts; after this split, everything behavioral in @agent-device/contracts/snapshot is policy that interprets the snapshot model. * refactor: restore ADR-0012 rationale docs and dedupe replay identity shapes The #1478/#1581 extraction moved the identity/structural helpers but compressed their invariant documentation to one-liners; the deliberate no-ancestry-exclusion rule on idMatchCountInTree, the fail-closed guard comparison, and the who-throws/who-detects contracts on the two divergence reason markers now travel with their definitions again. LocalIdentity and NodeStructuralDenotation move to contracts/target-annotation.ts (beside TargetAncestryEntry, which WaitLandmarkMismatchEvidence now references directly), so the guard shapes in contracts/replay.ts are nominal instead of hand-rolled structural twins; ad-script re-exports the vocabulary beside the readers that produce it. Also inlines the demoteNonUniqueId pass-through wrapper in session-target-evidence.ts. * style: fix oxfmt formatting in snapshot-visibility * refactor: move findSnapshotAncestor into contracts/snapshot-tree The last root value import from src/selectors: predicates.ts reached src/snapshot/snapshot-processing.ts for the ancestor walker. The walker is index-based tree traversal with no presentation policy, so it joins buildSnapshotNodeMap in contracts/snapshot-tree.ts; both consumers repoint to the façade and the non-contiguous-index/cycle coverage moves to the package test. src/selectors now has zero value imports from root src in production files. |
||
|
Michał Pierzchała
|
56d9ee605c |
fix(ios): preserve timed pan duration (#1572)
* fix(ios): preserve timed pan gesture execution * fix(gestures): encode linear pans as endpoint plans * fix(ci): pin wait contract exports * fix(android): lower endpoint gesture plans for touch transport * fix(gestures): preserve timed pan duration across adapters |
||
|
Michał Pierzchała
|
016577e6bc |
docs: remove superseded architecture proposals and design prototypes (#1478 P7) (#1580)
P7 cleanup per #1478's ratified defer decision: delete the daemon-modularity proposal, module-interface-principles (durable kernel folded into CONTEXT.md), the pre-package Maestro debt map, and the daemon-boundary prototypes with their package scripts; refresh CONTEXT.md's R10 bullet to post-arc state. Also reconciles the interaction façade symbol pin with #1570's wait symbols, which had left check:layering red on main via #1570 × #1574 branch-race skew. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
83322a3f2f |
test(layering): pin exact façade symbols for all workspace packages (#1574)
* test(layering): pin exact façade symbols for all workspace packages #1555 added the repo's first exact exported-symbol gate, pinning @agent-device/ad-replay's named export list. Every other workspace package was still covered only by the exports-subpath locks, which prove which files a package exposes but say nothing about what those files name — so any façade could grow a symbol silently. Pin all 29 exported subpaths across the remaining 8 packages: ad-script, contracts (14), kernel (8), maestro, provider-limrun, provider-webdriver, replay-test, and xml. The lists are the honest current surface, untrimmed — contracts/interaction alone names 140 symbols, and pinning the real number is what makes the next widening visible. The table is checked in both directions, so a new package or subpath that nobody pinned fails rather than being silently skipped. Pinning contracts needed the export-discovery helper widened: 13 of its 14 façades are bare `export * from '../x.ts'` barrels, and readNamedExports throws on those by design, because given only a source string the contributed set is genuinely unknowable. Given the FILE it is not, so readFacadeExports resolves the relative re-export chain and enumerates it. Resolution stays narrow — a package-specifier star still throws (that would mean re-entering another package's exports map, the unbounded widening the gate exists to refuse), cycles are visit-guarded, and a default export still throws through a barrel. Helper unit tests cover the shapes the merged AST scan handles but left unpinned: `export { default as x }` (the form between the two rejection rules — named, so reported, never `default`), a local `export { … }` list with no `from`, and multi-declarator `export const a = 1, b = 2`. Plant-verified per package rather than asserted: a stray export on ad-script, one two files deep behind contracts' `export *` chain, and an unpinned new subpath on xml each failed with a named diff; each reverted to green. Gates: check:layering (63 tests, up from 53) / typecheck / lint / format:check — green. * fix(layering): model real `export *` semantics; split the pinned table out Addresses both P1 findings on #1574. P1 — `readFacadeExports` did not model `export *` façade semantics. It unioned every child name and threw on every child default. Both are wrong: - Per GetExportedNames, a star export excludes the child's `default`, so a private `export default` in a leaf is not reachable through the barrel and does not widen the façade. It is now passed over rather than rejected; the previous test codified that false positive and is replaced. A default on the ENTRY file is still a real default export of the façade and still throws. - Per ResolveExport, a name two star sources resolve differently is `ambiguous` — importing it is a SyntaxError, so it is not part of the surface at all. Unioning would pin a symbol no consumer can import; ambiguity now throws and names both origins. Origins are tracked by declaring module rather than by path taken, so a diamond (two barrels reaching one declaration) resolves normally, and an explicit export shadows a star-provided name of the same name as the spec's own precedence does. Both counterfactuals are tested alongside the two rejection cases. P1 — module size. The 885-line generated FACADE_SYMBOLS table moves to a focused sibling, scripts/layering/facade-symbols.ts, leaving the behavioral tests at 642 lines (from 1,455) so the test file stays one bounded read per AGENTS.md. Gates: check:layering (66 tests, up from 63) / typecheck / lint / format:check — green. Contracts plant re-verified under the corrected semantics: a stray two files deep behind the `export *` chain still fails with a named diff, and reverts to green. * fix(layering): resolve re-export identity transitively; extract facade-exports Addresses both P1 findings on the second review round. P1 — named re-export identity stopped at the immediate source. Given `a` re-exporting `x` from `b`, `c` re-exporting `x` from `a`, and a façade starring both, ESM resolves ONE binding (b's `x`), but the walker identified the two paths as `b#x` and `a#x` and falsely rejected the façade as ambiguous. Reproduced before fixing. Origins now resolve through the chain to the binding a name ultimately names, by asking the child's own already-resolved map instead of synthesizing an identity from the specifier. A package specifier keeps a stable synthetic identity (it is not a file this gate reads), and a cycle in progress falls back to the immediate source. Two tests, counterfactual-verified against each other: the chain diamond now resolves to one name (confirmed failing with the old immediate-source identity, passing with the fix), and a same-depth chain whose branches bottom out in two genuinely distinct declarations still throws — so the fix cannot be satisfied by simply collapsing every duplicate. P1 — context-safety extraction was incomplete. Façade export enumeration moves to scripts/layering/facade-exports.ts (219 lines) with its own facade-exports.test.ts (245), registered in check:layering. package-boundaries.ts drops to 338 from 528 and its test file to 450 from 642: the boundary rules answer "may this file import that one?", this module answers "what does this façade name?". Every layering file is now under the 500-line tripwire except the generated symbol table, which the rule exempts. Gates: check:layering (68 tests, up from 66) / typecheck / lint / format:check — green. Contracts plant re-verified after the split. * fix(layering): filter `default` at the star, not at its source The reported P1 does not reproduce: intermediate `export { default } from './x.ts'` links are reported by oxc as kind `Name` with the name `default`, not kind `Default`, so they already resolve transitively; and for a terminal `export default <decl>`, the fallback identity `${child}#default` is exactly the canonical binding, so both paths agree. The exact five-module scenario from the review returns ['x']. That behavior is now pinned by a test so it cannot silently regress. Investigating it did surface a real spec violation in the opposite direction. Because a re-exported `default` is a named entry, it landed in the module's map and was then copied wholesale by star enumeration, so `export * from './mid.ts'` reported `default` as part of the surface — a name `GetExportedNames` explicitly skips, and which oxc itself labels `AllButDefault` on the star's own import. `default` is now filtered at the star rather than at the source. That placement is the point: the name has to stay in the module's map so a later `export { default as x }` can resolve its binding, while never being reachable through a star. Filtering at the source would have broken identity resolution — the very thing the review round before this one fixed. A façade entry re-exporting a default under the name `default` is now rejected too. It carries a default export exactly as `export default …` does; only the parse shape differs, and only the declared form was being caught. Three tests: the star filter (counterfactual-verified — removing the filter fails it — with a sibling name proving the module is still read), entry-level rejection, and the two-paths-to-one-default-binding case from the review. Gates: check:layering (71 tests, up from 68) / typecheck / lint / format:check — green. Contracts plant re-verified. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
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
|
b9509fe006 |
refactor: extract the .ad script codec into packages/ad-script (#1478) (#1536)
* refactor: extract the .ad script codec into packages/ad-script Moves the mutually-coupled .ad read/write codec (script.ts, script-utils.ts, script-formatting.ts, open-script.ts) plus the target-v1 annotation SERDE slice of target-identity.ts into a new private leaf package, @agent-device/ad-script, exporting only `.`. This is option 1 from the P5 scoping dossier on #1478: the codec is shared by the daemon's session-script publication writer, the future replay engine, the CLI's `replay export`, and Maestro's failure-label formatting, so it can no longer live in root src/ once packages/ad-replay lands (R11 forbids a package reaching into root src), and a second export subpath or writer-half duplication are both ruled out by existing gates/tests. target-identity.ts keeps only the record/replay-shared classification core (classifyTargetBindingMatch, local-identity/ancestry-prefix matching), importing its shared types from the new package. Every real consumer (re-derived by grep, not the dossier's list alone) is rewired to @agent-device/ad-script. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> * refactor: trim the ad-script façade to real consumers, lock the one-export boundary - packages/ad-script/src/index.ts: drop parseReplaySeriesFlags, formatTargetAnnotationCommentLine, parseTargetAnnotationCommentLine, TargetAnnotationLineParseResult, and TargetRect from the public façade — none has a consumer outside the package (re-swept every remaining export by grep; everything else kept has at least one real external importer). The functions/types stay exported from their declaring internal modules for the package's own internal use (script.ts, script-formatting.ts). - scripts/layering/package-boundaries.test.ts: add the parallel R11 assertions "the real tree parses, declares, and passes R11" already makes for maestro/provider-webdriver/provider-limrun/xml — ad-script exports exactly `.`, depends on exactly contracts+kernel, and is declared in root package.json — plus ad-script entries in the deep-resolution rejection coverage. Verified the lock catches a regression: temporarily added a fake `./codec` export to packages/ad-script/package.json and confirmed both the export-key-list assertion and the deep-resolution-rejection assertion fail; removed the plant and reconfirmed green. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> * fix: remove polynomial-redos ambiguity from the target-v1 annotation line regex CodeQL js/polynomial-redos flagged TARGET_ANNOTATION_LINE_RE (packages/ad-script/src/internal/target-annotation-serde.ts): the payload group's `\s+(.*)` let `\s+` and the unconstrained `.*` both match whitespace, so a run of separator whitespace that ultimately fails to complete the match has many `\s+`/`.*` splits to backtrack through before concluding failure. Anchor the payload group on `\S` (the exact complement of `\s`), so the mandatory `\s+` separator and the payload's first character can never overlap — the split point becomes unique and no backtracking is possible. Behavior-preserving: the only caller (parseTargetAnnotationCommentLine) always matches against an already-.trim()-ed line, whose last character (whenever the tag matches at all) is never whitespace — so a payload section `\S.*` would reject (content that is entirely whitespace) can never reach this regex through the real call path. Verified against the frozen replay-compat corpus and the full serde/parser test suites, unmodified. Added a regression test with the exact adversarial shape CodeQL/the reviewer cited (many tab pairs after the version digits), asserting sub-second parse. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> * test(ad-script): pin the annotation-line pattern's linear rejection directly The entry-point adversarial case matched greedily even with the retired regex (trim strips edge whitespace and per-line input carries no newline), so it proved nothing about the pattern. The regression surface is the pattern itself: an interior tab run with an x-newline tail fails the match, which the retired form re-split quadratically (3.7s at 100k tabs) and the \S anchor rejects in one attempt. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
ef66dcdf25 |
refactor(daemon): serialize replay transactions behind a locked coordinator (#1478 P4b) (#1535)
* refactor(daemon): serialize replay transactions behind a locked coordinator Adds session-replay-coordinator.ts, a ReplayCoordinator scoped to one locked native .ad replay request, and routes every repair-transaction write session-replay-runtime.ts and session-replay-resume.ts perform through it: arm, demote-for-rerun, mark-complete, hold-on-divergence stamping, the pendingRecordAndHeal corrective watermark (set + clear), and reap-tombstone clearing. Neither file imports session-replay-transaction.ts (P4a's ReplaySessionTransaction) or writes session.pendingRecordAndHeal directly anymore. Adds a minimal immutable ReplaySessionView (repairBoundary, pendingRecordAndHeal) so the three readers this slice touches (preflightReplayAgainstActiveRepair, isRepairArmedTerminalClose, the entry-index resolution in prepareReplayPlan) stop taking mutable SessionState. Close-time sequencing (session-close.ts's platform-close receipt, session-close-script.ts's commit/abort) stays a direct ReplaySessionTransaction caller by design: commit/abort happen at teardown, ordered against platform close and lease release, not during a replay request. Updates the R7 session-state ownership registry: pendingRecordAndHeal moves from session-replay-resume.ts to session-replay-coordinator.ts. The daemon-modularity baseline (writer-owned fields / owner claims) is unchanged. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> * docs: state the coordinator constraint, not the migration Co-Authored-By: Claude <noreply@anthropic.com> * fix(daemon): thread one bound resume-stamper instead of a second coordinator buildAndPersistReplayDivergenceResume (session-replay-resume.ts) constructed a SECOND ReplayCoordinator from a bare SessionStore + session name, reachable from both divergence paths (session-replay-target-verification.ts and the action-failure chain through session-replay-runtime-failure.ts / session-replay-divergence.ts). That let a lower handler manufacture repair authority by naming a session instead of using the request's own locked coordinator. Adds ReplayResumeStamper: a narrow capability bound to the coordinator runReplayScriptFile already created, exposing only sessionExists() and stampCorrectiveWatermark(). Threads it through ReplayStepContext and the failure-wrapper params into both chains. buildAndPersistReplayDivergenceResume now takes the stamper and holds no SessionStore or coordinator-construction ability at all. Adds src/daemon/__tests__/replay-coordinator-ownership.test.ts, an oxc-parser AST structural test (same approach as scripts/layering/session-state.ts) asserting: createReplayCoordinator has exactly one production call site (session-replay-runtime.ts); none of the five divergence-chain files import the coordinator factory or session-replay-transaction.ts; session-replay-resume.ts holds no session-store.ts import at all; and the other four hold SessionStore only as a type. Verified the test fails on a planted violation of each of the two structurally-distinct invariants (coordinator-construction, SessionStore value-import) and passes once removed. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
67f3d09d95 |
refactor(daemon): session script publication behind one capability (#1478 P4a) (#1532)
* refactor(daemon): add the tagged script-publication aggregate First step of P4a. Nine co-resident optional SessionState fields encode two lifecycles plus a shared output target, with nothing in the shape saying the lifecycles are disjoint — so readers re-derived that from field combinations and writers had to remember which siblings to clear. The aggregate makes both invariants structural: a session publishes nothing, authors ordinarily, or is under repair; and force lives inside the target, so retargeting replaces the authorization along with the path. Three corrections after an adversarial review of the first draft: - The target is a default|explicit union, not a mandatory path. A bare 'open --save-script' arms with no path and lets the writer resolve a daemon-owned destination at write time, and force can be granted before any path exists. Eagerly materializing a default path would have silently changed retarget semantics, because today's check requires a previously persisted path — so 'open --save-script --force' then 'close --save-script=out.ad' is not currently a retarget and the grant survives. That behavior is preserved here and flagged in the docblock as a probable #1258 gap; tightening it is a product change and belongs in its own commit. - The repair status relation is not linear. A failed commit followed by 'replay --from' demotes complete back to armed, so demoteRepairToArmed exists and deliberately RETAINS the close receipt: the platform close already succeeded for that operation identity, and dropping it would re-dispatch a close on retry — which is also how a migrator ends up reaching for the caller-computed platformCloseSucceeded boolean the brief forbids. - The receipt doc no longer claims it is set only at close-succeeded and later, since the demotion path makes {armed, receipt set} reachable. Still to come in this PR: both projections, and the writer migration. Note the brief's seven-file writer inventory omits session-open.ts, which holds the only two writers of the authoring armed/aborted states. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(daemon): migrate script publication onto the tagged aggregate (#1478 P4a) The eight co-resident SessionState fields (scriptRecordingState, saveScriptPath, saveScriptForce, saveScriptBoundary, saveScriptComplete, saveScriptCommitted, repairPlatformCloseReceipt, repairSourcePath) are gone; SessionState.scriptPublication holds the aggregate, and every writer migrated in this commit — no shadow state. Two daemon-private projections own the writes, enforced by the R7 ownership gate: - session-replay-transaction.ts (ReplaySessionTransaction): repair arm/demote/ complete/abort, close receipts, and the uncommitted/boundary/sourcePath reads that idle-reap, tombstones, divergence-hold, and the recorder's exclusion key off. - session-script-publication-capability.ts (SessionScriptPublication): authoring arm on open, the recorded --save-script flag ingress, active publication, the published transitions, and the effective per-target force decision (#1258). The writer keeps the commit transition so idempotence stays colocated with the atomic publish. Failure/retry transitions pinned as the brief requires: platform-close failure leaves state unchanged (no receipt, retry re-dispatches); publication failure retains target+force+receipt (same-identity retry skips close dispatch); committed and aborted are explicit terminal states that drop the receipt. Design decisions resolved: - Force retention across a default->explicit retarget is preserved as-is and still flagged in resolveScriptTarget's docblock as a probable #1258 gap; tightening it stays a separate product change. - The never-armed 'close --save-script' whole-log publication folds into the authoring lifecycle (armed at the recorded close, published in the same request) instead of a fourth variant: every close path that reaches the write deletes the session, so the transient armed state cannot leak into 'session save-script' eligibility, whose not-armed-before-this-journey rejection is untouched. One real bug caught by the migrated tests and fixed in resolveScriptTarget: a bare (pathless) re-arm collapsed an already-materialized explicit target back to the daemon default, wiping the healed-sibling path on every per-step repair re-arm and defeating the persisted-force preflight bypass. A bare re-arm now keeps the previous target and only adds a live force grant. R7 rows consolidated to one scriptPublication entry (three owners) and the recordSession row narrowed; the R10 baseline drops to 22 writer-owned fields / 28 owner claims so the consolidation cannot regrow. Gates: typecheck, lint, format, layering clean; 624 files / 5220 tests pass (two known contention-flake timeouts reproduce only under full-suite load and pass in isolation). Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> * refactor(daemon): satisfy the Fallow gate by extracting decisions, not suppressing - scriptPublicationTarget is module-private; both public target reads (scriptTargetPath/scriptTargetForce) go through it and nothing else did. - validatePublicationEligibility splits into a pure ineligibility classifier and an error table, so the four rejections read as one decision each. - prepareSaveScriptSession hands its two arm-time rejections (authoring re-arm, EEXIST preflight) to rejectSaveScriptArming and keeps only the demote-and-arm flow. - The repair-record-exclusion provider scenario extracts its three phases (arm-and-hold, exclusion contrast, healed-script contract) into named helpers; the test body is the journey again. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
cbe1a57094 |
refactor(replay-test): extract packages/replay-test (#1478 P3b) (#1525)
* refactor(replay-test): source the manifest device vocabulary from the kernel `session-test-types.ts` reached `ReplayScriptMetadata['platform']` and `['target']` through `replay/script.ts` — the native `.ad` engine. A format-neutral scheduler must not name an engine module, and P5 relocates that engine into `packages/ad-replay` regardless, so the import had to go before the scheduler can move. Both members already resolve to neutral kernel types (`Exclude<PlatformSelector, 'web'>` and `DeviceTarget` from `@agent-device/kernel/device`), so this re-sources them directly and the manifest shape is unchanged. Only the import direction differs. First increment of P3b; the scheduler still has request-global, engine and daemon imports to port before the physical move. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): inject the progress sink instead of reading a request global The scheduler called `emitRequestProgress` in eight places, which reads a sink out of a request-global `AsyncLocalStorage`. That is ambient authority a format-neutral scheduler cannot hold once it lives in `packages/replay-test`, and #1505 recorded it as a shrink-only R10 entry. The host now injects the capability through the existing `ReplayTestRuntimeDependencies` seam established in P3a, so no new seam is invented. `session-replay.ts` supplies `emitProgress: emitRequestProgress`; `src/request/progress.ts` keeps the sink and its AsyncLocalStorage binding for every other caller. The port is deliberately narrower than `RequestProgressSink`: it accepts only `ReplayTestSuiteProgressEvent | ReplayTestProgressEvent`, so the scheduler is not handed the ability to emit `CommandProgressEvent`. Authority narrows again one hop down: `runReplayTestAttempt` spread the whole dependency bag but uses three of its members and never publishes progress, so it now takes `Pick<..., 'runReplay' | 'cleanupSession' | 'finalizeAttempt'>`. That is why no runtime test fixture needed changing — the attempt runtime never gained the capability in the first place. Also drops the last two `replay/script.ts` type references from `session-test-runtime.ts`, so the engine import is gone from that file too. Reporter contract preserved: `session-test-reporter-values.test.ts` and `session-test-reporter-values-maestro.test.ts` both pass unmodified (27 tests green across the five scheduler suites). Typecheck clean. Remaining scheduler boundary for P3b: `request/cancel.ts`, `replay/format.ts`, `replay/script.ts` in discovery, `session-store.ts`, `daemon/types.ts`, `replay-source-discovery.ts`, `core/dispatch*`, `utils/diagnostics.ts`. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): ask the host whether the suite is canceled The scheduler called `isRequestCanceled(requestId)` in five places. That both reaches a request-global registry and forces the scheduler to name a daemon request id as the cancellation key — neither survives the move into `packages/replay-test`. The host now binds the predicate to its own request and passes `isCanceled: () => boolean`. The scheduler asks a question it is entitled to ask and learns nothing about how cancellation is tracked. `shouldStopReplayTestExecution` takes the capability rather than a request id, so no scheduler function threads a daemon identifier for this purpose any more. `session-test-attempt.ts` and `session-test.ts` no longer import `request/cancel.ts` at all. It remains in `session-test-runtime.ts`, which does something different — `registerRequestAbort`, `markRequestCanceled` and the parent-abort relay are cancellation *binding*, which the brief assigns to the daemon adapter, so that split is its own step. Behavior preserved: both pinned reporter characterizations pass unmodified, 32/33 across the five scheduler suites. The one failure is the pre-existing P2/#1506 discovery-ordering regression, unrelated and untouched here. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): drop the dead request-tracking call from attempt ids `buildReplayTestAttemptRequestId` wrapped its template in `resolveRequestTrackingId`, pulling `request/cancel.ts` into the scheduler. That wrapper substitutes a generated id only when its first argument is an empty string. The template here always contains `:test:`, so it is never empty and the wrapper always returned it unchanged — the call is unreachable in this path. Probed all three shapes (explicit request id, suite-id fallback with a shard, and degenerate empty inputs); every one returns the template verbatim. Removing it takes `request/cancel.ts` out of discovery without altering a single produced id. The scheduler mints attempt identity itself, which is what the brief asks for. Evidence the ids are byte-identical: the pinned reporter characterizations assert exact session strings such as `default:test:suite-reporter:1-02-retry:attempt-1` and pass unmodified — 30 tests green across the reporter, suite and discovery suites. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): move cancellation binding and diagnostics to the host `session-test-runtime.ts` held the last two request-globals in the scheduler: `request/cancel.ts` (registerRequestAbort, markRequestCanceled, clearRequestCanceled, plus the parent-abort relay) and `utils/diagnostics.ts`. These are different in kind from the earlier ports. The brief gives the daemon adapter the job of mapping an attempt id to daemon request identifiers and *binding cancellation*, while timeout policy stays scheduler-owned. So the scheduler now receives a per-attempt capability with exactly two verbs — `cancel()` on timeout and `release()` when the attempt settles — and every registry interaction, including `relayReplayTestAbortFromParent`, moved to `session-replay.ts` next to the rest of the adapter. Diagnostics became a narrow publish capability for the same reason: `emitDiagnostic` reads a request-global scope. The level vocabulary is spelled out at the seam rather than imported, so nothing engine- or daemon-shaped crosses it. The runtime fixtures drive the real exported host binding rather than a stub. They assert cancellation through `isRequestCanceled`, and a stubbed binding would have kept those assertions passing while proving nothing. 24 tests green across the runtime, suite and both reporter characterizations, which pass unmodified. Typecheck, lint and oxfmt clean. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): split discovery into host inspection and scheduler policy discoverReplayTestEntries expanded paths, read every file, and called both engines — readReplayScriptMetadata for .ad, inspectMaestroFlow for Maestro — plus resolveReplayFormat to choose between them. Four imports a format-neutral scheduler cannot hold. Inspection is now the host's discoverSources capability. What stays in the scheduler is the genuinely neutral half: which sources a --platform filter runs, which it skips and with what message, and the empty-suite error. The manifest carries exactly the four fields the scheduler consumes (platform, target, retries, timeoutMs) plus the reporter's title, per the brief's instruction not to add more without a demonstrated call site. The platform tag is what removes the last format leak. The filter used to ask resolveReplayFormat(...) === 'maestro' to decide whether a missing platform was disqualifying. It now reads a tag: caller-bound means the invocation supplies the platform, unspecified means the source declared none. Maestro is what caller-bound looks like from the scheduler's side, and the format cannot be recovered from it. Discovery tests drive the real inspection capability, writing actual .ad and Maestro sources — a stubbed host half would have kept them green while proving nothing about the composition they exist to pin. 35 tests green across discovery, suite, runtime and both reporter characterizations, which pass unmodified. The Maestro one is the direct check that titles still flow, since they now arrive via the manifest. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): build attempt ids from named segments; trim comments Review feedback on the attempt-id builder: the comment explained a deletion that git already records, and it sat above an opaque template literal. The id is now a segment list joined on ':', so its shape is readable without prose. Output is byte-identical — the reporter characterizations assert exact session and attempt strings and pass unmodified. Applied the same standard to four other docblocks in this PR that narrated what the code used to do rather than what it does. The durable 'why' stays: which side of the seam owns what, and why the vocabulary is neutral. The migration history goes, since git carries it and these docblocks will outlive the migration. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): move shard device binding to the host buildReplayTestShardPlan called listDeviceInventory to discover what to shard across, and buildReplayTestShardFlags constructed daemon CommandFlags for the nested request. Inventory enumeration, allowlists, simulator set paths, explicit --device selectors and the too-few-devices error are host concerns; what is scheduler-owned is deciding how many shards exist and which entries each one runs. The scheduler now receives resolved shard targets through a capability. The target is neutral: id and name for session labels and progress metadata, plus platform and target, which are already kernel vocabulary. DeviceInfo no longer crosses into scheduling. One behavior note: an explicit --device selector could in principle name a web target, which is not a shardable device. That is now rejected with INVALID_ARGS rather than widening the neutral platform vocabulary to carry something the scheduler can never run. Implicit selection already filtered to mobile. 919 of 920 handler tests pass. The one failure, session-test-runner.test.ts 'binds each replay script to its declared platform metadata', fails identically on clean origin/main in this container and is unrelated: directory discovery walks with opendirSync/readSync and directory results are deduped but not sorted, while glob results are sorted, so suite order is filesystem-dependent. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(replay-test): extract packages/replay-test behind a façade Completes the P3b extraction. The scheduler, attempt runtime, discovery policy, sharding distribution, artifacts and neutral types now live in packages/replay-test/src/internal/, with one package-root export. The façade takes a neutral ReplayTestSuiteRequest and returns a tagged ReplayTestSuiteOutcome. DaemonRequest, DaemonResponse and CommandFlags no longer reach the scheduler; the adapter translates flags and meta in, and the outcome back to a daemon response. Eight flags were read by the scheduler and each became a field it owns. Host work moved to daemon adapters: source inspection (both engines and format routing), shard device binding and shard-flag parsing, and artifacts-dir home expansion, which is why the package can resolve paths without SessionStore. The one remaining shared concern was the timing trace: the host writes video lifecycle events into the same trace the scheduler owns. Rather than export a writer from the façade, each attempt hands the host an appendTimingEvent closure, so the trace format stays private and the authority is scoped to that attempt. Tests mirror the topology. Discovery tests split along the seam they now cross: ordering, traversal and routing are pinned host-side against real files, filtering policy is pinned in the package against fake sources. The runtime tests assert the scheduler's cancellation obligation (cancel once on timeout, always release) against a recording binding, and a new daemon test pins the adapter's half — registry entries, the parent-abort relay, and detach on release — so that coverage moved rather than disappeared. R10 retargeted to packages/replay-test/src/ and the zone ranked alongside maestro. R11 confirms zero root-src imports from the package. 914 of 915 handler and package tests pass. The one failure, session-test-runner 'binds each replay script to its declared platform metadata', fails identically on clean main here: directory discovery walks with opendirSync and dedupes without sorting, while globs sort, so suite order is filesystem-dependent. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * refactor(daemon): simplify replay-test request translation Fallow flagged toReplayTestSuiteRequest at 14 cyclomatic in 18 lines. The branches were self-inflicted: every req.flags?.x is one, and each optional field was written as a conditional spread to avoid setting an undefined key. exactOptionalPropertyTypes is not enabled, so assigning undefined to an optional field is equivalent and the spreads bought nothing. Destructuring flags once and extracting two flag readers removes most of the rest. One correctness note on the simplification itself: the first version used `artifactsDir && expandHome(...)`, which returns '' for an empty-string flag where the previous code called expandHome(''). Replaced with an explicit undefined check so the empty-string path is unchanged. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test(live): share the replay test-suite harness across iOS and Android Both live journeys invoked the public test command and then re-derived the same value-contract assertions by hand — suite totals, per-script status, replay counts, non-empty JUnit. Those are claims about the published suite result and are identical on every platform, and they had already drifted: iOS iterated with readReplayCommands inline, Android cast data.tests at the call site. The shared helper owns exactly that boundary. It takes the caller's runStep rather than binding a context type, so it is not a platform-configured runner and cannot template a platform's journey. Everything a platform genuinely differs on stays with the caller: which scripts run, the retry policy (iOS 2, Android none — itself a claim worth keeping), which commands each script exercises, and the behavioral evidence. Both callers keep every verify* call they had. 67 lines removed, 15 added. Residual risk: this container has no iOS or Android devices, so the live suites could not be executed here. Typecheck and lint pass; the harness needs a run on real targets before the claim that behavior is unchanged is evidence rather than inference. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test: pin directory enumeration in the platform-binding suite test The test wrote two scripts into a temp directory and assumed discovery would return them in creation order. Directory expansion deliberately preserves filesystem order to match Maestro — only glob expansion sorts, and 'preserves Maestro directory filesystem order' pins that with a mocked opendirSync. So the ordering contract is correct; this test's assumption about enumeration was not. It passes on CI, where small directories usually enumerate in creation order, and fails on filesystems that do not — identically on clean main, where the platform-to-script binding appears reversed. Pinning enumeration the way the discovery tests already do keeps the subject intact (each script binds to ITS declared platform, and session numbering follows discovery order) without depending on the host filesystem. The fs import became a default import because vi.spyOn cannot redefine an ESM namespace export. 915 of 915 handler and package tests now pass here. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test: scope the enumeration spy and restore it in a finally The spy I added restored only on the happy path and asserted on its argument inside the mock implementation. Either would misfire for anything else sharing the worker: an assertion thrown from inside fs, or a leaked global opendirSync, surfaces as a worker crash with no failed test rather than a readable failure. It now delegates to the real implementation for any directory but this suite's own, and restores in a finally. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * fix(replay-test): put package tests where they are actually run Review found the moved package tests were neither executed nor typechecked. They sat under packages/replay-test/test/, but vitest's unit-core lane includes packages/*/src/**/*.test.ts, and neither the root nor the package tsconfig covers a top-level test directory. A plain unit-core run discovered zero files under the package. That is why they looked green: my earlier runs passed those paths explicitly on the command line, which masked that the default run skipped them. The count is the proof — 550 files/4741 tests before, 553/4753 now, and the delta is exactly the three files and twelve tests that were being skipped. The runtime test also imported runReplayTestAttempt from the package specifier, which the facade does not export. It would have failed the moment it was discovered. It now imports internally, like the rest of the internal tests. Also removed replayTestAttemptFailure from the facade: zero consumers outside the package, so exporting it widened the boundary for nothing. P3 asks for a one-function facade. 553 test files and 4753 tests pass; lint and the layering guard are clean. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * style: format the facade after removing the unused export A scripted edit removed the export line but left a stray blank line; oxfmt was not re-run on that file afterward, so Lint & Format caught what pnpm lint alone does not. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * fix(replay-test): typecheck the package and fix a type-only import Review found the moved package tests were transpiled by vitest but never typechecked: the root typecheck script builds six packages via tsc -b and packages/replay-test was not among them, so its tsconfig was never used. That hid a real TS2459. session-test-runtime.test.ts imported ReplayTestAttemptOutcome from ../session-test-runtime.ts, which imports that type but does not re-export it. It now imports from ../session-test-types.ts, where the type is defined. Adding the package to the tsc -b list closes the gap. Verified empirically rather than assumed: planting a string-to-number error in a package test makes typecheck fail, and removing it makes it pass. This is the second finding of the same shape on this PR — first the tests were not discovered by vitest, now they were not covered by typecheck — so the gate was confirmed to reach the files rather than trusted to. 12 package tests pass, lint, format and the layering guard are clean, and typecheck is clean with the package included. Refs #1478 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
da93191201 |
refactor: move Limrun provider behind package facade (#1518)
* refactor: move Limrun provider behind package facade * fix: preserve Limrun public provider types * fix: tighten Limrun provider facade boundaries * test: harden Limrun compatibility coverage * fix: narrow Limrun public type exports * fix: narrow Limrun provider exports |
||
|
Michał Pierzchała
|
b125435989 |
refactor: extract WebDriver provider package (#1504)
* refactor: extract webdriver provider package * refactor: consolidate shared XML codec |
||
|
Michał Pierzchała
|
a3ab69a110 |
refactor(replay-test): neutralize the values crossing the scheduler seam (#1478 P3, part 1) (#1509)
* refactor(replay-test): neutralize the values crossing the scheduler seam
#1478 P3, part 1 of 2. Prepares the replay-test extraction by removing every
non-neutral value that crosses the scheduler seam, in place under `src/`, so the
physical move to `packages/replay-test` is a file move rather than a redesign.
`DaemonResponse` no longer crosses the seam. `session-test-types.ts` typed
`runReplay`/`finalizeAttempt` as returning a daemon response and the scheduler read
`.error.code`, `.error.details`, and `.data.replayed/.healed/.warnings/
.snapshotDiagnostics` off it throughout. That is invisible to R10 today only
because `checkDaemonTypesImporters` skips `src/daemon/`; once the files live in a
package they become external `daemon/types.ts` importers, which the ratchet only
lets shrink. Attempts now resolve as tagged `ReplayTestAttemptOutcome` values
carrying exactly what the scheduler consumes, including an `infrastructure` tag —
classifying an environmental failure needs platform boot-diagnostic vocabulary the
scheduler must not import, so the host decides and the scheduler reads the verdict.
`session-test-outcome.ts` is the one place a daemon response becomes an outcome.
Step events get a narrow per-attempt port. They were emitted from
`session-replay-runtime.ts` and `session-replay-maestro-observer.ts`, both reading
a request-global `AsyncLocalStorage` seeded per attempt. The scheduler now hands
each attempt an `onStep` sink, threaded the way `tracePath` already is; both
engines call it and `withReplayTestActionProgress`/`readReplayTestActionProgress`
are gone. A direct `replay` simply has no sink.
ADR 0012 divergence becomes a neutral leaf. `src/replay/divergence.ts` depended
only on kernel contracts and redaction, yet Maestro constructs divergences too and
CLI/MCP both render them, so P5 could not have moved it into `packages/ad-replay`.
It is now `@agent-device/contracts/divergence`; the renderer's output text is
unchanged.
The progress wire vocabulary moves to `@agent-device/contracts/progress`. It is
serialized by `request-progress-protocol.ts` and reconstructed by the CLI reporter
path, so it belongs below both; `src/request/progress.ts` keeps only the sink and
its AsyncLocalStorage binding.
Together these clear all four of replay-test's recorded R10 migration imports, so
the rule now enforces unconditionally for that module.
Behavior is unchanged. The shipped reporter contract — export spellings,
object/factory loading, hook names, timing/order, value fields, the synchronous
live-hook rule, awaited suite completion, error handling, exit codes — is
untouched, and `session-test-reporter-values.test.ts` passes unmodified. The
`--shard-all` `total`/`runnable` asymmetry is preserved as characterized.
Refs #1478
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
* test(replay-test): pin the Maestro reporter step path against the onStep port
Review finding on #1509: the native `.ad` reporter ratchet exercises only one of
the two `onStep` forwarding chains, so deleting a link in the Maestro chain would
silently stop `onTestStep` for every `test --maestro` run while every existing
reporter test stayed green. Same defect class as the dropped diagnosticId/logPath
(#1501) and the dropped reporter `hint` (#1505).
Maestro is one of P3's two required real adapters and its chain shares no links
with the native one below `runReplayScriptFile`:
scheduler sink -> runReplayScriptFile -> runTypedMaestroReplayFile
-> createMaestroReplayObserver({ onStep }) -> actionStarted -> onStep
Adds a Maestro scenario driving `test --maestro` through the real session handler
and the real reporter registry. It asserts the step payload the engine produces
(`stepIndex`/`stepTotal`/`stepCommand`/`stepValue`, including that a value-less
command stays value-less) together with the attempt/session identity the scheduler
supplies, since that half of the event came from request-global AsyncLocalStorage
before P3. A second case drives a retry so step events must carry attempt-1's
session and then attempt-2's. The flow `name` also pins the reporter `title`, a
value only the Maestro path can produce.
New file rather than an addition to session-test-reporter-values.test.ts: that file
is the pinned characterization and must keep passing unmodified, and Maestro needs
its own vi.mock of core/dispatch for device resolution.
Counterfactual run, both links, each restored after:
- dropping `onStep` from createMaestroReplayObserver in
session-replay-maestro-runtime.ts
- dropping the emitMaestroStep call from actionStarted in
session-replay-maestro-observer.ts
Each dropped both onTestStep events ("expected [ 'onSuiteStart', 'onTestStart',
…(2) ] to deeply equal [ 'onSuiteStart', 'onTestStart', …(4) ]") and failed both
new cases, while session-test-reporter-values.test.ts passed all 4 — exactly the
hole the reviewer identified.
Test-only; no production change. Bundle output is byte-identical to
|
||
|
Michał Pierzchała
|
1fc9169188 |
refactor(daemon): consolidate session-script test factories and drop saveScriptDefaultedHealedPath (#1508)
Preparatory slice for P4a (#1478). No aggregate, no transaction type, no publication-writer migration — those land separately. Two things: 1. Name the session-script session states in the shared test factories (`makeAuthoringSession`, `makeRepairArmedSession`, `makeRepairCompleteSession`) and route 40 inline session literals across 11 test files through them. The `saveScript*` fields are not independent — recording without a boundary is ordinary authoring, a boundary without `saveScriptComplete` is an ARMED-but-uncommittable repair, and only the COMPLETE combination publishes — so re-deriving the combination per test buried the distinction each test was actually about. Pure refactor: the factories write today's fields and no assertion was weakened. 2. Delete `saveScriptDefaultedHealedPath`. It had zero production readers: the writer's refuse-on-exist guard has been uniform since #1235, so the flag was written in three places and never consulted. Removing it takes its R7 owner row with it and lowers the R10 baseline from 30/42 to 29/40 (the ratchet fails on a drop too, so this cannot be deferred). Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Michał Pierzchała
|
0e51007b04 |
refactor: isolate maestro engine package (#1506)
* refactor: isolate maestro engine package * perf: deepen maestro facade boundaries |
||
|
Michał Pierzchała
|
352428d37a |
test: characterize replay-test reporter contract and extend R10 (#1505)
* test: characterize replay-test reporter contract and extend R10 P3 of #1478 moves the replay-test scheduler into `packages/replay-test` and makes attempt identity scheduler-owned. Before production code moves, pin what a shipped custom reporter actually observes today, and close the import boundary the extraction has to end up satisfying. Reporter values (all pinned as shipped, none proposed): - the `RequestProgressEvent` -> reporter-value projection field by field, including key presence for absent optionals and the dropped `command` events; - `session` provenance across the seam: the start value is always `attempt-1` (it is built before any attempt runs), step values track the running attempt through the per-attempt AsyncLocalStorage context, and result values carry the attempt that produced them, so a retried case reports three different sessions to one reporter; - the shard-scoped session prefix and device identity a sharded run reports; - module export spelling precedence, the six optional hook names, the live-hook vs final-hook error asymmetry, and exit-code recommendation semantics. Late-timeout finalization/cleanup: - finalization always runs before cleanup, and the timing trace records that order (`finalize_start/stop` then `cleanup_start/stop`); - a replay settling inside the 2s grace window cleans up once and is not marked `timeout_cleanup_pending`, and the raced TIMEOUT response still wins; - a replay that misses the window defers its second cleanup until the abandoned replay settles, and that late cleanup's failure is swallowed. R10 now rejects replay-test imports from `src/request/**` and engine internals (`src/replay/`, `src/compat/`, `src/maestro/`, `src/ad-replay/`), with the four imports that exist today recorded as shrink-only migration entries so the rule enforces immediately and the extraction must delete them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 * test(daemon): assert reporter hint on the real onTestResult value The failing-suite characterization supplied `hint` as input but never asserted it on the hook value, so dropping `hint: error.hint` from the scheduler path left all reporter tests green. Assert it on the real reporter value so the ratchet catches a shipped field going missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
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
|
76453add71 |
refactor: pnpm workspace + @agent-device/kernel pilot (#1490 W0) (#1494)
* refactor: pnpm workspace + @agent-device/kernel pilot (#1490 W0) Extend the workspace with packages/* and move the kernel behind an enforced public API: packages/kernel with nine consumer-earned subpath exports (errors, device, snapshot, contracts, collections, rect, redaction, daemon-error, bounds — the last absorbed from utils as Rect vocabulary). Every kernel import repo-wide becomes the @agent-device/kernel/<sub> specifier; kernel tests move to src/__tests__/kernel/ and exercise the package surface. The root declares the package in devDependencies (workspace:*), tsdown bundles it (noExternal) so the published artifact and its runtime dependency manifest are unchanged. Gate rewiring in the same change, per the W0 brief: - R1 kernel-sink retires (physically subsumed); new R11 package-boundaries guards no-root-back-imports, relative tunnelling past exports maps, undeclared workspace deps, and non-exported subpaths, with runtime resolution pins via import.meta.resolve. - resolveImportEdges and mutation ownership follow workspace specifiers through exports maps, keeping R4 cycle checks, depgraph, and derived test ownership connected across the seam (kernel-errors still owns 495 tests). listSourceFiles includes packages/*/src. - kernel becomes an unranked zone; mutation registry, stryker mutate globs, and the mutation-affected workflow path filter move to packages/kernel/src/errors.ts. - check:affected gains packages/ ownership (manifests fail open); vitest and coverage include packages/*/src; fallow ignores packages/** (its resolver cannot follow workspace specifiers). - The affected-selector CI job installs dependencies: its closure now crosses workspace specifiers, and the R8 relative exception is unsafe for production src files (Node ESM does not realpath, so dual specifier/relative loads would instantiate modules twice). The R8 zero-dep set is pinned empty with that rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep * fix: address W0 review — mutation sandbox, exports-map resolution, tsc -b Review findings on #1494, all five: 1. contracts-schema-public.test.ts reads the kernel source at its packages/ path (fs access invisible to the codemod and typecheck). 2. Mutation lane: Stryker sandboxes the tree but pnpm's node_modules symlink resolves @agent-device/* back to the real repo, so mutants in the sandbox never load and vitest.related finds no tests. vitest.mutation.config.ts now aliases each EXPORTED specifier to its source (derived from exports maps, never a wildcard), keeping resolution inside the mutated tree. Validated: kernel-errors module runs end to end (dry run 3,984 tests, mutants killed, exit 0). 3. Layering/depgraph resolve workspace specifiers through the exports-derived map (workspaceSpecifierTargets) instead of reconstructing paths, so '.'-facade packages resolve; the positional fallback remains only for map-less fixtures (P0 pin). 4. Per-package project references implemented: packages/kernel is composite (emitDeclarationOnly -> dist-types, gitignored), the root references it, and typecheck becomes tsc -b — probed to catch type errors on both sides under TypeScript 7 native. 5. R11's relative-route exception now requires membership in an actual R8 zero-dep job closure (zeroDepClosureFiles walks entries), not mere scripts/ placement — closing the dual-instantiation bypass. Also from review discussion: daemon-error moves out of the kernel package to src/client/ — its consumers (cli, client facade) rehydrate wire DaemonErrors client-side; the daemon only produces them. Kernel drops to 8 exported subpaths before any of them ship. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep * refactor: one exports-map reader for mutation alias and ownership Fallow flagged workspaceExportAliases (cognitive 15, CRAP 90). The manifest-reading logic already exists as workspaceSpecifierTargets in scripts/layering/package-boundaries.ts, so both the Stryker sandbox alias table and the mutation ownership walker now consume it instead of carrying near-clones. Behavior unchanged; mutation suite 45/45 and changed-code fallow green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep * fix: composite kernel without a root references edge FreeRange runs plain `tsc -p tsconfig.json`, and a root `references` entry makes non-build-mode TypeScript demand the referenced project's built declarations (TS6305) — a standing "build first" tax on every plain -p consumer (fr, editors). Keep the per-package composite project and build it in typecheck (`tsc -b packages/kernel` before the root and examples/sdk passes), but drop the root references edge: root consumption resolves through exports to source, identical to runtime and to the bundler. Probed: plain -p green with no prebuilt output; kernel-side type errors still caught by its own build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUv7bvbWNryuXgSBuqTtep * fix: R11 uses the layering parser; mutation config is a fallow entry Review blockers on #1494: - R11's private single-quote regex could miss a double-quoted or re-export route into packages/*/src. specifierSites now delegates to the layering model's parseImports (both quote styles, side-effect imports, re-exports, dynamic imports), with direct regressions for each formerly-invisible form. - vitest.mutation.config.ts becomes a declared fallow entry instead of a tolerated unused-file finding: the full-repo audit now reports it reachable (unused files 2 -> 1; the remainder predates this PR). FreeRange clean-checkout evidence: with packages/kernel/dist-types and every *.tsbuildinfo deleted, `pnpm check:freerange` reports 0 findings on this head — the TS6305 topology died with the root references edge in the previous commit; check:freerange has no build precondition. 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
|
2316fd32c5 |
test: pin daemon modularity migration contracts (#1487)
* test: pin daemon modularity migration contracts * refactor: tighten daemon modularity ratchets * test: pin reporter live hook isolation |
||
|
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
|
255deb6c28 |
ci: fold single-grep jobs into steps, call named pnpm scripts (#1465)
* ci: fold single-grep jobs into steps, call named pnpm scripts - Merge ios-runner-swift-compat and no-test-di-seams (each just checkout + one rg assertion) into steps of a new static-checks job, keeping each step's own failure message. Removes two job-scheduling/ checkout overheads and two PR status-check lines. - Replace the layering-guard job's inlined copies of check:layering and depgraph:test with the named pnpm scripts, removing the silent-drift risk between the workflow and package.json. - Fix the same drift in conformance-regenerate.yml, which inlined maestro:conformance:regenerate byte-for-byte. - Leave affected-selector's inline node invocation as-is: R8's zero-dep closure check (scripts/layering/zero-dep-jobs.ts) finds a job's entry scripts by matching literal paths in the run: block, so switching to `pnpm check:affected:test` would zero out its entries and make R8 fail closed. Documented inline why this one stays inlined. - Leave publish-mcp-registry.yml's sync-mcp-metadata --check alone: that job never runs the setup-node-pnpm action, so pnpm isn't provisioned there at all. Refs #1462 * ci: teach R8 to resolve pnpm script names, drop affected-selector's inline copy R8's zero-dep-job entry scan matched literal script paths in a run: block, so a bare `pnpm <script>` invocation found zero entries and R8 failed closed — the reason affected-selector kept an inline node command instead of calling pnpm check:affected:test (#1462). zeroDepJobs now also resolves a pnpm script name against package.json and scans the resolved command for entry paths, so affected-selector can call the named script like every other job. Also replaced the other workflows' inlined copies of named package.json scripts (test:replay:*, perf, perf:android, maestro:conformance:differential, check:mcp-metadata, size) with their pnpm names, keeping each job's CI-specific trailing flags — found via a repo-wide sweep for any run: block whose text duplicates a scripts entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L67kDSTAJwaoLCwANEJFRM * fix: revert publish-mcp-registry pnpm regression, recurse R8 alias resolution publish-mcp-registry.yml's job only provisions Node via actions/setup-node, never the repo's setup-node-pnpm action, so pnpm is never installed there — the earlier sweep's `pnpm check:mcp-metadata` would have broken the release path. Reverted to the direct node invocation with a comment explaining why, matching the PR's own stated rationale for leaving it alone. zeroDepJobs' pnpm-alias resolution only expanded one level: a resolved script that itself invoked another named pnpm script had its entries silently dropped from R8's closure. resolveRunEntries now recurses through chained aliases with a per-chain visited set, so a nested alias's entries are found and a cycle stops re-expanding a repeated name instead of recursing forever. Added coverage for both the chained and cyclic cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L67kDSTAJwaoLCwANEJFRM --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
devin-ai-integration[bot]
|
6544e9a0c5 |
obs: repo-health snapshot command aggregating existing analyzers into one JSON (#1471)
* obs: add repo-health snapshot command aggregating existing analyzers into one JSON Adds `pnpm repo-health [--json]`, an offline command that aggregates the signals this repo already computes into one deterministic JSON snapshot by reusing each analyzer (depgraph, layering ratchets, coverage json-summary, size-report, fallow baselines, slow-test ratchet, bench/skillgym registries) rather than reimplementing any metric. Carries mandatory v1 provenance (schemaVersion, commit, per-analyzer content hashes, input provenance) so #1424 can persist history. Component metrics (instability/abstractness/main-sequence distance) are observatory-only. The only gating behaviour is the depgraph-vs-layering R6 consistency assertion, already wired into the Layering Guard job via scripts/depgraph/model.test.ts. Closes #1423 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * repo-health: hash read artifacts and flag staleness in provenance Coverage and size are artifacts repo-health reads but does not produce, so they carry no producing-commit stamp. Previously provenance recorded only their paths and the current HEAD, so a snapshot taken after a source edit without rerunning those producers would pair prior metrics with the new SHA — #1424 would persist a false commit-indexed history entry. Now each read artifact is bound via artifactProvenance() to its content hash (so history keys on the bytes the metrics came from, not a commit they may predate) and an explicit `stale` flag (true when a production source file is newer than the artifact). The coverage/size analyzer-config hashes are added to provenance.tool, and the human summary marks stale artifacts. Adds a pure stale-artifact regression test. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * repo-health: bind artifact freshness to the whole producer input set The stale check derived freshness only from listSourceFiles() (src/*.ts) and applied it to both coverage and size. Coverage also depends on tests and vitest config; size-report.mjs reads package.json and runs npm pack — so a change to a non-src producer input could leave an old artifact marked stale:false while provenance recorded the current HEAD, a commit the metrics predate. Each read artifact now declares its full producer input set (PRODUCER_INPUTS) and is flagged stale when ANY tracked input in that set is newer than the artifact (via git ls-files mtimes). Freshness that cannot be proven — an empty observable input set — is reported stale, never falsely fresh. The input set is recorded as provenance.inputs.*.producerInputs. Adds isArtifactStale() with a regression covering a non-src input newer than the artifact and the unprovable case. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * repo-health: prove artifact freshness from a stamped producer commit; route git through runCmdSync 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
|
ba1a5efbc6 |
refactor(layering): declare R1-R3 as a policy table, and test them (#1449)
R1-R3 were three hand-written predicate functions. Each was short, but each
buried its boundary in control flow: you had to read the early-returns to learn
that R3 tolerates dynamic imports, or that R1 opens exactly one door. They are
now data in scripts/layering/zone-policy.ts -- which zones a boundary governs,
which import kinds it tolerates, which path prefixes are its declared seam --
walked by one small evaluator. A fourth zone boundary becomes a table entry
rather than a fourth predicate to keep consistent with the other three.
R1 is deliberately two entries rather than one with a special case, because
"kernel may import contracts type-only" and "kernel may import nothing else at
all" are two statements, and writing them separately is what makes the single
open door visible.
The refactor exposed a real gap: checkLayeringRules had NO unit test. The only
thing exercising R1-R3 was the real tree, which is clean, so a rule that had
silently stopped matching would have looked exactly like a rule being obeyed.
zone-policy.test.ts now asserts each boundary fires and each documented
exemption holds, including that src/daemon/client/ is excluded from the daemon
seam. Verified end-to-end by injecting one violation per rule plus an exempt
file: the gate reports 4 zone-policy violations (R3 twice, catching the
daemon/client case), ignores the type-only and dynamic edges, and exits 1.
Also records in docs/dependency-graph-findings.md the result of spiking
eslint-plugin-boundaries under oxlint's jsPlugins, so nobody repeats it. It
does work -- jsPlugins loads npm ESLint plugins with no ESLint install, and
R1-R3 are all expressible once you know importKind: "value" and
settings["boundaries/dependency-nodes"]. Not adopted: no ratchet mechanism (the
thing that took R6 from 61 to 7 incrementally), it cannot express R4-R9 so the
architecture would be defined in two places, it misreads inline `{ type Foo }`
specifiers as value imports (one false positive on providers/limrun/android.ts),
message interpolation renders empty under the current selector syntax, and it
costs 230 transitive packages on an API documented as alpha.
No behaviour change: 932 source files, R6 = 7, R7 = 41 fields, R8 clean,
R9 = 102, all identical to before.
Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
Co-authored-by: Claude <noreply@anthropic.com>
|