diff --git a/.github/scripts/changelog-monotonic.sh b/.github/scripts/changelog-monotonic.sh index 1a3c55d..b1044d4 100755 --- a/.github/scripts/changelog-monotonic.sh +++ b/.github/scripts/changelog-monotonic.sh @@ -64,26 +64,18 @@ skip() { if [ "$strict" = "1" ]; then echo "changelog-monotonic: $* — and CHANGELOG_MONOTONIC_STRICT=1, so this is a FAILURE, not a skip." >&2 echo " CI sets STRICT because a guard that quietly stops guarding is worse than no guard." >&2 + echo " (Uniqueness on HEAD already passed; it is containment that cannot run.)" >&2 echo " Fix the checkout, not this script: the base ref must be fetched (fetch-depth: 0)." >&2 exit 1 fi - echo "changelog-monotonic: SKIPPED — $*" - echo " (Nothing was checked. In CI this same condition is a hard failure.)" + echo "changelog-monotonic: containment SKIPPED — $*" + echo " (Uniqueness on HEAD already ran and passed — only the deleted-heading" + echo " half needs the history. In CI this same condition is a hard failure.)" exit 0 } [ -f "$changelog" ] || { echo "changelog-monotonic: no such file: $changelog" >&2; exit 1; } -git rev-parse --is-inside-work-tree >/dev/null 2>&1 \ - || skip "not inside a git work tree, so there is no history to compare against" - -git rev-parse --verify --quiet "$base_ref^{commit}" >/dev/null \ - || skip "base ref '$base_ref' does not resolve here (a shallow clone, or a fork checkout without the upstream remote)" - -merge_base="$(git merge-base "$base_ref" HEAD 2>/dev/null || true)" -[ -n "$merge_base" ] \ - || skip "no merge base between '$base_ref' and HEAD (unrelated histories, or a clone too shallow to reach one)" - # The set of RELEASE headings: '## ...' where looks like a # version. Field $2, the same split the arming rule and release-notes.sh use, # so the three cannot disagree about what a section header is. 'Unreleased' @@ -95,14 +87,6 @@ headings_raw() { } headings() { headings_raw | sort -u; } -# The changelog may not exist at the merge base at all (the commit that adds -# it). Nothing to have deleted, so nothing to assert. -base_file="$(git show "$merge_base:$changelog" 2>/dev/null || true)" -[ -n "$base_file" ] || { - echo "changelog-monotonic: $changelog does not exist at the merge base ($(git rev-parse --short "$merge_base")) — nothing could have been deleted." - exit 0 -} - # --- uniqueness on HEAD (the box#118 class) ---------------------------------- # Containment catches a DELETED heading. It cannot catch a DUPLICATED one: the # duplicate is head-side SURPLUS, and `comm -23` (base minus head) is blind to @@ -168,6 +152,40 @@ EOF exit 1 fi +# --- everything below needs the HISTORY -------------------------------------- +# Uniqueness is settled. What follows is containment, which compares HEAD +# against the merge base and therefore genuinely depends on the base ref, the +# merge base, and the base blob. Each of those can be unavailable for reasons +# that are not the author's fault (a shallow clone, a fork checkout without the +# upstream remote, the commit that first adds the changelog), so each degrades +# rather than failing — which is exactly why the uniqueness half must NOT live +# down here (#133, box#143). It asks nothing of the history, and gating it +# behind these conditions let a duplicate exit 0 on a message about deletion. +# +# That ordering mattered MORE here than anywhere. cast's release-notes.sh has +# no `exit`, so `grab` re-arms on every matching '## ' line and a duplicate +# makes the published body ABSORB whatever sits between the copies — the live +# extraction bug this guard exists for. The half with that bug behind it was +# the half with the most ways to silently not run. + +git rev-parse --is-inside-work-tree >/dev/null 2>&1 \ + || skip "not inside a git work tree, so there is no history to compare against" + +git rev-parse --verify --quiet "$base_ref^{commit}" >/dev/null \ + || skip "base ref '$base_ref' does not resolve here (a shallow clone, or a fork checkout without the upstream remote)" + +merge_base="$(git merge-base "$base_ref" HEAD 2>/dev/null || true)" +[ -n "$merge_base" ] \ + || skip "no merge base between '$base_ref' and HEAD (unrelated histories, or a clone too shallow to reach one)" + +# The changelog may not exist at the merge base at all (the commit that adds +# it). Nothing to have deleted, so nothing to assert. +base_file="$(git show "$merge_base:$changelog" 2>/dev/null || true)" +[ -n "$base_file" ] || { + echo "changelog-monotonic: $changelog does not exist at the merge base ($(git rev-parse --short "$merge_base")) — nothing could have been deleted (uniqueness on HEAD already passed)." + exit 0 +} + base_headings="$(printf '%s\n' "$base_file" | headings)" head_headings="$(headings < "$changelog")" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 17934d9..dac6604 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,20 +37,31 @@ jobs: - name: labels state-machine tests run: bash test/labels-reconcile.sh - # ...and no SHIPPED release heading was deleted (#133; box#122's guard). - # Its own step so that when it goes red the log names the invariant that - # broke — and a DIFFERENT invariant from the arming rule npm test - # carries: arming is a fact about this tree, monotonicity is a fact - # about this tree versus its merge base. Pull requests only: on a push - # to main the merge base IS HEAD, so the assert is vacuous and would - # only add a green step that proves nothing. STRICT=1 so a checkout that - # cannot reach the base ref fails here instead of skipping quietly - # forever. - - name: no shipped changelog heading was deleted - if: github.event_name == 'pull_request' + # ...and no SHIPPED release heading was deleted or DUPLICATED (#133; + # box#122's guard, box#143's ordering fix). Its own step so that when it + # goes red the log names the invariant that broke — and a DIFFERENT + # invariant from the arming rule npm test carries: arming is a fact + # about this tree, monotonicity is a fact about this tree versus its + # merge base. STRICT=1 so a checkout that cannot reach the base ref + # fails here instead of skipping quietly forever. + # + # NOT pull-request-only, and that is the #133 fix at the workflow level. + # The two halves have different vacuity: DELETION is vacuous on a push + # to main (the merge base IS HEAD), but DUPLICATION is vacuous on no + # tree at all, so gating the whole script on `pull_request` left a + # duplicate that reached main by any other route unasserted forever. + # + # The `|| github.ref_name` fallback is load-bearing, not defensive. On a + # push event `github.base_ref` is EMPTY, so the argument would collapse + # to a bare `origin/`, which does not resolve — and STRICT=1 correctly + # promotes that to a hard failure, turning every push to main red. With + # the fallback it resolves to the pushed branch, whose merge base with + # HEAD is HEAD or its parent: containment passes vacuously, exactly as + # the old `if` intended, while uniqueness now runs on every push. + - name: no shipped changelog heading was deleted or duplicated env: CHANGELOG_MONOTONIC_STRICT: "1" - run: bash .github/scripts/changelog-monotonic.sh "origin/${{ github.base_ref }}" + run: bash .github/scripts/changelog-monotonic.sh "origin/${{ github.base_ref || github.ref_name }}" # The installer, proven by RUNNING it — CAST_INSTALL_SOURCE points it at # this checkout, so CI proves the installer under review (the versioned diff --git a/CHANGELOG.md b/CHANGELOG.md index daf4615..b5f344c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,12 +52,46 @@ actually cutting it, and this file starts there. - **A PR that deletes a shipped release heading is now CI-red** (#133, heavy-duty/box#122) — `.github/scripts/changelog-monotonic.sh` asserts that the set of `## X.Y.Z` headings on HEAD is a superset of the set at the merge - base, and that no version heading appears twice. Wired into `ci.yml` on pull - requests only (on a push to main the merge base *is* HEAD, so the assert is - vacuous), with `CHANGELOG_MONOTONIC_STRICT=1` and `fetch-depth: 0` so a + base, and that no version heading appears twice. Wired into `ci.yml` on + every event, with `CHANGELOG_MONOTONIC_STRICT=1` and `fetch-depth: 0` so a checkout that cannot reach the base ref fails loudly rather than skipping quietly forever. +- **...and a duplicate heading no longer slips through on the paths where the + guard cannot see the base** (#133, heavy-duty/box#143) — the uniqueness half + is a property of HEAD alone, but it sat downstream of the base-ref, + merge-base and base-blob conditions, so each of those degradations returned + success on a tree with a duplicate in plain sight. + + The base-blob case was the worst of the three because it was not a skip at + all: a branch that *introduces* `CHANGELOG.md` exited 0 through a bare + `exit 0`, on a message that was true about deletion and silent about the + duplicate in front of it. `STRICT=1` could not reach it — STRICT guards the + two `skip()` calls, and that path is not one of them. Off CI the two skips + had the same shape, so a shallow clone or an unpacked tarball would not look + at a duplicate the author was about to push. + + That inverted the two halves, and it inverted them hardest here. Deletion is + the failure that needs a diff to see; duplication is the one cast's + `release-notes.sh` actually mis-renders, and cast has the ABSORBING + extractor — no `exit`, so `grab` re-arms on the second heading and the + published body swallows whatever sits between the copies (heavy-duty/box#118). + The half with the live extraction bug behind it was the half with the most + ways to silently not run. + + Fixed by moving, not rewriting: uniqueness now runs directly after the file + exists, before any git access. The skip messages say *containment* skipped + and that uniqueness already passed, so a skip no longer claims nothing was + checked. The guard is also no longer gated to `pull_request` — deletion is + vacuous on a push to main, but duplication is vacuous on no tree, so a + duplicate reaching main by any other route went unasserted. That gate could + not simply be dropped: `github.base_ref` is empty on a push, and a bare + `origin/` under `STRICT=1` is a hard failure on every push to main, so the + base ref falls back to `github.ref_name`. + + Found by `claude-bot-andresmgsl` and `codex-bot-andresmgsl` reviewing #134; + cast inherited the ordering from box, fixed there in heavy-duty/box#144. + The failure it catches leaves no trace. An author adding an entry under `## Unreleased` types *over* the heading below it instead of inserting above it — a one-line edit, in a file nobody touched concurrently, so git merges it diff --git a/test/release.test.ts b/test/release.test.ts index 0bf61e7..fee5ca1 100644 --- a/test/release.test.ts +++ b/test/release.test.ts @@ -514,7 +514,12 @@ describe("changelog-monotonic.sh — release headings are append-only (#133)", ( expect((await check(repoWith(disarmed))).code).toBe(0); }); - it("a changelog absent at the merge base is nothing-to-have-deleted, not a failure", async () => { + /** + * A repo whose `base` has NO changelog at all — the PR INTRODUCES the file. + * The merge-base blob is absent, which is the degradation path that used to + * `exit 0` before uniqueness had run (#133, box#143). + */ + function repoIntroducing(head: string): string { const repo = mkdtempSync(join(tmpdir(), "cast-monotonic-new-")); git(repo, "init", "-q"); git(repo, "config", "user.email", "test@example.com"); @@ -524,14 +529,82 @@ describe("changelog-monotonic.sh — release headings are append-only (#133)", ( git(repo, "add", "README.md"); git(repo, "commit", "-qm", "base"); git(repo, "checkout", "-q", "-b", "pr"); - writeFileSync(join(repo, "CHANGELOG.md"), BASE); + writeFileSync(join(repo, "CHANGELOG.md"), head); git(repo, "add", "CHANGELOG.md"); git(repo, "commit", "-qm", "add the changelog"); - const r = await check(repo); + return repo; + } + + it("a changelog absent at the merge base is nothing-to-have-deleted, not a failure", async () => { + const r = await check(repoIntroducing(BASE)); expect(r.code).toBe(0); expect(r.output).toContain("does not exist at the merge base"); }); + // --- #133: uniqueness is a property of HEAD, so nothing base-side may gate + // it. Containment needs the merge base; uniqueness needs only the file in + // front of it. Before this fix the duplicate check sat DOWNSTREAM of the + // base-ref, merge-base and base-blob conditions, so each of the degradation + // paths below exited 0 on a tree carrying a duplicate in plain sight — the + // base-blob one not even via skip(), but a bare `exit 0` that STRICT could + // not reach. These cases pin the ORDER, which is the actual invariant; + // asserting the exit code alone is what let the original ship (the + // base-absent case above was green before and after). + // + // The inversion mattered most here: cast's release-notes.sh re-arms `grab` + // on every '## ' line, so duplication is the half with a LIVE extraction bug + // behind it — and it was the half with the most ways to silently not run. + + it("a duplicate introduced where the base had NO changelog is caught (#133)", async () => { + const dup = `# Changelog\n\n## Unreleased\n\n${dated("0.1.1")}${body}\n- **A stranded entry**\n\n${dated("0.1.1")}${body}`; + const r = await check(repoIntroducing(dup)); + expect(r.code).toBe(1); + expect(r.output).toContain("DUPLICATE release heading(s)"); + expect(r.output).toContain("## 0.1.1"); + // The old message must NOT be what this tree gets. + expect(r.output).not.toContain("nothing could have been deleted"); + }); + + it("...and STRICT does not change that — it was never a skip", async () => { + const dup = `# Changelog\n\n## Unreleased\n\n${dated("0.1.1")}${body}\n${dated("0.1.1")}${body}`; + const r = await check(repoIntroducing(dup), { + CHANGELOG_MONOTONIC_STRICT: "1", + }); + expect(r.code).toBe(1); + expect(r.output).toContain("DUPLICATE release heading(s)"); + }); + + it("...while a CLEAN introduced changelog still passes, SAYING uniqueness ran", async () => { + const r = await check(repoIntroducing(BASE)); + expect(r.code).toBe(0); + expect(r.output).toContain("nothing could have been deleted"); + expect(r.output).toContain("uniqueness on HEAD already passed"); + }); + + it("a duplicate OUTSIDE a git work tree is caught (#133)", async () => { + // No git at all — a tarball, an unpacked release. Uniqueness still has + // everything it needs; only containment does not. + const dir = mkdtempSync(join(tmpdir(), "cast-monotonic-nogit-")); + writeFileSync( + join(dir, "CHANGELOG.md"), + `# Changelog\n\n${dated("0.1.1")}${body}\n${dated("0.1.1")}${body}`, + ); + const r = await run("bash", [MONOTONIC, "base"], {}, dir); + expect(r.code).toBe(1); + expect(r.output).toContain("DUPLICATE release heading(s)"); + }); + + it("a duplicate is caught even when the base ref will not resolve (#133)", async () => { + const twice = BASE.replace( + `${dated("0.1.1")}${body}`, + `${dated("0.1.1")}${body}\n${dated("0.1.1")}${body}`, + ); + const r = await check(repoWith(twice), {}, "origin/no-such-branch"); + expect(r.code).toBe(1); + expect(r.output).toContain("DUPLICATE release heading(s)"); + expect(r.output).not.toContain("containment SKIPPED"); + }); + it("a missing changelog refuses by path — never a silent pass", async () => { const r = await run("bash", [MONOTONIC, "base", "nope.md"], {}, repoWith()); expect(r.code).toBe(1); @@ -541,11 +614,13 @@ describe("changelog-monotonic.sh — release headings are append-only (#133)", ( // The fail-closed switch, both directions. A guard that can quietly stop // guarding is the failure shape this whole family of checks refuses, so the // degradation that is sensible locally must be RED in CI. - it("an unresolvable base ref is a SKIP locally", async () => { + it("an unresolvable base ref SKIPS CONTAINMENT locally — not everything (#133)", async () => { const r = await check(repoWith(), {}, "origin/no-such-branch"); expect(r.code).toBe(0); - expect(r.output).toContain("SKIPPED"); - expect(r.output).toContain("Nothing was checked"); + expect(r.output).toContain("containment SKIPPED"); + // ...and it must not claim nothing was checked: uniqueness already ran. + expect(r.output).toContain("already ran and passed"); + expect(r.output).not.toContain("Nothing was checked"); }); it("...and the SAME condition is a hard FAILURE under STRICT=1, naming fetch-depth", async () => { @@ -558,16 +633,23 @@ describe("changelog-monotonic.sh — release headings are append-only (#133)", ( expect(r.output).toContain("CHANGELOG_MONOTONIC_STRICT=1"); expect(r.output).toContain("fetch-depth: 0"); expect(r.output).not.toContain("SKIPPED"); + // Even here the message must scope itself to containment (#133). + expect(r.output).toContain("it is containment that cannot run"); }); // The wiring, pinned the same way release.yml's is — the script existing is // no use if CI stops running it, and every clause here is load-bearing. - it("ci.yml runs it on pull requests only, STRICT, against the base ref, with full history", () => { + it("ci.yml runs it on EVERY event, STRICT, against the base ref, with full history", () => { const CI = readFileSync(join(ROOT, ".github/workflows/ci.yml"), "utf8"); expect(CI).toContain(".github/scripts/changelog-monotonic.sh"); - expect(CI).toContain('"origin/${{ github.base_ref }}"'); - // Pull requests only: on a push to main the merge base IS HEAD. - expect(CI).toContain("if: github.event_name == 'pull_request'"); + // #133: NOT pull-request-only. Deletion is vacuous on a push to main, but + // duplication is vacuous on no tree — gating the whole script left a + // duplicate that reached main by any other route unasserted forever. + expect(CI).not.toContain("if: github.event_name == 'pull_request'"); + // ...and dropping that gate is only safe WITH the fallback: on a push + // `github.base_ref` is empty, a bare `origin/` does not resolve, and + // STRICT promotes that to a hard failure on every push to main. + expect(CI).toContain('"origin/${{ github.base_ref || github.ref_name }}"'); expect(CI).toContain("CHANGELOG_MONOTONIC_STRICT"); // ...which is only reachable because the checkout has the base history. expect(CI).toContain("fetch-depth: 0");