docs: ADR-0012 Decision 6 — repair-transaction lifecycle (R7 + commit semantics) by thymikee · Pull Request #1234 · callstack/agent-device

thymikee · 2026-07-13T08:25:14Z

Summary

Amends the merged Decision 6 ("Agent-supervised re-record repair") of ADR-0012 with the repair-loop lifecycle rules two independent design reviews settled. The unifying frame: --save-script opens a multi-invocation repair TRANSACTION, committed only when the full plan completes.

New normative rule R7 — repair transaction lifetime. A repair-armed replay that returns REPLAY_DIVERGENCE with resume.allowed: true MUST NOT tear down the owning daemon/session; heal actions and replay --from continuations target that same live session until the transaction ends (strengthens R2's same-session continuation into a lifetime guarantee). Commit happens at teardown, gated on COMPLETE: any teardown of the session — explicit close, idle-reap, or daemon shutdown — commits when the transaction is COMPLETE, and aborts (no publish) when it is not. A reaped incomplete repair session must surface a specific REPAIR_SESSION_EXPIRED recovery error, never a bare SESSION_NOT_FOUND. A persistent-daemon precondition is explicitly rejected: an implementation that cannot keep-alive must fail-fast before step 1 with actionable guidance, never later with SESSION_NOT_FOUND.
Decision 4 resume clarification. One sentence: when resume.allowed: true, the diverging session is kept live and addressable for the in-flight repair, so the flag is not misleading.
Commit semantics sharpened (teardown-commits). The healed .ad is committed on any teardown of the session (explicit close, idle-reap, or daemon shutdown) but only when the transaction is COMPLETE, and is written atomically (transaction-scoped temp → atomic publish). A teardown while the transaction is INCOMPLETE never publishes (never a prefix): a plain close discards, an idle-reap/shutdown leaves a REPAIR_SESSION_EXPIRED tombstone. R6 defines the slice; this defines when the slice is complete/committed.
Terminal lifecycle steps. Non-target steps (a source close, or any step without targetEvidence) are already exempt from target-binding divergence per Decision 3 (a clarification, not a change) — they may still action-failure on a genuine dispatch failure but never selector-miss/identity-mismatch/identity-unverifiable for lacking evidence. The source plan's terminal close should be SKIPPED while armed; the agent finalizes via close --save-script.
Clobber P2 — "complete healed artifact" defined. The no-clobber guard protects only a COMPLETE, review-worthy artifact (established by a # agent-device:heal-complete sentinel written only on commit, made trustworthy by the atomic temp→publish). Incomplete/partial files are always overwritable. Auto-versioned names (.healed.2.ad) are explicitly out of scope.

R1-R6 are kept intact; this adds R7, the four clarifications, and the matching Validation + Migration-step-8 items.

Test plan

Docs-only change; no code touched.
Repo lint/format (oxlint/oxfmt) do not cover docs/**, so no markdown lint gate applies.
Fresh end-to-end read: rules run R1-R7 with "seven rules"; headings 1-6 intact; balanced markers; no contradiction between R7 keep-alive, the teardown-commits emission rule, and R2 (commit is gated on the transaction reaching COMPLETE and happens at any teardown — close, idle-reap, or shutdown; a teardown while INCOMPLETE aborts with no prefix; --from continuation stays on the one live session).

thymikee · 2026-07-13T08:43:05Z

Review at c7be7f6 found six architecture blockers:

resume.allowed overpromises liveness outside repair transactions. Decision 4 emits this field for ordinary replay divergences too, while R7 is opened only by --save-script. Scope the new lifetime guarantee explicitly to a repair-armed divergence (or add a separate repair-liveness signal); allowed currently proves only plan resumability.
close --save-script can still commit an incomplete prefix. The ADR says artifacts are committed only after successful full-plan completion, but also permits explicit close --save-script as a commit trigger. After a divergence, an agent can call that command before resuming the remaining plan. Define a machine-enforced completed transaction state and reject/abort finalize before it; otherwise the “never prefix-only” guarantee is contradictory.
The post-auto-commit state is undefined. If full-plan success publishes automatically, R7 still says the session lives until close; a later close --save-script could attempt a second commit and trip no-clobber. Specify whether close is teardown-only/idempotent after commit and whether the finalizer close is serialized into the healed script.
Terminal source close skipping lacks a complete contract. Define “terminal” precisely, add it to migration step 8, and require a regression proving an armed replay skips that source action without deleting the session. Current replay dispatches every action and close deletes the session.
Expiry and atomicity need implementable ownership. REPAIR_SESSION_EXPIRED requires a bounded tombstone (key, owner, lifetime, restart behavior) to distinguish a reaped repair from an ordinary missing session. Atomic publication also requires the temp file to be created beside the final target (explicit outputs may cross filesystems) and a race-safe no-clobber operation, not only “transaction-scoped temp.”
The ADR status is materially stale. It still says “Nothing in this ADR is implemented yet,” although feat(replay): ADR 0012 decision 6 — agent-supervised re-record repair #1228 and earlier migration steps are on main. Update status/progress so current behavior is clearly separated from the new unimplemented R7/commit requirements, ideally with a tracking issue for the remaining implementation.

No fixer was dispatched. External Claude review was not run because export was not separately authorized.

…C4/C5) Machine-enforceable refinements from the ADR review: - C1: keep-alive keys on the REPAIR-ARMED condition, not resume.allowed. isHeldRepairDivergence (daemon-client-lifecycle.ts) gates on the daemon's new resume.repairSessionHeld signal (set on every repair-armed divergence in runReplayScriptFile), so a held divergence with allowed:false still keeps the session addressable; a plain non-repair divergence gets neither. - C2 (key gap): commit is gated on a COMPLETED transaction, not merely on close. New session state ARMED -> COMPLETE (saveScriptComplete, set when a repair-armed run reaches its last executable step) -> COMMITTED (saveScriptCommitted). SessionScriptWriter.write publishes only when complete, so a close/close --save-script after a divergence but before the plan finishes ABORTS (no prefix) rather than committing one; a committed session is idempotent. - C4: isRepairArmedTerminalClose skips the source plan's terminal close while armed WITHOUT deleting the session; the run reaches COMPLETE and the agent finalizes via close --save-script. - C5a: repair-armed un-committed sessions no longer block idle-reap (hasReapBlockingOpenSessions); teardown auto-commits a COMPLETE transaction or, when incomplete, writes a bounded repair tombstone. The router rewrites the resulting SESSION_NOT_FOUND to REPAIR_SESSION_EXPIRED with re-run guidance; a fresh replay --save-script clears the tombstone. - C5b: publishHealedScriptAtomically writes the temp file in the target's own directory (same-filesystem rename, even for cross-mount --save-script=<path>) and publishes via linkSync (atomic create-if-absent) so the complete-artifact no-clobber guard has no TOCTOU window.

thymikee · 2026-07-13T09:47:38Z

Coordinator review at 90fe25590: the R7 direction is sound, but two contract ambiguities block merging this ADR before #1235.

Choose one clean-completion model. The ADR says there is no auto-commit before explicit close and requires tombstones for reaped uncommitted COMPLETE sessions, while sibling fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish #1235 auto-commits COMPLETE repair output during teardown. Specify whether clean replay --save-script keeps the daemon/session addressable until explicit close or teardown commits it, then align fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish #1235.
Specify continuation flag semantics. The ADR examples continue with replay --from without repeating --save-script, implying persisted repair-armed state. fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish #1235 currently keeps a held divergence alive only when the current request also carries saveScript. Either require the flag on every continuation and update all examples/help, or require implementation to key off persisted transaction state.

Ordering: finalize/merge #1234 first, then rebase and review #1235 against that contract. No fixer dispatched; readiness remains withheld.

…C4/C5) Machine-enforceable refinements from the ADR review: - C1: keep-alive keys on the REPAIR-ARMED condition, not resume.allowed. isHeldRepairDivergence (daemon-client-lifecycle.ts) gates on the daemon's new resume.repairSessionHeld signal (set on every repair-armed divergence in runReplayScriptFile), so a held divergence with allowed:false still keeps the session addressable; a plain non-repair divergence gets neither. - C2 (key gap): commit is gated on a COMPLETED transaction, not merely on close. New session state ARMED -> COMPLETE (saveScriptComplete, set when a repair-armed run reaches its last executable step) -> COMMITTED (saveScriptCommitted). SessionScriptWriter.write publishes only when complete, so a close/close --save-script after a divergence but before the plan finishes ABORTS (no prefix) rather than committing one; a committed session is idempotent. - C4: isRepairArmedTerminalClose skips the source plan's terminal close while armed WITHOUT deleting the session; the run reaches COMPLETE and the agent finalizes via close --save-script. - C5a: repair-armed un-committed sessions no longer block idle-reap (hasReapBlockingOpenSessions); teardown auto-commits a COMPLETE transaction or, when incomplete, writes a bounded repair tombstone. The router rewrites the resulting SESSION_NOT_FOUND to REPAIR_SESSION_EXPIRED with re-run guidance; a fresh replay --save-script clears the tombstone. - C5b: publishHealedScriptAtomically writes the temp file in the target's own directory (same-filesystem rename, even for cross-mount --save-script=<path>) and publishes via linkSync (atomic create-if-absent) so the complete-artifact no-clobber guard has no TOCTOU window.

… semantics) Frames --save-script as a multi-invocation repair TRANSACTION committed only on completion: - R7 (new normative rule): a repair-armed replay returning resume.allowed:true must keep its daemon/session live until close; heal/--from target that same session (strengthens R2). Plain close/teardown/idle-reap while armed = abort/ discard. Bounded-expiry must surface REPAIR_SESSION_EXPIRED, not bare SESSION_NOT_FOUND. Persistent-daemon precondition rejected: fail-fast before step 1, never a later SESSION_NOT_FOUND. - Decision 4 resume: one sentence noting the session is kept addressable so resume.allowed:true is not misleading. - Commit semantics: healed .ad committed only on full-plan completion or explicit close --save-script; never on divergence-only exit, teardown, or idle-reap; atomic temp->publish. R6 defines the slice, this defines when it is complete. - Terminal lifecycle steps: non-target steps (incl. source close, unannotated steps) are already exempt from target-binding divergence per decision 3 (clarification, not a change); prefer SKIPPING the source terminal close while armed and finalize via close --save-script. - Clobber P2: no-clobber guards a COMPLETE (heal-complete sentinel) artifact only; partials are overwritable; auto-versioned names out of scope. Validation + migration step 8 extended accordingly.

- C1: R7 keep-alive keys off a DISTINCT resume.repairSessionHeld signal, not resume.allowed (which means plan-resumability and fires for every divergence). - C2: define the ARMED -> COMPLETE -> COMMITTED commit state machine. close before COMPLETE = abort (publish nothing); close at COMPLETE = atomic commit; close after COMMITTED = idempotent teardown, no re-publish. No auto-commit. - C4: precise terminal-source-close contract (last source action == close) — SKIPPED (not dispatched) under armed repair so the session is not deleted; regression required (added to migration step 9). - C5a: REPAIR_SESSION_EXPIRED backed by a bounded tombstone keyed by session key (owner + expiry), cleared by a fresh replay --save-script. - C5b: atomic publication temp file in the target's own directory; race-safe no-clobber via create-exclusive/rename-if-absent. - C6: Status block corrected — Decisions 1-6 base MERGED (#1228 et al.); R7 + commit machine UNIMPLEMENTED, tracked by #1235. Migration split into step 8 (merged) and step 9 (#1235). Validation extended for all of the above.

…tion Resolves the two contract ambiguities blocking merge (aligns with #1235): - Completion model = TEARDOWN-COMMITS (not explicit-close-only). A repair-armed session stays addressable until the transaction ends; ANY teardown (explicit close, idle-reap, daemon shutdown) commits the healed .ad atomically iff the transaction is COMPLETE, else aborts with no publish (never a prefix). An incomplete reap/shutdown leaves the REPAIR_SESSION_EXPIRED tombstone; an explicit close of an incomplete tx just discards. Kept the ARMED->COMPLETE->COMMITTED machine and idempotent post-COMMITTED teardown; removed the "no auto-commit / commit only on explicit close" language. - Continuation by PERSISTED transaction state, not the per-request flag: replay --from <n> --plan-digest <sha> resumes on the persisted repair-armed session WITHOUT repeating --save-script; --save-script appears only on the transaction opener. Implementation MUST key keep-alive/continuation off persisted state. Decision 4 repairSessionHeld updated to match. Edited: R7, Decision 4 resume signal, terminal-close, Emitting, commit state machine, tombstone, migration step 9, and validation.

…C4/C5) Machine-enforceable refinements from the ADR review: - C1: keep-alive keys on the REPAIR-ARMED condition, not resume.allowed. isHeldRepairDivergence (daemon-client-lifecycle.ts) gates on the daemon's new resume.repairSessionHeld signal (set on every repair-armed divergence in runReplayScriptFile), so a held divergence with allowed:false still keeps the session addressable; a plain non-repair divergence gets neither. - C2 (key gap): commit is gated on a COMPLETED transaction, not merely on close. New session state ARMED -> COMPLETE (saveScriptComplete, set when a repair-armed run reaches its last executable step) -> COMMITTED (saveScriptCommitted). SessionScriptWriter.write publishes only when complete, so a close/close --save-script after a divergence but before the plan finishes ABORTS (no prefix) rather than committing one; a committed session is idempotent. - C4: isRepairArmedTerminalClose skips the source plan's terminal close while armed WITHOUT deleting the session; the run reaches COMPLETE and the agent finalizes via close --save-script. - C5a: repair-armed un-committed sessions no longer block idle-reap (hasReapBlockingOpenSessions); teardown auto-commits a COMPLETE transaction or, when incomplete, writes a bounded repair tombstone. The router rewrites the resulting SESSION_NOT_FOUND to REPAIR_SESSION_EXPIRED with re-run guidance; a fresh replay --save-script clears the tombstone. - C5b: publishHealedScriptAtomically writes the temp file in the target's own directory (same-filesystem rename, even for cross-mount --save-script=<path>) and publishes via linkSync (atomic create-if-absent) so the complete-artifact no-clobber guard has no TOCTOU window.

thymikee · 2026-07-13T11:45:26Z

Readiness follow-up: the amended ADR is now technically coherent, but this PR description still states that daemon teardown and idle-reap never publish. The ADR now says teardown commits a COMPLETE transaction. Please update the PR description to match the accepted contract before merge.

Ordering remains #1234 first, then #1235.

thymikee · 2026-07-13T12:41:46Z

Addressed: updated the PR description to the finalized teardown-commits model — any teardown (close, idle-reap, or shutdown) commits when the transaction is COMPLETE, an incomplete teardown aborts (plain close discards; reap → REPAIR_SESSION_EXPIRED tombstone). No longer states teardown/idle-reap 'never publish'.

@ref

review) BLOCKER 1 — a reaped `replay --from` continuation surfaced as REPLAY_DIVERGENCE instead of REPAIR_SESSION_EXPIRED. runReplayScriptFile now preflights a resume (entryIndex > 0) against a missing session and returns SESSION_NOT_FOUND, which the router's tombstone translation turns into REPAIR_SESSION_EXPIRED — instead of the missing session surfacing later as a divergence that slipped past it. BLOCKER 2 — teardown/close commit failures were invisible and cost the session. SessionScriptWriter.write no longer swallows/throws a repair-commit failure; it returns { written:false, error } (no-clobber, bare-@ref, or fs error, all distinguishable). handleCloseCommand commits the repair BEFORE any destructive teardown and, on failure, returns the error WITHOUT deleting the session (kept for retry); on success it reports the healed `savedScript` path. BLOCKER 3 — the no-clobber publish is already race-safe via linkSync (atomic create-exclusive, temp in the target dir). Added a competing-writer regression proving a second writer never overwrites a COMPLETE artifact and gets a clear error. BLOCKER 4 — a minimal `[open, terminal close]` never armed (pre-open arming is a no-op). The armer now runs BEFORE the terminal-close check each iteration, plus a post-loop arm, so a script whose first step CREATES the session still arms and the terminal-close/commit-state machine applies. Refactored handleCloseCommand (resource teardown + non-repair write extracted) to stay under the complexity gate; removed a now-stale fallow suppression.

thymikee · 2026-07-13T13:46:53Z

Coordinator final readiness at ac367d6b3: the ADR contract was independently reviewed and is coherent, the PR description now matches the finalized teardown-commits model, and all checks pass. Clean and mergeable.

Merge ordering remains #1234 first; #1235 must follow and independently satisfy its outstanding implementation/evidence findings.

github-actions · 2026-07-13T13:47:56Z

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-13 13:47 UTC

…C4/C5) Machine-enforceable refinements from the ADR review: - C1: keep-alive keys on the REPAIR-ARMED condition, not resume.allowed. isHeldRepairDivergence (daemon-client-lifecycle.ts) gates on the daemon's new resume.repairSessionHeld signal (set on every repair-armed divergence in runReplayScriptFile), so a held divergence with allowed:false still keeps the session addressable; a plain non-repair divergence gets neither. - C2 (key gap): commit is gated on a COMPLETED transaction, not merely on close. New session state ARMED -> COMPLETE (saveScriptComplete, set when a repair-armed run reaches its last executable step) -> COMMITTED (saveScriptCommitted). SessionScriptWriter.write publishes only when complete, so a close/close --save-script after a divergence but before the plan finishes ABORTS (no prefix) rather than committing one; a committed session is idempotent. - C4: isRepairArmedTerminalClose skips the source plan's terminal close while armed WITHOUT deleting the session; the run reaches COMPLETE and the agent finalizes via close --save-script. - C5a: repair-armed un-committed sessions no longer block idle-reap (hasReapBlockingOpenSessions); teardown auto-commits a COMPLETE transaction or, when incomplete, writes a bounded repair tombstone. The router rewrites the resulting SESSION_NOT_FOUND to REPAIR_SESSION_EXPIRED with re-run guidance; a fresh replay --save-script clears the tombstone. - C5b: publishHealedScriptAtomically writes the temp file in the target's own directory (same-filesystem rename, even for cross-mount --save-script=<path>) and publishes via linkSync (atomic create-if-absent) so the complete-artifact no-clobber guard has no TOCTOU window.

@ref

review) BLOCKER 1 — a reaped `replay --from` continuation surfaced as REPLAY_DIVERGENCE instead of REPAIR_SESSION_EXPIRED. runReplayScriptFile now preflights a resume (entryIndex > 0) against a missing session and returns SESSION_NOT_FOUND, which the router's tombstone translation turns into REPAIR_SESSION_EXPIRED — instead of the missing session surfacing later as a divergence that slipped past it. BLOCKER 2 — teardown/close commit failures were invisible and cost the session. SessionScriptWriter.write no longer swallows/throws a repair-commit failure; it returns { written:false, error } (no-clobber, bare-@ref, or fs error, all distinguishable). handleCloseCommand commits the repair BEFORE any destructive teardown and, on failure, returns the error WITHOUT deleting the session (kept for retry); on success it reports the healed `savedScript` path. BLOCKER 3 — the no-clobber publish is already race-safe via linkSync (atomic create-exclusive, temp in the target dir). Added a competing-writer regression proving a second writer never overwrites a COMPLETE artifact and gets a clear error. BLOCKER 4 — a minimal `[open, terminal close]` never armed (pre-open arming is a no-op). The armer now runs BEFORE the terminal-close check each iteration, plus a post-loop arm, so a script whose first step CREATES the session still arms and the terminal-close/commit-state machine applies. Refactored handleCloseCommand (resource teardown + non-repair write extracted) to stay under the complexity gate; removed a now-stale fallow suppression.

Agent-supervised re-record repair lifecycle, rebased onto #1225's failure-isolated close teardown. Consolidated from the earlier iterative rounds into the final teardown-commits model: - R7 keep-alive keyed off PERSISTED transaction state (repairSessionHeld signal), so a `replay --from` continuation without --save-script is still held on divergence. - Commit gated on transaction COMPLETION (saveScriptComplete/saveScriptCommitted), never on `close` alone — no prefix is ever published. - Single commit path: `commitRepairBeforeClose` runs before #1225's `runSessionCloseTeardown` destructive steps; a repair-armed session skips the teardown's ordinary writeSessionLog, non-repair keeps it. Idle-reap/shutdown commit-on-completion or tombstone via `finalizeRepairTeardown`. - BLOCKER fixes: reaped `replay --from` -> REPAIR_SESSION_EXPIRED; commit failures surfaced (not swallowed) and keep the session for retry (with healed-path reporting on success); race-safe atomic no-clobber publish; minimal `[open, close]` arms the transaction. Integrated with #1225: keeps runSessionCloseTeardown's failure-isolated cleanup + preserved platform-close error; repair commit happens first so a failed commit keeps the session addressable.

@ref

…it, close-as-lifecycle, atomic publish (#1235) * fix(replay): repair-transaction lifecycle (ADR 0012 decision 6 / #1234) Agent-supervised re-record repair lifecycle, rebased onto #1225's failure-isolated close teardown. Consolidated from the earlier iterative rounds into the final teardown-commits model: - R7 keep-alive keyed off PERSISTED transaction state (repairSessionHeld signal), so a `replay --from` continuation without --save-script is still held on divergence. - Commit gated on transaction COMPLETION (saveScriptComplete/saveScriptCommitted), never on `close` alone — no prefix is ever published. - Single commit path: `commitRepairBeforeClose` runs before #1225's `runSessionCloseTeardown` destructive steps; a repair-armed session skips the teardown's ordinary writeSessionLog, non-repair keeps it. Idle-reap/shutdown commit-on-completion or tombstone via `finalizeRepairTeardown`. - BLOCKER fixes: reaped `replay --from` -> REPAIR_SESSION_EXPIRED; commit failures surfaced (not swallowed) and keep the session for retry (with healed-path reporting on success); race-safe atomic no-clobber publish; minimal `[open, close]` arms the transaction. Integrated with #1225: keeps runSessionCloseTeardown's failure-isolated cleanup + preserved platform-close error; repair commit happens first so a failed commit keeps the session addressable. * fix(replay): make the atomic publish primitive decide the no-clobber race winner BLOCKER 1 (coordinator re-review): after linkSync saw an existing target, the no-clobber publish fell back to an unconditional renameSync once the target was classified "incomplete" — two concurrent writers could both read the SAME pre-existing partial as overwritable and both renameSync over it, each returning success with no signal to the loser. A silent, undetectable clobber. publishNoClobberAtomically now makes every winner decision an atomic primitive: - linkSync is the only way to win outright (EEXIST iff a file is at the target at that instant). - On EEXIST, the existing file is grabbed via an atomic renameSync into a private, uniquely-named quarantine path *before* it is inspected, so the completeness check never races the shared path. A competing writer's own grab racing ours surfaces as ENOENT, and we re-evaluate from the top instead of trusting a stale read. - A COMPLETE quarantined file is restored (best effort) and the publish is refused; a genuinely partial one is discarded and the exclusive linkSync is retried. Every interleaving converges on exactly one winning linkSync and every other writer observing a definitive, thrown "already exists" — never two silent successes, never a torn file. Adds a regression (session-script-writer.test.ts) with both writers starting against the SAME pre-existing partial target, using a renameSync spy to force a genuine interleaving (writer B's whole publish runs inside writer A's grab step) instead of the existing competing-writer test's sequential complete-vs-complete scenario, which never exercised this race. * fix(daemon): preserve failed COMPLETE-transaction commits instead of a generic expiry BLOCKER 2 (coordinator re-review): finalizeRepairTeardown ignored the writer's { written: false, error } outcome, so a COMPLETE transaction whose commit failed at idle-reap/daemon-shutdown teardown (no-clobber refusal, bare-@ref, or a filesystem error) was silently swallowed. Daemon teardown then deleted the session and left a generic "reaped before it was finalized" REPAIR_SESSION_EXPIRED tombstone — losing the only record that a commit was even attempted, let alone why it failed. finalizeRepairTeardown now captures the writer's result. On a real commit failure it writes a distinct commit-failure tombstone (RepairSessionTombstone gains an optional commitFailure: { code, message }); request-router's repairExpiredIfTombstoned surfaces that as a new REPAIR_COMMIT_FAILED error carrying the real cause instead of folding it into REPAIR_SESSION_EXPIRED. Adds a new AppErrorCode REPAIR_COMMIT_FAILED (kernel/errors.ts) with its own hint, a session-store regression proving finalizeRepairTeardown preserves the failure (and leaves the prior complete artifact untouched), and a request-router regression proving the router translates a commit-failure tombstone to REPAIR_COMMIT_FAILED rather than the generic expiry. * fix(daemon): record the skipped terminal close in idle-reap/shutdown auto-commit BLOCKER 3 (coordinator re-review): the source plan's terminal `close` is skipped-while-armed (Fix 3), so it never lands in session.actions. The explicit `close --save-script` path accounts for this by recording a synthetic finalize close (commitRepairBeforeClose) before committing, but finalizeRepairTeardown's auto-commit at idle-reap/daemon-shutdown never runs that handler — its committed healed .ad was missing its own terminal close, so the ADR's "self-contained, fresh-replayable artifact" requirement didn't hold for this path even though the existing auto-commit test only checked existence + the completeness sentinel. finalizeRepairTeardown now calls a new recordRepairFinalizeCloseIfCommitting before writeSessionLog, mirroring commitRepairBeforeClose's recording exactly (same command/positionals/flags shape), but only when the transaction is actually about to be committed (COMPLETE, not yet COMMITTED) — an aborted transaction's write is a no-op regardless. Strengthens the existing auto-commit test (session-replay-repair-transaction .test.ts) to use a source plan with a real terminal close, and to parse the committed script and assert it ends with ['open', 'click', 'close'] with no bare @ref — not just sentinel/existence. Adds a session-store.ts unit regression exercising finalizeRepairTeardown directly for the same self-contained-artifact assertion. * fix(daemon): serialize the no-clobber publish decision behind an exclusive lock BLOCKER 1 (review follow-up): publishNoClobberAtomically's inspect/restore/ publish sequence was atomic per-step but not exclusive as a whole. Writer A could quarantine an existing COMPLETE target, and — before A restored it — writer B could linkSync its own COMPLETE artifact into the now-empty target and return success, only for A's restore (renameSync, which replaces an existing destination per POSIX) to silently stomp B's freshly published bytes. Wrap the whole decide-and-act sequence in an exclusive publish lock (acquireNoClobberLock/releaseNoClobberLock, an atomic linkSync claim over a PID-stamped lock file) so a competing writer for the same scriptPath cannot begin its own decision until the lock holder's sequence has finished and released it. A lock whose PID is provably dead is reclaimed immediately; a lock held by a live process is never stolen, only waited on with a bounded backoff before failing loudly. Adds a deterministic regression that forces the exact reported interleaving via a renameSync spy (mirroring the existing BLOCKER 1 test's technique) and confirms the pre-existing COMPLETE artifact is never clobbered. Also relaxes the older PARTIAL-race test's loser-message assertion, since a losing writer may now fail via lock contention instead of the no-clobber-specific message, depending on interleaving timing. * fix(daemon): report retriable:true for a preserved repair-close failure BLOCKER 3: buildRepairCloseFailureResponse preserves the session specifically so the agent can retry close/close --save-script, but reported details.retriable: false — machine-consistent recovery guidance requires retriable: true whenever the session was kept addressable for a retry. Extends the existing BLOCKER 2b/2c no-clobber-failure test with an assertion on this contract. * fix(daemon): run the repair close's platform close before committing BLOCKER 2: commitRepairBeforeClose recorded a successful terminal `close` and published the healed artifact BEFORE dispatchTargetedPlatformClose ran. If the platform close then failed, the session was torn down and the committed .ad falsely contained a successful close — contradicting the existing failed-close lifecycle contract (a failed close is never recorded as Closed). For a repair-armed session, dispatch the targeted platform close first; only on success does the commit (record + publish) proceed. On failure, return without touching the session at all — same as the existing commit-failure contract, so the agent can fix the cause and retry. runSessionCloseTeardown gains a skipPlatformClose flag so the already-confirmed-successful close is never dispatched a second time during teardown. Adds a regression: a COMPLETE repair whose targeted platform close rejects must not commit a healed .ad, must not record a close action, and must keep the session addressable for retry; a subsequent successful retry then commits cleanly with exactly one terminal close. * fix(daemon): close the no-clobber lock's dead-writer TOCTOU with rename-CAS Two waiters could both observe the same dead-PID publish lock and both decide to reclaim it. If one waiter's reclaim (remove + re-acquire with its own LIVE lock) completed inside the other's decision window, the first waiter's stale rmSync(lockPath) deleted the SECOND waiter's live lock by pathname (not the dead one it actually inspected), letting both enter the exclusive publish section at once. Reclaim is now a rename-based compare-and-swap: renameSync(lockPath, uniquePath) is the atomic claim (only one racer's rename of a given source ever succeeds; the loser gets ENOENT and retries). Only the winner inspects what it actually grabbed at the private claim path — if genuinely dead, discard it; if the claim raced with someone else's fresh reclaim and grabbed their live lock instead, restore it untouched and back off. The live holder's lock is never stolen. Regression drives the exact two-reclaimer interleaving deterministically via a readFileSync spy (writer B reclaims+re-acquires live, inside writer A's reclaim window) and confirms it fails against the prior rmSync-based reclaim. * fix(daemon): surface repair-close retriable/diagnosticId/logPath at the wire top level buildRepairCloseFailureResponse hand-rolled its response shape instead of going through normalizeError, so it put retriable under error.details.retriable — a location neither the router's enrichDaemonError nor the client reads (both read the top-level DaemonError.retriable) — and silently dropped the underlying platform/ commit error's details, diagnosticId, and logPath entirely. Now routes through normalizeError like every other AppError -> DaemonResponse conversion in this codebase, preserving the underlying error's details/diagnosticId/logPath, with retriable forced true at the top level (the session is retained specifically for retry, which must never be contradicted by the underlying error's own classification). Also fixes a companion gap in finalizeDaemonResponse: it rebuilds every handler-RETURNED (non-thrown) failure response into a fresh AppError before re-normalizing, but only carried hint/diagnosticId/logPath through that reconstruction, not retriable/supportedOn — so even a handler setting them correctly at the top level still lost them at this step. Both are now carried through the same way, discovered only by verifying the close fix through the actual router boundary as requested. Regression: an updated handler-level test confirms diagnosticId/ logPath/details survive a repair-close failure, and a new router-level test (through createRequestHandler, not just the raw builder) confirms retriable:true and the platform error's diagnosticId/logPath/details all survive to the client. Both fail against the pre-fix code. * fix(daemon): never re-dispatch an already-succeeded repair-close platform close When a repair-armed close's targeted platform close SUCCEEDED but the subsequent script commit FAILED (no-clobber refusal, a bare-@ref failure, or an fs error), the session was correctly retained for retry -- but nothing recorded that the platform close had already happened. A retry (close --save-script=<other>) dispatched dispatchTargetedPlatformClose again, so a non-idempotent backend could fail or wedge recovery on a second close of an already-closed target. SessionState now carries repairPlatformCloseSucceeded, set the moment the platform close returns success. A subsequent repair close consumes it and skips straight to the commit instead of re-dispatching; it is cleared once the transaction's outcome (commit or abort) is settled, so it never lingers past a single close attempt. Regression: platform close succeeds, commit fails (no-clobber), session is retained; a retry does not re-invoke dispatchTargetedPlatformClose (asserted via call count) and still commits cleanly to the retry path. Fails against the pre-fix code (dispatch called twice). * fix(daemon): enforce complete-artifact protection on explicit --save-script targets The explicit --save-script=<path> publish path bypassed the no-clobber completeness guard entirely (protectComplete only gated on the DEFAULT healed-sibling marker), so it silently overwrote even a sentinel-marked COMPLETE healed artifact at a caller-directed path. An explicit target is caller-DIRECTED (which path to write to), never caller-AUTHORIZED to destroy an unreviewed prior healed diff sitting there. Gate protectComplete on repairArmed instead of the defaulted-path marker, so every repair-armed publish (default sibling or explicit target alike) refuses to clobber a COMPLETE artifact. Ordinary (non-repair) recordings are unaffected: they never carry the completeness sentinel, so the guard never actually engages for them. * fix(daemon): replace PID-liveness lock reclaim with a TTL publish lease reclaimDeadLock (grab lock away -> inspect PID -> restore if live) was structurally race-prone: a three-writer interleaving let waiter A rename waiter B's now-LIVE lock away (to inspect it), waiter C linkSync its own lock into the momentarily-empty path, then A's "restore" (renameSync, which replaces an existing destination) silently clobbered C's freshly acquired lock -- and the pathname-based release could then remove a successor's lock, not the caller's own. Replace it with a TTL lease (LEASE_TTL_MS = 30s). The lock file's content is now a unique owner token plus its own creation timestamp (pid:random:createdAtMs); staleness is judged purely from that embedded timestamp, never by asking the OS whether a PID is alive. A stale lease is stolen via a single atomic renameSync(lockPath, <lockPath>.expired.<id>) -- exactly one caller can ever win that rename for a still-existing source -- and the grabbed content is always discarded outright: there is no restore path at all. verifyOwnership re-checks the lease immediately before the publish critical section, so a writer whose lease gets displaced underneath it (by a stale steal decision racing a concurrent re-acquire) is never fooled into publishing unprotected -- it safely aborts instead. releaseLease only unlinks the lock file when its current token still matches the caller's own, so release can never delete a successor's lock. Regression coverage (session-script-writer.test.ts): a fresh lease is never stolen; an expired lease is stolen and reclaimed cleanly; and the reviewer's exact three-writer interleaving (a stale steal decision grabbing a concurrently-re-acquired fresh lease) is driven deterministically via spies on renameSync/linkSync, asserting exactly one holder ever enters the critical section, no live claim is silently clobbered by a restore, and release never deletes a successor's lock. Confirmed the new tests fail against the old reclaimDeadLock implementation and pass with the lease. * fix(daemon): bind the repair-close platform-close marker to request identity repairPlatformCloseSucceeded was session-wide, not bound to WHICH close request actually succeeded. An untargeted close performs no platform operation (shouldDispatchPlatformClose is false with no positional target), yet the flag was still set as though a real close had run; a retry with a DIFFERENT identity -- a target newly added, or a changed target -- then wrongly skipped the platform close entirely and committed as though it had run. Bind the marker to the request's identity: repairPlatformCloseIdentity records the target (positionals) of the close whose platform close last succeeded -- the only thing that changes what dispatchTargetedPlatformClose actually does (close's other flags, shutdown and saveScript, feed the post-teardown shutdown and the commit path respectively, never the platform close dispatch itself). A retry only skips the platform close when BOTH repairPlatformCloseSucceeded is true AND the identity matches; otherwise it re-runs. Regressions (session-replay-repair-transaction.test.ts): an untargeted-then-targeted retry and a changed-target retry both must re-dispatch the platform close (asserted via the dispatch mock call count/args), not skip it. Confirmed both fail against the prior session-wide boolean and pass with the identity-bound marker. * fix(daemon): surface a shutdown-time repair-commit failure before client cleanup A successful owned one-shot replay --save-script marks the transaction COMPLETE and returns success BEFORE publication -- the actual commit is deferred to daemon teardown (finalizeRepairTeardown), which runs inside the daemon process's own shutdown handler and, on failure, writes a REPAIR_COMMIT_FAILED tombstone. cleanupDaemonAfterRequest then removed the owned ephemeral state dir REGARDLESS of that tombstone, so the caller received success while the failure and its only recovery evidence were deleted in the same breath. session-store.ts exports findUnrecoveredRepairCommitFailure(sessionsDir), scanning every session subdirectory for a non-expired tombstone carrying commitFailure -- the client has no live SessionStore/session name to key off of, only the owned state dir's filesystem path. cleanupDaemonAfterRequest checks for it (after stopDaemonProcessForTakeover, which waits for the daemon to actually exit -- by then any tombstone the daemon's own shutdown handler would write is already on disk) before rmSync'ing the state dir: if found, the state dir is preserved and the response is overridden to a REPAIR_COMMIT_FAILED error instead of the raw success. daemon-client.ts's sendToDaemon now returns cleanup's result rather than the raw request result (restructured as a caught-and-rethrown error rather than a `return` inside `finally`, which oxlint's no-unsafe-finally rejects and which would also swallow a thrown request failure). Regression (daemon-client-lifecycle.test.ts): forces a shutdown-time commit failure by pre-seeding the tombstone in the owned state dir before the client's cleanup runs, and asserts the REPAIR_COMMIT_FAILED response is surfaced and the state dir (with the tombstone) survives. Confirmed it fails without the fix (raw success returned, state dir removed). * fix(daemon): replace the no-clobber publish lock with refuse-on-exist The TTL-lease/reclaim machinery only existed to auto-overwrite a partial healed artifact while never clobbering a complete one — but a concurrent complete-vs-complete race was already correct with a plain exclusive linkSync (first wins, second sees EEXIST), and a leftover partial is a degenerate state, not something to silently replace. Publish is now a single exclusive linkSync: absent target succeeds, ANY pre-existing target (complete or partial, default sibling or explicit --save-script path) is refused. Removes the whole lock/lease/reclaim race class. * docs(daemon): scope refuse-on-exist contract/comments to ordinary recording too PR #1235 review blocker: SessionScriptWriter.write's refuse-on-exist publish is uniform across repair-armed heals AND ordinary (non-repair) open/close --save-script recording, but the ADR contract and several comments still read as if only healed repair publication is refused and ordinary recording keeps the old rename-replace overwrite. Maintainer decision is to keep the behavior uniform and fix the docs/comments/coverage instead of re-scoping. - session-script-writer.ts: clarify isRepairArmedWriteBlocked only gates whether a publish is attempted, not what refuse-on-exist does once it is; fix write()'s catch-block comment, which claimed no AppError was ever raised on the ordinary path (now false since refuse-on-exist is uniform); broaden publishHealedScriptAtomically's doc to state it is write()'s only publish primitive for every target, referencing the removed publishOverwriteAtomically and the future --force/--overwrite (#1258). - session-action-recorder.ts / session-replay-runtime.ts / types.ts: fix comments claiming an explicit --save-script=<path> (or the saveScriptDefaultedHealedPath marker) is exempt from the clobber guard; the guard is uniform regardless of path origin or repair-armed status. - docs/adr/0012-interactive-replay.md: add a "Scope" paragraph making the refusal explicitly uniform across repair and ordinary recording, and extend the decision-6 acceptance-test bullet and migration-plan step 9 bullet to require ordinary-recording no-clobber coverage too. - session-script-writer.test.ts: add the missing existing-target coverage for the ORDINARY (non-repair, no saveScriptBoundary) path — refused with bytes unchanged when the target exists (thrown, since ordinary writes rethrow AppErrors rather than returning them), and confirmed to still succeed against an absent target. No runtime behavior change: publish is still a single uniform exclusive linkSync for every --save-script target.

thymikee mentioned this pull request Jul 13, 2026

fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish #1235

Merged

5 tasks

thymikee added 3 commits July 13, 2026 13:20

thymikee force-pushed the docs/adr-0012-decision6-repair-transaction branch from e9cba2b to ac367d6 Compare July 13, 2026 11:20

thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 13, 2026

thymikee merged commit b5e0e59 into main Jul 13, 2026
5 checks passed

thymikee deleted the docs/adr-0012-decision6-repair-transaction branch July 13, 2026 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

docs: ADR-0012 Decision 6 — repair-transaction lifecycle (R7 + commit semantics)#1234

docs: ADR-0012 Decision 6 — repair-transaction lifecycle (R7 + commit semantics)#1234
thymikee merged 3 commits into
mainfrom
docs/adr-0012-decision6-repair-transaction

thymikee commented Jul 13, 2026 •

edited

Loading

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

thymikee commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

thymikee commented Jul 13, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thymikee commented Jul 13, 2026 •

edited

Loading