ci: fail when capabilities markdown disagrees with yaml

[Kiran01bm]

Require the generated capabilities page to remain byte-identical to the authoritative YAML in pull requests and releases.

Why

The semantic unit test validates the matrix data, but CI also needs to prove that the checked-in human page is exactly what the generator emits.

What

• Add make check-capabilities to regenerate the page, print any diff, and fail on drift.
• Run the gate on every CI change, including docs-only pull requests, and require it through all-green.
• Repeat the gate on the tagged tree before the release test sweep.
• Mark the regenerate-and-diff contract step complete.

Before / after

Before: a stale docs/capabilities.md page could merge green.
After:  docs/capabilities.md disagrees with pkg/capabilities/capabilities.yaml;
        run make gen-capabilities and commit the result

Refs: docs/capabilities-contract.md

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Reviewed 7d8fb6b8..54af0880 (4 files, +37/−8). No Go logic here, so instead of mutating source I probed the gate itself: five cases against make check-capabilities, covering every way the page can be stale plus the one case the recipe's comment says it tolerates. All five behave correctly.

Probe	Expected	Actual
clean tree	pass	0
a generated cell hand-edited inside a marker region	fail	2, with the unified diff printed
hand-written prose edited outside every marker region	pass	0
YAML changed so the committed page is stale	fail	2 + docs/capabilities.md disagrees with pkg/capabilities/capabilities.yaml; run make gen-capabilities and commit the result
YAML invalid (tier: "t9")	fail closed	2 + row 1 (…): tier "t9" is not one of [t1 t2 t3]

The third row is the one I expected to find broken, since "only the generator's own edits count" is a strong claim for a whole-file diff to make. It holds because regeneration only rewrites marker regions, so an edit outside them survives into both sides of the comparison. Worth stating that the recipe's comment is accurate about the property that makes it usable mid-edit.

Also verified:

• The marginal-value claim in the ci.yml comment is exactly right. internal/cmd/gen-capabilities/main.go is executed by no test — a grep for it across the tree returns only its own file — so TestCheckedInMarkdownIsGenerated covers RenderDocument while the binary's file read, render, write path is covered by nothing. This target is its only smoke test, and release.yml depending on that same entry point is the reason it should fail on a PR first.
• The generator fails closed on every error (internal/cmd/gen-capabilities/main.go — every branch routes to fail, which exits 1), and the recipe's if ! diff treats both "files differ" (1) and "diff itself errored" (2) as failure. I probed the unchecked mktemp/cp too: an empty $before makes diff error, which the ! catches. Every failure mode I could construct fails closed.
• all-green genuinely requires this. It greps needs for failure|cancelled (ci.yml:232-237) and unit is in its needs list, and unit has no if: or needs: (ci.yml:81-92), so the gate runs on docs-only PRs as the contract doc claims.
• release.yml places it correctly: checkout → ancestry gate → Go → capabilities check (:45) → test the tagged tree (:50) → GoReleaser. The check can only mutate the tree on the path that aborts the job, so nothing dirty reaches the release build.

---

1. A failing run leaves the fix in the tree, so an immediate retry passes

The recipe regenerates in place and, on failure, deliberately leaves the regenerated file behind (Makefile:65-75) — the comment says so: "the stale generated output is left in place, with the diff printed, for review." The consequence it does not mention is that the second run has nothing left to detect:

# committed page is stale (YAML edited, page not regenerated)
$ make check-capabilities   ; echo $?     # 2   ← correct
$ make check-capabilities   ; echo $?     # 0   ← nothing was done in between
$ git status --porcelain
 M docs/capabilities.md                             # the fix, uncommitted
 M pkg/capabilities/capabilities.yaml

In CI this is moot — one run per fresh checkout. Locally it is the shape that teaches people to distrust a gate: the developer's instinct on a red check is to rerun it, the rerun is green, and the reason is invisible unless they think to run git status. The fix is already sitting in their tree, which is the helpful part, but nothing says so.

Two small options, either of which keeps the current helpful behavior: print what happened on the failure path (echo "docs/capabilities.md has been regenerated in your working tree; commit it"), or restore $before on failure so the target is a true check and make gen-capabilities stays the only thing that writes. The message already tells the operator to run make gen-capabilities, which reads as though nothing has been written yet — so at minimum the message and the behavior should agree.

2. In ci.yml the check runs after the entire unit suite; in release.yml it runs before

ci.yml:91-92 is make test-unit then make check-capabilities; release.yml:45-50 is the capabilities check then the test sweep. The release ordering is the better one, and the same reasoning applies on a PR: the check takes one go run and a diff, the unit suite is go test -race ./... over the whole module, and a failed step stops the job.

As written, the generator binary's only smoke test in the entire repo is gated behind every unit test passing. A PR that breaks an unrelated test therefore reports nothing about the generator — the cheap, targeted signal is withheld by the expensive, broad one. Swapping the two steps costs nothing and makes the two workflows consistent.

---

Nothing here blocks; the gate does what it says. The stack interaction with #99 is in 2/2 and is the one I would act on before merge.

---

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 2/2 — stack interaction and the contract doc. Gate behavior is in 1/2.

3. This PR and #99 rewrite the same **Status:** line from the same base, and neither result is correct after both merge

Both branches sit on merge-base 7d8fb6b8 and neither contains the other (git merge-base --is-ancestor pr99 pr100 → false), so docs/capabilities-contract.md:270 has three different values in flight:

origin/main   **Status:** step 1 is complete; steps 2–4 remain planned.
#99           **Status:** steps 1–2 are complete; steps 3–4 remain planned.
#100          **Status:** steps 1 and 3 are complete; steps 2 and 4 remain planned.

I ran the three-way merge to be sure rather than eyeballing it:

$ git merge-file -p <#99> <main> <#100>   # exit 1
<<<<<<< /tmp/a.md
**Status:** steps 1–2 are complete; steps 3–4 remain planned.
=======
**Status:** steps 1 and 3 are complete; steps 2 and 4 remain planned.
>>>>>>> /tmp/b.md

One conflict hunk, on that line alone — the rest of the file merges cleanly, since #99 touches the example row and this PR rewrites the paragraph at :157, and those do not overlap.

The consequence is worse than the conflict itself. Whoever merges second resolves a conflict by picking one side, and both sides are wrong once both PRs are in: step 2 (pg-sprite capabilities) and step 3 (this gate) are then both complete, so the true text is "steps 1–3 are complete; step 4 remains planned." Neither branch contains that sentence. A conflict resolved by taking "ours" or "theirs" — the reflex on a one-line prose conflict — silently publishes a status that contradicts the shipped code, in the document whose entire job is recording which steps have shipped.

Cheapest fix is to stop encoding the cross-product in one line: mark completion per step, in the list itself, so each PR edits only its own entry and the two changes merge without touching each other.

1. add the typed package, … *(done)*
2. add `pg-sprite capabilities`, including `--json` and the embedded binary version;
3. add the regenerate-and-diff CI gate to the normal pipeline; *(done)*
4. add documentation and `jq` recipes for consumers.

That is also the shape refusal-classes.md already uses for its own sequence (*(done)* / *(pending)* per numbered step), so it is the repo's existing convention rather than a new one. Failing that, the two PRs need a merge order and the second one rebased onto the first.

4. The go run invocation is duplicated, so the gate can drift from the command it tells people to run

Makefile:59 and :67 both spell $(GO) run ./internal/cmd/gen-capabilities. The failure message points the operator at make gen-capabilities, so the two are meant to be the same action — but a future flag or env var added to one is silently absent from the other, and the mode that goes stale is the gate, which is the one nobody runs by hand. check-capabilities calling $(MAKE) gen-capabilities (or both sharing a variable) keeps them one thing.

5. The all-green comment lost the clause that explains why this gate is safe there

The all-green comment (ci.yml:220-223) drops the parenthetical: "the unit job (and its docs guards) still had to pass" → "the unit job still had to pass." The removed words are precisely the property this PR depends on — that a docs-only PR still runs the unconditional unit job, which is now where check-capabilities lives. The long comment at :69-80 still explains it, so nothing is lost outright, but trimming that clause in the PR that adds another guard to that job is backwards. I would leave it, and add the capabilities gate to it.

---

On the contract-doc rewrite, which is an improvement. Replacing "require an empty git diff" with "require a no-op rewrite" (:77) is more accurate than what it replaced: the old wording implied git diff --exit-code over the whole tree, which would have failed on any unrelated dirty file and would not have tolerated the out-of-marker prose edit the recipe actually permits. The new paragraph at :157-164 names both workflows and the tolerance rule, so a reader can predict the gate's behavior from the doc — which my probes in 1/2 confirm it matches on all five cases.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving. The gate does what it claims. Rather than mutate source, I probed the target directly with five cases — clean tree, a hand-edited cell inside a marker region, a prose edit outside every region, a YAML change that staleness the page, and invalid YAML — and all five behave correctly, including the hard one: "only the generator's own edits count" genuinely holds, because regeneration rewrites marker regions only. Every failure mode I could construct fails closed, all-green really does require it on docs-only PRs, and the ci.yml comment's marginal-value argument is exactly right — no test executes the generator binary, so this target is its only smoke test.

Five findings in the two comments above, none blocking. The one to take before merge is finding 3: this PR and #99 rewrite the same **Status:** line from the same base, git merge-file confirms a real conflict, and both sides are wrong once both merge — the true text is "steps 1–3 are complete." Per-step *(done)* markers, the convention refusal-classes.md already uses, would let the two changes merge without touching each other.

This stamp was left by Claude Code (claude-opus-5).

[aparajon]

🤖 Re-reviewed the delta 54af0880..d75dfeda (+51/−15, 4 files). All five findings are resolved, and I re-ran the probe set against the new head to confirm the gate still behaves the way its recipe claims.

Probe	Expected	Actual
clean tree	pass, no side effect	0, tree clean
stale page (YAML edited)	fail and restore	2, docs/capabilities.md restored to the committed content
immediate rerun, nothing done in between	fail identically	2, same message — the idempotence hole is closed
prose edit outside every marker region	pass	0 — the tolerance survived the rewrite
invalid YAML (tier: "t9")	fail closed, page untouched	2, row 1 (…): tier "t9" is not one of [t1 t2 t3], page unmodified

Restoring rather than printing an explanation is the better of the two options I offered, because it makes the failure message true: "run make gen-capabilities and commit the result" now describes work the developer still has to do. Swapping the step order, hoisting GEN_CAPABILITIES so the gate and the command cannot drift, and putting the capabilities gate into the all-green comment alongside the docs guards all land as asked.

---

1. The collision with #99 survived the fix, and resolving it the obvious way is now worse than before

Adopting the per-step markers was the right call, but #99 still rewrites the aggregate line this PR deletes, so docs/capabilities-contract.md still conflicts — and the shape changed:

$ git merge-file -p <#99> <base> <#100>     # exit 1
<<<<<<< #99
Implementation order is:

**Status:** steps 1–2 are complete; steps 3–4 remain planned.
=======
Implementation order is, with each step marked as it ships:
>>>>>>> #100

One hunk again, but it is now modify-versus-delete, and the four *(done)* / *(pending)* markers below it merge cleanly — they are pure additions to lines #99 does not touch. So resolving the conflict by taking #99's side, which is the reflex on a two-line prose conflict, produces this:

Implementation order is:

**Status:** steps 1–2 are complete; steps 3–4 remain planned.

1. add the typed package, … *(done)*
2. add `pg-sprite capabilities`, … *(pending)*
3. add the regenerate-and-diff CI gate … *(done)*
4. add documentation and `jq` recipes … *(pending)*

Both conventions present, contradicting each other on both of the steps in flight — the aggregate line says 2 is done and 3 is pending, the markers say the reverse — and every one of those four claims is wrong once both PRs are in. The reviewer resolving that conflict sees only the two lines git shows them and has no reason to scroll to the list.

Before, the two branches disagreed about one sentence and the wrong answer was visible in the conflict itself. Now the wrong answer is assembled from a hunk you resolve and a hunk you never see. That makes a merge order between the two PRs necessary rather than merely tidy: land this one first, then rebase #99 onto it.

2. #99 is step 2, and nothing in it flips step 2's marker

Following from the above: after both merge, step 2 has shipped and its marker still reads *(pending)*. That is the same defect the aggregate line had, relocated into the list. The convention is right — it is the one refusal-classes.md already uses, and it is why the four marker lines merge cleanly — but it only pays off if each PR edits its own entry. #99 currently edits a line that will not exist.

3. "Pure check" is true on every path but one

The Makefile comment and capabilities-contract.md:160-162 both state it without qualification: the target "is a pure check," and "a failing run restores the committed page so the check has no side effect." That is very nearly total, and for a good structural reason worth stating — the generator validates and renders entirely before it writes:

// internal/cmd/gen-capabilities/main.go:13-27
rows, err := capabilities.Rows()       // validation
input, err := os.ReadFile(path)
output, err := capabilities.RenderDocument(input, rows)
if err = os.WriteFile(path, output, 0o644); err != nil { fail(err) }

Every fail() before line 25 happens with the page untouched, which is exactly what my invalid-YAML probe showed. The one exception is WriteFile itself: a truncating write that fails partway leaves a mangled page, and that error takes the generator-failure branch, which restores nothing:

$(GEN_CAPABILITIES) || { rm -f "$$before"; exit 1; }

Narrow, but the claim is stated twice as absolute and the fix is the same cp already on the other branch. Worth doing so the sentence in the contract doc is true without a caveat.

---

Re-approving. Finding 1 is the one to settle before either PR merges, and it is a merge-order decision rather than a code change.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Re-approving on d75dfeda. All five findings from my review of 54af0880 are resolved, and the probe set passes against the new head — including the idempotence case that failed before.

The one thing to settle before either PR merges is in the comment above: the capabilities-contract.md collision with #99 survived the fix, and taking #99's side now leaves both conventions in the file contradicting each other. It needs a merge order, not a code change.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — block/pg-sprite pull/100, follow-up commit

Verdict: all five findings were accurate and are fixed; check-capabilities is now a pure check that restores the committed page on failure, runs before the unit suite in ci.yml, shares one GEN_CAPABILITIES command with gen-capabilities, the all-green comment names the gate, and the contract doc's status moved onto per-step markers so this PR and pull/99 no longer rewrite the same sentence.

#	Concern	Status	Explanation
C1-F1	A failing run leaves the regenerated page in the tree, so an immediate rerun passes with nothing done in between; the message says "run make gen-capabilities" as though nothing had been written.	fixed	On the failure path the recipe prints the diff, then copies $before back over docs/capabilities.md before exiting 1, so the target has no side effect and make gen-capabilities is the only command that writes the page. The recipe comment and the contract doc say so. Probed: with a stale YAML the check exits 2 on run 1 and again on run 2, and git status shows docs/capabilities.md untouched; a clean tree still exits 0.
C1-F2	ci.yml runs the gate after the whole unit suite while release.yml runs it before; the generator's only smoke test was withheld whenever an unrelated unit test failed.	fixed	The unit job now runs make check-capabilities first, then make test-unit, matching release.yml, with a comment giving the reason (one go run and a diff; report a broken generator even when an unrelated test fails).
C2-F3	This PR and pull/99 both rewrite the single **Status:** sentence from the same base; the three-way merge conflicts, and resolving it by taking either side publishes a wrong status once both are in.	fixed	The **Status:** sentence is removed. The list reads "Implementation order is, with each step marked as it ships:" and each step carries its own *(done)* / *(pending)* marker — here steps 1 and 3 done, 2 and 4 pending; pull/99 marks 1 and 2 done, 3 and 4 pending. A three-way merge of the two branches still touches adjacent lines (steps 2 and 3) so git reports one hunk, but both sides of it are *(done)* and the resolution is mechanical rather than semantic: steps 1–3 done, 4 pending.
C2-F4	$(GO) run ./internal/cmd/gen-capabilities is spelled twice, so the gate can drift from the command the failure message tells people to run.	fixed	One GEN_CAPABILITIES = $(GO) run ./internal/cmd/gen-capabilities variable; both gen-capabilities and check-capabilities expand it.
C2-F5	The all-green comment dropped "(and its docs guards)", the clause that explains why the gate is safe to require on docs-only PRs.	fixed	The comment reads "the unit job (its docs guards and the capabilities gate) still had to pass."

The five probe rows and the four "also verified" points (fail-closed generator, all-green requiring unit, release-workflow placement, out-of-marker prose tolerance) need no action.

Source: #100, review comments 5625252676 and 5625253450 and review 5172142750 at head 54af0880

[aparajon]

🤖 Re-reviewed the move d75dfeda..456ef2e3. It is a main merge picking up #81, #99, #101 and #102, plus one genuinely new piece — the release-time gate. Against the new base 2c2dfc6 this PR's own diff is 4 files, +47/−10. Both open findings are resolved, and finding 1 was resolved better than I proposed.

I re-ran the probe set against the new head and added two cases:

Probe	Expected	Actual
clean tree	pass, no side effect	0, tree clean
stale page (valid reason_notes edit)	fail, print the diff, restore	2, 4 diff lines, page restored
immediate rerun, nothing done in between	fail identically	2, same diff, same message
invalid tier (t9)	fail closed, page untouched	2, tier "t9" is not one of [t1 t2 t3], untouched
prose edit outside every marker	pass	0
docs/capabilities.md missing	no empty page written back	2, open docs/capabilities.md: no such file or directory, page not recreated
tier changed without status_mark	fail closed	2, row 1 (…): status_mark does not agree with tier

The last two are the ones I went looking for trouble in and did not find any. A cp-based save-and-restore usually gets the missing-source case wrong — cp fails unchecked, the generator writes a fresh page, the diff differs, and the recipe restores an empty temp file over it. That cannot happen here because the generator reads the page before it writes anything, so the recipe's own || branch catches it first. And my first attempt at a stale-page probe flipped a tier and was refused by a cross-field check rather than rendered — the semantic validator earning its keep next to the textual gate.

---

Finding 1 dissolved rather than being resolved, which is the better outcome

I argued the conflict with #99 had become modify-versus-delete with the four marker lines merging cleanly underneath, so the reflex resolution would assemble a file carrying both conventions and four wrong claims — and that a merge order was therefore necessary.

What happened instead is that #99 adopted this PR's convention before merging. origin/main's docs/capabilities-contract.md:264-271 now reads "Implementation order is, with each step marked as it ships:" with per-step markers, introduced by 321d5b0 (#99), and step 2 already reads *(done)*. So there was nothing left to conflict with, and finding 2 — that #99 was step 2 and nothing in it flipped step 2's marker — went with it.

That also leaves this PR's contract edit in exactly the shape the convention needs: its only change to the list is its own entry, :275 flipping step 3 to *(done)*. The convention only pays off if each PR edits its own line, and this is now the first instance of that working.

The release gate is well placed, and it makes the "pure check" property load-bearing outside CI

release.yml:46 runs the same target on the tagged tree. The placement is right on both sides: after the ancestry gate (:33) and setup-go, so it is not wasting a run on a tag that will be rejected anyway, and before both the test sweep (:50) and GoReleaser (:53), so a tag whose matrix disagrees with its YAML fails before anything is built, signed or published.

Worth naming what that placement now depends on. GoReleaser validates a clean git tree, so a gate that left the page rewritten would not fail at the gate — it would fail several steps later, in a tool that reports it as a dirty working tree rather than as a stale capabilities matrix. Every branch of the recipe that does not restore the page exits 1 (Makefile:70-78), so GoReleaser never sees one it touched. That is correct today; it is also the property most likely to be lost by someone simplifying the recipe later, and the contract sentence at :160 is the only place it is written down.

Relatedly, ci.yml:93 sitting in the unconditional unit job is the right home — I confirmed that job carries no needs or if, so the gate really does run on docs-only PRs, which is where a stale page is most likely to arrive.

Finding 3 is still open, and now sits upstream of a release

Unchanged from the last pass: the generator-failure branch is the one path that does not restore.

$(GEN_CAPABILITIES) || { rm -f "$$before"; exit 1; }    # Makefile:72

Every fail() inside the generator before its os.WriteFile leaves the page untouched — the invalid-YAML and missing-page probes both show that — so the exposure is only a truncating write that fails partway. Narrow, and it was worth doing when the claim was merely stated twice in the contract; it is worth a little more now that the same target guards a tag. The fix is the cp already on the other branch.

---

Re-approving. Nothing here blocks the merge; finding 3 is a two-line change whenever this file is next open.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Re-approving on 456ef2e3. Both open findings are resolved — #99 adopted this PR's per-step marker convention before merging, so the contract conflict dissolved rather than needing a merge order. I re-ran the gate probe set against the new head and added two cases (missing page, tier/status_mark disagreement); all seven behave as documented. The new release-time gate is well placed. Details in the review comment.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/100, follow-up commit

Verdict: the one finding still open after the d75dfeda and 456ef2e3 rounds — the generator-failure branch did not restore the page — is fixed in the follow-up commit, along with the two wording findings from the unposted scratch review of d75dfeda; the #99 collision findings dissolved when #99 adopted the per-step markers, and the gate-ordering suggestion is declined with the reason below.

#	Concern	Status	Explanation
C3-F3 / C4	"Pure check" is stated twice as absolute, but the generator-failure branch ($(GEN_CAPABILITIES) || { rm -f "$$before"; exit 1; }) restores nothing, so a truncating os.WriteFile that fails partway leaves a mangled page — and the same target now guards a release tag	fixed	The branch reads cp "$$before" docs/capabilities.md; rm -f "$$before"; exit 1, the same restore the diff branch already performs. Probed: invalid YAML (tier: "t9") exits 2 with the page byte-identical; a read-only page makes the generator's write fail, exits 2, page byte-identical; clean tree still 0; stale page fails twice identically with the pre-run page restored.
S1-F1 (pg-sprite-pr100-review-d75dfeda.md)	The Makefile comment says a failing run "puts the committed page back", but the recipe snapshots the working tree and has no git-based restore, so an uncommitted edit survives a failing run in place	fixed	The comment now says "puts the page back as it was before the run" and names both failure causes (a generator error or a diff).
S1-F2 (pg-sprite-pr100-review-d75dfeda.md)	docs/capabilities-contract.md repeats "restores the committed page", so a reader could treat make check-capabilities as a safe way to discard local edits	fixed	The sentence reads "a failing run — a generator error or a diff — restores the page as it was before the run".
C3-F1	With the aggregate **Status:** line deleted here and rewritten by #99, the merge became modify-versus-delete with the four markers merging cleanly underneath, so the reflex resolution assembles both conventions with four wrong claims; a merge order was therefore necessary	rejected	No action: dissolved rather than resolved. #99 adopted the per-step markers before merging (321d5b0), so main already carries "Implementation order is, with each step marked as it ships:" and this branch's only edit to the list is its own entry, as C4 confirmed. The later main merges (456ef2e3, be2e7398) resolved the adjacent step-3/step-4 markers to both *(done)*.
C3-F2	#99 is step 2 and nothing in it flipped step 2's marker	rejected	No action: #99 marked step 2 *(done)* itself; went with C3-F1.
S1-F3 (pg-sprite-pr100-review-d75dfeda.md)	Consider running the gate after make test-unit (or in its own if: always() step) so page drift does not short-circuit the job and hide the other unit results	rejected	The gate stays first. Drift is a one-command fix and the gate's message says which command; the unit suite's TestCheckedInMarkdownIsGenerated still reports the same drift as a whole-file diff, so no signal is lost when the job stops early. What gate-first buys is the other direction — a broken generator is reported even when an unrelated unit test fails — which was the earlier round's finding and the reason for the current order.

Source: block/pg-sprite#100, review comments 5626146601 (with review 5172782043) at head d75dfeda and 5627608684 (with review 5173795709) at head 456ef2e3;

[aparajon]

🤖 Re-reviewed the move 456ef2e3..ba1ac756. One authored commit — 5ba612a — with the rest being main merges picking up #104, #105, #107, #108 and #103. Against the new base 4951478 this PR's own diff is 4 files, +48/−10. Finding 3 is resolved, and I confirmed the fix is load-bearing. The same commit introduces one new side effect, in the one case the restore cannot restore.

5ba612a does exactly what I proposed — the cp from the diff branch, moved onto the generator-failure branch (Makefile:72) — and the commit message is more careful than my finding was: the recipe snapshots the working tree, so "the page as it was before the run" is the right claim and "the committed page" was not.

I re-ran the probe set against the new head, driving the generator through a stub where a real partway-failed write cannot be forced:

Probe	Expected	Actual
clean tree	pass, no side effect	0, tree clean
write fails partway (stub truncates the page, then exits 1)	page restored	2, page restored byte-for-byte to the committed content, tree clean
same probe against the pre-5ba612a recipe	page left mangled	2, page left at 500 bytes — the fix is load-bearing
stale page (valid reason_notes edit)	fail, print the diff, restore	2, diff printed, page restored
invalid tier (t9)	fail closed, page untouched	2, tier "t9" is not one of [t1 t2 t3], untouched
docs/capabilities.md missing	page not created	2, a 0-byte docs/capabilities.md written

The last row is the new finding; the two bold rows above it are the fix working.

---

The unchecked snapshot cp turns the new restore into a writer when the page is absent

Makefile:71 saves the page without checking the copy:

@before=$$(mktemp); cp docs/capabilities.md "$$before"; \
$(GEN_CAPABILITIES) || { cp "$$before" docs/capabilities.md; rm -f "$$before"; exit 1; }; \

When the page is not there, that cp fails, $before stays the empty mktemp file, the generator fails on its own read, and the new branch copies the empty temp file onto the missing page:

$ make check-capabilities
cp: docs/capabilities.md: No such file or directory
open docs/capabilities.md: no such file or directory
make: *** [check-capabilities] Error 1
$ git status --porcelain
 M docs/capabilities.md          # 0 bytes

Before 5ba612a this branch only did rm -f "$$before", so the page stayed absent — that is the "no empty page written back" row from my last pass, and it inverts here. The verdict is still right (exit 2 either way); what changes is that a target documented as a pure check now writes a file, and the two sentences the same commit strengthens are both false in this case: Makefile:68 promises "a rerun fails the same way," and :69 promises make gen-capabilities "is the only command that writes the page."

The wedge is the part worth fixing rather than the empty file itself. Both claims break in the direction that leaves someone stuck:

$ make check-capabilities     # first run: page missing
open docs/capabilities.md: no such file or directory
$ make check-capabilities     # rerun, from the page the first run created
capability marker "<!-- capabilities:begin summary -->" is missing
$ make gen-capabilities       # the documented way out
capability marker "<!-- capabilities:begin summary -->" is missing

The generator reads the page for its markers before it writes, so once a 0-byte page exists the only documented write path refuses too, and recovery is git checkout docs/capabilities.md — which the contract does not mention because until now it was never needed. The error also degrades from no such file or directory, which names the problem, to a marker complaint that reads like a corrupt page.

Reachability, stated honestly: CI and the release job always check out the page, so both gates keep failing correctly and nothing gets published — release.yml:46 still stops the workflow before GoReleaser sees the tree. This is a local and bad-rebase case (and a sparse checkout), which is why I would not hold the merge for it. It is a one-line fix, and it is in a recipe that is about to be cited by two workflows:

@before=$$(mktemp); cp docs/capabilities.md "$$before" || { rm -f "$$before"; exit 1; }; \

I ran both cases against that guard: missing page exits 2 with the page still absent, and the partway-failed write still restores the full page. Guarding the snapshot rather than the restore also covers every other unreadable-source cause, and it keeps the invariant the recipe is really asserting — never write the page unless $before holds a real snapshot of it — in one place instead of two.

Smaller notes

• Nothing in the repo exercises this recipe, so all six rows above are probes rather than tests. That is defensible for a Makefile target, but it does mean the restore semantics this PR now states twice in prose are held only by review — the pre-5ba612a probe row is what a regression here would look like, and nothing would have caught it. If the gate is going to guard a tag, a small shell test that runs the target against a stub generator and asserts git status --porcelain is empty afterwards would pin the pure-check property directly.
• The doc edit stays inside its own paragraph. docs/capabilities-contract.md:160 now names both failure branches instead of implying one, and the step-3 marker edit at :270 is still this PR's only change to the rollout list even after main flipped step 4 to *(done)* underneath it. That is the per-PR-line convention working a second time, which is the point at which it is a convention rather than a coincidence.
• The rewrapped contract paragraph leaves The unconditional dangling at :162, which is cosmetic but makes the next diff on that paragraph noisier than it needs to be.

---

Re-approving. The fix I asked for is in and verified in both directions. The new finding is the mirror of it and is a one-line guard on the line above — worth doing whenever this recipe is next open, not worth another round trip on its own.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Re-approving ba1ac756. Finding 3 is fixed and I verified it in both directions — the partway-failed write now restores the page, and the pre-5ba612a recipe leaves it mangled under the same probe. The one new finding is the mirror case (a missing page is now written back as a 0-byte file, because the snapshot cp on Makefile:71 is unchecked); it is a one-line guard and does not block the merge. Details in the review comment.

This stamp was left by Claude Code (claude-opus-5).