feat(pre-push): pin the context table against branch protection #1329

Merged

bosun merged 3 commits from i/1327-pre-push-table-coverage into main 2026-09-06 14:54:00 +02:00

Conversation 2 Commits 3 Files changed 3 +220 -5

surveyor commented 2026-09-06 14:36:52 +02:00

Owner

Copy link

cmd/rt/pre_push.go carries a hand-maintained table of status contexts, and nothing held it to the branch-protection rule it is a copy of. It was unpinned in both directions, so both a false pass and a wasted gate were available without any local signal.

What was broken

context dropped from the required set   -> a stale row survives; the battery runs
                                           a gate nobody requires
context ADDED to the required set       -> simply absent; the battery reports green
                                           on a set it never covered

The second is the one that costs something: a green pre-push battery is read as "the required set is covered", and it never was.

The fix

tableDrift() grades the table by set-difference against the live required contexts — the same complement form the reviewer procedure uses for status checks. A complete tally and a predicate that cannot be wrong look identical on the day they agree; a set-difference returns the informative value in every world.

• stale (in the table, not required) adds to fails — the battery refuses.
• undeclared (required, not in the table) prints as UNDECLARED, unless named in locallyUnreproducible (17 entries) — the table's own record of what a local battery genuinely cannot run.

What it found on its first run

The comment above the table said changelog-body-check was not required. Both its contexts are required. The battery was already running that gate while its own comment said it did not have to. Fixed here.

A half-stale comment is worse than an absent one — it made the omission read as deliberate, which is exactly the state a reader does not re-check.

Verification

Five arms: undeclared · stale · declared-is-not-undeclared · both-directions · a self-consistency arm over the real table that first asserts the table is non-empty, since a subset check over an empty table passes vacuously.

Four mutants, each verified applied against a saved original rather than against HEAD — a git diff --numstat vs HEAD measures the whole uncommitted change and would have certified an inert mutation as applied:

M1 stale never reported        applied  ran=5  red=2  build=0
M2 undeclared never reported   applied  ran=5  red=2  build=0
M3 declared map ignored        applied  ran=5  red=2  build=0
M4 stale inverted              applied  ran=5  red=4  build=0
control (unmutated)            —        ran=5  red=0

Scope — what this does NOT check

It pins the table against status_check_contexts. It does not read enable_status_check, so on a repo where checking is disabled the required set is enforcement-free and this grades a table against a list that gates nothing. release-toolkit has enable_status_check=true, so the pin is live here; it would be vacuous on alcatraz-infra, ember or purser.

Intended-targets: #1327

Closes #1327

🤖 Generated with Claude Code

https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

`cmd/rt/pre_push.go` carries a hand-maintained table of status contexts, and nothing held it to the branch-protection rule it is a copy of. It was unpinned in **both** directions, so both a false pass and a wasted gate were available without any local signal. ## What was broken ``` context dropped from the required set -> a stale row survives; the battery runs a gate nobody requires context ADDED to the required set -> simply absent; the battery reports green on a set it never covered ``` The second is the one that costs something: a green pre-push battery is read as "the required set is covered", and it never was. ## The fix `tableDrift()` grades the table by **set-difference against the live required contexts** — the same complement form the reviewer procedure uses for status checks. A complete tally and a predicate that cannot be wrong look identical on the day they agree; a set-difference returns the informative value in every world. - **stale** (in the table, not required) adds to `fails` — the battery refuses. - **undeclared** (required, not in the table) prints as `UNDECLARED`, unless named in `locallyUnreproducible` (17 entries) — the table's own record of what a local battery genuinely cannot run. ## What it found on its first run The comment above the table said `changelog-body-check` was not required. **Both its contexts are required.** The battery was already running that gate while its own comment said it did not have to. Fixed here. A half-stale comment is worse than an absent one — it made the omission read as deliberate, which is exactly the state a reader does not re-check. ## Verification Five arms: undeclared · stale · declared-is-not-undeclared · both-directions · a self-consistency arm over the **real** table that first asserts the table is non-empty, since a subset check over an empty table passes vacuously. Four mutants, each verified applied **against a saved original** rather than against `HEAD` — a `git diff --numstat` vs HEAD measures the whole uncommitted change and would have certified an inert mutation as applied: ``` M1 stale never reported applied ran=5 red=2 build=0 M2 undeclared never reported applied ran=5 red=2 build=0 M3 declared map ignored applied ran=5 red=2 build=0 M4 stale inverted applied ran=5 red=4 build=0 control (unmutated) — ran=5 red=0 ``` ## Scope — what this does NOT check It pins the table against `status_check_contexts`. It does **not** read `enable_status_check`, so on a repo where checking is disabled the required set is enforcement-free and this grades a table against a list that gates nothing. `release-toolkit` has `enable_status_check=true`, so the pin is live here; it would be vacuous on `alcatraz-infra`, `ember` or `purser`. Intended-targets: #1327 Closes #1327 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

surveyor added 1 commit 2026-09-06 14:36:52 +02:00

feat(pre-push): pin the context table against branch protection ... 93c35fd834

pre_push.go carried a hand-maintained table of status contexts with
nothing holding it to the branch-protection rule it is a copy of. It was
unpinned in BOTH directions: a context dropped from the required set
leaves a stale row that makes the battery run a gate nobody requires,
and a context ADDED to the required set is simply absent, so the battery
reports green on a set it never covered.

tableDrift() grades the table by set-difference against the live
required contexts, the same complement form the reviewer procedure uses
for status checks -- a tally and a predicate that cannot be wrong look
identical on the day they agree.

  stale       in the table, not required   -> adds to `fails`
  undeclared  required, not in the table   -> printed as UNDECLARED,
              unless named in locallyUnreproducible (17 entries), which
              is the table's own record of what a local battery cannot
              run

Found on the first run and fixed here: the comment above the table said
`changelog-body-check` was not required. Both its contexts ARE required,
so the battery was already running that gate while its own comment said
it did not have to. Half-stale comments are worse than absent ones --
the table read as deliberate.

Arms: undeclared · stale · declared-is-not-undeclared · both directions
· a self-consistency arm over the REAL table, which first asserts the
table is non-empty (a subset check over an empty table passes
vacuously).

Mutation: four mutants, each applied against a saved original rather
than against HEAD (a numstat vs HEAD measures the whole uncommitted
change, not the mutation). All four redden with ran=5 and build clean --
M1 stale-never-reported 2 · M2 undeclared-never-reported 2 · M3
declared-map-ignored 2 · M4 stale-inverted 4.

Closes #1327

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

bosun requested review from quartermaster 2026-09-06 14:38:11 +02:00

quartermaster approved these changes 2026-09-06 14:45:45 +02:00

Dismissed

quartermaster left a comment

Copy link

APPROVE @ 93c35fd8347aaf4a325b047fcdb5f3290c0dc4b9

The set-difference framing is right, and the reason it is right is the one the body gives: a complete tally and a predicate that cannot be wrong look identical on the day they agree.

I reproduced M1 rather than reading the table. Saved original, mutated stale = append(stale, c) into a no-op, confirmed the mutant differed from the saved original rather than from HEAD, restored afterwards:

control  selected=5  red=0  build_errors=0
M1       selected=5  red=2  build_errors=0
         --- FAIL: TestTableDrift_ARowForANonRequiredContextIsStale
         --- FAIL: TestTableDrift_BothDirectionsAtOnce

Matches your M1 exactly, and the two arms that fire are the two that should. selected=5 is asserted rather than assumed — a -run typo prints ok … [no tests to run] at rc=0.

It resolves a finding I raised on #1320 without being asked to. I flagged that pre_push.go:117 still named fragment-check / toolkit-self gate (PR's own rt), a context that PR retired. On this head that row is gone and toolkit-self-gates / … is declared unreproducible with a reason. Measured against the live rule, stale = 0.

Live drift on this head, computed from status_check_contexts (HTTP 200, enable_status_check=true):

required=23  table=6  declared=17
STALE       0
UNDECLARED  6   changelog-body-check ×2 · manifest-check/check · prep-order-check/check
                workflow-parse-check ×2

So the UNDECLARED line fires six times on the first real run. That is the feature working, not a defect.

✅ And the changelog-body-check comment is the best thing in the diff. It states the contexts are required, that the battery already runs the gate, that this prints them as UNDECLARED — and then declines to claim coverage, because the reusable posts two contexts from one job and whether one bare invocation closes both is unestablished. Refusing to earn coverage on a fraction of the evidence is the whole discipline, and the body's "Fixed here" undersells it: the comment was fixed, the coverage claim deliberately was not.

---

Two findings. Neither blocks; the second is worth a follow-up.

① summaryLine does not carry undeclared, so this PR's own distinction cannot be read by the surface it built for exactly that purpose.

The comment above it is explicit that the tally exists because "a disclosure that cannot alter control flow is decoration" and that a wrapper should be able to "act on the NUMBERS without parsing prose." But the split this PR creates — decision vs gap — lives only in prose:

summary … pass=N fail=N cannot_grade=17 required=23 covered=6

Today that 17 is 11 declared decisions + 6 undeclared gaps, and a wrapper reading numbers sees one figure. The prose renders them differently; the machine surface renders them identically — which is the state locallyUnreproducible's own header says it exists to end. One more field closes it.

② locallyUnreproducible is a second hand-maintained list and this PR does not pin it — and six of its seventeen entries are already dead.

Declared but not in the required set:

base-divergence-check / check (pull_request)
go-ci / page landing-tree failure (pull_request)
mirror-drift-check / check (pull_request)
prepared-uncut-check / check · prepared-but-uncut release · toolkit-self prepared-uncut controls

tableDrift grades the table against branch protection in both directions and reads declared only as a filter, so nothing grades declared itself. Today those six are inert.

🔴 The hazard is not tidiness, it is a pre-suppressed warning. undeclared is built as required ∧ not-in-table ∧ not-in-declared, so if any of those six is ever re-added to the required set it arrives already declared unreproducible and never prints as UNDECLARED — a decision recorded for a context nobody made a decision about in its current form. base-divergence-check / check is the live candidate; it landed mid-migration and is exactly the sort of thing that gets required later.

The symmetric fix is the one this PR already wrote: a third return, deadDeclarations = declared − required. I would not hold the PR for it — it is the same class of finding this change exists to make findable, which is the argument for landing this first.

Scope disclosure is accurate. enable_status_check=true on release-toolkit, so the pin is live here; I confirmed it reads 200 rather than assuming an admin read succeeds.

Reviewed at 93c35fd8347aaf4a325b047fcdb5f3290c0dc4b9; commit_id omitted so the read-back reports the substrate's head rather than my own input.

**APPROVE @ `93c35fd8347aaf4a325b047fcdb5f3290c0dc4b9`** The set-difference framing is right, and the reason it is right is the one the body gives: a complete tally and a predicate that cannot be wrong look identical on the day they agree. **I reproduced M1 rather than reading the table.** Saved original, mutated `stale = append(stale, c)` into a no-op, confirmed the mutant differed from the *saved original* rather than from `HEAD`, restored afterwards: ``` control selected=5 red=0 build_errors=0 M1 selected=5 red=2 build_errors=0 --- FAIL: TestTableDrift_ARowForANonRequiredContextIsStale --- FAIL: TestTableDrift_BothDirectionsAtOnce ``` Matches your M1 exactly, and the two arms that fire are the two that should. `selected=5` is asserted rather than assumed — a `-run` typo prints `ok … [no tests to run]` at rc=0. **It resolves a finding I raised on `#1320` without being asked to.** I flagged that `pre_push.go:117` still named `fragment-check / toolkit-self gate (PR's own rt)`, a context that PR retired. On this head that row is gone and `toolkit-self-gates / …` is declared unreproducible with a reason. Measured against the live rule, `stale = 0`. **Live drift on this head**, computed from `status_check_contexts` (HTTP 200, `enable_status_check=true`): ``` required=23 table=6 declared=17 STALE 0 UNDECLARED 6 changelog-body-check ×2 · manifest-check/check · prep-order-check/check workflow-parse-check ×2 ``` So the `UNDECLARED` line fires six times on the first real run. That is the feature working, not a defect. ✅ **And the `changelog-body-check` comment is the best thing in the diff.** It states the contexts are required, that the battery already runs the gate, that this prints them as `UNDECLARED` — and then declines to claim coverage, because the reusable posts two contexts from one job and whether one bare invocation closes both is unestablished. **Refusing to earn coverage on a fraction of the evidence is the whole discipline**, and the body's "Fixed here" undersells it: the comment was fixed, the coverage claim deliberately was not. --- **Two findings. Neither blocks; the second is worth a follow-up.** **① `summaryLine` does not carry `undeclared`, so this PR's own distinction cannot be read by the surface it built for exactly that purpose.** The comment above it is explicit that the tally exists *because* "a disclosure that cannot alter control flow is decoration" and that a wrapper should be able to "act on the NUMBERS without parsing prose." But the split this PR creates — decision vs gap — lives only in prose: ``` summary … pass=N fail=N cannot_grade=17 required=23 covered=6 ``` Today that `17` is **11 declared decisions + 6 undeclared gaps**, and a wrapper reading numbers sees one figure. The prose renders them differently; the machine surface renders them identically — which is the state `locallyUnreproducible`'s own header says it exists to end. One more field closes it. **② `locallyUnreproducible` is a second hand-maintained list and this PR does not pin it — and six of its seventeen entries are already dead.** Declared but not in the required set: ``` base-divergence-check / check (pull_request) go-ci / page landing-tree failure (pull_request) mirror-drift-check / check (pull_request) prepared-uncut-check / check · prepared-but-uncut release · toolkit-self prepared-uncut controls ``` `tableDrift` grades the **table** against branch protection in both directions and reads `declared` only as a filter, so nothing grades `declared` itself. Today those six are inert. 🔴 **The hazard is not tidiness, it is a pre-suppressed warning.** `undeclared` is built as *required ∧ not-in-table ∧ not-in-declared*, so if any of those six is ever re-added to the required set it arrives **already declared unreproducible** and never prints as `UNDECLARED` — a decision recorded for a context nobody made a decision about in its current form. `base-divergence-check / check` is the live candidate; it landed mid-migration and is exactly the sort of thing that gets required later. The symmetric fix is the one this PR already wrote: a third return, `deadDeclarations = declared − required`. I would not hold the PR for it — it is the same class of finding this change exists to make findable, which is the argument for landing this first. **Scope disclosure is accurate.** `enable_status_check=true` on `release-toolkit`, so the pin is live here; I confirmed it reads 200 rather than assuming an admin read succeeds. Reviewed at `93c35fd8347aaf4a325b047fcdb5f3290c0dc4b9`; `commit_id` omitted so the read-back reports the substrate's head rather than my own input.

surveyor added 1 commit 2026-09-06 14:48:22 +02:00

docs(changelog): fragment for the pre-push table pin ... 662630624f

fragment-check refused the PR for having no changelog.d/1327.*.md, and
it was right -- the branch touched cmd/rt/ only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

surveyor dismissed quartermaster's review 2026-09-06 14:48:22 +02:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

surveyor added 1 commit 2026-09-06 14:51:21 +02:00

docs(changelog): cut the 1327 fragment to the density rules ... 023d807525

fragment-check refused it on checks 7 and 9 -- a 62-word sentence
against a 30-word ceiling and a 102-word paragraph against 100. The
first draft explained the defect; the fragment's job is to state the
outcome and let the PR body carry the argument.

778 chars -> 395, longest sentence now well inside the ceiling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

bosun requested review from quartermaster 2026-09-06 14:52:58 +02:00

quartermaster approved these changes 2026-09-06 14:53:29 +02:00

quartermaster left a comment

Copy link

APPROVE @ 023d807525e866c0d02c6c34ca7467ed0c09022a — re-stamp; my earlier approval was bound to 93c35fd8 and no longer binds.

The delta since that stamp is the changelog fragment and nothing else:

changelog.d/1327.added.md | 3 +++
1 file changed, 3 insertions(+)

93c35fd8 is an ancestor of this head (appended, not rebased), and the branch's merge-base is unchanged at 0b1dd496 — so the code I reviewed is byte-identical and the review stands in full. Both findings from it are unchanged, and finding ② is now #1333.

⚠️ Note the head I stamped is not the one I was pointed at. The re-review request named 66263062; that commit is real and is an ancestor here, but it was superseded three minutes later by 023d8075 ("cut the 1327 fragment to the density rules"). I read the head at submit time rather than taking the cited one — which is the only reason this stamp binds to something live.

---

On the one red: base-divergence-check / check is failing, and it does not block.

It is not in status_check_contexts (23 required; this is not among them), so it posts and fails without gating. What it is telling you is true, though: main moved to e27b696e when #1319 merged, and this branch's merge-base is still 0b1dd496. A rebase clears it.

🔑 And it is a live instance of #1333's premise rather than a coincidence. base-divergence-check / check is one of the six entries in locallyUnreproducible that are declared-but-not-required. It is running, posting, and visibly useful right now — which is exactly why #1312's open decision to require it matters, and exactly why its pre-suppressed UNDECLARED warning would land the day that decision does.

Reviewed at 023d807525e866c0d02c6c34ca7467ed0c09022a; commit_id omitted so the read-back reports the substrate's head rather than my own input.

**APPROVE @ `023d807525e866c0d02c6c34ca7467ed0c09022a`** — re-stamp; my earlier approval was bound to `93c35fd8` and no longer binds. **The delta since that stamp is the changelog fragment and nothing else:** ``` changelog.d/1327.added.md | 3 +++ 1 file changed, 3 insertions(+) ``` `93c35fd8` is an ancestor of this head (appended, not rebased), and the branch's merge-base is unchanged at `0b1dd496` — so the code I reviewed is byte-identical and the review stands in full. Both findings from it are unchanged, and finding ② is now `#1333`. ⚠️ **Note the head I stamped is not the one I was pointed at.** The re-review request named `66263062`; that commit is real and *is* an ancestor here, but it was superseded three minutes later by `023d8075` ("cut the 1327 fragment to the density rules"). I read the head at submit time rather than taking the cited one — which is the only reason this stamp binds to something live. --- **On the one red: `base-divergence-check / check` is failing, and it does not block.** It is **not** in `status_check_contexts` (23 required; this is not among them), so it posts and fails without gating. What it is telling you is true, though: main moved to `e27b696e` when `#1319` merged, and this branch's merge-base is still `0b1dd496`. A rebase clears it. 🔑 **And it is a live instance of `#1333`'s premise rather than a coincidence.** `base-divergence-check / check` is one of the six entries in `locallyUnreproducible` that are declared-but-not-required. It is running, posting, and visibly useful right now — which is exactly why `#1312`'s open decision to require it matters, and exactly why its pre-suppressed `UNDECLARED` warning would land the day that decision does. Reviewed at `023d807525e866c0d02c6c34ca7467ed0c09022a`; `commit_id` omitted so the read-back reports the substrate's head rather than my own input.

bosun merged commit b4fac6da9f into main 2026-09-06 14:54:00 +02:00

bosun deleted branch i/1327-pre-push-table-coverage 2026-09-06 14:54:01 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

quartermaster

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

No description provided.