fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish - #1235
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
- 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.
c3829c4 to
d72ec31
Compare
|
Coordinator review at
Ordering remains: resolve and merge governing ADR #1234 first, then rebase #1235 onto it and align the stale PR description. Practical shipped-CLI simulator evidence is still required for held divergence/continuation, commit and fresh replay, expiry recovery, clobber/unwritable/concurrent output, and cleanup. No fixer dispatched; readiness remains withheld. |
- 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.
d72ec31 to
d55395b
Compare
|
Additional documentation finding: this PR description still presents The previously reported implementation blockers remain unchanged; this comment does not supersede them. |
|
Addressed: PR description updated to match the amended ADR + implementation — commit is teardown-gated on |
… semantics) (#1234) * docs: ADR-0012 Decision 6 — repair-transaction lifecycle (R7 + commit 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. * docs: ADR-0012 R7 — fix 6 architecture blockers (transaction contract) - 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. * docs: ADR-0012 R7 — teardown-commits model + persisted-state continuation 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.
Live CLI evidence — repair transaction end-to-end (iOS iPhone 17 Pro, RN prevent-remove)Captured on the shipped CLI built from this branch ( 1. Held divergence + keep-alive (cold and warm). A 2. Continuation to completion (terminal-close no longer diverges). Corrective 3. Commit → complete healed artifact.
4. Fresh-session replay (acceptance). Replaying the committed healed The Not captured live here: |
6beb13f to
1a09ed1
Compare
|
Coordinator + independent re-review at
The |
5d4d999 to
fb44cf4
Compare
|
Addressed all three blockers from the 1. Partial-target no-clobber race — fixed.
Every interleaving converges on exactly one winning 2. Invisible COMPLETE-commit failures on idle-reap/shutdown — fixed. 3. Auto-commit now records the skipped terminal Checks: |
|
Current-head review at Please make the restore/publish protocol mutually exclusive with competing publishers (an atomic lock/claim protocol is one viable shape) and add a deterministic regression where writer B publishes after writer A quarantines a COMPLETE target but before A restores it. Assert no writer reports success unless its complete bytes remain at the target, and that the pre-existing COMPLETE artifact is never silently clobbered. No fixer was dispatched. |
|
Independent exact-head review confirmed the publication race above and found two additional close-path issues:
Readiness remains withheld. No fixer was dispatched. |
fb44cf4 to
65d6679
Compare
|
Addressed the 1. No-clobber restore race — fixed with an exclusive lock. The prior grab/restore made each step atomic but not the sequence, so A could quarantine a COMPLETE target, B win the emptied slot, and A's 2. Successful 3. Checks: tsc / oxlint / oxfmt / fallow clean; full vitest green (only the documented Android runtime-hints / runner-client contention flakies, pass in isolation). I traced the lock interleaving and confirmed the close-ordering + double-dispatch guard myself. CI green. |
|
Blocking findings on
CI is green, but these keep the PR from readiness. |
|
Blocking finding on The revised stale-lock reclaim still breaks exclusion under a three-writer interleaving. In The new regression covers preservation of B's live lock with only A and B; it does not introduce C during the rename/restore gap, so it misses the remaining race. The ownership primitive must never make a live holder's canonical claim disappear, and release must remove only the caller's own claim. The error-contract and already-succeeded platform-close findings are otherwise addressed. Readiness remains withheld. No fixer was dispatched. |
|
Additional independent-review blockers on
These are in addition to the live-lock rename gap in the preceding comment. No fixer was dispatched. |
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.
…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.
…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.
…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.
…usive 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.
…dentity 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.
…ent 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).
b1e039d to
7762614
Compare
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.
|
Addressed all four blockers from the Blocker 1 (lock exclusion) — the lock is gone. Per maintainer decision, replaced the whole lease/reclaim machinery with a single exclusive publish: Blocker 4 (explicit Blocker 2 (shutdown-time commit loss) and Blocker 3 (request-bound close-retry identity) — the Deliberate scope note: the refuse-on-exist is uniform — ordinary Checks: tsc/oxlint/oxfmt/fallow clean; full vitest green modulo the documented Android runtime-hints contention flakies (pass in isolation). |
|
Exact-head review of One scope regression remains. Keep exclusive refuse-on-exist for repair-armed writes while preserving ordinary overwrite behavior, or explicitly update the ordinary recording contract/docs and add existing-target route coverage. Repair-path code and CI otherwise look clean on this head. A live prevent-remove repair transaction remains required before readiness because the PR changes replay/close/teardown behavior and the PR body states no live simulator run was performed. |
…ording 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.
|
Exact-head re-review of |
|
Correction to my preceding readiness note: requiring another exact-head live device run was too strict. The existing live replay/repair run covers the production command/session route, while the commits after that evidence change daemon/filesystem publication, teardown-failure reporting, and retry identity rather than platform behavior. Their focused boundary regressions plus the fully green CI/platform smoke matrix are sufficient. The only remaining cleanup is to update the PR body, which still describes the superseded overwrite behavior. No additional simulator/device run is required for readiness. |
Live repair-transaction evidence — fresh build, iOS simulatorCloses the "no live simulator run was performed" gate. Everything below ran through Build (fresh, from this PR head 1. Held divergence + cold-session persistence (the core fix). 2. Heal +
3. 4. Atomic commit → complete healed artifact. To exercise the commit lifecycle deterministically I used a minimal single-heal variant of the same prevent-remove drift (
5. Acceptance — fresh-session replay of the healed script. 6. Refuse-on-exist (the lock replacement). A later
The existing file is not clobbered — byte-identical (719 B) before and after. Runner-lease note: a second daemon on a different |
thymikee
added
the
ready-for-human
|
…light #1235 made healed-script publication refuse-on-exist. #1258 adds an escape hatch: --force (alias --overwrite) on open/close/replay makes publishHealedScriptAtomically atomically REPLACE an existing target (renameSync) instead of refusing. The flag threads CLI -> daemon request -> SessionState.saveScriptForce (persisted at arm time, like saveScriptPath, so a later close/auto-commit that doesn't repeat the flag still honors it) -> the publish primitive. Default (flag absent) is unchanged: refuse-on-exist. Second half: an arm-time EEXIST preflight in session-replay-runtime.ts now fails a repair-armed replay --save-script BEFORE any step dispatches when its target already exists and --force is not set, instead of only failing at publish time after the whole repair run (and its corrective steps) has already executed against the device.
…light #1235 made healed-script publication refuse-on-exist. #1258 adds an escape hatch: --force (alias --overwrite) on open/close/replay makes publishHealedScriptAtomically atomically REPLACE an existing target (renameSync) instead of refusing. The flag threads CLI -> daemon request -> SessionState.saveScriptForce (persisted at arm time, like saveScriptPath, so a later close/auto-commit that doesn't repeat the flag still honors it) -> the publish primitive. Default (flag absent) is unchanged: refuse-on-exist. Second half: an arm-time EEXIST preflight in session-replay-runtime.ts now fails a repair-armed replay --save-script BEFORE any step dispatches when its target already exists and --force is not set, instead of only failing at publish time after the whole repair run (and its corrective steps) has already executed against the device.
…light #1235 made healed-script publication refuse-on-exist. #1258 adds an escape hatch: --force (alias --overwrite) on open/close/replay makes publishHealedScriptAtomically atomically REPLACE an existing target (renameSync) instead of refusing. The flag threads CLI -> daemon request -> SessionState.saveScriptForce (persisted at arm time, like saveScriptPath, so a later close/auto-commit that doesn't repeat the flag still honors it) -> the publish primitive. Default (flag absent) is unchanged: refuse-on-exist. Second half: an arm-time EEXIST preflight in session-replay-runtime.ts now fails a repair-armed replay --save-script BEFORE any step dispatches when its target already exists and --force is not set, instead of only failing at publish time after the whole repair run (and its corrective steps) has already executed against the device.
…ht (#1266) * feat: add --force/--overwrite for --save-script, arm-time EEXIST preflight #1235 made healed-script publication refuse-on-exist. #1258 adds an escape hatch: --force (alias --overwrite) on open/close/replay makes publishHealedScriptAtomically atomically REPLACE an existing target (renameSync) instead of refusing. The flag threads CLI -> daemon request -> SessionState.saveScriptForce (persisted at arm time, like saveScriptPath, so a later close/auto-commit that doesn't repeat the flag still honors it) -> the publish primitive. Default (flag absent) is unchanged: refuse-on-exist. Second half: an arm-time EEXIST preflight in session-replay-runtime.ts now fails a repair-armed replay --save-script BEFORE any step dispatches when its target already exists and --force is not set, instead of only failing at publish time after the whole repair run (and its corrective steps) has already executed against the device. * refactor: extract write() catch-block into handleSessionScriptWriteFailure Pure structural refactor, no behavior change: moves the diagnose + classify-and-return / AppError-rethrow logic out of SessionScriptWriter.write into a module-level helper. Drops write()'s cyclomatic below Fallow's threshold (the finding CI flagged on #1266) — the extracted throw still propagates out of the catch exactly as before. * fix: make persisted save-script force consistent (preflight + per-target) Addresses two authorization inconsistencies in the #1258 --force work flagged in re-review: 1. Arm-time EEXIST preflight now uses the SAME effective force decision as publication — `req.flags?.force || preRunSession.saveScriptForce` — instead of the live flag alone. A repair armed with `--save-script --force` and continued via `replay --from … --save-script` (without repeating --force) is no longer rejected on a target the earlier forced leg authorized. 2. Force is now per-target, not sticky-across-retarget. Re-arming a DIFFERENT `--save-script=<other>` without a live --force CLEARS the persisted `saveScriptForce` (new shared `applySaveScriptRetarget`, used by both the replay armer and `recordActionEntry`/close), so a retarget can never silently overwrite a file nobody opted into. A live --force on the retarget re-grants it for the new target. Updated the SessionState doc accordingly. Regressions: persisted-force `--from` continuation is not preflight-rejected; retarget-without-force refuses an existing target (and the --force contrast overwrites it). Both verified to fail without their respective fix. * fix: match arm-time preflight force to per-target retarget contract Re-review ordering-mismatch fix. The preflight computed effective force as `req.flags?.force || preRunSession.saveScriptForce`, accepting persisted force regardless of whether THIS request retargets. So a `--from` continuation with explicit `--save-script=b.ad` (no live force) on a session forced for a.ad would PASS the preflight, run every step, then have `applySaveScriptRetarget` clear the force for b.ad, and finally refuse the existing b.ad at publish time — defeating the arm-time-preflight point and mutating the session mid-run. Now the effective-force decision is computed inside `preflightSaveScriptTarget` against the target THIS request resolves to (the same one the armer will set), matching `applySaveScriptRetarget`'s per-target contract: live force always bypasses; persisted force bypasses ONLY when `targetPath === existingSaveScriptPath` (same target). A retarget to a different path without live force is now refused BEFORE any step dispatches. The preflight stays read-only (runs before the armer; never mutates the session). Regression: a --from continuation retargeting to an existing b.ad without live force fails at the preflight, dispatches zero steps, and leaves saveScriptPath/ saveScriptForce unchanged. Verified it fails (leg passes preflight, runs) when the fix is reverted. Same-target continuation and live-force retarget stay correct. * fix: expose close savedScript to clients; preserve COMPLETE on retarget reject Two re-review blockers. BLOCKER 1 (client-contract gap): the typed close APIs accept --save-script/ --force inputs but dropped the daemon's `savedScript` response, so a Node client could request publication but not learn where the file landed. Add `savedScript?: string` to SessionCloseResult and AppCloseResult, and project it via readOptionalString in both close normalizers (sessions.close, apps.close). New public-client coverage asserts it round-trips (and is absent when the daemon published nothing). BLOCKER 2 (P1 ordering): the C2 `saveScriptComplete = false` reset ran BEFORE the EEXIST preflight's early-return, so a retarget REJECTION corrupted a prior COMPLETE transaction — a later close would then refuse to commit the original target. Move the reset to AFTER `if (saveScriptPreflight) return ...` (verified nothing between the two positions reads saveScriptComplete; applySaveScriptRetarget already runs later, so the original saveScriptPath is preserved). The reset is correct only when the run proceeds to re-arm. Strengthened the retarget-rejection regression to assert (a) saveScriptComplete survives the rejection and (b) a later bare close commits the ORIGINAL target, not the rejected new one. Both verified to fail without their respective fix.
…migration logs ADR 0012 alone was 42% of the ADR corpus by bytes; consulting it cost ~28k tokens of mostly process history. Restructure per the new shape convention (added to the ADR README): Status + a normative 'Rules at a glance' first so a reader can stop after ~50 lines, rationale and refuted alternatives kept below the fold, and completed migration plans/landing tables deleted — git history is the archive. - 0012: delete migration plan/progress; fix the Status section that still claimed #1235 unimplemented against its own landing table; demote the 2026-07-10 evidence audit to the end (still cited by the decisions). - 0014: same; the accepted Android blocking-dialog-recovery evidence gap and its covering fixture tests move into Status so the waiver survives. - 0016: verified implemented; rules summary added (nothing was history). No rule's meaning changed; edits are reorganization plus stale-status fixes.
…vent journal) (#1399) * docs(adr): rules-first restructure of 0012/0014/0016, drop completed migration logs ADR 0012 alone was 42% of the ADR corpus by bytes; consulting it cost ~28k tokens of mostly process history. Restructure per the new shape convention (added to the ADR README): Status + a normative 'Rules at a glance' first so a reader can stop after ~50 lines, rationale and refuted alternatives kept below the fold, and completed migration plans/landing tables deleted — git history is the archive. - 0012: delete migration plan/progress; fix the Status section that still claimed #1235 unimplemented against its own landing table; demote the 2026-07-10 evidence audit to the end (still cited by the decisions). - 0014: same; the accepted Android blocking-dialog-recovery evidence gap and its covering fixture tests move into Status so the waiver survives. - 0016: verified implemented; rules summary added (nothing was history). No rule's meaning changed; edits are reorganization plus stale-status fixes. * docs(adr): propose ADR 0017 — unified request event journal Apply ADR 0008's registry thesis to events. Inventory (2026-07-24) found four parallel event vocabularies — ~155 stringly-typed diagnostics phases, the session events.ndjson, the progress wire stream, and the replay timing trace (one of its two writers unredacted) — with consumers coupled to emit sites by string: agent-cost counts runner round-trips by matching two phase names. Proposal: an EVENT_CATALOG in contracts making every kind a typed, trait-carrying declaration; the diagnostics scope becomes the single journal append point; every consumer becomes an explicitly registered sink; all existing file/wire formats stay byte-compatible behind golden fixtures. Explicitly rejects pub-sub and event sourcing. Status: Proposed — not indexed in the ADR README until accepted. * docs(adr): revise ADR 0017 per architecture review Address all five review findings and adopt both requested judgments: - P1 out-of-request events: finalizeRepairTeardown records a synthesized close during idle-reap/daemon-shutdown with no live request; a request-scoped-only journal would silently drop it. Added an explicit session-scoped teardown scope model (fatal-scope precedent) and rejected the ambient-fallback alternative. - P1 redaction vs byte-compat: progress stays unredacted on its own channel; the replay-trace unredacted->redacted change is now a declared, intentional compatibility change with its own fixture update, not smuggled under a byte-compat claim. - P1 per-attempt trace routing: sinks with dynamic destinations read scope-bound routing context (logPath-rebind precedent); drop-when-unbound semantics; sink ordering/isolation/flush contract made normative. - P2 progress typing: progress streaming removed from the journal entirely - it is a transport-owned output port (ordering, disconnect-as-cancellation, closed typed union); mirror emits noted as the future opt-in shape. - P2 completeness check: orphan detection is now a static source scan in the layering-lint style; runtime unit-suite observation explicitly rejected. Also per review: catalog keys are internal identities; sinks map to legacy wire discriminators, which are never automatically canonical. Migration plan reduced to 4 steps. * docs(adr): ADR 0017 — fork, never rebind, for per-attempt trace routing Review found a blocking concurrency flaw in the revised routing design: sharded test attempts run concurrently (Promise.allSettled in runReplayTestShards) under one inherited AsyncLocalStorage request scope, so mutable scope rebinding would let one attempt overwrite or clear another's replay-timing destination after an await — cross-writing or dropping events. Replace rebinding with a journal fork primitive: journal.fork(bindings, fn) runs fn in a new ALS scope object sharing the parent's buffer/phaseCounts/ envelope/sinks but carrying frozen routing bindings. Each attempt wraps its work (including nested replay dispatch) in a fork binding its own trace path; the binding dies with the fork, so no clearing step exists to race. Existing updateDiagnosticsScope rebinds stay confined to sequential request setup, pre-fan-out. Validation gains a concurrent-shard regression proving each replay-timing.ndjson contains only its own attempt's events. * docs(adr): ADR 0017 — scope identity on the envelope for future exporter sinks Reserve the one shape decision an OTel-style exporter would otherwise force a retrofit for: every scope (request, teardown, fork) carries scopeId, forks record parentScopeId, both ride the event envelope. Forks already form a tree, so an exporter sink can emit parent-child spans from envelope fields alone. Cross-process correlation stays requestId; a traceparent-style meta field is additive under ADR 0006 and deferred. No exporter in this ADR. * docs(adr): address re-review — renumber to 0018, full fork isolation, 0016 record-as 1. Renumber the proposal 0017 -> 0018: main now carries accepted ADR 0017 (parameterized recorded inputs, #1369); branch rebased onto it. 2. Fork contract strengthened: forks clone EVERY mutable scope field (envelope, logPath, routing bindings) and own their event buffer; only the sink list and the request-global phaseCounts tally are shared. Verified in code: nested dispatch creates child execution scopes (request-router.ts:257) whose updateDiagnosticsScope rebinds session/logPath mid-flight, so a shared mutable envelope would cross-route debug/session-log events between concurrent shards even with frozen trace bindings. updateDiagnosticsScope now specified as mutating only the innermost scope. The regression now covers all three routed outputs (replay-timing, per-request diagnostics ndjson, events.ndjson). 3. 0016 rules summary updated for shipped #1348: sensitive fills use fill --record-as <VAR> (ADR 0017); unparameterized fill/type stays literal (body sections already updated by #1369's merge). * docs(adr): ADR 0018 — name the usage sink as first consumer, privacy by construction The motivating consumer is opt-in usage analytics over agent behavior: command frequencies, typed failure codes, and outcome sequences that trip agents (consecutive snapshots, screenshot-after-snapshot). Decision 4 pins its discipline now, before any exporter exists: an allowlist-by-construction UsageRecord schema whose every field draws from a registry-enumerated vocabulary (command names, ADR 0010 error codes, flag names, durations, hashed session + sequence number) — positionals, selectors, labels, fill text, and error messages are unrepresentable by type, not redacted. Anti-pattern detection is downstream analysis over the stream, never emission-side logic; the sink itself is a follow-up after migration step 3. Adds the matching invariant and a schema gate to validation.
…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>
1 participant
Summary
Implements the four "repair transaction" lifecycle fixes for ADR-0012 Decision 6 (agent-supervised re-record repair, #1228), closing real end-to-end gaps found in live testing.
Unifying model:
replay <file>.ad --save-scriptopens a multi-invocation repair TRANSACTION (ARMED → COMPLETE → COMMITTED). It is committed (the healed.adatomically published) on any teardown of the session — explicitclose, idle-reap, or daemon shutdown — but only when the transaction isCOMPLETE; a teardown while INCOMPLETE never publishes a prefix. Continuation keys off persisted transaction state, soreplay --fromcontinues without repeating--save-script. Everything below serves that model.cleanupDaemonAfterRequest(src/daemon/client/daemon-client-lifecycle.ts) no longer tears down a cold, owned-ephemeral daemon when a repair-armed replay returns a heldREPLAY_DIVERGENCE. Keep-alive keys off the daemon'sresume.repairSessionHeldsignal (markRepairSessionHeldIfArmed, set from persisted armed-uncommitted session state — not the per-request--save-scriptflag, so areplay --fromcontinuation that itself diverges stays held) viaisHeldRepairDivergence; it is independent ofresume.allowed(plan-resumability). Because an owned/ephemeral state dir (fs.mkdtempSync) is otherwise unaddressable by the agent's next command, the response carries ahintpointing at--state-dir <path>(attachRepairSessionAddressHint,src/daemon/client/daemon-client.ts).SessionScriptWriter.write(src/daemon/session-script-writer.ts) publishes a repair-armed session's (saveScriptBoundaryset) healed.adonly when the transaction isCOMPLETE(isRepairArmedWriteBlockedgates onsaveScriptComplete, idempotent oncesaveScriptCommitted). Commit happens at any teardown of a COMPLETE transaction — explicitclose, idle-reap, and daemon shutdown all route throughfinalizeRepairTeardown(src/daemon/session-store.ts) → the writer. A teardown while INCOMPLETE publishes nothing (never a prefix): a plainclosediscards; an idle-reap/shutdown leaves aREPAIR_SESSION_EXPIREDtombstone (surfaced byrequest-router), andhasReapBlockingOpenSessions(src/daemon/handlers/daemon-idle-reap.ts) lets an uncommitted repair session reap rather than pinning the daemon forever.closeis skipped (isRepairArmedTerminalClose,src/daemon/handlers/session-replay-runtime.ts) while a repair is armed, instead of dispatching as an ordinary step and diverging on lifecycle. Verification already exempted unannotated actions from target-binding divergence kinds (verifyReplayActionTarget); the fix removes the remainingaction-failuremisfire by never dispatching it at all during repair.linkSync, with the temp file created in the target's own directory (publishHealedScriptAtomically). The publish contract is uniformly no-clobber: repair heals and ordinary recordings, default and explicit--save-script=<path>targets, and complete or partial existing files are all refused and left byte-for-byte unchanged. A--force/--overwriteescape hatch is tracked separately in Add --force/--overwrite flag for --save-script now that publish refuses any existing target #1258. Completed repair artifacts carry a# agent-device:heal-completesentinel.Files touched
src/daemon/client/daemon-client-lifecycle.ts—cleanupDaemonAfterRequest(+isResumableRepairDivergence,attachRepairSessionAddressHint)src/daemon/client/daemon-client.ts—sendToDaemon(+withRepairSessionAddressHintIfOwned)src/daemon/session-script-writer.ts—SessionScriptWriter.write,isRepairArmedWriteBlocked,HEAL_COMPLETE_SENTINEL,publishHealedScriptAtomically,assertNoCompleteHealedClobbersrc/daemon/session-store.ts—finalizeRepairTeardown(commit-or-tombstone at teardown)src/daemon/handlers/session-replay-runtime.ts—isRepairArmedTerminalClose,markRepairSessionHeldIfArmed,createReplaySaveScriptArmer,preflightReplayAgainstActiveRepair, setssaveScriptCompleteat plan completionsrc/daemon/request-router.ts—REPAIR_SESSION_EXPIREDfor a reaped uncommitted repair session (tombstone)src/daemon/handlers/daemon-idle-reap.ts—hasReapBlockingOpenSessionssrc/daemon/types.ts—SessionState.saveScriptComplete/saveScriptCommitted/saveScriptBoundary;resume.repairSessionHeldNew tests
src/utils/__tests__/daemon-client-lifecycle.test.ts: keeps an owned daemon alive + hints--state-diron a resumable divergence; still tears down on a non-resumable divergence; still tears down without--save-script.src/daemon/__tests__/session-script-writer.test.ts: discards an unfinalized repair-armed write; never sentinel-tags ordinary recordings; refuses every pre-existing target without changing its bytes (complete, partial, explicit, and ordinary-recording cases); permits absent targets; proves first-writer-wins atomic publication and leaves no stray temp file.src/daemon/handlers/__tests__/session-replay-repair-loop.test.ts: terminal close skipped while armed; ordinary replay still dispatches its close; only the terminal close is skipped (mid-plan close still dispatches); a--fromresume landing on the terminal close completes instead of diverging.src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts(new): end-to-end chain — cold divergence stays alive (session never deleted) → corrective press +--fromresume runs to completion (terminal close skipped) →close --save-scriptcommits a complete, atomically-published healed.adwith no bare@refs → a separate diverged-and-abandoned repair (closewithout--save-script) leaves no partial file on disk.Test plan
npx vitest run— full suite: 467 files / 4097 tests, all greennpx tsc --noEmit— cleannpx oxlint --deny-warnings— cleannpx oxfmt --check— clean on all touched files (37 pre-existing unrelated files were already failing onorigin/mainbefore this change)pnpm check:fallow --base origin/main— no issues in 11 changed filesLive route evidence
A simulator run on route-equivalent head
6beb13f5fexercised the production repair workflow: a held divergence preserved the session, corrective interaction plusreplay --fromcompleted the plan,close --save-scriptpublished the complete healed script, and a fresh replay consumed that artifact successfully. Later commits changed daemon/filesystem publication, teardown-failure reporting, and close-retry identity rather than platform behavior; focused boundary regressions and the fully green platform smoke matrix cover those changes, so another device run is not required.No device-lease semantics were changed beyond the session staying open (
hasOpenSessions) for the repair transaction's lifetime.