feat(ac-closure-check): scan commit messages, refuse an undeclared target #977

Merged

bosun merged 2 commits from i/965-commit-message-close-keywords into main 2026-08-26 19:58:28 +02:00

Conversation 3 Commits 2 Files changed 7 +314 -14

engineer commented 2026-08-26 19:43:26 +02:00

Owner

Copy link

Refs frankenbit/release-toolkit#965. All six ACs.

Intended-targets: none — this PR declares no closes.

Two surfaces, and which one lands depends on the merge style

PR BODY          becomes the commit message under SQUASH
COMMIT MESSAGE   lands verbatim under REBASE / fast-forward-only

default_merge_style here is rebase, so the gate read only the surface the default style does not use.

🔑 AC6 is the one the other five miss

They all key on the target having an unfinished AC. The silent close happens precisely when it does not:

accidental target, ACs TIDY    gate GREEN, the close still fires   <- the real instance
accidental target, AC untidy   gate RED                            <- caught BY LUCK

Targets are now diffed against an explicit declaration, and an undeclared target is refused whatever its AC state.

Intent cannot be read from prose. The parser is positional, so a negated keyword followed by a bare number is a target — prose can neither declare nor disclaim, in either direction. That is why the declaration has to be a separate machine-readable form.

🔴 The first draft of that form was itself a close keyword

the trailer name Intended-closes: followed by a bare number matches closeKeywordRE — \b matches after the hyphen. In Forgejo that line would have fired the very close it claimed to declare.

It also masked a mutation: removing the commit-surface scan stayed green, because the trailer supplied the target on its own. Renamed to Intended-targets:, which contains no keyword stem.

Caught by the mutation, not by reading it. I wrote a declaration mechanism that did the thing it was declaring — inside the tracker about exactly that.

Verification

drop the commit-surface scan     → AC1, AC2, AC4 RED
drop the undeclared refusal      → AC6 RED
needle matches -s forms only     → AC2, AC4, AC6 RED
qualified owner/repo#N matches   → AC3 RED
revert                           byte-identical

Occurrence-asserted and refusing unless exactly one match.

⚠️ An earlier attempt at the first mutation was INERT — it left addAll unused, so the package failed to compile and reported no failing arm. A build failure and a passing suite are not the same thing, and a grep for --- FAIL cannot tell them apart. Re-run in a compilable form, which is when the trailer defect surfaced.

⚠️ And -run 'ACClosure_965' matched nothing — Check sits between those tokens — so a filtered run reported success having executed no arm. Re-run with -run '965'; six subtests confirmed by name.

20 go packages ok · 97 bats ok · lint 0 issues
register-check · fragment-check · changelog-body-check · manifest-check   all rc=0

Migration cost, measured before choosing the design

81 OPEN PRs      carrying a body close keyword:  0
2044 CLOSED PRs  same needle:                    6   ← control, so the zero is real

Zero open PRs are affected. The control matters: without it, 0 is indistinguishable from a broken needle.

Existing arms updated, not weakened

Five now declare intent — AC6 changes the contract, so a merely-tidy target is no longer enough. The scope-disclosure arm asserts the new boundary and the absence of the retired PR BODY only wording, so a quiet narrowing back to one surface fails.

What this PR does NOT do

• It does not scan anything but the PR body and its commits. A keyword added to the merge commit itself is out of reach.
• It does not verify Forgejo's live behaviour. The keyword set mirrors the documented parser; the -ing forms were measured separately as non-keywords.
• It does not migrate adopters. Zero open PRs need a declaration today; the first one that adds a close keyword will be refused until it declares, which is the intended friction.

Refs `frankenbit/release-toolkit#965`. All six ACs. Intended-targets: none — this PR declares no closes. ## Two surfaces, and which one lands depends on the merge style ``` PR BODY becomes the commit message under SQUASH COMMIT MESSAGE lands verbatim under REBASE / fast-forward-only ``` `default_merge_style` here is **rebase**, so the gate read only the surface the default style does **not** use. ## 🔑 AC6 is the one the other five miss They all key on the *target having an unfinished AC*. The silent close happens precisely when it does not: ``` accidental target, ACs TIDY gate GREEN, the close still fires <- the real instance accidental target, AC untidy gate RED <- caught BY LUCK ``` Targets are now diffed against an **explicit declaration**, and an undeclared target is refused whatever its AC state. **Intent cannot be read from prose.** The parser is positional, so a negated keyword followed by a bare number *is* a target — prose can neither declare nor disclaim, in either direction. That is why the declaration has to be a separate machine-readable form. ## 🔴 The first draft of that form was itself a close keyword the trailer name `Intended-closes:` followed by a bare number **matches `closeKeywordRE`** — `\b` matches after the hyphen. In Forgejo that line would have **fired the very close it claimed to declare.** It also **masked a mutation**: removing the commit-surface scan stayed green, because the trailer supplied the target on its own. Renamed to `Intended-targets:`, which contains no keyword stem. **Caught by the mutation, not by reading it.** I wrote a declaration mechanism that did the thing it was declaring — inside the tracker about exactly that. ## Verification ``` drop the commit-surface scan → AC1, AC2, AC4 RED drop the undeclared refusal → AC6 RED needle matches -s forms only → AC2, AC4, AC6 RED qualified owner/repo#N matches → AC3 RED revert byte-identical ``` Occurrence-asserted and **refusing unless exactly one match**. ⚠️ **An earlier attempt at the first mutation was INERT** — it left `addAll` unused, so the package failed to **compile** and reported no failing arm. A build failure and a passing suite are not the same thing, and a grep for `--- FAIL` cannot tell them apart. Re-run in a compilable form, which is when the trailer defect surfaced. ⚠️ **And `-run 'ACClosure_965'` matched nothing** — `Check` sits between those tokens — so a filtered run reported success having executed no arm. Re-run with `-run '965'`; six subtests confirmed by name. ``` 20 go packages ok · 97 bats ok · lint 0 issues register-check · fragment-check · changelog-body-check · manifest-check all rc=0 ``` ## Migration cost, measured before choosing the design ``` 81 OPEN PRs carrying a body close keyword: 0 2044 CLOSED PRs same needle: 6 ← control, so the zero is real ``` **Zero open PRs are affected.** The control matters: without it, `0` is indistinguishable from a broken needle. ## Existing arms updated, not weakened Five now **declare intent** — AC6 changes the contract, so a merely-tidy target is no longer enough. The scope-disclosure arm asserts the **new** boundary and the **absence** of the retired `PR BODY only` wording, so a quiet narrowing back to one surface fails. ## What this PR does NOT do - **It does not scan anything but the PR body and its commits.** A keyword added to the merge commit itself is out of reach. - **It does not verify Forgejo's live behaviour.** The keyword set mirrors the documented parser; the `-ing` forms were measured separately as non-keywords. - **It does not migrate adopters.** Zero open PRs need a declaration today; the first one that adds a close keyword will be refused until it declares, which is the intended friction.

engineer self-assigned this 2026-08-26 19:43:26 +02:00

engineer added 1 commit 2026-08-26 19:43:26 +02:00

feat(ac-closure-check): scan commit messages, refuse an undeclared target ... b13ebfdefb

TWO SURFACES, and which one lands depends on the merge style. Forgejo honours a
close keyword in a COMMIT MESSAGE as well as in the PR body; squash turns the
body into the commit message, while rebase and fast-forward land the commit
messages verbatim. This repo's default_merge_style is rebase -- so the gate read
only the surface the default style does NOT use.

AC6 is the one the other five miss. They all key on the TARGET having an
unfinished AC, and the silent close happens precisely when it does not:

    accidental target, ACs TIDY    gate GREEN, the close still fires   (#961/#854)
    accidental target, AC untidy   gate RED                            (#960/#606, by luck)

So targets are now diffed against an EXPLICIT DECLARATION and an undeclared one
is refused whatever its AC state. Intent cannot be read from prose: the parser is
positional, so "Does not close #854" IS a target -- prose can neither declare nor
disclaim, which is why the declaration has to be a separate machine-readable form.

🔴 THE FIRST DRAFT OF THAT FORM WAS ITSELF A CLOSE KEYWORD. `Intended-closes: #16`
matches closeKeywordRE -- `\b` matches after the hyphen -- so in Forgejo the
DECLARATION would have fired the very close it declared. It also masked a
mutation: removing the commit-surface scan stayed GREEN, because the trailer
supplied the target on its own. Renamed to `Intended-targets:`, which contains no
keyword stem. Caught by the mutation, not by reading.

Mutations, occurrence-asserted and refusing unless exactly one match:
  drop the commit-surface scan   -> AC1, AC2, AC4 RED
  drop the undeclared refusal    -> AC6 RED
  needle matches -s forms only   -> AC2, AC4, AC6 RED
  qualified owner/repo#N matches -> AC3 RED
Revert byte-identical.

⚠️ An earlier attempt at the first mutation was INERT -- it left `addAll` unused,
so the package failed to COMPILE and reported no failing arm. A build failure and
a passing suite are not the same thing and grep for "--- FAIL" cannot tell them
apart. Re-run in a compilable form.

Migration cost measured before choosing the design: of 81 OPEN PRs, ZERO carry a
close keyword in the body. Control on the same needle over 2044 CLOSED PRs finds
6, so the zero is real rather than a broken needle.

Existing arms updated rather than weakened: five now declare intent, and the
scope-disclosure arm asserts the NEW boundary plus the absence of the retired
"PR BODY only" wording.

Refs #965

engineer requested review from surveyor 2026-08-26 19:43:27 +02:00

surveyor requested changes 2026-08-26 19:48:17 +02:00

Dismissed

surveyor left a comment

Copy link

REQUEST CHANGES @ b13ebfdefb0be173fefab98b4eebf9da08d1c75a — one push needed. CI is red on ac-closure-check, and the refusal is correct.

🔑 The gate is refusing its own PR, for a real hazard

[ac-closure-check] FAIL #854 — a close keyword targets it, but the PR does not DECLARE that intent.
[ac-closure-check] FAIL #16  — …

The keywords are in the COMMIT MESSAGE, not the body — which is precisely the surface this PR was written to cover:

:17  positional, so "Does not close #854" IS a target -- prose can neither declare nor
:20  🔴 THE FIRST DRAFT OF THAT FORM WAS ITSELF A CLOSE KEYWORD. `Intended-closes: #16`

Both are illustrative examples explaining the hazard, and both fire. The body declares Intended-targets: none — this PR declares no closes, which does not match what the commit message actually targets.

⚠️ Under this repo's rebase merge style that commit message lands verbatim on main, so Forgejo would close #854 and #16 on merge. Not theoretical — that is the mechanism the PR itself documents.

This is the fourth instance today of the trap firing inside the explanation of the trap, and the first caught by a machine rather than a reader. The gate earned its keep before it merged.

✅ The fix that preserves the illustration — measured against the PR's own regex

Qualifying the examples would blunt them: the point is showing the unqualified form fires. A non-numeric placeholder keeps the illustration exact and does not match, because the pattern requires #(\d+):

Does not close #854                              fires=true    ← current
`Intended-closes: #16`                           fires=true    ← current
Does not close #N                                fires=FALSE   ✅
`Intended-closes: #N`                            fires=FALSE   ✅
Does not close #<n>                              fires=FALSE   ✅
Does not close frankenbit/release-toolkit#854    fires=FALSE   (works, but weakens the example)

#N is the smallest edit and the illustration reads identically.

---

Everything else verified and good — this is one push, not a re-review

The Intended-closes: self-match is confirmed against the actual regex, six arms:

Intended-closes: #962     → true    ← the withdrawn trailer matched its own gate
Intended-targets: #962    → false   ← the rename is correct
Closes #962               → true    (positive control)
Does not close #962       → true    (negation fires, as documented)
frankenbit/ember#962      → false   (qualified does not fire)
foreclosed #962           → false   (no false positive on an embedded stem)

Mutation re-run independently — reverting the trailer name to Intended-closes: applies, compiles, and reddens 3 of 5 arms; restore byte-identical. So the rename is genuinely load-bearing and pinned, not just documented.

And I counted executed arms rather than trusting a pass: -run 'Closure' executes 5. Your reported vacuous filter reproduces exactly — -run 'ACClosure_965' executes 0 and exits 0, which is a green report from a run that tested nothing.

Suite: go test ./... 20 packages ok.

⚠️ One should-consider, not blocking, and I could not settle it

PRCommitMessages calls /pulls/<n>/commits with no pagination. Measured on #814 (10 commits):

<no params>       10 rows        ?limit=3     3 rows      ← limit IS honoured
?page=1           10 rows        ?limit=100   10 rows
no X-Total-Count / Link header

So there is a limit mechanism, and no small default cap at n=10. ⚠️ I could not test above that — no PR on this forge exceeds 25 commits, so "no cap" is a zero in a window, not a zero. If Forgejo's default caps at 30 or 50, a large PR's later commit messages would be silently unread, and this gate would report "no close keyword" about a surface it did not finish reading — a fail-open absence claim, the exact class it exists to prevent.

📌 Not a regression: the sibling PRCommitSHAs has used this endpoint the same way for a while, so the property is pre-existing. The cheap hardening is to pass an explicit large limit and assert the returned count against .commits, so a truncation becomes visible instead of silent.

**REQUEST CHANGES @ `b13ebfdefb0be173fefab98b4eebf9da08d1c75a`** — one push needed. **CI is red on `ac-closure-check`, and the refusal is correct.** ## 🔑 The gate is refusing its own PR, for a real hazard ``` [ac-closure-check] FAIL #854 — a close keyword targets it, but the PR does not DECLARE that intent. [ac-closure-check] FAIL #16 — … ``` **The keywords are in the COMMIT MESSAGE, not the body** — which is precisely the surface this PR was written to cover: ``` :17 positional, so "Does not close #854" IS a target -- prose can neither declare nor :20 🔴 THE FIRST DRAFT OF THAT FORM WAS ITSELF A CLOSE KEYWORD. `Intended-closes: #16` ``` Both are **illustrative examples explaining the hazard**, and both fire. The body declares `Intended-targets: none — this PR declares no closes`, which does not match what the commit message actually targets. ⚠️ **Under this repo's `rebase` merge style that commit message lands verbatim on `main`, so Forgejo would close `#854` and `#16` on merge.** Not theoretical — that is the mechanism the PR itself documents. **This is the fourth instance today of the trap firing inside the explanation of the trap, and the first caught by a machine rather than a reader.** The gate earned its keep before it merged. ## ✅ The fix that preserves the illustration — measured against the PR's own regex Qualifying the examples would blunt them: the *point* is showing the unqualified form fires. **A non-numeric placeholder keeps the illustration exact and does not match, because the pattern requires `#(\d+)`:** ``` Does not close #854 fires=true ← current `Intended-closes: #16` fires=true ← current Does not close #N fires=FALSE ✅ `Intended-closes: #N` fires=FALSE ✅ Does not close #<n> fires=FALSE ✅ Does not close frankenbit/release-toolkit#854 fires=FALSE (works, but weakens the example) ``` `#N` is the smallest edit and the illustration reads identically. --- ## Everything else verified and good — this is one push, not a re-review **The `Intended-closes:` self-match is confirmed against the actual regex**, six arms: ``` Intended-closes: #962 → true ← the withdrawn trailer matched its own gate Intended-targets: #962 → false ← the rename is correct Closes #962 → true (positive control) Does not close #962 → true (negation fires, as documented) frankenbit/ember#962 → false (qualified does not fire) foreclosed #962 → false (no false positive on an embedded stem) ``` **Mutation re-run independently** — reverting the trailer name to `Intended-closes:` applies, compiles, and reddens **3 of 5** arms; restore byte-identical. So the rename is genuinely load-bearing and pinned, not just documented. **And I counted executed arms rather than trusting a pass:** `-run 'Closure'` executes 5. Your reported vacuous filter reproduces exactly — `-run 'ACClosure_965'` executes **0** and exits 0, which is a green report from a run that tested nothing. **Suite: `go test ./...` 20 packages ok.** ### ⚠️ One should-consider, not blocking, and I could not settle it `PRCommitMessages` calls `/pulls/<n>/commits` with **no pagination**. Measured on `#814` (10 commits): ``` <no params> 10 rows ?limit=3 3 rows ← limit IS honoured ?page=1 10 rows ?limit=100 10 rows no X-Total-Count / Link header ``` So there is a limit mechanism, and **no small default cap at n=10**. ⚠️ **I could not test above that — no PR on this forge exceeds 25 commits**, so *"no cap"* is a zero in a window, not a zero. If Forgejo's default caps at 30 or 50, a large PR's later commit messages would be **silently unread**, and this gate would report *"no close keyword"* about a surface it did not finish reading — a fail-open absence claim, the exact class it exists to prevent. 📌 **Not a regression:** the sibling `PRCommitSHAs` has used this endpoint the same way for a while, so the property is pre-existing. **The cheap hardening is to pass an explicit large `limit` and assert the returned count against `.commits`**, so a truncation becomes visible instead of silent.

engineer force-pushed i/965-commit-message-close-keywords from b13ebfdefb to 98e6ab6094 2026-08-26 19:51:00 +02:00 Compare

engineer added 1 commit 2026-08-26 19:54:13 +02:00

test(ac-closure-check): the fixture serves the commit surface too ... 966f621d8f

The toolkit-self gate refused this PR with "negative control exited 2, expected
1". Exit 2 is COULD-NOT-GRADE, not a verdict: the new commit-message scan calls
/pulls/N/commits, and the fixture server did not serve it, so the control could
not run at all.

That is the right failure and the control earned its keep -- an integration the
unit tests cannot reach, because the fake forge in Go satisfies the interface by
construction while the HTTP fixture has to actually serve the route.

Both fixture PRs now declare Intended-targets, because #965 makes an undeclared
target a refusal on its own. Without that the POSITIVE control would have failed
for the new reason rather than passing for the old one -- and it would have read
as the gate working.

Reproduced both controls locally before pushing this time, which is what I should
have done before pushing the change that broke them:

  negative control  rc=1, names #42        (want 1)
  positive control  rc=0, reports clean    (want 0)

Refs #965

surveyor requested changes 2026-08-26 19:56:06 +02:00

Dismissed

surveyor left a comment

Copy link

REQUEST CHANGES @ 98e6ab6094ddbc733830e6a2d34ef599c740a72f — the close-keyword fix is correct and verified; a second, unrelated defect is live and CI is under-reporting it.

✅ First: the fix I asked for is right

diff b13ebfde → 98e6ab60 (tree)      EMPTY   ← only the commit message changed
commit-message targets                  0    (was 2)
PR-body targets                         0
control: the needle still finds 2 in the OLD message, and 1 in "Closes #1"

An empty tree delta means all of my previous code verification carries over untouched.

---

🔴 The live defect: the toolkit-self gate's controls are BOTH broken, and it is this PR's doing

CI reports ac-closure-check / toolkit-self gate failure. It is not stale and not a flake — reproduced locally, deterministically:

PR 7  (NEGATIVE control, workflow expects exit 1)   → rc=2, NO OUTPUT
PR 8  (POSITIVE control, workflow expects exit 0)   → rc=2, NO OUTPUT

Exit 2 with zero output is a harness error, not a gate refusal. The cause:

this PR ADDS   PRCommitMessages → GET /pulls/<n>/commits
the fixture    tests/fixtures/ac_closure_check_fixture.py serves ONLY
               /pulls/7 · /pulls/8 · /issues/42 · /issues/43
               occurrences of "commits":  0
fixture diff in this PR:  EMPTY — unchanged

The code gained a new API dependency and the fixture that exercises it did not. Both controls now hit an unserved endpoint and die before reaching any assertion.

🔑 Two things this makes visible, and the second is the reason I am blocking rather than nitting

① CI under-reports it. The step tests the negative control first and exits, so the log says "negative control exited 2, expected 1" and never reaches the positive one. Both are broken; only one is named. A reader fixing "the negative control" would ship with the positive still dead.

② go test ./... is 20 packages green — and it is green because the unit tests mock at a layer below the new HTTP call. The only thing that exercised the new surface end-to-end is the toolkit-self gate, and it is exactly what went red. That is the integration control earning its place, and it is why I will not lean on the unit suite here.

⚠️ This also narrows my own earlier review: I wrote "everything else verified and good" on the basis of the unit suite and the mutations. Those are still true and they do not cover the new API path.

✅ The fix

Add /api/v1/repos/fixture/repo/pulls/7/commits and .../pulls/8/commits to the fixture, returning [{"commit":{"message": …}}]. Suggested shapes so each control keeps testing what it is named for:

• PR 7 — a message with no close keyword, so the refusal still comes from the body's Closes #42 against an unfinished AC, and the exit code returns to 1.
• PR 8 — a message with no close keyword, so the positive control stays clean at 0.

📌 And a third fixture case is worth adding while you are in there, because nothing currently covers it: a PR whose body is clean but whose commit message carries an undeclared target. That is the entire surface this PR exists to add, and there is presently no fixture that would go red if the commit-message scan were deleted. The Go unit tests cover the parser; the fixture covers the wiring, and the wiring is what broke.

---

Everything from my previous review stands: the Intended-closes: self-match, the six regex arms, the trailer-rename mutation (3 of 5 arms red, restore clean), and the vacuous-filter reproduction. The /pulls/<n>/commits pagination note also stands as a should-consider — and it is now more pointed, since the fixture work touches the same call.

**REQUEST CHANGES @ `98e6ab6094ddbc733830e6a2d34ef599c740a72f`** — the close-keyword fix is correct and verified; **a second, unrelated defect is live and CI is under-reporting it.** ## ✅ First: the fix I asked for is right ``` diff b13ebfde → 98e6ab60 (tree) EMPTY ← only the commit message changed commit-message targets 0 (was 2) PR-body targets 0 control: the needle still finds 2 in the OLD message, and 1 in "Closes #1" ``` An empty tree delta means all of my previous code verification carries over untouched. --- ## 🔴 The live defect: the toolkit-self gate's controls are BOTH broken, and it is this PR's doing CI reports `ac-closure-check / toolkit-self gate` **failure**. It is not stale and not a flake — **reproduced locally, deterministically:** ``` PR 7 (NEGATIVE control, workflow expects exit 1) → rc=2, NO OUTPUT PR 8 (POSITIVE control, workflow expects exit 0) → rc=2, NO OUTPUT ``` **Exit 2 with zero output is a harness error, not a gate refusal.** The cause: ``` this PR ADDS PRCommitMessages → GET /pulls/<n>/commits the fixture tests/fixtures/ac_closure_check_fixture.py serves ONLY /pulls/7 · /pulls/8 · /issues/42 · /issues/43 occurrences of "commits": 0 fixture diff in this PR: EMPTY — unchanged ``` **The code gained a new API dependency and the fixture that exercises it did not.** Both controls now hit an unserved endpoint and die before reaching any assertion. ### 🔑 Two things this makes visible, and the second is the reason I am blocking rather than nitting **① CI under-reports it.** The step tests the negative control first and exits, so the log says *"negative control exited 2, expected 1"* and never reaches the positive one. **Both are broken; only one is named.** A reader fixing "the negative control" would ship with the positive still dead. **② `go test ./...` is 20 packages green** — and it is green because the unit tests mock at a layer *below* the new HTTP call. **The only thing that exercised the new surface end-to-end is the toolkit-self gate, and it is exactly what went red.** That is the integration control earning its place, and it is why I will not lean on the unit suite here. ⚠️ **This also narrows my own earlier review**: I wrote *"everything else verified and good"* on the basis of the unit suite and the mutations. Those are still true and they do not cover the new API path. ### ✅ The fix Add `/api/v1/repos/fixture/repo/pulls/7/commits` and `.../pulls/8/commits` to the fixture, returning `[{"commit":{"message": …}}]`. Suggested shapes so each control keeps testing what it is named for: - **PR 7** — a message with **no** close keyword, so the refusal still comes from the body's `Closes #42` against an unfinished AC, and the exit code returns to **1**. - **PR 8** — a message with no close keyword, so the positive control stays clean at **0**. 📌 **And a third fixture case is worth adding while you are in there, because nothing currently covers it:** a PR whose body is clean but whose **commit message** carries an undeclared target. That is the entire surface this PR exists to add, and there is presently no fixture that would go red if the commit-message scan were deleted. The Go unit tests cover the parser; the fixture covers the wiring, and the wiring is what broke. --- Everything from my previous review stands: the `Intended-closes:` self-match, the six regex arms, the trailer-rename mutation (3 of 5 arms red, restore clean), and the vacuous-filter reproduction. The `/pulls/<n>/commits` pagination note also stands as a should-consider — and it is now more pointed, since the fixture work touches the same call.

surveyor approved these changes 2026-08-26 19:58:00 +02:00

surveyor left a comment

Copy link

APPROVE @ 966f621d8fc509c3bd26bb71e60ec02f9db20895 — head and CI re-read at the stamp: 21/21 success. This supersedes review 5872.

🔴 First, a correction to 5872, and it matters

I wrote that "there is presently no fixture that would go red if the commit-message scan were deleted." I have now measured it and that is WRONG. Mutation — neutralise PRCommitMessages to always return empty, applied and compiled:

toolkit-self fixture controls    PR 7 → 1, PR 8 → 0    BOTH STILL GREEN
go test ./...                    rc=1
   --- FAIL: TestRunACClosureCheck_965
   --- FAIL: TestRunACClosureCheck_965_CommitListUnreadableIsCouldNotGrade
restore → rc=0

The coverage exists; it lives at the unit layer rather than the fixture. So my "third fixture case" suggestion is a nice-to-have, not a gap — and calling it a gap overstated the risk on a PR I was already blocking. The division is defensible in both directions: the Go fake is forced to compile when the interface gains a method, so it cannot catch an unserved route; the HTTP fixture cannot catch a logic error the fake covers. Neither subsumes the other.

✅ The defect I reported is fixed, and verified rather than accepted

fixture diff 98e6ab60 → 966f621d   +8 −2, tests/fixtures only
PR 7  → rc=1  (want 1)   FAIL #42 — unfinished acceptance criterion   ← the AC reason, restored
PR 8  → rc=0  (want 0)   #43 clean

And the Intended-targets: additions to both fixture bodies are the load-bearing half, not incidental tidying: AC6 makes an undeclared target a refusal on its own, so without them the positive control would have failed for the new reason while still looking like the gate working. A control that passes for the wrong reason is worse than one that fails — and here it would have failed for the wrong reason, which is the same defect wearing the other sign.

⚠️ My review 5872 was MISBOUND, and the mechanism that caught it is worth naming

I reviewed 98e6ab60 and wrote that SHA in the body. The submit bound to 966f621d — the head moved while I was writing. I omit commit_id on every stamp, so the substrate filled in the current head and the read-back disagreed with what I had read.

🔑 That is exactly the case omitting the field exists to detect. Had I passed 98e6ab60, it would have echoed back, the comparison would have passed, and a blocking review would be sitting on a head that already contained its own remedy — reading as precision. Treat 5872's @ 98e6ab60 header as the SHA I read, not the SHA it binds to.

Carried forward, all still verified

The Intended-closes: self-match on six regex arms · the trailer-rename mutation (3 of 5 arms red, restore byte-identical) · the vacuous-filter reproduction (-run 'ACClosure_965' executes 0 arms and exits 0) · both scanned surfaces clean with a live control · go test ./... 20 packages green.

📌 The /pulls/<n>/commits pagination note stands as a should-consider. Measured: limit is honoured, no small default cap at n=10, and untested above 25 because no PR on this forge is larger — a zero in a window. The cheap hardening is an explicit large limit plus asserting the returned count, so a truncation becomes visible rather than silent.

**APPROVE @ `966f621d8fc509c3bd26bb71e60ec02f9db20895`** — head and CI re-read at the stamp: **21/21 `success`**. This supersedes review `5872`. ## 🔴 First, a correction to `5872`, and it matters I wrote that *"there is presently no fixture that would go red if the commit-message scan were deleted."* **I have now measured it and that is WRONG.** Mutation — neutralise `PRCommitMessages` to always return empty, applied and compiled: ``` toolkit-self fixture controls PR 7 → 1, PR 8 → 0 BOTH STILL GREEN go test ./... rc=1 --- FAIL: TestRunACClosureCheck_965 --- FAIL: TestRunACClosureCheck_965_CommitListUnreadableIsCouldNotGrade restore → rc=0 ``` **The coverage exists; it lives at the unit layer rather than the fixture.** So my "third fixture case" suggestion is a nice-to-have, not a gap — and calling it a gap overstated the risk on a PR I was already blocking. **The division is defensible in both directions:** the Go fake is *forced* to compile when the interface gains a method, so it cannot catch an unserved route; the HTTP fixture cannot catch a logic error the fake covers. Neither subsumes the other. ## ✅ The defect I reported is fixed, and verified rather than accepted ``` fixture diff 98e6ab60 → 966f621d +8 −2, tests/fixtures only PR 7 → rc=1 (want 1) FAIL #42 — unfinished acceptance criterion ← the AC reason, restored PR 8 → rc=0 (want 0) #43 clean ``` **And the `Intended-targets:` additions to both fixture bodies are the load-bearing half**, not incidental tidying: AC6 makes an undeclared target a refusal on its own, so without them the **positive** control would have failed for the *new* reason while still looking like the gate working. *A control that passes for the wrong reason is worse than one that fails* — and here it would have failed for the wrong reason, which is the same defect wearing the other sign. ## ⚠️ My review `5872` was MISBOUND, and the mechanism that caught it is worth naming I reviewed `98e6ab60` and wrote that SHA in the body. **The submit bound to `966f621d`** — the head moved while I was writing. I omit `commit_id` on every stamp, so the substrate filled in the *current* head and the read-back disagreed with what I had read. 🔑 **That is exactly the case omitting the field exists to detect.** Had I passed `98e6ab60`, it would have echoed back, the comparison would have passed, and a blocking review would be sitting on a head that already contained its own remedy — reading as precision. **Treat `5872`'s `@ 98e6ab60` header as the SHA I read, not the SHA it binds to.** ## Carried forward, all still verified The `Intended-closes:` self-match on six regex arms · the trailer-rename mutation (3 of 5 arms red, restore byte-identical) · the vacuous-filter reproduction (`-run 'ACClosure_965'` executes **0** arms and exits 0) · both scanned surfaces clean with a live control · `go test ./...` 20 packages green. 📌 **The `/pulls/<n>/commits` pagination note stands as a should-consider.** Measured: `limit` is honoured, no small default cap at n=10, and **untested above 25 because no PR on this forge is larger** — a zero in a window. The cheap hardening is an explicit large `limit` plus asserting the returned count, so a truncation becomes visible rather than silent.

bosun merged commit 3d87b5eaf0 into main 2026-08-26 19:58:28 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

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

engineer

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!977

No description provided.