fix(daemon): reject unarmed close --save-script before teardown - #1558
Open
thymikee wants to merge 1 commit into
Open
fix(daemon): reject unarmed close --save-script before teardown#1558thymikee wants to merge 1 commit into
thymikee wants to merge 1 commit into
Conversation
Live evidence (2026-08-02) showed a plain `open` followed by `close --save-script` silently published a script: the close request armed authoring at record time and published moments later in the same request, folding the never-armed case into the ADR 0016 authoring lifecycle. The resulting .ad carries selector fallback chains but no recording-time target-v1 evidence, and nothing told the caller evidence capture never ran — degraded replay verification with no signal beats a loud refusal. `assertTerminalRecordingCloseAllowed` (src/daemon/handlers/session-close.ts) now rejects an unarmed `close --save-script` with INVALID_ARGS before any teardown or filesystem work runs, the same seam that already rejected ABORTED/PUBLISHED terminal recordings. The rejection does not tear the session down, so a plain `close` retry still completes cleanly; recovery names `open --save-script` since evidence can only be captured from action zero. Repair transactions (ADR 0012) are a disjoint lifecycle and are explicitly unaffected. This is distinct from #1533 (an already-armed-then-aborted session whose flag ingress re-enables recordSession and lets a *bare* close publish); that case remains open.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Live evidence on 2026-08-02 showed that a plain
openfollowed byclose --save-scriptsilently succeeded and published a script — but with a critical gap:open-time arming (recordSession + scriptPublication{authoring,armed}) is the only point ADR 0016 captures recording-timetarget-v1identity evidence from action zero. An unarmed close-time save folded the request into the authoring lifecycle at close time (applyRecordedSaveScriptFlags'snone -> authoringbranch) and published anyway — a script with selector fallback chains but no target-v1 evidence, and no signal to the caller that evidence capture never ran. Degraded replay verification with no signal is worse than a loud refusal, so the fix rejects instead of warning.Seam and behavior
assertTerminalRecordingCloseAllowedinsrc/daemon/handlers/session-close.tsis the existing gate that already rejectsclose --save-scripton an ABORTED/PUBLISHED terminal recording, before any teardown or filesystem work. This PR extends the same gate/seam to the never-armed (scriptPublication.kind === 'none') case:close --save-scripton a never-armed session now throwsINVALID_ARGSwith a hint to start over withopen --save-script, before platform close, cleanup, or any.adwrite.closeretry immediately afterward still completes cleanly (session deleted, no script written). This mirrors how ABORTED/PUBLISHED already behave and lets the caller recover/rearm instead of losing the live session on a mistake.scriptPublication.kind === 'repair'lifecycle armed byreplay --save-script) are explicitly unaffected — the guard returns early for them, exactly as before.session save-script(mid-session active publication,src/daemon/handlers/session-script-publication.ts) is untouched; it already has its own "not armed before this journey began" rejection and remains the sanctioned way to publish without closing.Aggregate consistency (#1478 P4a)
The P4a arc deliberately folded "never-armed close-time publication" into the authoring lifecycle's shared
--save-scriptingress (applyRecordedSaveScriptFlags'snone -> authoring armedbranch insrc/daemon/session-script-publication-capability.ts). That branch is now unreachable forclose— the new daemon-seam guard rejects before any action recording (and therefore before that ingress) runs for an unarmed close. I updated its docstring to say so rather than leave it describing behavior that no longer happens forclose; the branch is kept as a generic safety net for any future non-close caller of the shared ingress, not as documented close-time behavior. ADR 0016 (docs/adr/0016-active-session-script-publication.md) gets two small amendments recording the same decision (recording-lifecycle prose + a validation bullet), same style as its existing #1349 amendment.Relationship to #1533
#1533 is a different, already-armed case: a session ARMED then ABORTED by a second
open, whose flag ingress re-enablesrecordSessionif that second open (or a later action) itself carries--save-script, letting a bareclose(no--save-scriptflag on the close itself) publish anyway. This PR's guard only fires whencloseitself is passed--save-script, so it does not touch or resolve #1533 — noted in the ADR amendment so the two decisions don't collide later.CLI help
The shared
--save-scriptflag's usage description (src/commands/cli-grammar/flag-definitions-action.ts) implied a standalone close-time save just works; updated to state the arm-at-open requirement and theclose --save-script-alone rejection, plus thesession save-scriptmid-session alternative. No test pins the old string.Tests
src/daemon/handlers/__tests__/session-close-shutdown.test.ts, appended):close --save-script on a never-armed session is rejected before teardown, with no script written— assertsINVALID_ARGS+ hint, session survives the rejection, no file written, then a follow-up plainclosetears down cleanly.close --save-script on a session with an active .ad repair transaction is unaffected by the unarmed-authoring guard— proves the repair lifecycle's own close path is untouched.test/integration/provider-scenarios/active-session-script-publication.test.ts, appended):an unarmed session refuses close --save-script and closes cleanly on plain close— full daemon round trip via the real RPC surface.an armed session still publishes target-v1 evidence through close --save-script— proves the legitimate armed close-time path (not justsession save-script) still works and still carriestarget-v1evidence end to end.test/integration/provider-scenarios/android-lifecycle.test.ts's long composed scenario used aclose --save-scripton a session that had never been armed (opened many times across the flow, so it couldn't retroactively arm at its originalopen). Updated only its setup: end that session with a plainclose, then open a fresh armed session before exercisingclose --save-script+--shutdown; all prior assertions preserved. No other provider-scenario or unit suite assumed the old unarmed-close-publishes behavior (repair-armedclose --save-scriptflows all arm viareplay --save-scriptfirst, unaffected).Counterfactual (per docs/agents/testing.md)
Reverted just the new unarmed branch locally and reran both new regression tests — both went red exactly as expected, proving they pin real behavior:
Unit:
Provider-scenario:
Restored the fix afterward; both tests pass again.
Gates run
pnpm typecheck✅pnpm lint✅pnpm format:check✅pnpm check:layering✅npx vitest run src/daemon test/integration— 253 files / 1916 tests ✅pnpm check:replay-compat✅npx vitest run(full) — 13 failures on first pass, all reproduced as CPU-contention timeouts (12) plus one flaky process-liveness assertion (owner-identity.test.ts, unrelated file); isolate-rerun of all 10 affected files together: 151/151 pass. Matches the documentedunit-suite-flaky-under-contentionpattern; none touch this change's files beyondandroid-lifecycle.test.ts, which passes standalone.Test plan
pnpm typecheck && pnpm lint && pnpm format:check && pnpm check:layeringnpx vitest run src/daemon test/integrationpnpm check:replay-compatnpx vitest run(full, contention flakes isolate-rerun clean)Generated by Claude Code