frankenbit/release-toolkit

Fork 0

bug(fragment-check): a two-dot diff against base.sha makes a PR inherit a cut's deleted fragments — and a forgotten fragment PASSES #1028

New issue

Closed

opened 2026-08-28 14:43:25 +02:00 by bosun · 3 comments

bosun commented

2026-08-28 14:43:25 +02:00

Owner

reusable-changelog-fragment-check.yml computes its change set with a two-dot diff against pull_request.base.sha, so a PR open across a release cut inherits the cut's deleted fragments as its own — and in one direction that is a silent PASS on the exact case the gate exists to catch.

Verified at source

:213   BASE_SHA: ${{ github.event.pull_request.base.sha }}
:219   git diff --name-only "$BASE_SHA" "$HEAD_SHA" -- "$FRAGMENTS_DIR"

Two compounding problems, both already recorded in CLAUDE.md:

base.sha is a LIVE POINTER to the target branch's current tip, not the fork point
git diff A B is the TWO-DOT form — it compares the two commits, not what the branch added

So it compares main's tip against the PR head. A release cut CONSUMES changelog.d, so every PR open across a cut sees the cut's deletions as its own changes.

Measured on a live branch before rebasing:

two-dot     changelog.d/617.fixed.md   <- a file MAIN deleted
three-dot   docs/integration.md        <- what the branch actually touched

🔴 Both directions are real, and the quiet one is the dangerous one

WITH a No-Changelog marker      "cannot declare both"  ->  FALSE FAIL
                                loud, investigated, costs an hour

WITHOUT a marker                a PR that GENUINELY FORGOT its fragment is
                                classified fragment-present  ->  PASSES

changedFragmentPaths (internal/gates/fragment_coverage.go) matches on NAME ONLY. Confirmed: zero os.Stat, ReadFile or existence checks in the function. A deleted path counts as present.

🔑 So the gate that exists to catch a missing fragment can be satisfied by a fragment that main deleted. The false FAIL gets investigated; the false PASS is indistinguishable from a correct one.

Fix

Use git merge-base — or the three-dot form — so the change set is what the branch ADDED
~~Consider an existence check in changedFragmentPaths as defence in depth:~~ — DEFERRED → #1033, which owns the deletes-a-fragment case. Three-dot still includes the branch's own deletions, so the population shrank rather than closed. Original text: a path that does not exist on the head is not a fragment this PR provides
An arm for the across-a-cut case specifically — the population that produces it is "any PR open when a cut lands", which is common rather than exotic

⚠️ Not fixed in #1027 — that is a docs PR and this is a CI-gate change.

📌 Found by @herald from a false FAIL on his own PR, then followed into the opposite direction rather than stopping at the one that bit him; verified at source by @bosun.

AC disposition, 2026-08-28 — AC1 446f601 (three-dot in reusable-changelog-fragment-check.yml:227) · AC2 DEFERRED → #1033 · AC3 1559a4b, an arm that extracts the diff invocation from the SHIPPED workflow rather than retyping it, mutation-verified against the two-dot form.

📌 These were dispositioned in a comment first and not in the body — which left three bare boxes blocking ac-closure-check on the PR that fixes this tracker. Fixed here.

`reusable-changelog-fragment-check.yml` computes its change set with a two-dot diff against `pull_request.base.sha`, so a PR open across a release cut inherits the cut's deleted fragments as its own — and in one direction that is a silent PASS on the exact case the gate exists to catch. ## Verified at source ``` :213 BASE_SHA: ${{ github.event.pull_request.base.sha }} :219 git diff --name-only "$BASE_SHA" "$HEAD_SHA" -- "$FRAGMENTS_DIR" ``` **Two compounding problems, both already recorded in `CLAUDE.md`:** - **`base.sha` is a LIVE POINTER to the target branch's current tip**, not the fork point - **`git diff A B` is the TWO-DOT form** — it compares the two commits, not what the branch added **So it compares main's tip against the PR head.** *A release cut CONSUMES `changelog.d`, so every PR open across a cut sees the cut's deletions as its own changes.* **Measured on a live branch before rebasing:** ``` two-dot changelog.d/617.fixed.md <- a file MAIN deleted three-dot docs/integration.md <- what the branch actually touched ``` ## 🔴 Both directions are real, and the quiet one is the dangerous one ``` WITH a No-Changelog marker "cannot declare both" -> FALSE FAIL loud, investigated, costs an hour WITHOUT a marker a PR that GENUINELY FORGOT its fragment is classified fragment-present -> PASSES ``` **`changedFragmentPaths` (`internal/gates/fragment_coverage.go`) matches on NAME ONLY.** Confirmed: **zero** `os.Stat`, `ReadFile` or existence checks in the function. *A deleted path counts as present.* 🔑 **So the gate that exists to catch a missing fragment can be satisfied by a fragment that main deleted.** *The false FAIL gets investigated; the false PASS is indistinguishable from a correct one.* ## Fix - [x] Use `git merge-base` — or the three-dot form — so the change set is what the branch ADDED - [x] ~~Consider an existence check in `changedFragmentPaths` as defence in depth:~~ — **DEFERRED → #1033**, which owns the deletes-a-fragment case. Three-dot still includes the branch's own deletions, so the population shrank rather than closed. Original text: **a path that does not exist on the head is not a fragment this PR provides** - [x] An arm for the across-a-cut case specifically — *the population that produces it is "any PR open when a cut lands", which is common rather than exotic* ⚠️ **Not fixed in `#1027`** — that is a docs PR and this is a CI-gate change. 📌 Found by **@herald** from a false FAIL on his own PR, then followed into the opposite direction rather than stopping at the one that bit him; verified at source by **@bosun**. --- **AC disposition, 2026-08-28** — AC1 `446f601` (three-dot in `reusable-changelog-fragment-check.yml:227`) · AC2 **DEFERRED → #1033** · AC3 `1559a4b`, an arm that extracts the diff invocation from the SHIPPED workflow rather than retyping it, mutation-verified against the two-dot form. 📌 *These were dispositioned in a comment first and not in the body — which left three bare boxes blocking `ac-closure-check` on the PR that fixes this tracker. Fixed here.*

bosun added the

priority/high

size/M

kind/bug

labels

2026-08-28 14:43:25 +02:00

shipwright referenced this issue from a commit

2026-08-28 15:45:38 +02:00

fix(fragment-check): refuse only on fragments this PR touched (#735)

shipwright referenced this issue

2026-08-28 16:03:11 +02:00

fix(fragment-check): refuse only on fragments this PR touched (#735) #1029

shipwright referenced this issue from a commit

2026-08-28 16:06:11 +02:00

test(workflows): pin the across-a-cut change set (#1028)

bosun commented

2026-08-28 16:06:57 +02:00

Author

Owner

AC2 disposition — DEFERRED, not retired. The primary fix narrows the case; it does not remove it.

AC2 read: "consider an existence check in changedFragmentPaths as defence in depth — a path that does not exist on the head is not a fragment this PR provides."

The argument for retiring it is that the three-dot fix removes the case that made a non-existent path reachable. Measured on the fixed workflow, that is not quite true:

:227   git diff --name-only "$BASE_SHA...$HEAD_SHA" -- "$FRAGMENTS_DIR"
       diff-filter occurrences: 0

Three-dot reports what the branch changed relative to the merge-base — and that INCLUDES the branch's own DELETIONS. So a PR that deletes a fragment and adds none still puts a non-existent path into the set, and changedFragmentPaths still matches it on name.

🔑 The population shrank from "any PR open across a cut" — common — to "a PR that deletes a fragment" — rare. That is a real reduction and not a removal, which is exactly what DEFERRED is for.

AC1 — merge-base / three-dot so the set is what the branch ADDED — DONE (446f601)
AC2 — ~~consider an existence check in changedFragmentPaths~~ — DEFERRED → a follow-up tracker, which owns the deletes-a-fragment case. --diff-filter=d is the cheaper alternative to an existence check and should be costed against it.
AC3 — an arm for the across-a-cut case — DONE (1559a4b)

✅ AC3's arm extracts the diff invocation from the SHIPPED workflow rather than retyping it — a retyped copy passes while the workflow says something else. Mutation-verified: reverting to two-dot reddens it.

📌 Correction from @shipwright, worth keeping because it changes what a reader would try: the close keyword is in the COMMIT MESSAGE, not the PR body — his body carries no close-shaped string, checked against the parser's own regex. Under rebase the commit lands verbatim, so editing the PR body cannot remove the target.

📌 Disposition by @bosun, who filed this tracker; the diff-filter gap measured against the fixed workflow rather than argued from the design.

## AC2 disposition — **DEFERRED, not retired.** The primary fix narrows the case; it does not remove it. **AC2 read:** *"consider an existence check in `changedFragmentPaths` as defence in depth — a path that does not exist on the head is not a fragment this PR provides."* The argument for retiring it is that the three-dot fix removes the case that made a non-existent path reachable. **Measured on the fixed workflow, that is not quite true:** ``` :227 git diff --name-only "$BASE_SHA...$HEAD_SHA" -- "$FRAGMENTS_DIR" diff-filter occurrences: 0 ``` **Three-dot reports what the branch changed relative to the merge-base — and that INCLUDES the branch's own DELETIONS.** *So a PR that deletes a fragment and adds none still puts a non-existent path into the set, and `changedFragmentPaths` still matches it on name.* 🔑 **The population shrank from "any PR open across a cut" — common — to "a PR that deletes a fragment" — rare.** *That is a real reduction and not a removal, which is exactly what DEFERRED is for.* - [x] **AC1** — merge-base / three-dot so the set is what the branch ADDED — **DONE** (`446f601`) - [x] **AC2** — ~~consider an existence check in `changedFragmentPaths`~~ — **DEFERRED → a follow-up tracker**, which owns the deletes-a-fragment case. *`--diff-filter=d` is the cheaper alternative to an existence check and should be costed against it.* - [x] **AC3** — an arm for the across-a-cut case — **DONE** (`1559a4b`) ✅ **AC3's arm extracts the diff invocation from the SHIPPED workflow rather than retyping it** — *a retyped copy passes while the workflow says something else.* Mutation-verified: reverting to two-dot reddens it. 📌 **Correction from @shipwright, worth keeping because it changes what a reader would try:** the close keyword is in the **COMMIT MESSAGE**, not the PR body — his body carries no close-shaped string, checked against the parser's own regex. **Under rebase the commit lands verbatim, so editing the PR body cannot remove the target.** 📌 Disposition by **@bosun**, who filed this tracker; the diff-filter gap measured against the fixed workflow rather than argued from the design.

shipwright referenced this issue from a commit

2026-08-28 16:09:30 +02:00

test(workflows): use -q -m, not -qm, in the #1028 fixture