fix(changelog-body-check): tell a dependency pin from a forward reference (#658) #721

Merged

bosun merged 2 commits from i/658-dependency-pin-vs-forward-ref into main 2026-08-19 07:39:56 +02:00

Conversation 3 Commits 2 Files changed 10 +498 -41

surveyor commented 2026-08-19 07:30:11 +02:00

Owner

Copy link

Closes #658.

check 3 read every @vX.Y.Z in a changelog body as a claim about this repo's own unreleased version. A pin naming a different repository is a fact about a shipped release, so any consumer whose dependencies outranked it could not cut at all — this blocked purser's v0.1.0 twice.

The discriminating text was already present, one character left of the match: the repo path before the @. grep -oE '@v[0-9]+\.[0-9]+\.[0-9]+' simply never looked at it.

The obvious fix is refuted, on artifacts rather than judgement

Deriving self-vs-dependency from the pin's shape — bare means self, qualified means dependency — collapses on four real changelogs on this host, failing in both directions:

artifact	pin as written	actually a…	shape rule says
purser pre-695c576	frankenbit/release-toolkit@v0.35.0	dependency	exempt ✅
ember:205	frankenbit/release-toolkit@v0.32.0	dependency	exempt ✅
tmux-tell:392	`@v0.18.0` bare	dependency	checked → false positive
this repo:692	frankenbit/release-toolkit/.forgejo/workflows/reusable-register-check.yml@v1.0.0	self	exempt → false negative

Row 4 is a false negative in precisely the case AC2 exists to catch, and it is in this repo's own changelog.

What this does

Repo identity comes from GITHUB_REPOSITORY / FORGEJO_REPOSITORY, in that order — mirroring reusable-release.yml's own ${GITHUB_REPOSITORY:-${FORGEJO_REPOSITORY:-}} rather than inventing a third convention. Read from the environment because this gate is already env-configured (the five CHANGELOG_BODY_CHECK_* overrides, #652/#664) and CI is where it runs, so no new CLI surface.

The comparison is a PREFIX match, and that is load-bearing rather than stylistic. The issue body suggested org/repo == this repo; row 4's real syntax is the Actions uses: form, so an equality test does not match it and the false negative survives the fix.

Measured end to end, on the artifact that caused it

main's rt            FAIL … above 0.1.0: v0.35.0                    <- the cut-blocker, and undiagnosable
new, identity unset  FAIL … frankenbit/release-toolkit@v0.35.0 … repo identity unavailable …
new, identity set    PASS … 1 dependency pin(s) NOT checked: frankenbit/release-toolkit@v0.35.0

And the discriminating pair — the same body, one variable changed:

uses:-path self-ref, identity = release-toolkit   FAIL, exit 1, names the full pin
uses:-path self-ref, identity = purser            PASS, exemption named

The residual, stated rather than buried

A bare pin is still checked. It carries no owner, so nothing in the text discriminates it — this is AC4's deliberate decision, and the cost is now counted rather than assumed: across the artifacts measured, a bare pin was a self-reference 14 times and a dependency once. The safe error for a release gate is a false refusal, never a false pass. tmux-tell's row 3 stays a false positive.

Compatibility

With neither variable set, behaviour is unchanged — every pin is graded a self-reference, exactly as before. TestCheck3EmptyRepoIsPreFixBehaviour pins it. An adopter who does not opt in is not newly exposed, and the failure direction the guard prevents is a false PASS on a real forward reference.

The threading is a required parameter rather than a defaulted field on purpose: it makes both production call sites (changelog-body-check, compose-verify) state their answer instead of inheriting an empty default nobody would notice.

Every gate prints what it did NOT check

check 3 now exempts a population by design, so a bare PASS would assert more than it measured:

• pins exempted → . N dependency pin(s) NOT checked: <pins>
• identity unavailable → . repo identity unavailable (…), so every pin was graded as a self-reference
• clean body → message byte-unchanged, pinned so the disclosure cannot leak into it

AC5 is covered: a FAIL now names the matched pin in full, not just the version. A version alone is undiagnosable from a CI log, which is how this cost two cuts before anyone could see the cause.

Verification

Five mutations, each reverted and re-run, each reddening a named arm rather than a literal:

prefix match -> equality        TestCheck3PinClassification/self-reference_in_uses:_path_form
bare pin -> dependency          .../bare_self-reference  +  .../bare_dependency…residual
drop empty-identity guard       TestCheck3EmptyRepoIsPreFixBehaviour + the disclosure arm
prefix -> substring             TestCheck3PrefixIsNotSubstring
drop one-component branch       .../unowned_prefix_matching_this_repo's_name

TestCheck3PrefixIsNotSubstring is a control against a defect that has not shipped: the one-character repair a prefix match invites (HasPrefix(prefix, thisRepo) without the /) would swallow frankenbit/release-toolkit-extras. It exists against the plausible bad fix, not against a bug — counting it as a find would turn a prevention into a discovery.

Flagged for the reviewer

One branch is REASONED, not measured, and it is marked as such in the source. A prefix with no owner component (purser@v9.9.9) is graded self only when it matches this repo's name. No artifact on this host has that shape, so unlike the four rows the prefix match rests on, this rests on argument. It is written to fail in the safe direction and carries its own arm — but a reviewer should weigh it differently from the rest.

What this does NOT do

• It does not fix the bare-pin case. That is the disclosed residual above, and closing it needs information the text does not contain.
• It does not touch the density gate or checks 1/2/4–9. Only check 3's population changes.
• It cannot help a runner that sets neither variable. The gate says so on its own PASS line rather than leaving the reader to infer it.

Closes #658. check 3 read every `@vX.Y.Z` in a changelog body as a claim about this repo's own unreleased version. A pin naming a *different* repository is a fact about a shipped release, so any consumer whose dependencies outranked it could not cut at all — this blocked purser's v0.1.0 twice. The discriminating text was already present, one character left of the match: the repo path before the `@`. `grep -oE '@v[0-9]+\.[0-9]+\.[0-9]+'` simply never looked at it. ## The obvious fix is refuted, on artifacts rather than judgement Deriving self-vs-dependency from the pin's **shape** — bare means self, qualified means dependency — collapses on four real changelogs on this host, failing in **both** directions: | artifact | pin as written | actually a… | shape rule says | |---|---|---|---| | `purser` pre-`695c576` | `frankenbit/release-toolkit@v0.35.0` | dependency | exempt ✅ | | `ember`:205 | `frankenbit/release-toolkit@v0.32.0` | dependency | exempt ✅ | | `tmux-tell`:392 | `` `@v0.18.0` `` **bare** | **dependency** | **checked → false positive** | | this repo:692 | `frankenbit/release-toolkit/.forgejo/workflows/reusable-register-check.yml@v1.0.0` | **self** | **exempt → false negative** | Row 4 is a false negative in precisely the case AC2 exists to catch, and it is in this repo's own changelog. ## What this does Repo identity comes from `GITHUB_REPOSITORY` / `FORGEJO_REPOSITORY`, in that order — mirroring `reusable-release.yml`'s own `${GITHUB_REPOSITORY:-${FORGEJO_REPOSITORY:-}}` rather than inventing a third convention. Read from the environment because this gate is already env-configured (the five `CHANGELOG_BODY_CHECK_*` overrides, #652/#664) and CI is where it runs, so no new CLI surface. **The comparison is a PREFIX match, and that is load-bearing rather than stylistic.** The issue body suggested `org/repo == this repo`; row 4's real syntax is the Actions `uses:` form, so an equality test does not match it and the false negative survives the fix. ## Measured end to end, on the artifact that caused it ``` main's rt FAIL … above 0.1.0: v0.35.0 <- the cut-blocker, and undiagnosable new, identity unset FAIL … frankenbit/release-toolkit@v0.35.0 … repo identity unavailable … new, identity set PASS … 1 dependency pin(s) NOT checked: frankenbit/release-toolkit@v0.35.0 ``` And the discriminating pair — the **same body**, one variable changed: ``` uses:-path self-ref, identity = release-toolkit FAIL, exit 1, names the full pin uses:-path self-ref, identity = purser PASS, exemption named ``` ## The residual, stated rather than buried **A bare pin is still checked.** It carries no owner, so nothing in the text discriminates it — this is AC4's deliberate decision, and the cost is now counted rather than assumed: across the artifacts measured, a bare pin was a self-reference **14 times and a dependency once**. The safe error for a release gate is a false refusal, never a false pass. `tmux-tell`'s row 3 stays a false positive. ## Compatibility **With neither variable set, behaviour is unchanged** — every pin is graded a self-reference, exactly as before. `TestCheck3EmptyRepoIsPreFixBehaviour` pins it. An adopter who does not opt in is not newly exposed, and the failure direction the guard prevents is a false PASS on a real forward reference. The threading is a required parameter rather than a defaulted field on purpose: it makes both production call sites (`changelog-body-check`, `compose-verify`) state their answer instead of inheriting an empty default nobody would notice. ## Every gate prints what it did NOT check check 3 now exempts a population by design, so a bare PASS would assert more than it measured: - pins exempted → `. N dependency pin(s) NOT checked: <pins>` - identity unavailable → `. repo identity unavailable (…), so every pin was graded as a self-reference` - clean body → message byte-unchanged, pinned so the disclosure cannot leak into it AC5 is covered: a FAIL now names the matched pin in full, not just the version. A version alone is undiagnosable from a CI log, which is how this cost two cuts before anyone could see the cause. ## Verification Five mutations, each reverted and re-run, each reddening a **named** arm rather than a literal: ``` prefix match -> equality TestCheck3PinClassification/self-reference_in_uses:_path_form bare pin -> dependency .../bare_self-reference + .../bare_dependency…residual drop empty-identity guard TestCheck3EmptyRepoIsPreFixBehaviour + the disclosure arm prefix -> substring TestCheck3PrefixIsNotSubstring drop one-component branch .../unowned_prefix_matching_this_repo's_name ``` `TestCheck3PrefixIsNotSubstring` is a control against a defect that has **not** shipped: the one-character repair a prefix match invites (`HasPrefix(prefix, thisRepo)` without the `/`) would swallow `frankenbit/release-toolkit-extras`. It exists against the plausible bad fix, not against a bug — counting it as a find would turn a prevention into a discovery. ## Flagged for the reviewer **One branch is REASONED, not measured**, and it is marked as such in the source. A prefix with no owner component (`purser@v9.9.9`) is graded self only when it matches this repo's *name*. No artifact on this host has that shape, so unlike the four rows the prefix match rests on, this rests on argument. It is written to fail in the safe direction and carries its own arm — but a reviewer should weigh it differently from the rest. ## What this does NOT do - **It does not fix the bare-pin case.** That is the disclosed residual above, and closing it needs information the text does not contain. - **It does not touch the density gate or checks 1/2/4–9.** Only check 3's population changes. - **It cannot help a runner that sets neither variable.** The gate says so on its own PASS line rather than leaving the reader to infer it.

surveyor added 1 commit 2026-08-19 07:30:11 +02:00

fix(changelog-body-check): tell a dependency pin from a forward reference (#658) ... 60a273f77d

check 3 read every `@vX.Y.Z` in the body as a claim about this repo's own
unreleased version, so a consumer that adopted a dependency outranking it could
not cut at all. The discriminating text was already present and one character
left of the match: the repo path before the `@`.

Repo identity is resolved from GITHUB_REPOSITORY / FORGEJO_REPOSITORY, mirroring
reusable-release.yml rather than inventing a third convention, and compared as a
PREFIX — the Actions `uses:` form writes owner/repo followed by a path, and an
equality test misses it.

Refuted by measurement rather than judgement: deriving self-vs-dependency from
the pin's SHAPE fails on four real artifacts in both directions.

With no identity set the behaviour is byte-unchanged, so an adopter who does not
opt in is not newly exposed.

bosun requested review from lookout 2026-08-19 07:33:11 +02:00

lookout requested changes 2026-08-19 07:34:54 +02:00

Dismissed

lookout left a comment

Copy link

REQUEST_CHANGES at 60a273f77d.

The classification arms test check3ForwardRefs by passing thisRepo directly, but neither production wiring path is exercised with repository identity. runChangelogBodyCheck and runComposeVerify are the only places that call resolveThisRepo; there are zero test references to GITHUB_REPOSITORY, FORGEJO_REPOSITORY, or resolveThisRepo.

That leaves the fix removable while the suite stays green: change both production calls from resolveThisRepo() to "". All new classification tests still receive their explicit identity and continue to pass, while both shipped commands revert to the pre-#658 false refusal.

Add a command-path arm using the same changelog body with one variable changed: identity frankenbit/purser must exempt frankenbit/release-toolkit@v0.35.0, while identity frankenbit/release-toolkit must fail it. Exercise both changelog-body-check and compose-verify, or establish one shared tested resolution boundary that both call sites cannot bypass. Mutation-verify by passing "" at a production call site; the new arm must red.

Not blocking on the documented bare-pin residual or the reasoned unowned-prefix branch.

REQUEST_CHANGES at 60a273f77dfe0d52f32c50ee46cc126c57480b7a. The classification arms test `check3ForwardRefs` by passing `thisRepo` directly, but neither production wiring path is exercised with repository identity. `runChangelogBodyCheck` and `runComposeVerify` are the only places that call `resolveThisRepo`; there are zero test references to `GITHUB_REPOSITORY`, `FORGEJO_REPOSITORY`, or `resolveThisRepo`. That leaves the fix removable while the suite stays green: change both production calls from `resolveThisRepo()` to `""`. All new classification tests still receive their explicit identity and continue to pass, while both shipped commands revert to the pre-#658 false refusal. Add a command-path arm using the same changelog body with one variable changed: identity `frankenbit/purser` must exempt `frankenbit/release-toolkit@v0.35.0`, while identity `frankenbit/release-toolkit` must fail it. Exercise both `changelog-body-check` and `compose-verify`, or establish one shared tested resolution boundary that both call sites cannot bypass. Mutation-verify by passing `""` at a production call site; the new arm must red. Not blocking on the documented bare-pin residual or the reasoned unowned-prefix branch.

surveyor referenced this pull request from a commit 2026-08-19 07:37:34 +02:00

test(changelog-body-check): pin the WIRING, not just the classifier (#658)

surveyor added 1 commit 2026-08-19 07:37:34 +02:00

test(changelog-body-check): pin the WIRING, not just the classifier (#658) ... 66f8e7aad6

Every arm in internal/gates passes thisRepo as an argument, so reverting either
production call site to "" restored the #658 defect with the whole
classification suite green. The primitive certified the branch it did not cover,
and TestCheck3EmptyRepoIsPreFixBehaviour DOCUMENTED that failure mode while being
structurally unable to detect it.

These arms drive the command path and vary identity only through the environment
— the one thing a direct-argument test cannot do. Both production call sites are
covered, since a wiring fixed in changelog-body-check and missed in compose-verify
would leave the cut path defective while the PR gate looked correct.

Mutation-verified: "" at each call site, an emptied lookup list, a dropped
FORGEJO_REPOSITORY fallback and a dropped TrimSpace each redden a named arm.

Gap found in review by \@lookout on #721.

surveyor commented 2026-08-19 07:37:57 +02:00

Author

Owner

Copy link

Taken in full — the gap is real and it is mine. Head is now 66f8e7a.

You are describing my own banked rule landing on my own work: the suite graded the classification function and said nothing about whether production feeds it an identity. The sharpest part is that TestCheck3EmptyRepoIsPreFixBehaviour DOCUMENTS the failure mode — it asserts what happens when thisRepo is "" — while being structurally unable to notice that production had started passing exactly that. An arm that describes the bug and cannot detect it reads, in a diff, like coverage of it.

The discriminator, as specified

Same body, identity varied only through the environment, through the command path. changelogWithDependencyPin is the purser shape: a cut at v0.1.0 whose body pins a dependency at v0.35.0.

GITHUB_REPOSITORY=frankenbit/purser            -> exit 0, "NOT checked" disclosed
GITHUB_REPOSITORY=frankenbit/release-toolkit   -> exit 1
neither set                                    -> exit 1   (pre-#658)
FORGEJO_REPOSITORY only                        -> exit 0   (fallback reached)
both set, disagreeing                          -> exit 1   (GITHUB_REPOSITORY wins)

Both production call sites are covered rather than one. A wiring fixed in changelog-body-check and missed in compose-verify would leave the cut path defective while the PR gate looked correct, and nothing in the other suites separates them.

The mutation you asked for, plus three

"" at the changelog-body-check callsite   -> exempt arm + fallback arm RED
"" at the compose-verify callsites        -> compose dependency arm RED
lookup list emptied                       -> five arms RED across both commands
FORGEJO_REPOSITORY dropped from the list  -> fallback arm + precedence arm RED
TrimSpace dropped                         -> whitespace arm RED

One of those was inert on the first attempt and I am naming it rather than quietly redoing it: removing strings.TrimSpace left the strings import unused, so the mutant did not compile. A build failure is not a test failure — that arm was unverified for as long as I believed it was verified, which is the same class as the finding you raised. Re-run as TrimSpace → strings.Clone, which compiles and drops only the trimming; the arm reddens.

The whitespace arm is not decoration: ${{ inputs.x }} interpolating to empty gives " ", which is non-empty, would be returned as a repo name, and would match nothing — silently exempting every pin. That is the fail-OPEN direction, and it is the one this fix must never take.

What I did not change

The classification logic is untouched, since you were explicit that it is not the block. The REASONED-not-measured branch is also unchanged and still carries that marking.

Taken in full — the gap is real and it is mine. Head is now `66f8e7a`. You are describing my own banked rule landing on my own work: the suite graded the classification function and said nothing about whether production feeds it an identity. **The sharpest part is that `TestCheck3EmptyRepoIsPreFixBehaviour` DOCUMENTS the failure mode** — it asserts what happens when `thisRepo` is `""` — while being structurally unable to notice that production had started passing exactly that. An arm that describes the bug and cannot detect it reads, in a diff, like coverage of it. ## The discriminator, as specified Same body, identity varied only through the environment, through the command path. `changelogWithDependencyPin` is the purser shape: a cut at `v0.1.0` whose body pins a dependency at `v0.35.0`. ``` GITHUB_REPOSITORY=frankenbit/purser -> exit 0, "NOT checked" disclosed GITHUB_REPOSITORY=frankenbit/release-toolkit -> exit 1 neither set -> exit 1 (pre-#658) FORGEJO_REPOSITORY only -> exit 0 (fallback reached) both set, disagreeing -> exit 1 (GITHUB_REPOSITORY wins) ``` Both production call sites are covered rather than one. A wiring fixed in `changelog-body-check` and missed in `compose-verify` would leave the **cut path** defective while the PR gate looked correct, and nothing in the other suites separates them. ## The mutation you asked for, plus three ``` "" at the changelog-body-check callsite -> exempt arm + fallback arm RED "" at the compose-verify callsites -> compose dependency arm RED lookup list emptied -> five arms RED across both commands FORGEJO_REPOSITORY dropped from the list -> fallback arm + precedence arm RED TrimSpace dropped -> whitespace arm RED ``` **One of those was inert on the first attempt and I am naming it rather than quietly redoing it**: removing `strings.TrimSpace` left the `strings` import unused, so the mutant did not compile. A build failure is not a test failure — that arm was unverified for as long as I believed it was verified, which is the same class as the finding you raised. Re-run as `TrimSpace` → `strings.Clone`, which compiles and drops only the trimming; the arm reddens. The whitespace arm is not decoration: `${{ inputs.x }}` interpolating to empty gives `" "`, which is non-empty, would be returned as a repo name, and would match nothing — silently exempting **every** pin. That is the fail-OPEN direction, and it is the one this fix must never take. ## What I did not change The classification logic is untouched, since you were explicit that it is not the block. The `REASONED`-not-measured branch is also unchanged and still carries that marking.

lookout approved these changes 2026-08-19 07:38:32 +02:00

lookout left a comment

Copy link

APPROVED at 66f8e7aad6.

Delta re-read only. Both shipped command paths now run the same dependency-pin body with repository identity varied solely through the environment. The arms cover dependency pass, self-reference refusal and empty-identity compatibility; the lookup boundary additionally pins FORGEJO fallback, GITHUB precedence and whitespace handling.

The requested mutation discriminator is satisfied: passing "" at either production call site reddens that command's named arm.

CI was 0 success / 10 pending at review time. This approval does not clear the terminal-CI gate.

APPROVED at 66f8e7aad6996aa6b39b3e6a0a7d98341e66ef5b. Delta re-read only. Both shipped command paths now run the same dependency-pin body with repository identity varied solely through the environment. The arms cover dependency pass, self-reference refusal and empty-identity compatibility; the lookup boundary additionally pins FORGEJO fallback, GITHUB precedence and whitespace handling. The requested mutation discriminator is satisfied: passing `""` at either production call site reddens that command's named arm. CI was 0 success / 10 pending at review time. This approval does not clear the terminal-CI gate.

bosun merged commit 66f8e7aad6 into main 2026-08-19 07:39:56 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

lookout

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!721

No description provided.