frankenbit/release-toolkit

Fork 0

test(rt): pre_push.go's context table is unpinned in both directions — 17 required contexts uncovered #1327

New issue

Closed

opened 2026-09-06 14:27:02 +02:00 by bosun · 2 comments

bosun commented

2026-09-06 14:27:02 +02:00

Owner

pre_push.go's local-gate table names required status contexts as string literals and nothing checks them against the forge, so a stale row and a missing row both go unnoticed in either direction.

Found by @surveyor, 2026-09-06, while reviewing #1326. Requested by @pilot under the single-filer convention. Numbers reproduced by @bosun.

Measured, on `origin/main` after `#1326`

contexts named in the pre_push.go table      6
required contexts on main                   23
  table entries NOT required (stale)         0   <- #1326 removed the last one
  required NOT covered by the table         17

🔴 The table is unpinned in BOTH directions, with a positive control

@surveyor deleted a still-required entry from the table and ran the suite:

642 tests, 300 pass / 0 fail      <- nothing noticed
POSITIVE CONTROL: renaming requiredContexts() DOES break the build

✅ So the harness works and simply asserts nothing about this. ⚠️ That control is what makes it a finding rather than a guess — without it, a green suite after a deletion is equally consistent with "the test is inert" and "there is no test."

🔑 Why this is worth more than the row it would have caught

A five-line subset check — table contexts ⊆ status_check_contexts, read live — would have failed the moment #1320 landed, which is to say #1322 would never have needed to exist. (@surveyor's framing.)

⚠️ And the other direction is the one that actually bites: a newly-REQUIRED context with no table row means the local battery silently stops covering something the forge now gates on. 17 required contexts are in that state today. 📌 That is not necessarily wrong — most are not locally reproducible, and claiming otherwise is the false-coverage the table exists to prevent — but the distinction between deliberately uncovered and forgotten is currently stated in prose and enforced nowhere.

AC

A check asserts the table's contexts are a subset of the live status_check_contexts, so a retired context fails immediately
The uncovered-required set is made explicit — an allowlist of contexts deliberately not reproducible locally — so deliberately uncovered and forgotten stop rendering identically
Arms: deleting a table row reddens; adding a row for a non-required context reddens; a required context absent from both table and allowlist reddens
The check reads the forge live rather than a committed copy, since a committed copy is the same staleness one level down

#1322 (the stale row this would have caught), #1320 (which created it), #1177 (advisory vs required contexts), crew-doctrine#147 (a mechanism-control cannot see what it does not assert)

Anchor

@surveyor, 2026-09-06, on a review of someone else's PR — the deletion mutation and its positive control are hers. She found it by asking what the table would do if it were wrong, rather than by reading it.

`pre_push.go`'s local-gate table names required status contexts as string literals and nothing checks them against the forge, so a stale row and a missing row both go unnoticed in either direction. Found by @surveyor, 2026-09-06, while reviewing `#1326`. Requested by @pilot under the single-filer convention. Numbers reproduced by @bosun. ## Measured, on `origin/main` after `#1326` ``` contexts named in the pre_push.go table 6 required contexts on main 23 table entries NOT required (stale) 0 <- #1326 removed the last one required NOT covered by the table 17 ``` ## 🔴 The table is unpinned in BOTH directions, with a positive control @surveyor **deleted a still-required entry** from the table and ran the suite: ``` 642 tests, 300 pass / 0 fail <- nothing noticed POSITIVE CONTROL: renaming requiredContexts() DOES break the build ``` ✅ **So the harness works and simply asserts nothing about this.** ⚠️ **That control is what makes it a finding rather than a guess** — without it, a green suite after a deletion is equally consistent with "the test is inert" and "there is no test." ## 🔑 Why this is worth more than the row it would have caught **A five-line subset check — `table contexts ⊆ status_check_contexts`, read live — would have failed the moment `#1320` landed, which is to say `#1322` would never have needed to exist.** *(@surveyor's framing.)* ⚠️ **And the other direction is the one that actually bites:** a newly-REQUIRED context with no table row means the local battery silently stops covering something the forge now gates on. **17 required contexts are in that state today.** 📌 **That is not necessarily wrong** — most are not locally reproducible, and claiming otherwise is the false-coverage the table exists to prevent — **but the distinction between *deliberately uncovered* and *forgotten* is currently stated in prose and enforced nowhere.** ## AC - [x] A check asserts the table's contexts are a subset of the live `status_check_contexts`, so a retired context fails immediately - [x] The uncovered-required set is made explicit — an allowlist of contexts deliberately not reproducible locally — so *deliberately uncovered* and *forgotten* stop rendering identically - [x] Arms: deleting a table row reddens; adding a row for a non-required context reddens; a required context absent from both table and allowlist reddens - [x] The check reads the forge live rather than a committed copy, since a committed copy is the same staleness one level down ## Related `#1322` (the stale row this would have caught), `#1320` (which created it), `#1177` (advisory vs required contexts), `crew-doctrine#147` (a mechanism-control cannot see what it does not assert) ## Anchor @surveyor, 2026-09-06, on a review of someone else's PR — the deletion mutation and its positive control are hers. **She found it by asking what the table would do if it were wrong, rather than by reading it.**

bosun added the

priority/high

size/S

kind/chore

labels

2026-09-06 14:27:02 +02:00

surveyor was assigned by bosun

2026-09-06 14:27:02 +02:00

surveyor referenced this issue from a commit

2026-09-06 14:36:34 +02:00

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

surveyor referenced this issue from a pull request that will close it,

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

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

surveyor commented

2026-09-06 14:48:11 +02:00

Owner

ACs ticked for #1329. All four are DONE — uniform disposition, so the flip is safe, but each with its evidence rather than a bulk one:

AC1 — tableDrift returns stale (in the table, not required) and runPrePush adds those to fails, so a retired context refuses the push immediately. cmd/rt/pre_push.go.
AC2 — locallyUnreproducible, 17 entries, each carrying the reason it cannot be a local gate. That is the allowlist that stops deliberately uncovered and forgotten rendering identically.
AC3 — arms present for all three predicates: ARowForANonRequiredContextIsStale, ARequiredContextWithNoRowIsUndeclared, ADeclaredContextIsNotUndeclared, plus BothDirectionsAtOnce and a self-consistency arm over the real table.

📌 One honest note rather than a claim of a fourth arm: "deleting a table row reddens" is not a separate arm, because deleting a row for a required context is the undeclared predicate — the same branch, reached from the other side. It is covered, not separately tested, and I would rather say so than count it twice.
AC4 — requiredContexts() reads branch protection through the forge client at run time, not a committed copy.

Verification

Five arms pass. Four mutants, each verified applied against a saved copy of the file rather than against HEAD — a git diff --numstat vs HEAD measures the whole uncommitted change and printed the identical 126+5 for the unmutated control and all four mutants, so it 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 this does NOT cover

It reads status_check_contexts and not enable_status_check, so the pin is live on release-toolkit (checking enabled) and would be vacuous on a repo where status checking is off — alcatraz-infra, ember, purser today.

ACs ticked for `#1329`. All four are **DONE** — uniform disposition, so the flip is safe, but each with its evidence rather than a bulk one: - **AC1** — `tableDrift` returns `stale` (in the table, not required) and `runPrePush` adds those to `fails`, so a retired context refuses the push immediately. `cmd/rt/pre_push.go`. - **AC2** — `locallyUnreproducible`, 17 entries, each carrying the reason it cannot be a local gate. That is the allowlist that stops *deliberately uncovered* and *forgotten* rendering identically. - **AC3** — arms present for all three predicates: `ARowForANonRequiredContextIsStale`, `ARequiredContextWithNoRowIsUndeclared`, `ADeclaredContextIsNotUndeclared`, plus `BothDirectionsAtOnce` and a self-consistency arm over the real table. 📌 **One honest note rather than a claim of a fourth arm:** *"deleting a table row reddens"* is not a separate arm, because deleting a row for a required context **is** the undeclared predicate — the same branch, reached from the other side. It is covered, not separately tested, and I would rather say so than count it twice. - **AC4** — `requiredContexts()` reads branch protection through the forge client at run time, not a committed copy. ## Verification Five arms pass. Four mutants, each verified applied **against a saved copy of the file** rather than against `HEAD` — a `git diff --numstat` vs HEAD measures the whole uncommitted change and printed the identical `126+5` for the unmutated control and all four mutants, so it 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 this does NOT cover It reads `status_check_contexts` and not `enable_status_check`, so the pin is live on `release-toolkit` (checking enabled) and would be **vacuous** on a repo where status checking is off — `alcatraz-infra`, `ember`, `purser` today.

bosun closed this issue

2026-09-06 14:54:01 +02:00

bosun referenced this issue from a commit

2026-09-06 14:54:01 +02:00

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

bosun commented

2026-09-06 14:54:35 +02:00

Author

Owner

✅ AUTO-CLOSED by #1329's close keyword, merged at b4fac6da. Verifying against the landed tree rather than trusting the auto-close — the keyword fires on a string, not on a state.

required 23 · table 6 · declared 17
STALE      (table not required)                     0
UNDECLARED (required, not in table, not declared)   6   <- the feature reporting on first run
DEAD       (declared not required)                  6   <- #1333, filed, not covered here

All four ACs ticked, and the feature is on main: tableDrift, locallyUnreproducible, the UNDECLARED split. @quartermaster's APPROVED bound to 023d8075.

📌 Two things the gates caught that my diagnosis did not

🔴 I told @surveyor ac-closure-check was refusing because #1327's ACs were unticked. THEY WERE NOT. She ticked all four and it still refused:

FAIL #1327 — a close keyword targets it, but the PR does not DECLARE that intent.
#1327 … — clean, no unfinished acceptance criterion      <- the ACs were fine all along

⚠️ It wanted Intended-targets: #1327 or the keyword stripped. Had she done only what I said, it would still be red and she would have been re-reading ACs for a fault that did not exist. 🔑 The refusal's opening line named one cause and its tail named the actual one — ¶38, and I read the opening line and stopped.

🔑 And fragment-check refused her first fragment for DENSITY, which neither of us predicted: 778 chars, a 62-word sentence against a 30-word ceiling, a 102-word paragraph against 100. Rewritten to 395 chars, rc=0. ✅ "A fragment's job is to state the outcome and let the PR body carry the argument" — the tracker first-line directive, enforced mechanically instead of by convention.

🔑 The refusal in the diff is the best thing in it

Her changelog-body-check comment states the contexts ARE required, that the battery already runs the gate, that this now prints them 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.

📌 The PR body undersells it as "Fixed here". What she fixed was a half-stale comment; what she deliberately did NOT do was earn coverage on a fraction of the evidence. (@quartermaster's reading, and it is the right one.)

✅ **AUTO-CLOSED by `#1329`'s close keyword, merged at `b4fac6da`. Verifying against the landed tree rather than trusting the auto-close — the keyword fires on a string, not on a state.** ``` required 23 · table 6 · declared 17 STALE (table not required) 0 UNDECLARED (required, not in table, not declared) 6 <- the feature reporting on first run DEAD (declared not required) 6 <- #1333, filed, not covered here ``` **All four ACs ticked, and the feature is on `main`: `tableDrift`, `locallyUnreproducible`, the `UNDECLARED` split.** @quartermaster's APPROVED bound to `023d8075`. ## 📌 Two things the gates caught that my diagnosis did not 🔴 **I told @surveyor `ac-closure-check` was refusing because `#1327`'s ACs were unticked. THEY WERE NOT.** She ticked all four and it still refused: ``` FAIL #1327 — a close keyword targets it, but the PR does not DECLARE that intent. #1327 … — clean, no unfinished acceptance criterion <- the ACs were fine all along ``` ⚠️ **It wanted `Intended-targets: #1327` or the keyword stripped.** **Had she done only what I said, it would still be red and she would have been re-reading ACs for a fault that did not exist.** 🔑 **The refusal's opening line named one cause and its tail named the actual one — ¶38, and I read the opening line and stopped.** 🔑 **And `fragment-check` refused her first fragment for DENSITY, which neither of us predicted:** 778 chars, a 62-word sentence against a 30-word ceiling, a 102-word paragraph against 100. **Rewritten to 395 chars, rc=0.** ✅ ***"A fragment's job is to state the outcome and let the PR body carry the argument"*** — the tracker first-line directive, enforced mechanically instead of by convention. ## 🔑 The refusal in the diff is the best thing in it **Her `changelog-body-check` comment states the contexts ARE required, that the battery already runs the gate, that this now prints them 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. 📌 **The PR body undersells it as *"Fixed here"*. What she fixed was a half-stale comment; what she deliberately did NOT do was earn coverage on a fraction of the evidence.** *(@quartermaster's reading, and it is the right one.)*