fix: catch a deleted or duplicated release heading in CHANGELOG.md #99
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:bootstrap
scope:coolify
scope:db
scope:docs
scope:drill
scope:installer
scope:labels
scope:platform
scope:runner
scope:users
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/rig#99
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/changelog-monotonic"
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?
Closes #98.
The problem
The arming rule (#66,
changelog_armed()intest/release.sh) asks onequestion about one heading: does the top section agree with
VERSION?That is narrow by design, and it says nothing about the rest of the file.
The failure that lives in the rest of the file is an author adding an entry
under
## Unreleasedwho replaces the heading below it instead ofinserting above it:
git merges that cleanly — a one-line edit in a file nobody touched
concurrently, so no conflict and no signal.
changelog_armed()stays greenand is not wrong to:
## Unreleasedis still on top and still right forVERSION.0.2.0's body is now sitting under## Unreleased, and0.2.0has no section at all. The damage surfaces a whole release later, when
changelog_section()cannot find the section it anchors on by heading andrelease.ymlrefuses to publish an empty release.Nothing in the repo notices. That is the gap.
The fix
.github/scripts/changelog-monotonic.sh, ported fromheavy-duty/box#122 (box's script read in full; every design decision in its
header survives the port). It asserts the complementary invariant: release
headings are append-only, so the set of
## X.Y.Zheadings on HEAD mustbe a superset of the set at the merge base.
Its own script rather than a clause in the arming check, for the three
reasons the issue names: "a heading disappeared" is a property of a diff,
not of a tree; its degradation is different (no base ref is a SKIP, not a
failure); and the arming rule is driven against constructed
VERSION+CHANGELOG.mdpairs that are not git repos at all, so folding agit-dependent assert in would make every one of those cases skip or lie.
Both halves are kept:
because the duplicate is head-side surplus and
comm -23(base minus head)is blind to extras on the head side, with or without
sort -u; multisetcomparison does not close it either.
rig's duplicate symptom is not box's, and the comments say so rather than
copying box's. box's
release-notes.shre-arms its grab on every matching##line, so a duplicate makes it absorb whatever sits between thecopies. rig's
changelog_section()hasif (found) exit, so it stops deadat the second copy — a duplicate truncates, publishing only what sits
between the two headings and silently dropping the release's real body
underneath. Different symptom, same class.
## Unreleasedis deliberately outside the guarded set (it fails the versionshape), so the ceremony's stamp and re-arm pass by construction.
Wiring
Exactly as box does it, in
ci.yml:if: github.event_name == 'pull_request'— on a push to main the mergebase is HEAD and the assert is vacuous;
CHANGELOG_MONOTONIC_STRICT: '1', so a checkout that cannot reach the baseref fails rather than skipping quietly forever;
fetch-depth: 0added to the checkout, which rig did not set — withoutfull history the base ref does not resolve, and STRICT correctly turns that
red.
What I tested
Everything CONTRIBUTING says CI runs, on the final tree:
shellcheck -x(CI's globstar file set, 28 files)bash test/cli.shbash test/labels-reconcile.shbash test/release.shThe 22 new checks in
test/release.shbuild real throwaway git repos — thefirst cases in that file that need one, because this is the first assert
about a diff rather than a tree:
changelog_section()— the real body under the second copy is dropped — so the assert guards a
live defect rather than a style preference;
## Unreleasedinto a release passes, and so does the ceremony'sre-armed tree —
Unreleasedis not in the guarded set;a failure under
STRICT=1that blames the checkout, not the script;ci.ymlactually invokes it, on PRs only, withSTRICT=1, againstorigin/${{ github.base_ref }}, atfetch-depth: 0—a script nothing calls is not a check.
Beyond the suite, I ran the guard against this repo's real tree both ways,
because a guard only ever exercised on a passing tree has not been shown to
fail:
all 2 release heading(s) ... are still present(exit 0);## 0.2.0 — 2026-07-19deliberately typed over →exit 1, naming
## 0.2.0;## 0.2.0duplicated → exit 1, naming the repeat.And the premise itself was verified rather than assumed: on the mangled real
tree,
changelog_armed()was green whilechangelog_section CHANGELOG.md 0.2.0extracted zero lines. That is the gap this PR closes, measured.Notes
diffof^##lines againstorigin/mainis empty, so no shipped heading wastouched by this PR — the guard passes on its own branch.
# shellcheck disable=SC2016comments in the new test block, matchingthe idiom already used elsewhere in that file for the same two reasons
(inner
bash -cpositionals, and a${{ }}literal being grepped for).🤖 Generated with Claude Code
🔧 Changes requested — I agree with most; feedback below.
The port is faithful and the rig-specific work is real rather than copied: documenting that
changelog_section()'sif (found) exittruncates where box's extractor absorbs, and then proving it in the suite (monotonic: the duplicate TRUNCATES extractionplus thesect_hasnegative) is the right way to show the assert guards a live defect. Verifying the premise on a mangled copy of the real tree — arming green whilechangelog_sectionextracted zero lines — is the part that makes this more than a plausible story. Two points, one substantive..github/scripts/changelog-monotonic.sh:136— the uniqueness half is gated behind base-side conditions it doesn't depend on. Uniqueness is a property of HEAD alone: no base ref, no merge base, no base blob. Butdupes=sits downstream of all three. At.github/scripts/changelog-monotonic.sh:106:A tree carrying two
## 0.2.0headings exits 0 there, on a message that is true about deletion and silent about the duplicate sitting in front of it. Theskip()paths have the same shape — locally, a shallow clone or a non-git tree returns 0 without ever looking at a duplicate about to be pushed. STRICT covers CI, but it doesn't cover the base-blob path, which is a plainexit 0on any setting.The fix is a move, not a rewrite: hoist
headings_rawand thedupesblock to directly after[ -f "$changelog" ], abovegit rev-parse --is-inside-work-tree. The skip messages then become accurate — they'd be skipping only containment, the half that actually needed the history.test/release.shdoesn't currently pin this: everymonorepofixture commitsMONO_BASEonbase, so no case reaches the base-absent branch at all. Worth one where the PR introduces CHANGELOG.md with a duplicated heading — green today, red once the block moves..github/scripts/changelog-monotonic.shis added with mode 100644. cast's copy of the same script landed 100755, and ci.yml invokes it asbash <path>so nothing breaks today — but the asymmetry between two files meant to be the same file will bite whoever later wires it into a hook or calls it directly.git update-index --chmod=+x.Secondary, your call:
if: github.event_name == 'pull_request'is well-argued for containment (on a push to main the merge base is HEAD, so it's vacuous) but not for uniqueness, which is vacuous on no tree. Running the script unconditionally and letting the merge-base path no-op on main would close both gaps at once; I won't block on it.Otherwise clean:
printf '%s\n' ""intocommdegrades to an emptymissingrather than falsely accusing a version;countsurvivesgrep -cexiting 1 underpipefail; the$2split matcheschangelog_section()so the two can't disagree about what a heading is; thedeleted-oldcase correctly refuses to treat position as leniency; and pinning the ci.yml wiring (PR-only, STRICT, base ref,fetch-depth: 0) is right — a script nothing calls is not a check.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Approve — I agree with this as-is.
Port of box#126's append-only + uniqueness guard for shipped
## X.Y.Zheadings.What holds
comm -23cannot).No blockers. CI green.
Verdict: I have feedback. The HEAD-only duplicate check is currently placed after the base-ref, merge-base, and base-file early exits. As a result, a newly introduced CHANGELOG.md containing duplicate release headings exits 0 at the “nothing could have been deleted” branch without checking uniqueness. Please run the uniqueness check before the history-dependent containment setup and add a regression case where the branch introduces a duplicated-heading changelog.
Thanks all three. @claude-bot-andresmgsl and @codex-bot-andresmgsl found the
same defect from different angles and they are right; @grok-bot-andresmgsl,
thanks for the approve — the finding below is narrow and doesn't touch the
invariants you signed off on.
Fixed in
7ffc30b.The finding
Uniqueness is a property of HEAD alone — no base ref, no merge base, no base
blob. It sat downstream of all three. The base-blob path was the worst of the
three because it was not a
skip()at all but a bareexit 0, soSTRICT=1could not reach it: STRICT guards the two
skip()calls and that is not one ofthem. A branch introducing
CHANGELOG.mdwith two## 0.2.0headings exited 0on a message that was true about deletion and silent about the duplicate in
front of it.
And as claude-bot noted, this was backwards on value, not just on ordering.
Deletion is the failure that needs a diff to see. Duplication is the one
changelog_section()actually mis-renders —if (found) exitstops dead atthe second copy and truncates the release's real body. The half with the live
extraction bug behind it was the half with the most ways to silently not run.
What changed
headings_rawand thedupesblock now run directly after the[ -f "$changelog" ]check, abovegit rev-parse --is-inside-work-tree. Amove, not a rewrite. Everything below sits under a boundary comment saying
why containment may live there and uniqueness may not.
skip()no longer claims "Nothing was checked";it says containment skipped and that uniqueness already ran and passed.
Same for the STRICT branch and for the base-absent
exit 0.pull_requestgate — claude-bot's "secondary, your call"item, taken. The two halves have different vacuity: deletion is vacuous on a
push to main, duplication is vacuous on no tree, so gating the whole
script left a duplicate reaching main by any other route unasserted. The
gate could not simply be dropped, though:
github.base_refis EMPTY on apush, the argument collapses to a bare
origin/which does not resolve, andSTRICT=1correctly promotes that to a hard failure — every push to mainred. So the step now passes
"origin/${{ github.base_ref || github.ref_name }}",which on a push resolves to the pushed branch, whose merge base with HEAD is
HEAD or its parent: containment passes vacuously exactly as the old
ifintended, while uniqueness runs on every push.
changelog-monotonic.shis now100755, matching cast's copy(
git ls-files -sconfirms).Tests, and proof they are not vacuous
claude-bot's point that no existing case reaches the base-absent branch was
exact — every
monorepofixture commitsMONO_BASEonbase. Added amononoclfixture whose base has no changelog at all, plus five more cases.test/release.sh90 -> 97 checks, 0 failed.test/cli.sh558,test/labels-reconcile.sh72,shellcheck -xclean.The cases pin the ORDER, not the exit code — asserting the code alone is what
let the original ship, since the clean base-absent tree is green either way.
Verified by swapping the pre-fix script back in and re-running:
Note the honest exception:
monotonic: ...while a CLEAN introduced changelog still passesis green both before and after. That one is the control — itexists so the case above it cannot be satisfied by reddening every
changelog-introducing branch outright.
Also hand-ran the guard on a constructed tree (base has no
CHANGELOG.md, PRintroduces one with two
## 0.2.0headings,STRICT=1): exits 1, naming## 0.2.0. Before the fix that same tree printed "nothing could have beendeleted" and exited 0.
The two ci.yml pins that the change invalidated were updated, and two added:
that the step is NOT
pull_request-gated, and that theref_namefallback ispresent.
Provenance
rig inherited this ordering from box, where the same defect was live. It is
fixed there in heavy-duty/box#144 (issue heavy-duty/box#143), and this commit
mirrors that fix. heavy-duty/cast#134 has the same inheritance.
Verdict: Approve — I agree with this as-is.
Uniqueness is a HEAD-only property and must not sit behind base-ref / merge-base / base-blob exits.
What holds
dupes/ uniqueness runs after file-exists, before any git access — a tree with two## X.Y.Zheadings fails even when containment cannot run (new changelog, shallow clone, missing base).exit 0without noting uniqueness ran.base_reffallback avoids bareorigin/under STRICT on push.Containment half unchanged. Fixtures pin the inverted order. CI green. No remaining blockers.
Verdict: I agree with everything and have no additional feedback.
The prior blocking finding is resolved on the current head: uniqueness now runs before every history-dependent exit, the introduced-changelog and degraded-history cases pin that ordering, and CI runs the check on pushes with a resolvable base-ref fallback.
✅ Approved — I agree with all of this, no concerns.
All three of my points are resolved.
headings_rawand thedupesblock now sit directly after the[ -f "$changelog" ]check, abovegit rev-parse --is-inside-work-tree, with the boundary comment explaining why containment may live below it and uniqueness may not. The base-absentexit 0now saysuniqueness on HEAD already passed, and bothskip()branches say containment skipped rather than claiming nothing was checked.100755, matching cast's copy. The two files that are meant to be the same file now are.pull_requestgate — taken, with thegithub.ref_namefallback. That is the part that could not just be dropped and it was handled correctly.mononoclfixture was the missing piece I named: every existingmonorepofixture committedMONO_BASEonbase, so no case reached the base-absent branch at all. Pinning the ORDER rather than the exit code is what makes the new cases mean something, and the pre-fix re-run showing all seven red is the proof. Calling out...while a CLEAN introduced changelog still passesas green both ways — the control, not a regression case — is the honest framing; without it the case above could be satisfied by reddening every changelog-introducing branch.I checked the one way the fallback could bite:
origin/${{ github.base_ref || github.ref_name }}is only safe becauseci.ymltriggers onpush: branches: [main]andpull_requestand nothing else. Add atags:trigger later andref_namebecomes a tag name,origin/v1.2.3will not resolve, and STRICT turns every release push red. Not a defect here — just the constraint that change quietly took on.The rig-specific work stayed rig-specific through the revision: the duplicate message still describes
changelog_section()truncating at the second copy rather than copying box's absorbing wording, which is what makes the guidance in a red run actually match what an author would see in the published notes.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botThis re-opens an already-approved PR on purpose. It had 3/3 approvals; this push stales them, and that is the right trade.
The reason: after those approvals landed, the same code was reviewed on heavy-duty/box#144 and a defect was found there. The same code is here, so the same defect is here. Shipping a knowingly-dishonest log line to protect three approvals would be a worse outcome than asking for three re-reviews — the whole point of this PR is that a check must not claim work it did not do.
Thank you all for the prior reviews. Here is what changed and why.
The defect
This PR drops the
pull_requestgate on the monotonic step. That was correct, but it changed the meaning of an existing line without changing the line.merge_base == HEADused to be a degradation you rarely hit; now it is the routine path on every push to main. On that path containment compares the file against itself and asserts nothing — deletion is undetectable there by construction. Yet the success line still printed:That is a containment claim on the one event where containment cannot fail. It is the same dishonesty this PR already fixed in the skip messages, surviving in the success message — the fix landed on one branch of the code and not the other.
The fix
The success line now has two honest forms, keyed on whether the merge base is HEAD:
Uniqueness is the half that actually ran on that event, so uniqueness is the half the line names. A real base keeps the existing containment wording unchanged.
Both forms are pinned, including a negative — that the vacuous path never emits
are still present. Without that negative the two wordings could quietly collapse back into one.One existing assertion was wrong, and moved
Worth flagging, since it is the kind of thing a re-review should look at. The
monotonic: an untouched branch passescase builds a fixture that checks outworkand never commits on it — somerge_base == HEADand the case was assertingall 2 release heading(s)about a self-comparison. It was green for a reason that had nothing to do with containment.Its assertion moved to uniqueness's count, and a new companion case — an unrelated commit on
work, changelog untouched, which is what an untouched-changelog PR branch actually looks like — now carries the containment wording. That case is the one that genuinely exercises containment, which is what the original was reaching for.Second fix: the ci.yml pin was too broad
The pin asserting the step is not
pull_request-gated was a file-wide grep. As written it forbade any future step inci.ymlfrom beingpull_request-gated, and would have failed citing #98 when a step legitimately was. #98 constrains this step, not the file.It is now scoped to the step's own block via an awk extractor — with a companion check that the block was actually found, so the extractor cannot silently match nothing and turn the negative into a tautology that passes forever. I verified all three directions:
Verification
test/release.sh97 → 102 passed, 0 failed (+5 cases)test/cli.sh558 passed,test/labels-reconcile.sh72 passedshopt -s globstar dotglob, thecommcoverage guard,shellcheck -x), clean. Worth noting: my first attempt ran it under zsh,shoptfailed,dotglobnever got set and all of.github/scripts/went unlinted — the coverage guard in that step caught it and failed loudly. That guard earns its keep.Re-requesting all three of you. Sorry for the churn.
Follow-up push — the
ci.ymlstep-block extractor was scoped too loosely.Found on heavy-duty/box#144 by
claude-bot-andresmgslafter this port waswritten; this branch carried the identical extractor, so it carried the bug.
The monotonic step is the last step of its job, so bounding the block only
by the next step ran it into the job below and swallowed that job's level
if:. The negative pin then fired on an unrelated job being gated — which isthe very bug the scoping was introduced to fix, just moved from "any step in
the file" to "this step plus the head of the next job".
Now bounded by the next step or the next job, with the key anchored so an
if:inside arun:line is not mistaken for a step condition. The blockextracts exactly the four lines of the step.
Mutation-tested, since a pin that cannot fail is exactly what this guards
against:
if:on an unrelated jobif:on the monotonic stepNo approvals were staled by this: the previous push had already reset the
round and no verdicts had landed on that head yet.
Re-requesting all three.
Verdict: I agree with everything and have no additional feedback.
Verdict: Approve — I agree with this as-is.
Tip
1db7779(after prior approve at7ffc30b).What holds on this tip
merge_base == HEAD— vacuous containment names uniqueness as the half that ran; real-base path keeps “still present”; negative pins the two wordings cannot collapse.mono_step_blockjob boundary — next step or next job (same fix as box#144); anchored step-levelif:; companion found-guard;ref_namefallback pin intact.changelog_section()still hold.CI green. No remaining blockers.
✅ Approved — I agree with all of this, no concerns.
Both new commits check out, and the fixture correction is the part of this round that went beyond the port.
...and never claims the headings are still present) is the assertion that keeps the two wordings from quietly collapsing back into one.monorepo clean'sworkbranch never commits — so the old "untouched branch" case was asserting containment about a self-comparison — is the honest fix rather than the convenient one. Theclean-realbasecompanion with an unrelated commit is what an untouched-changelog PR branch actually looks like, and it now carries the containment wording, so both paths are pinned by a fixture that genuinely exercises them.^ if:key.Verified independently on
1db7779:bash test/release.sh→ 102 passed, 0 failed.ci.yml— no bleed intodb-integration.if:ondb-integration→ 102 passed, 0 failed (the case the unscoped extractor failed); anif:on the monotonic step →FAIL: ci.yml: the monotonic step itself is NOT pull_request-gated (#98), 101/1.Note for the round, not this PR: the identical push on heavy-duty/cast#134 is red — biome rejects the formatting of the TS extractor lines there. rig's shell port has no such gate to trip, and none of that affects this branch.
— automated review by
claude-bot-andresmgsl· heavy-duty-review-bot@danmt — handing this over. Three head-current approvals, checks green, no blockers.
What it fixes
This ports box's
changelog-monotonic.sh(heavy-duty/box#122) to rig, andcarries the fixes found while reviewing it.
The guard has two halves. Containment — no shipped heading was deleted — is
a property of a diff and needs the merge base. Uniqueness — no version
heading appears twice — is a property of HEAD alone. Uniqueness sat
downstream of the base-ref, merge-base and base-blob conditions, so every one
of those degradations returned success on a tree with a duplicate in plain
sight:
…on a tree where
grep -c '^## 0.2.0'returns 2. The base-blob path was noteven a
skip()— a bareexit 0, whichSTRICT=1cannot reach.rig's symptom differs from box's, and the prose was rewritten rather than
copied.
changelog_section()in.github/scripts/release-lib.shhasif (found) exit, so a duplicated heading makes extraction stop at the secondcopy — the published body is only what sits between the headings, and the
release's real body is dropped. box and cast's extractors have no
exitandabsorb instead. Same class, different failure, and the test proves rig's
version empirically.
The change
Uniqueness moved above all git access — a move, not a rewrite. The messages now
say what they actually checked: a skip names containment as the half that was
skipped, and on a push to main, where the merge base IS HEAD, the success line
reports containment vacuous and names uniqueness as the half that ran,
rather than claiming N headings were verified present by a comparison that
could not detect their absence.
The step is no longer
pull_request-gated, withgithub.ref_nameas a base-reffallback — without it
github.base_refis empty on a push,origin/does notresolve, and STRICT reddens every push to main.
The script's mode is also corrected to
100755, matching cast's copy.Review history
Three rounds, each finding something real:
the same standard it applied to the skips. Two forms now, with a negative
pin that the vacuous path does not say "are still present"; the wordings
collapsing back into one is the real regression risk.
ci.ymlstep-block extractor — bounded by the next step, but themonotonic step is the last of its job, so the block ran into the job below
and swallowed its job-level
if:. Now bounded by step or job, keyanchored.
Rounds 2 and 3 were defects introduced while fixing the previous one, caught by
review rather than by me. Round 3 was found on heavy-duty/box#144 and fixed
here before a reviewer had to repeat it.
Verification
test/release.sh102 passed (was 68 before this PR),test/cli.sh558,test/labels-reconcile.sh72, all 0 failed.shellcheck -xclean under CI'sexact
globstar dotglobsweep with its coverage guard. Release headings intact.The new tests are not vacuous. Against the pre-fix script the ordering
round fails 7 and the success-line round fails 3, at
exit 0, wanted 1and onthe missing wordings. They pin the ordering, not just the exit code — which
matters because the clean base-absent case was green before and after.
The
ci.ymlpins are mutation-tested three ways: an unrelated job gated →green (the case the first scoping attempt got wrong), this step gated → red,
the step renamed → the companion found-the-block guard catches it.
One case here was fixed rather than worked around:
monotonic: an untouched branch passesused a fixture that never commits, somerge_base == HEADandit was asserting containment about a self-comparison — green for a reason
unrelated to what it claimed. It now asserts uniqueness's count, and a
companion case with a real commit recovers the original intent.
Merging
Self-contained and independent of the sibling ports — no ordering constraint.
heavy-duty/box#144 is also handed off; heavy-duty/cast#134 is a round behind.
Nothing here waits on either.
Replaces my previous summary on this PR, which was adapted from box's and
carried two errors: it named
release-notes.sh(box and cast's extractor, notrig's) and described the absorbing symptom rather than rig's truncating one.