Skip to content

fix(fix): write governed files atomically and state the run-level contract - #126

Merged
pablontiv merged 1 commit into
masterfrom
pablontiv/w5-issue61-apply-contract
Aug 6, 2026
Merged

fix(fix): write governed files atomically and state the run-level contract#126
pablontiv merged 1 commit into
masterfrom
pablontiv/w5-issue61-apply-contract

Conversation

@pablontiv

Copy link
Copy Markdown
Owner

What

Every write in internal/fix is now atomic, and the run-level contract is stated instead of
left to be guessed. This is the last remaining sub-defect of issue #61.

Related issue

Fixes #61

Residue re-derivation (done before writing any code)

Issue #61 names three sub-defects. Two of them already shipped and are green on master;
only the third still reproduced. Each was tested against master@2ccc7c0 first:

# Sub-defect Status on master Evidence
1 apply commands exit 0 on failure GREEN TestRepairApplyExitStatus + TestSchemaApplyExitStatus pass. Shipped by #96.
2 writes are not atomic RED A concurrent reader polling a governed document during repair apply observed 20-21 half-written states per run, reproducible across every run.
3 the two commands disagree on report roots GREEN resolveReportRoot + apply_report_root_test.go pass. Shipped by #104.

Why sub-defect 2 was still open: PR #107 carried this exact fix and was
CLOSED at 2026-08-06T01:40:12Ztwo seconds after #104 merged at 01:40:10Z.
That is the gh pr merge --delete-branch collateral-close of a stacked child, not a review
rejection. Its reasoning was sound and is reused here, with the scope and the test
strengthened (below).

Only sub-defect 2 is implemented. Nothing green was rewritten.

How

Per-file atomicity. New fix.WriteFileAtomic(target, content, perm) stages the bytes in a
sibling .rootline-*.tmp file, Syncs, Chmods and renames over the target. A file is
therefore only ever observed as its old self or its new self.

Three details that are load-bearing rather than incidental:

  • The staging file goes in the target's own directory, because os.Rename across
    filesystems fails.
  • perm is applied explicitly, because os.CreateTemp creates its file 0600 — without the
    Chmod the staging file's mode would silently become the document's mode.
  • Every failure path removes the staging file, so a failed write leaves the directory as it
    found it.

Scope is wider than #107's. It replaces all thirteen production write sites in the
package, not five:

The run-level contract, stated — and deliberately not all-or-nothing. A run that fails
partway leaves the files it already wrote in place. This is a decision, not an omission:

  • Buffering every rewrite until the last proposal succeeded would discard the good repairs in
    a report because of one unreadable path.
  • It would still not be atomic against a kill — the flush itself is many writes.

Best-effort plus an exact account of what happened is more useful, and more honest, than a
guarantee that cannot be kept. Documented as a named "Atomicity contract" section in
docs/fix.md and mirrored into .claude/skills/rootline/.

complete in the envelope. Both result types gain a boolean: true exactly when the run
carried through everything it accepted, which is exactly when the command exits 0. It is not
redundant with the exit status — a report saved as a CI artifact is read long after $? is
gone, and a consumer should not have to re-implement the rule:

rootline repair apply --report r.json -o json > result.json   # exit status observed here
jq -e .complete result.json                                    # ...and still answerable here

One seal() per result type sets it once, after every phase has had its say, so it cannot
disagree with the fields it summarizes. A test binds it to the exit status per case, so the
envelope and the shell cannot drift apart.

The regression test is self-validating, not merely probabilistic

Atomicity has no honest black-box RED test in isolation: a read-only directory makes the
atomic version fail while os.WriteFile succeeds, so that scenario inverts the signal. The
test that does work is a concurrent reader polling a ~300KB governed document while rewrites
run — os.WriteFile truncates before it writes, so the first state it exposes is a zero-length
file.

A timing-dependent test that only ever asserts "I saw nothing" is worth little, so it does not
stop there. TestApplyRepairIsObservablyAtomic runs a cheap control pass with a bare
truncating write first. The control must observe tearing (it reliably sees 25-29 half-written
states); only then does the assertion against the real repair path run. If the control sees
nothing, the machine has no power and the test declares itself inconclusive via t.Skip
rather than passing on no evidence.

The asymmetry is deliberate: the test can never fail spuriously red, only skip.

Cost was tuned rather than ignored. The naive version took 55s under -race and dominated
the fast cycle; splitting the rounds (control cheap and many, experiment expensive and few)
brings internal/fix from 1.7s to 7.2s.

Verification beyond the test suite

The issue's own reproductions were re-run against a build of this branch:

# sub-defect 3 shape: report stored in a sibling reports/ directory
{"...","root":".../smoke/docs","complete":true,"changed":["correct estado: \"Pendiente\"->\"Pending\" in a.md"],"errors":null}
exit=0

# sub-defect 1 shape: a value its schema rejects
{"...","complete":false,"changed":[],"rolled_back":[{"path":"a.md","errors":["estado: value TOTALLY_INVALID is not in allowed values: [Pending, Done]"]}],"errors":null}
exit=1

The document was restored to its original bytes and no .rootline-*.tmp debris was left in
the governed directory in either run.

Semver

fixnot breaking, matching the judgment already reviewed on #107. Behaviour on the happy
path is identical; this closes a durability gap and adds one envelope field.

One edge case is worth naming rather than burying: WriteFileAtomic fails where
os.WriteFile succeeded
— writing into a read-only directory that holds a writable file,
since staging needs to create a sibling. That is narrow and arguably a case repair should not
have been serving anyway, but it is a real divergence. If you would rather ship it as fix!,
say so before squashing and I will retitle.

Scope notes

Checklist

  • Tests pass (just testgo test ./... -race, all 14 packages ok)
  • Code is clean (just check — gofmt + golangci-lint 0 issues + build)
  • Coverage gate green (just coverage-check — total 89.4%, internal/fix 87.2%, no package below the 85% floor)
  • Changes are documented (docs/fix.md "Atomicity contract", CLAUDE.md, .claude/skills/rootline/ref-advanced.md)
  • Linked to its issue with a closing keyword

…tract

Every write in internal/fix was a bare os.WriteFile, which truncates the target
and then writes it. A process that dies in between leaves a governed document
truncated — in a state its own schema rejects — and the only record of how far
the run got was JSON on stdout. A concurrent reader could see it too: a probe
polling a document during repair apply caught 20-21 half-written states per run
against the previous code.

Writes now stage into a sibling temp file and rename over the target, so a file
is only ever observed as its old self or its new self. The staging file is
created in the target's own directory because os.Rename across filesystems
fails, its mode is set explicitly because os.CreateTemp makes files 0600, and
every failure path removes it. This covers all thirteen write sites: repair.go's
five, including the rollback-restore in postValidateWrittenTargets, where a
restore that half-succeeded would be strictly worse than the write it was
undoing; and fix.go's eight, which the fix --all pipeline uses to rewrite both
documents and .stem schemas.

The regression test is self-validating rather than merely probabilistic. It runs
a cheap control pass with a bare truncating write first, and only asserts on the
real path once the control has proved this machine can observe tearing at all;
otherwise it reports itself inconclusive instead of passing on no evidence.

The run-level contract is stated rather than left to be guessed, and it is
deliberately NOT all-or-nothing. Buffering every rewrite until the last proposal
succeeded would discard the good repairs in a report because of one unreadable
path, and would still not be atomic against a kill, since the flush is itself
many writes. Best-effort with an exact account of what happened is both more
useful and more honest than a guarantee that cannot be kept.

So both envelopes gain `complete`: true exactly when the run carried through
everything it accepted, which is exactly when the command exits 0. It is not
redundant with the exit status — a report saved as a CI artifact is read long
after $? is gone, and a consumer should not have to re-derive the rule. One
seal() per result type sets it once, after every phase has had its say, so it
cannot disagree with the fields it summarizes; a test binds it to the exit
status per case so the two cannot drift.

Fixes #61
@pablontiv pablontiv closed this Aug 6, 2026
@pablontiv pablontiv reopened this Aug 6, 2026
@pablontiv pablontiv closed this Aug 6, 2026
@pablontiv pablontiv reopened this Aug 6, 2026
@pablontiv
pablontiv merged commit dfc2543 into master Aug 6, 2026
33 checks passed
@pablontiv
pablontiv deleted the pablontiv/w5-issue61-apply-contract branch August 6, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

repair apply and schema apply exit 0 on failure, are not atomic, and disagree on how to resolve report paths

1 participant