🔒 fix(run): AUD-S08 emit the DecisionRecord before forge reconcile (REL-08/D-122) - #20
Merged
Merged
Conversation
REL-08: `orchestrate` emitted the DecisionRecord AFTER `forge.Reconcile`, so a failed emit (or a process death) left the forge already mutated — thread posted, MR approved, MR merged — with no durable record of the decision that caused it. Per D-122, the order is now build -> marshal -> schema-validate -> EMIT -> Reconcile -> summary, establishing the invariant: NO forge write without a schema-valid, durably emitted DecisionRecord. An emit failure is a HARD error with ZERO forge writes (no record => no action), which is the same fail-closed posture as an unloadable policy. The `--emit` file write becomes atomic: `<path>.tmp` in the SAME directory (so os.Rename is a same-filesystem atomic replace) then rename into place; a failure at either step removes the temp file. Stdout emission is unchanged, and so is stdout ordering (record line, then summary line). The reorder is pure: `recordJSON` is fully determined before Reconcile (the reconcile receipt lives in the summary line, never in the record), so the emitted bytes and the marker `decision`/`occurrence` digests derived from them are byte-identical to the previous order. Post-reconcile record stamping was rejected by D-122 precisely because it would break that byte-stability. Exit codes are unchanged on every existing path: a fail-closed reconcile refusal (arming unmet, SHA moved, incomplete preconditions) is still a clean 0 with the record already emitted; a hard reconcile failure is still non-zero and now leaves a schema-valid record behind. The record states the DECISION and its pins — it never claims the forge actions completed. REQ-AUD-S08-01, REQ-AUD-S08-02.
The end-to-end emit-order tests did not discriminate the ATOMIC write from a plain in-place one: with `--emit` pointing at an existing directory, a plain `os.WriteFile` fails before any `<path>.tmp` exists, so the "no temp file survives" assertion passed vacuously against a non-atomic mutant. File mode on overwrite is the observable that separates them. `os.WriteFile` truncates an existing target in place and KEEPS its mode; the tmp+rename replaces the inode, so the target inherits the 0600 staging mode. The new case seeds a stale 0644 record at the --emit path and asserts the run leaves it 0600 with the stale content gone. That also pins the semver-visible side effect it implies: re-running `assent run` over an existing `--emit` target now resets that file's mode to 0600. Mutation-verified: reverting the emit/reconcile order reds TestEmitFailureBlocksForgeWrites on its forge-write-count assertion; replacing the tmp+rename with a plain os.WriteFile reds this case; making emitTempPath return a path outside the target's directory reds TestEmitTempIsSameDirectory. REQ-AUD-S08-01, REQ-AUD-S08-02.
…-122) The failure polarity was only covered for `--emit <file>`. stdout is the DEFAULT emit mode, and the D-122 invariant is not "no forge write without a record FILE" — it is no forge write without a durably emitted record, whichever sink the operator chose. Nothing in `orchestrate` writes to stdout before `emitRecord`, so an io.Writer that always errors lands the failure exactly on the emit. The new case drives `runRun` at the APPROVE + armed polarity with such a writer and asserts a non-zero exit with ZERO forge writes. Mutation-verified: moving the emit back below `forge.Reconcile` reds this case on its forge-write-count assertion, alongside the two file-mode cases. REQ-AUD-S08-01.
…cile (D-122) `docs/usage/cli.md` described the run as "…build and schema-validate the `DecisionRecord`, reconcile against GitLab, and emit the record" — an ordering this branch's D-122 change makes false. Corrected to emit-then-reconcile, with one clause naming the user-visible consequence: a run whose emit fails aborts without touching the forge (no record, no action). Confined to that prose paragraph. No fenced ```console block is touched — those are pinned byte-for-byte to the binary's --help output by AUD-S05 (TestCLIDocCoversSubcommands / TestCLIDocDriftIsDetected, both still green). Closes the doc drift this lane introduced and reported.
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
1 participant
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.
Closes audit finding REL-08. Implements D-122.
Why
The DecisionRecord was emitted after
forge.Reconcile. A reconcile failure — or a crash mid-way —left the forge mutated (thread posted, approval given, merge performed) with no durable record of
the decision that caused it. The invariant this establishes: no forge write without a durably
emitted record.
What
A pure reordering:
emitRecordmoves to immediately aftervalidateRecord, before the reconcileswitch. The
--emitfile write becomes atomic — same-directory<path>.tmp(the name D-122mandates),
os.WriteFile0600, thenos.Rename, with the temp removed on either failure.Emit fails ⇒ zero forge writes. Verified structurally, not just by fixture: enumerating every
mutating method on the forge port and every mutating HTTP verb in the adapter, the only invocation
reachable from
cmd/assentnon-test code isforge.Reconcile, and emit strictly precedes it. Everypre-emit client call is a read.
Emit succeeds, reconcile fails ⇒ the record is still truthful. It carries decision, findings and
pins only — the reconcile receipt is not even declared at marshal time. The reviewer dumped a real
APPROVE+armed record and confirmed no token that could be read as a forge-state claim.
Unchanged, and proven so
Record bytes, marker
occurrence/decisiondigests, stdout ordering, summary format, and exit codeson every pre-existing path — verified byte-identical against
mainby a cross-branch goldencapture, so a rerun still recognises its own thread. Fail-closed refusals (
ErrArmingRefused,ErrSHAMoved,ErrIncompletePreconditions) still exit 0 with the record emitted — all three checkedby direct injection. ADR-0019 author-identity filtering is untouched.
Semver-visible
--emitover an existing file now resets its mode to 0600 — tmp+rename replaces the inode,where the old in-place write preserved whatever mode the stale file had.
<path>.tmpappears in the emit directory during a run — observable by a CI artifactcollector globbing
record.json*.Verification
task check(coverage 90.1%),task determinism,CI=true go test ./...green. The lane disclosed amutant that initially survived — replacing tmp+rename with a plain
os.WriteFilepassedeverything, because the rename-onto-a-directory case is vacuous without a rename — and fixed it with
the correct discriminator (a 0644 file must come out 0600). The reviewer re-ran all three mutants,
confirmed M1's discriminator is the forge-write-count assertion rather than the exit code (which is
non-zero under the mutant too), and ran 600 concurrent-emit iterations against the fixed
.tmpname:outcomes are {lost run, fail-closed abort}, never a corrupt or partial record.
Recorded as a D-122 follow-up, not a lane defect: there is no
fsyncbefore the rename, so a machinecrash or power loss (as distinct from a process kill, which is fully covered) can still leave the
forge mutated and the record absent. D-122 prescribes the mechanism verbatim; changing it would be a
deviation from a normative decision.
Refs: REL-08, D-122, ADR-0019 · REQ-AUD-S08-01/02 · SEMVER-VISIBLE