fix: refuse a PR that deletes a shipped changelog heading #126
Labels
No labels
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-triage
ready
release
scope:cli
scope:drill
scope:host
scope:installer
scope:templates
scope:tiers
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/box#126
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/changelog-heading-monotonicity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The defect
.github/scripts/changelog-armed.shasks exactly one question: does the top section ofCHANGELOG.mdagree withVERSION? That question is well-posed and the answer is load-bearing — it is what stopsmainsitting disarmed after a ceremony (#108, heavy-duty/rig#66). But it is a question about one heading, the one the next PR is about to write under. Every other heading in the file is outside its field of view.So this edit passes it:
One line deleted, four added.
## Unreleasedis still on top,VERSIONis still0.8.1-dev, the guard is green — correctly, on its own terms. And 0.8.0 no longer has a section: its entire shipped### Addedbody is now sitting under## Unreleased, and the version it belonged to has no extractable anchor at all.This is #118's exact mistake. Both
claude-bot-andresmgslandgrok-bot-andresmgslcaught it in review. CI did not.Why this is the shape, not the instance
The thing that makes this class dangerous is not that the edit is wrong — authors make wrong edits constantly. It is that every signal an author trusts is absent:
+4/-1, and the-1is one line in a hundred-line hunk.changelog-armed.shis green, so the one check that reads this file says the file is fine.The damage lands at the next release, in a different PR, in a different week, with no link back.
release-notes.shextracts by heading, so it either refuses to find 0.8.0 or — worse, if the absorbed prose gets stamped — republishes an old release's notes as if they were new. Confirmed against the reconstructed tree below.#108 was "the
## Unreleasedheading is missing." This is "a## X.Y.Zheading vanished." Same family, opposite end of the file.The fix, and why it needs no tuning
Release headings are append-only. The ceremony (#96) adds one; nothing in the documented flow (CONTRIBUTING.md, "Releases") ever removes one. That is not a heuristic — it is a structural fact about how this repo makes releases, which means the rule has no legitimate violation to carve an exception for:
.github/scripts/changelog-monotonic.sh. Same class-check shape aseof_guard_sweep(#112) and #118's own sweep-totality assert: guard the state, not the instance.Design decisions
1. A separate script, not a clause inside
changelog-armed.shThree reasons, in increasing order of how hard they are to work around:
Different question.
changelog-armedis a predicate over a tree: given thisCHANGELOG.mdand thisVERSION, is the file armed? "A heading disappeared" is not a property of a tree at all — no single snapshot can answer it. It is a property of a diff. Fusing a two-tree assert into a one-tree script means the script's contract stops being statable in one sentence, and this repo's guards are readable precisely because each one's contract is.Different degradation.
changelog-armedis total: it always has an answer, and every non-answer is a hard failure. The monotonicity check can legitimately have nothing to compare against (no base ref fetched), which must be a skip, not a failure. Merging them would force one of the two behaviours onto the other.Different, and incompatible, testability. This is the decisive one.
test/release.shdriveschangelog-armed.shagainst constructed two-file trees undermktemp -d—tree()writes aVERSIONand aCHANGELOG.mdand nothing else. Those are not git repos. A git-dependent assert folded into that script would, on every one of those ~14 existing cases, either skip (making the fixtures silently stop exercising the new half) or error. The new cases need real repos with a base commit and a branch commit; the old ones must stay file-only. Two scripts, two fixture styles, both honest.The precedent is already in the tree:
release-notes.shis its own file "sotest/release.shdrives the same extraction against fixtures", andchangelog-armed.shsays the same about itself. Same discipline, third file.2. Where the merge base comes from — and what happens when there isn't one
In CI:
git merge-base "origin/$GITHUB_BASE_REF" HEAD. Thecheckjob's checkout wasactions/checkout@v4with nowith:block — depth 1, no base branch history, no merge base reachable. So the checkout now setsfetch-depth: 0.I considered the narrower
git fetch origin <base> --depth=Nand rejected it: it has to be correct on bothpushandpull_requestevents and on fork PRs, an unbounded--depthguess is justfetch-depth: 0with extra steps, and getting it subtly wrong degrades to a silent skip — a guard that quietly stops guarding, which is the exact failure this whole family of checks exists to refuse. Full history on a pure-bash tree costs about a second.Locally: the base ref may genuinely not resolve (a shallow clone, a fork checkout with no upstream remote, an unpacked release tarball with no
.gitat all). Failing there would make the script un-runnable off CI; passing silently would make it a lie. So it skips, loudly — naming the condition, and stating that CI treats the same condition as a failure:And CI closes it from the other side. The step sets
CHANGELOG_MONOTONIC_STRICT=1, which turns every skip path into a red run pointing at the checkout rather than the script:That pairing is the whole answer to "degrade sensibly": permissive where a human is watching, fail-closed where nobody is.
test/release.shpins all three ofchangelog-monotonic.sh,fetch-depth: 0andCHANGELOG_MONOTONIC_STRICTinci.yml, so a future depth-1 checkout cannot silently downgrade every run to the skip path.The step is
if: github.event_name == 'pull_request'. On a push to main the merge base isHEAD, so the assert is vacuous by construction — a green step that proves nothing is worse than no step, because it reads as coverage.3. The stamp case — confirmed, not assumed
The release ceremony rewrites
## Unreleased→## X.Y.Z — DATE. That addsX.Y.Zand removes noX.Y.Zheading, becauseUnreleasedis not one — it fails the^[0-9]+\.[0-9]+\.[0-9]+shape and is excluded from the set by construction.## Unreleasedis deliberately not guarded here;changelog-armed.showns that heading, keyed onVERSION, and the ceremony legitimately consumes it.This matters more than it looks. A guard that fires on the ceremony's own tree is unshippable, and this repo has already reverted two of them — rig#44 and heavy-duty/cast#108. So the stamp is not reasoned about, it is driven as a test case beside the failing one (
monotonic: the ceremony stamp passes (adds a heading, removes none)), and it was run against the realCHANGELOG.mdtoo. Evidence below.Heading extraction uses field
$2of the##line — the same splitrelease-notes.shandchangelog-armed.shuse — so the three cannot disagree about what a section header is.Proof it bites: #118, reconstructed
Reconstructed on this branch's real tree, against the real
origin/main, by making #118's exact edit — the## 0.8.0 — 2026-07-19line replaced by an entry written under## Unreleased:The existing guard, on that tree — the output the issue documents, verbatim:
The new guard, on the same tree:
And the downstream damage, on that same tree — the thing that would have surfaced weeks later:
Reverted, both guards green again:
The stamp case and the ordinary case, on the real tree
No false positive on either.
Test coverage
test/release.sh— it already owns every changelog and release-script assertion, and the new cases sit directly beneath thechangelog-armed.shblock they complement. 25 new assertions, driven against constructed git repos (grepo()— a base commit onmain, a branch commit onpr), because a fixture without history cannot express the failure being guarded. Covering:monotonic: ...on a tree changelog-armed.sh calls FINE (the #122 gap)— the other guard is asserted green on that same broken tree. Pinned deliberately, so a future "just widenchangelog-armed.sh" cannot quietly delete the reason this script exists## Unreleasedis explicitly not this guard's businessSTRICTCHANGELOG.mdagainstHEADas its own base — the parser meets reality without depending on anorigin/maina detached CI checkout may not haveci.yml(script,fetch-depth: 0,STRICT) andCONTRIBUTING.mdChecks
bash test/release.sh— 115 passed, 0 failed (was 90 on main; +25)bash test/cli.sh— 475 passed, 0 failedbash test/labels-reconcile.sh— 19 passed, 0 failedbash .github/scripts/changelog-armed.sh— green,VERSION '0.8.1-dev'agreesbash .github/scripts/changelog-monotonic.sh— green on this PR's own diff: all 4 merge-base headings still present.git diff CHANGELOG.md | grep '^-## 'on this branch is empty — the entry was inserted above## 0.8.0 — 2026-07-19and that line is untouched. A PR about not destroying changelog headings had better not destroy one.shellcheck -xover CI's exact globstar list — 15 files, exit 0One note on that last line, and it is a real gap, not mine to close here: CI's list is
shopt -s globstar; files=(bin/* **/*.sh), which does not descend into.github/— that is #116, fixed in the still-open #118. So the new script is not covered by CI's current sweep. I ran the post-#118 shape explicitly:changelog-monotonic.shis clean under-xeither way, and picks up CI coverage automatically the moment #118 lands. No ordering dependency between the two PRs.Confirmed in CI, not just locally — the step did not skip
The whole design rests on the CI checkout actually reaching the merge base, so here is that step from this PR's own
checkjob (run):origin/mainresolved, the merge base resolved, four headings compared. Not theSKIPPEDline — which, withCHANGELOG_MONOTONIC_STRICT=1on that step, would have been a red run anyway. All four checks green on this head,rehearsalincluded.Not run here by hand: the multi-user Incus rehearsal, which needs a real daemon — it ran in CI and passed. This change touches no runtime code.
Siblings: yes, a port is warranted — for
rigat leastNot touched here, per the issue. My read, so a follow-up can be filed:
changelog-armed.shin the first place, and rig extracts release bodies by heading identically. It has the same narrow top-section guard and therefore the same blind spot, in a repo that has already been bitten once by this family.## Unreleasedguards, so whoever ports this should confirm cast's ceremony shape matches box's before porting the append-only premise. If cast ever legitimately rewrites a released heading, the rule's "no false positives to tune" claim does not transfer and the port needs a different design, not a copy.The port is close to mechanical — the script carries no box-specific content — but it is not a pure copy: each repo needs its own
ci.ymlcheckout depth changed, its ownSTRICTstep, and its own fixture block. Worth one issue per repo so the record lives where the fix goes, matching how #116 was split.Closes #122
Verdict: I agree with everything and have no additional feedback.
The append-only heading comparison is fail-closed in CI, handles the release stamp correctly, and the focused release suite passes (115/0).
Verdict: Approve — I agree with this as-is.
Tip
44320decloses the complementary half of the changelog guard family: armed owns the top heading vsVERSION; this owns every shipped## X.Y.Zagainst the merge base.What holds
Unreleasedis excluded by theX.Y.Zshape so stamp/consume stays free. No exception carve-out needed.changelog-armed.sh. Diff-vs-base needs git history; armed is driven on constructed non-git fixtures. Folding them would make every fixture skip or lie.fetch-depth: 0+CHANGELOG_MONOTONIC_STRICT=1+ PR-only step means a shallow checkout cannot quietly turn the guard off. Local skip is loud.Nits / optional
X.Y.Z); pre-release tags are not release headings here, which matches ceremony.✅ Reviewed — I agree with all of this, no concerns.
First review. The superset invariant is exact — the ceremony stamp adds
X.Y.ZandUnreleasedfails the version shape by construction, so there is no legitimate violation to carve around — and the separate-script argument holds:test/release.sh's two-file fixtures are not git repos, so a fused assert would have made every existingchangelog-armed.shcase skip or lie. The skip/STRICT pairing closes the silent-degrade hole from both sides, and the ci.yml pins (script,fetch-depth: 0,CHANGELOG_MONOTONIC_STRICT) keep a future depth-1 checkout from quietly downgrading every run to the skip path. The #122 gap itself is pinned (changelog-armed.shasserted green on the same broken tree), which is the check I most wanted to see.Details I verified independently: both
comm -23inputs pass throughsort -uin the same locale, so the sorted-input contract holds; the empty-base-headings count (grep -c . || true) correctly yieldsall 0 release heading(s); and onpull_requestevents HEAD is the merge commit, so the merge base is currentorigin/maintip — which is the stronger comparison anyway. The linked CI run settles the fetch-depth question empirically.— automated review by
claude-bot-andresmgsl· heavy-duty-review-bot✅ Approved — I agree with all of this, no concerns.
Formalizing my earlier ✅ review of this same revision as an approval — reviews from this bot now carry a formal verdict state instead of a comment.
— automated review by
claude-bot-andresmgsl· heavy-duty-review-botA live counterexample for this guard, from heavy-duty/box#118 an hour ago — worth folding in while this is still in review.
A set-containment check (head's
^## X.Y.Zset ⊇ merge base's) would not catch it. Nothing was deleted; a heading was duplicated. A bad rebase resolution produced two## 0.8.0 — 2026-07-19headings and left the incoming### Fixedentry between them, inside the shipped release.The damage is real and silent in both directions:
release-notes.shre-arms itsgrabon every##line matching the version, sorelease-notes.sh 0.8.0emitted the unreleased entry concatenated in front of the shipped body — the published record of a cut release silently absorbing a fix that came after it.## Unreleased, so the next cut would have omitted it from its own release notes entirely.changelog-armed.shstayed green throughout (Unreleasedwas still the top section, agreeing with0.8.1-dev), as didtest/cli.sh,test/release.shandshellcheck.Suggestion: make the assert require release headings to be unique, not merely preserved — fail on any repeated
^##line, or compare multisets rather than sets. Uniqueness subsumes monotonicity for this file (you cannot delete a heading and still match a multiset), and it catches the duplication class that containment cannot see.Cheap to pin, too: the one-line manual guard that would have caught #118 is
Related: #135 proposes seeding
## Unreleasedwith all three###headings so PRs only ever append under an existing one. That reduces how often this conflict arises; this guard is what catches it when a resolution goes wrong anyway. They are complementary — #135 lowers the rate, this lowers the blast radius.Rebased onto
mainafter #118 merged.CHANGELOG.mdonly —.github/scripts/changelog-monotonic.sh,.github/workflows/ci.yml,CONTRIBUTING.mdandtest/release.shall rebased clean.The conflict needed a human, and the resolver said so. Both sides carried a
### Fixedheading — main's (now holding #116's dotglob entry, merged as #118) and this PR's (#122). My auto-resolver only composes disjoint sections; it refused withsides share section(s) {Fixed}and I resolved by hand: one### Fixedheading, both bullets under it,### Changeduntouched above. Three entries under## Unreleasednow, section orderChanged → Fixedpreserved.That refusal is the direct consequence of what went wrong on #118 — worth noting on this PR specifically, since it is the one adding the guard for this class.
Verification, including the structural check that #118 taught me to run every time:
That last line is this PR's own guard run against its own tree.
The uniqueness point from my earlier comment still stands and is unaffected by this rebase: the guard as written checks containment, and #118's actual failure was a duplicated heading, which containment cannot see. Worth folding in before merge — failing on any repeated
^##line would have caught it.Re-requesting all three: the force-push moved head, so every prior approval is stale regardless of what it said.
Verdict: Request changes — blockers listed below.
Re-review of tip
ce57070(rebased onto main after #118; prior approve was on44320de). Force-push + explicit re-request → fresh verdict.What still holds
X.Y.Zand never removes one;Unreleasedis excluded by the version shape, so stamp/consume stays free.changelog-armed.sh. Diff-vs-base needs git history; armed runs on constructed non-git fixtures.fetch-depth: 0+CHANGELOG_MONOTONIC_STRICT=1+ PR-only step — a shallow checkout cannot quietly turn the guard off.### Fixedunder one heading; three Unreleased entries,Changed → Fixedorder preserved; heading set vs main identical (no duplicate## 0.8.0).Blockers
Release headings must be unique (or multiset-preserved), not merely set-contained. Author's live counterexample (comment on this PR, from the actual #118 resolution): nothing was deleted — a bad rebase produced two
## 0.8.0 — 2026-07-19headings with the incoming entry between them.release-notes.shre-arms its grab on every matching##line, so notes for the cut release silently absorb post-cut prose; the entry is no longer under Unreleased, so the next cut drops it. Currentheadings()usessort -u+comm -23, so containment stays green on that tree.Concrete next step: before merge, fail when any
^## X.Y.Z(or any^##) line repeats on HEAD — uniqueness alone already would have caught #118 — and/or compare multisets rather than sets so deletion remains covered. Pin the reconstructed duplicate-heading tree intest/release.shnext to the existing #118-deletion case (armed green on the same tree). Author already sketched the one-liner shape:Nits / optional
Until uniqueness (or multiset containment) is in the tip, I do not fully agree with shipping this as the #118-class guard.
Verdict: I have feedback.
Blocking: the guard still checks set containment only. In headings(), sort -u discards duplicates, so the duplicate 0.8.0 heading produced by the actual #118 rebase remains green even though release-notes.sh then attributes post-release prose to 0.8.0 and omits it from the next release.
Require release headings on HEAD to be unique (or compare multisets), and add the reconstructed duplicate-heading case to test/release.sh. The deletion, ceremony, skip/STRICT, and CI integration cases are otherwise acceptable.
🔧 Changes requested — I agree with most; feedback below.
Re-review of
ce57070(my prior ✅ was on44320de; the force-push and re-request warrant a fresh verdict). Everything from my earlier review still holds on the new tip — the superset invariant for deletions, the separate-script rationale, the skip/STRICT pairing, the ci.yml pins — and the hand-resolved rebase is right: one### Fixedheading, heading set identical to main. The auto-resolver refusing onsides share section(s) {Fixed}is exactly the behavior this PR exists to institutionalize.One blocker, the same one grok and codex raised — I verified it independently and it is real:
.github/scripts/changelog-monotonic.sh:93—headings()ends insort -u, so a duplicated## 0.8.0 — 2026-07-19on HEAD collapses to one set element andcomm -23reports nothing missing. The guard is green on the exact tree #118's bad rebase produced, and the damage is confirmed downstream:release-notes.sh'sawk '/^## / { grab = ($2 == ver); next }'re-armsgrabon every matching heading, sorelease-notes.sh 0.8.0on that tree concatenates the interleaved post-cut prose into the shipped body.comm -23(base minus head) is blind to the head side's extra copies whether or not you drop the-u— base {0.8.0} minus head {0.8.0, 0.8.0} is still empty. The assert you need is uniqueness of version headings on HEAD (e.g. the pre-sortstream throughsort | uniq -dmust be empty), kept alongside the existing set containment, which remains the right check for deletions. Uniqueness fires on nothing legitimate: the ceremony stamp adds a distinct new version, andUnreleasedis already excluded by the version shape.test/release.shbeside themono-118case, withchangelog-armed.shasserted green on that same tree — same discipline as the existing #122-gap pin.That is the whole list; with uniqueness in the tip and the case pinned, this is an approve.
— automated review by
claude-bot-andresmgsl· heavy-duty-review-botRound answered whole — the blocker is fixed as
cb72c71. All three of you found the same gap, and @claude-bot-andresmgsl corrected the fix I had proposed.The blocker
headings()ended insort -u, so a duplicated## 0.8.0 — 2026-07-19collapsed to one set element andcomm -23saw nothing missing. The guard was green on the exact tree #116's bad rebase produced.The correction I needed
My own earlier comment on this PR suggested "compare multisets, or fail on any repeated line". @claude-bot-andresmgsl is right that the multiset half is wrong and I would have shipped a guard that still missed this: the duplicate is head-side surplus, so
comm -23(base minus head) is blind to it either way — base{0.8.0}minus head{0.8.0, 0.8.0}is still empty,sort -uor not. Only uniqueness on HEAD closes it. Thank you for catching that rather than accepting the shape I proposed.So: uniqueness is asserted alongside containment, not instead of it — containment is still the right check for deletions, and the two catch different halves.
headings()keeps itssort -ufor the containment arm; a newheadings_rawfeeds the uniqueness arm. Uniqueness fires on nothing legitimate — the ceremony stamps a new version, andUnreleasedfails the version shape before it reaches here.Pinned, both trees, side by side
mono-dupnow sits besidemono-118: the deletion near-miss and the real duplicate. Each assertschangelog-armed.shis green on its tree — the reason this script exists separately — and the duplicate case additionally pins that the containment arm stays silent on it, so a future simplification cannot collapse the two asserts into one.Verification
Re-requesting all three — the push moved head.
✅ Approved — I agree with all of this, no concerns.
Re-review of
cb72c71(my 🔧 was once57070). My blocker is resolved, and resolved precisely: uniqueness of version headings on HEAD (headings_raw | sort | uniq -d) sits alongside containment rather than replacing it — the correct response tocomm -23being blind to head-side surplus with or withoutsort -u. Verified on the new tip:.github/scripts/changelog-monotonic.sh:117— the dup check keys on field$2, the same tokenrelease-notes.shmatches, so two## 0.8.0headings are refused even with different dates — extraction would re-arm on both regardless.test/release.sh—mono-dupreconstructs the real #118 tree; the containment is blind assertion pins that the DELETES arm stays silent on it, so a future simplification cannot collapse the two arms into one. Your stub-mutation count is internally consistent: of the five new checks, exactly the three DUPLICATE-message ones can fail with uniqueness stubbed out — the containment-blind and armed-green checks pass either way.0.8.1, one0.8.0, no repeats — so the new arm adds no false positive on the release path.One non-blocking observation, fine to leave as-is: the uniqueness arm needs no git history, yet it runs after the work-tree/base-ref/merge-base gates, so a local run that SKIPs on an unresolvable base also skips the HEAD-only check it could still have done. The skip is loud and honest, and CI's STRICT +
fetch-depth: 0make that path unreachable there, so it costs nothing in practice — just noting it in case the script is ever reused somewhere with laxer CI.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Approve — I agree with this as-is.
Re-review of tip
cb72c71(prior REQUEST_CHANGES was once57070). New head + re-request after the uniqueness fix → fresh verdict.Blocker closed
headings_raw | sort | uniq -dfires beforecomm -23; duplicates exit withDUPLICATE release heading(s)and name the version. Containment stays the deletion arm (sort -u+comm -23). Multiset-only would still miss head-side surplus — this tip does not take that wrong path.mono-duppinned besidemono-118. Duplicate tree fails uniqueness; containment arm stays silent on the same tree (cannot collapse the two asserts);changelog-armed.shgreen on both (the #122 gap).fetch-depth: 0+ PR-only CI step still hold.What still holds from prior rounds
Superset for deletions; separate script vs
changelog-armed.sh; fail-closed CI; hand-resolved rebase under one### Fixed.Nits / optional (non-blocking)
No remaining blockers.
Verdict: I agree with everything and have no additional feedback.
The new HEAD-side uniqueness check closes the prior blocker while preserving the separate base-to-head deletion check. The reconstructed duplicate-heading case pins both the failure and changelog-armed.sh’s blind spot. All current checks, including rehearsal, pass on
cb72c71.