mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
refactor/adr19-boot-unit
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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
|
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
|
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
|
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
|
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 |
||
|
devin-ai-integration[bot]
|
edca35d122 |
chore(deps): Renovate config, packageManager-derived pnpm in CI, repo-wide format (#1444)
* chore(deps): add Renovate config and enforce packageManager pnpm version in CI Refs #1422 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore: bump pnpm to 11.17.0 and format the whole repo with oxfmt format/format:check drop their hand-maintained path list: oxfmt already skips node_modules and honors .gitignore, so the only exclusion list is .oxfmtrc.json ignorePatterns. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(mutation): accept either quote style in the affected-lane path filter Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * chore(deps): keep fixture-app runtime deps as individual Renovate PRs 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
|
e8b779cb32 |
fix(daemon): keep close-time script-save failures from leaking the session/device claim (#1392)
* fix(daemon): keep close-time script-save failures from leaking the session/device claim A close-time script write (implicit from `open --save-script`, or this close's own `--save-script`) that refuses to publish (e.g. a no-clobber target-exists AppError) threw uncaught out of `handleCloseCommand`, skipping lease release, device-claim release, and `sessionStore.delete` entirely — while the `close` action had already been recorded with no rollback. Live-repro'd over the real CLI against an Android emulator: this single gap explained both symptoms split out of #1384 into #1391 — a lingering `DEVICE_IN_USE` claim after a failed `close`, and a published `.ad` rewritten with duplicated trailing `close` lines when the same close was retried (each attempt re-recorded a `close` action on top of the one never rolled back from the prior failure). Catch the write failure, roll back the just-recorded `close` action (mirroring the existing repair-armed commit-failure pattern), and let teardown (lease release, device-claim clear, session delete) complete regardless — exactly as an ordinary platform-close failure already doesn't block them. The failure is still surfaced to the caller, but after teardown, with a corrected hint: retrying the same close is no longer meaningful since the session is now gone. Fixes #1391 * refactor(daemon): shrink handleCloseCommand/runSessionCloseTeardown under fallow's complexity gate CI's fallow code-quality check flagged handleCloseCommand (126 lines, 19 cyclomatic / 16 cognitive) and runSessionCloseTeardown (73 lines) as exceeding the large-function/high-complexity thresholds after the prior commit's fix. Extract runCloseTeardownAndRelease (teardown + lease release + claim clear + delete + ordered error surfacing) and buildCloseSuccessResponse (final response shaping) out of handleCloseCommand, and finalizeOrdinaryCloseScript out of runSessionCloseTeardown. No behavior change — same control flow, split into named, independently-readable steps; fallow now reports 0 complexity findings for this diff. * fix(daemon): preserve the write error's structured details in the close-time save failure Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure rebuilt the AppError from only the original message, dropping its machine-readable details.reason ("script_target_exists"), details.path, and cause. A caller dispatching on those fields (or reading the CLI's --json error.details) lost them even though the underlying write failure carried them. Preserve the original error's details/cause, overriding only the close-specific hint and retriable:false. Extends the #1391 regression test to assert the routed close response still carries reason/path. * refactor(daemon): drop the vestigial close-time rollback, add router-level #1391 coverage Review feedback on #1392 (thymikee), P2 items: - The close-time save-script failure's session.actions rollback (finalizeOrdinaryCloseScript) was left over from an earlier design where a failed save could keep the session alive for retry. It never does now — runCloseTeardownAndRelease always tears the session down regardless of the outcome — so there is no surviving session for a later write to duplicate the close action on. Drop the rollback; the durable events.ndjson entry (which the rollback never touched anyway) and the in-memory action now agree, both accurately recording that the close happened. - Add a request-router-level regression (request-router-typed-error.test.ts, alongside the existing repair-close BLOCKER 2 test it mirrors) proving the normalized JSON error shape a real client sees: top-level retriable:false, details.reason/path preserved, and the session torn down — not just that handleCloseCommand throws the right AppError when called directly. * test(daemon): assert the durable close event survives a failed close-time save Review feedback on #1392 (thymikee), final P2 item: the previous commit removed the actions rollback because there's no surviving session to duplicate the close action on, but nothing actually asserted the durable events.ndjson action.recorded:close event stays put. Flush and read it back so a future rollback or event-order change can't silently recreate the in-memory/durable mismatch the removed rollback used to paper over asymmetrically. * test(daemon): assert the retained session's in-memory close action, not just the durable event Review feedback on #1392 (thymikee): the durable-event assertion alone doesn't catch a reintroduced session.actions.length = actionsBeforeClose rollback, because that event is queued (and durable) before the write even attempts — a regression there would leave the assertion passing while silently reintroducing the in-memory/durable mismatch. Retain the session object past handleCloseCommand (store.delete only drops the map entry, not the object a local variable still points at) and assert its actions array contains exactly one close entry, matching the durable event count. Verified by temporarily reintroducing the old rollback locally: this assertion fails (0 !== 1) where the prior durable-only check did not, then reverted. * refactor(daemon): model repair close retry as receipt * refactor(daemon): merge blockingError to state, not explain, the save-script exclusion Following up on the comment-trimming pass already on this branch: the device-claim condition (!platformCloseError && !cleanupAggregate) and the two-line throw sequence right below it both needed a paragraph explaining why saveScriptError is excluded from one but not the other. Merge platformCloseError and cleanupAggregate into a single named blockingError — its name now states the exclusion the comment used to argue for, and the throw sequence collapses from two ifs to one. Trimmed the remaining long docblocks in this file the same way: state what's non-obvious in 1-3 lines instead of re-deriving it in prose. * refactor(daemon): clarify close script finalization |
||
|
Michał Pierzchała
|
56b72c5cf7 |
refactor(boundaries): put shared contracts below their consumers, gate the result (#1405)
* refactor(boundaries): move shared contracts below their consumers Acts on the depgraph findings: type-only edges are invisible to R5, so vocabulary that everything depends on had drifted above the zones that use it. - contracts/: the four platform-plugin facet tags (LogBackend, RecordingBackendTag, PerfMetricsSamplerTag, PlatformGatedProviderResolverKey) now live beside the plugin contract itself, which also moves out of core/; NetworkEntry moves next to the command surface that renders it; and the click-button, recording-export-quality, interactor-types and runner-lease-context vocabularies move down out of core/. - (root) drops from 29 files to 13: the internal *-contract/output/annotation modules move into contracts/, kernel/ (daemon-error, observability-redaction beside kernel/redaction), core/ (batch-policy, an ADR 0008 projection), commands/ (cli-command-aliases) and remote/ (upload-progress, upload-stream). What remains is entrypoints and the composition roots that R2 requires to sit outside the spine. - utils/ joins the ranked spine at rank 1 after its only two upward files move to the zones they were reaching for (cli/resolve-cli-options, cli-schema/cli-config), putting ~336 value edges under the gate. - Internal imports that routed types through the client-types re-export hub now name their real source. Type-only spine inversions drop from 61 to 35; the remainder is two clusters (client/client-types.ts and the ADR 0003 daemon facet). No behaviour change: 4470 unit tests and the layering gate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * style: merge the duplicate contract imports the tag moves created Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * refactor(imports): name the declaring module, share find's argument rules Two follow-ups from re-measuring the graph after the boundary moves. 1. 89 type imports across 79 files routed through a re-export hub in another zone: `CliFlags` reached through commands/cli-grammar/flag-types.ts (52) when it is declared in contracts/cli-flags.ts, the replay suite result types reached through daemon/types.ts when they are declared in contracts/replay.ts, the doctor types through a daemon handler module, and so on. Each hop invented a cross-zone edge the architecture never asked for — including every apparent replay -> daemon and utils -> commands dependency. They now name the module that declares them. Within-zone hops are left alone; those are a local style choice, not a boundary claim. 2. `find`'s three positional/flag checks existed in both daemon entry points with hand-repeated messages, and the copy in dispatchFindReadOnlyViaRuntime was unreachable — its only caller validates first. Both now call checkFindArgs in selectors/find.ts, beside parseFindArgs and isReadOnlyFindAction, for the reason that module's own comment already gives: so the two paths cannot disagree. The refusal is returned rather than thrown, because the two mechanisms are not observationally identical in the session event log. Type-only spine inversions: 61 -> 35. 4470 unit tests and every gate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * feat(layering): ratchet type-only spine inversions (R6) R5 ignores type-only edges by design — they cost nothing at runtime and do not affect cold start — so nothing was watching the direction they point. Ranking them the same way found 61 inversions, including contracts/ and utils/ declared in terms of rank-4 zones. 26 are fixed by the preceding commits; R6 pins the rest per zone pair so they can only shrink, and a new pair fails outright rather than being added to the baseline. The two remaining clusters each need their own change, and the baseline says so: the per-command Options/Result vocabulary declared inside the public Node-client surface, and the ADR 0003 daemon facet shape that core's descriptor registry composes. Both ratchet directions are covered: growth fails, and shrinking without lowering the number fails too, so the baseline cannot quietly stop describing the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs: record the import-graph findings behind this refactor A dated snapshot, not a normative document: when it disagrees with scripts/layering/, the gate wins. The graph tool that produced it lives on the claude/depgraph-viewer branch, deliberately out of this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * refactor(selectors): state the shared selector argument rules once R2 (commands-floor) forbids the daemon from importing commands/, and that is the right call: commands/ is the client-side surface — its only consumers are cli/, cli-schema/, mcp/, client/ and the composition roots — while the daemon is the executor on the other side of the wire. ADR 0008 protects exactly that seam. Relaxing R2 would let the executor depend on a client projection and pull CLI grammar and output formatting into the daemon's bundle. But the rule does force duplication: the daemon must validate independently because it accepts requests from any client, so 10 refusal messages existed in both zones. The only place a shared rule can live is below both, and selectors/ already held the parsers (splitIsSelectorArgs, splitSelectorFromArgs, isSupportedPredicate) and even the `is` predicate message — just not the checks that use them. Three drifts had already appeared in the `is` predicate rule alone: - commands/interaction/selectors.ts re-implemented the predicate list as an inlined seven-way `!==` chain while importing the message and hint from selectors/predicates.ts, so adding a predicate to the shared list would not have reached the CLI grammar. - That inlined chain compared the raw token, so the CLI rejected `is TEXT ...` while the daemon it hands the command to accepts it. The CLI now matches the executor; this is an intentional alignment, not an accident. - isCommand raised the same refusal without IS_PREDICATE_USAGE_HINT, so whether an agent got recovery guidance depended on which layer noticed first — the failure mode ADR 0010's audit calls out. checkIsPredicate, checkIsArgs, checkGetFormat, checkElementTargetArgs and checkWaitText now hold those rules, each beside the parser it wraps, and report a refusal rather than choosing how to raise it: the daemon returns a response, the command surface throws. Those mechanisms are not interchangeable — they write different session events — so the shared check stays out of that decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * feat(daemon): give ADR 0014's ref frame one transition, pin SessionState owners `SessionStore.get()` returns the live record out of a private Map and `set()` re-puts the same reference, so every `session.<field> = …` in the daemon is a durable write to store-owned state: 57 of them across 17 files, against 26 `set()` calls that are therefore ceremonial. Nothing at the store boundary can check what those writes are supposed to keep true. Measuring which module writes which field showed the problem is narrower than the raw count suggests — 16 of 27 fields already have exactly one writer. The sharp case is ADR 0014's ref frame: `refFrameState`, `refFrameScope`, `refFrameTree` and `refFrameGeneration` must move together or the frame is incoherent (an `active` state with a stale tree resolves refs against a namespace nobody authorized), yet complete issuance wrote them in ref-frame.ts and partial issuance wrote the same four in session-snapshot.ts. ref-frame.ts's own header claims to be "the single owner of the frame's transitions", and session-snapshot.ts documented itself as the exception. Both forms now go through `activateRefFrame`; they differ only in scope. `recordSession` deliberately moves alone in two paths (recording without arming a publication), so the save-script cluster gets no invented abstraction — it gets ownership instead. R7 records every field's owner and stops the set from growing quietly: a new SessionState field must declare one, a foreign write fails naming the owner to call, and an owner that stops writing must be removed so the table cannot drift into fiction. Field names are read out of the `SessionState` declaration, so a daemon module with an unrelated local named `session` — a provider or runner session — cannot trip it. 4475 unit tests and every gate pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs: record the reference semantics and refresh the findings SessionStore.get/set now document that the record is handed out live, since that is the fact behind R7. The findings snapshot picks up the resolved R2 question, the ref-frame consolidation and the two new gate scopes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * refactor(boundaries): rank every satellite zone, extract the provider port Second-order effect of the earlier rounds. With `utils` on the spine and `(root)` emptied of shared contracts, the eleven zones that were unranked "because ranking them would invent an order the architecture had not committed to" turned out to have a consistent rank already — the order was there, unasserted. Solving the constraint system showed one blocker: `utils/remote-config.ts` projected a remote-config profile into `CliFlags` while reaching up into `remote/`, and its only three consumers were in `cli/`. It moves there as `cli/remote-config-flags.ts`, and every satellite zone joins the spine. Ranked coverage goes from 730/895 files to 882/895. Only `(root)` stays out, and now for one stated reason: R2 forbids `daemon/` from importing `commands/`, so the files that wire them compose the spine from above. Ranking them exposed 22 type-only inversions R6 had never been able to see, and they were concentrated rather than scattered: - The device-provider port. `providers/` and `cloud-webdriver/` implement what the daemon calls, so both sides name `DeviceLease`, `LeaseLifecycleProvider`, `LeaseLifecycleContext` and `DeviceInventoryProvider` — now declared in contracts/device-provider.ts, below both. The adapters also imported the daemon's NARROWED `DaemonRequest` while only ever reading `req.flags`; they now name the public one from kernel/contracts. - `MetroPrepareKind` and the remote-config profile field groups move to contracts/ for the same reason: the command surface validates them and contracts/cli-flags.ts is composed from them. Two clusters remain, ratcheted with their reasons in TYPE_INVERSION_BASELINE: the client-types vocabulary, and `SessionAction`, which needs `CommandFlags` and `DaemonBatchStep` to move with it. Also fixes two things CI caught: the eight type re-exports my earlier import redirection orphaned (none published through any src/sdk/* entrypoint, so no public surface changes) and `isSupportedPredicate`, now module-private since `checkIsPredicate` is the admission API. `fallow-baselines/health.json` is keyed by path, so the moved cli-config entry moves with the file rather than being regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * fix(selectors): use the admitted predicate, not the raw option Review finding. `isCommand` called `checkIsPredicate` and then kept reading `options.predicate` for the capture policy, the `exists` branch, `evaluateIsPredicate`, the failure message and the returned result. Admission normalizes case, so an upper-case predicate was let past the gate and then evaluated against lower-case branches: `EXISTS` skipped its own branch and fell through to the generic path, and the result echoed the raw token. I widened admission at that surface without threading the normalized value through it — the CLI-grammar surface in the same change does use the admitted value. Every decision after admission now reads it. Two tests, both verified to fail without the fix: - a production-route regression driving `device.selectors.is` with `EXISTS`/`TEXT`, plus one pinning that an unknown predicate is still refused WITH the ADR 0010 usage hint; - a surface parity gate (selectors/__tests__/is-argument-surface-parity.test.ts) in the repo's existing parity style, asserting the daemon and CLI-grammar surfaces reach the same verdict and hand the same normalized predicate downstream across an input table. A helper-only test cannot catch a surface that admits correctly and then discards the result, which is what happened here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs: name the pre-push gate, and the formatter's path allowlist Both misses in this PR's review were process, not judgement, and the docs pointed the wrong way for both. AGENTS.md said "prefer the aggregate package.json scripts" without naming which aggregate, and CONTRIBUTING listed `pnpm test` and the targeted checks but never `pnpm check`. `check:tooling` looks like the gate and is a subset of it: it stops before the Fallow audit, so the dead exports this PR introduced passed a clean `check:tooling` and failed CI. Both files now name `pnpm check`, say what it covers, and say what it cannot (the device matrix). The same gap produced a second mistake twice: `oxfmt <path>` reformats whatever you point it at, while the repo's `format` script is an allowlist that excludes `scripts/` and every `.md`. One run reformatted 50 unrelated script files into a commit; the next nearly did it to AGENTS.md. AGENTS.md now says to run `pnpm format`, never `oxfmt <path>`. It also records the rule that cost a CI cycle: Fallow's baselines are keyed by path, so a renamed file needs its baseline entry moved, not the baselines regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * revert: undo stray formatter output across docs and scripts Three separate `oxfmt <path>` runs in this branch reformatted files the repo's `format` script deliberately excludes: 55 files under scripts/maestro-conformance plus scripts/perf, sync-mcp-metadata and the slow-test reporter, and 12 markdown files including six ADRs and docs/agents/. All of it was whitespace, quote style and markdown table padding — no content — but it inflated the diff a reviewer has to read and would have rewritten prose ownership across files this change has no business touching. All 70 are back to their origin/main content, so the diff outside src/ is now exactly this change's scope: three docs, scripts/layering, the Fallow baseline, and five provider integration tests. The rule this violated is now in AGENTS.md: run `pnpm format`, never `oxfmt <path>`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * style: reformat two provider tests with the repo's pinned oxfmt `pnpm format:check` failed in CI on the two files whose imports I merged by hand. The repo pins oxfmt 0.42.0 as a devDependency and both `format` scripts invoke `./node_modules/oxfmt/bin/oxfmt`; I had reformatted with `npx oxfmt`, which resolved 0.60.0, and the two versions disagree about wrapping a 100-column import. This is the rule AGENTS.md already states — run `pnpm format`, never oxfmt directly — so there is nothing to add to the docs, only to do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * fix(ci): install deps for the layering guard, and gate the zero-dep contract The Layering Guard job failed with ERR_MODULE_NOT_FOUND on `oxc-parser`. The job ran with `install-deps: false` — no `pnpm install`, so no `node_modules` — and R7 had started parsing the daemon with oxc-parser instead of matching assignment operators with a regex. `pnpm check:layering` passed on every local run, because locally `node_modules` is always there. The job now installs dependencies. The alternative was to put R7 back on a regex, which cannot see `??=` or a computed `session[key] =` write, so it would trade a correct rule for a fast job. That leaves the interesting part: the zero-dep contract is real for the jobs that keep it, and it is invisible to every local run, which is the worst combination a constraint can have. R8 makes it checkable. It reads the zero-dep job list out of `.github/workflows/` rather than restating it — declaring a job zero-dep is what puts it under the rule — walks each job's entry scripts and their whole relative-import closure, and requires every specifier to be a Node builtin or another repo file. A zero-dep job whose entry scripts the scan cannot identify fails too, so the rule cannot be escaped by changing how the job invokes them. Specifiers come from oxc-parser's module record, not a line scan. The closures include `--test` files, and a test about imports naturally embeds import syntax in a fixture string; the line scanner reported two such phantom violations in model.test.ts before the switch, which is how a gate stops being trusted. Verified by re-running the real gate against three injected regressions: the layering job back on `install-deps: false` (reproduces the exact CI failure, pointing at session-state.ts:24), a package import added to the still-zero-dep affected-selector closure, and a zero-dep job whose run step names no script. Also corrects the CONTEXT.md spine paragraph, which still described the satellite zones as deliberately unranked after they had all joined the ranked spine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * fix(layering): make R7 exhaustive, and follow session records through aliases Review finding: `SESSION_STATE_FIELD_OWNERS` covered 27 of `SessionState`'s 42 fields and nothing asserted parity, so a new field could be added and pass the gate by being invisible to it. R7's advertised claim — "every SessionState write is inside its declared owner" — was broader than what it checked. Investigating that turned up a second, larger gap the finding did not name: the scan only recognized a binding literally named `session`. The daemon names these records by role, so `nextSession`, `provisionalSession`, `completedSession`, `preRunSession` and `preEntrySession` were all invisible — and three of those writes were genuine violations R7 existed to catch: src/daemon/snapshot-runtime.ts:256 nextSession.snapshotScopeSource src/daemon/snapshot-runtime.ts:265 nextSession.snapshotGeneration src/daemon/handlers/session-replay-runtime.ts:707 preEntrySession.pendingRecordAndHeal The first two are the #1076 versioned-ref invariant: the generation advances exactly when the stored tree is replaced. That rule lived in `setSessionSnapshot` and had acquired a second statement of itself in snapshot-runtime.ts, whose own comment admitted the bypass. It now goes through `setSnapshotLineage` in the owning module. The third clears a watermark stamped by session-replay-resume.ts; `clearPendingRecordAndHealWatermark` puts the clear beside the stamp. Gate changes: - Binding detection accepts aliases, paired with the existing declared-field filter so an unrelated `…Session` local only registers if it also writes a field SessionState owns — where the remedy is the same anyway. - `fieldClassificationDrift` asserts parity in all three directions: unclassified, in-both, and naming a field SessionState no longer declares. - `STORE_OWNED_SESSION_STATE_FIELDS` classifies the 11 fields the store establishes at construction. It is a positive claim, so a direct write to one fails and names both remedies. - Four fields the widened scan made visible (`lease`, `deviceClaim`, `appName`, `saveScriptComplete`) got real owners. `nextSnapshotGeneration` is now module-private: replacing its only external call site orphaned the export, which `pnpm check` caught via Fallow. Verified against three injected regressions: a new SessionState field with no direct write (the reviewer's exact scenario), a foreign write through an alias binding, and a direct write to a store-established field. All three rejected. `pnpm check` green, 4486 unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur * docs(daemon): correct the snapshot-lineage claim, and pin the real contract Device verification of the snapshot-lineage route found that a ref pinned before a `diff` keeps resolving with no pinned-ref warning. That is the designed ADR 0014 behaviour, not a regression — the comment describing it was wrong, and I propagated it. `main`'s comment in snapshot-runtime.ts said a diff "leaves client refs pinned to the previous generation, which is exactly what the pinned warning diagnoses". The counter and the authorization epoch are different clocks: - `diff` passes `issuesRefsToClient: false`, so it never reactivates the frame; - `resolveRefStalenessWarning` compares a pin against the frame EPOCH, not the observation counter, and its own comment says why — a capture that bumped the counter must not make a valid pin from the issuing frame look stale. So advancing the counter is not the same as invalidating client refs, and the observable the comment promised does not exist. I carried the sentence into `setSnapshotLineage`'s doc when the transition moved, and then into a hardware verification request, which cost a reviewer a device run against a false claim. `setSnapshotLineage` itself is unchanged and was a pure move: same expressions, same inputs as the inline assignments it replaced, so this route behaves exactly as it does on main. A comment that contradicts the code should be an assertion instead, so the contract is now pinned in session-snapshot.test.ts: the diff advances the counter, preserves the epoch, leaves the pre-diff pin resolving without a warning, and still warns for a pin from a different frame. Verified to fail when the epoch comparison is swapped for the counter. A second test covers the keep-current branch, which had no coverage. `pnpm check` green, 4488 unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur --------- Co-authored-by: Claude <noreply@anthropic.com> |