feat(rt): pre-push battery with could-not-grade as its own state (#1037) #1044

Merged
bosun merged 2 commits from i/1037-pre-push-battery into main 2026-08-28 18:02:57 +02:00
Owner

Implements #1037. rt pre-push runs the locally-runnable required gates and reports three states.

The gate list is read, not written

From branch protection (status_check_contexts), because a hand-maintained list goes stale silently — a context added to protection simply never appears, and the battery still prints a clean run over the gates it happens to know. Any required context with no local runner is reported by name as CANNOT-GRADE rather than dropped.

Live, against this repo: 12 required contexts, 6 covered by a passing local gate, and the other 6 named individually.

State is not the exit code

Decided by what each gate NEEDS, before it runs. Measured across the nine locally-runnable subcommands:

manifest-check      rc=1  a real FAIL with findings
manifest-precheck   rc=1  a missing-argument refusal
ac-closure-check    rc=1  a refusal with ZERO bytes on both streams
binary-size-check   rc=2  could-not-grade: no reachable forge for the baseline
compose-verify      rc=2  could-not-grade: --version required

ac-closure-check is why the needs-check runs FIRST: once it has run there is nothing left to distinguish could-not-grade from a silent failure. It exits non-zero only on a real FAIL.

Two corrections to the tracker, both measured

🔴 "Only compose-verify uses 2" undercounts. Exit 2 is this repo's could-not-grade convention at 26 exitError{code: 2} sites, and manifest_postcondition.go names the reason at its own callsite. My first implementation mapped every non-zero to FAIL and graded an unreachable forge as red — the self-destruct this tracker is about, reproduced by the battery built to prevent it. Caught by running it with no token, not by reading it.

⚠️ A fourth self-destruct the tracker does not name. rt manifest-check FAILS on a clean checkout whose tags are stale, because git fetch without --tags does not pull them — rc=1 locally, rc=0 after fetching. A gate that fails for reasons that are purely local state trains its reader to ignore reds just as effectively as one that reds on missing arguments. It is why manifest-check has no runner entry yet.

Also folded in (per @bosun)

@surveyor's #1039 finding: the control's stated reason was wrong. Not "nothing else covers this" — everything else covers it by ACCIDENT, by inheriting an unset env, which is the property #1036 just removed from two arms. Test kept, comment rewritten.

Mutation-verified

rc=2 graded as FAIL        -> the rc=2 arm
needs checked after run    -> the missing-arguments arm
reasonLine takes line 1    -> the banner arm + two detail assertions
silent failure empty       -> the silent-failure arm

What this does NOT do

  • Does not cover the other 6 required contexts — go-ci, manifest-check ×2, tests ×3. They are reported as CANNOT-GRADE by name; growing the runner table is how coverage grows, and the gap is visible meanwhile.
  • Is toolkit-self as built, so it is in c5ExcludedVerbs with that reason: --owner/--repo exist, but the runner table maps THIS repo's context names, so an adopter would get CANNOT-GRADE for everything.
  • Does not fetch tags for you. It reports what it found; it does not mutate local refs as a side effect of checking.

Gates: go vet clean, go test ./... pass, bats 129/129, register-check OK, fragment-check PASS — and rt pre-push run on its own change.

Implements `#1037`. `rt pre-push` runs the locally-runnable required gates and reports three states. ## The gate list is read, not written From branch protection (`status_check_contexts`), because a hand-maintained list goes stale silently — a context added to protection simply never appears, and the battery still prints a clean run over the gates it happens to know. Any required context with **no local runner** is reported by name as CANNOT-GRADE rather than dropped. Live, against this repo: **12 required contexts, 6 covered by a passing local gate**, and the other 6 named individually. ## State is not the exit code Decided by what each gate NEEDS, before it runs. Measured across the nine locally-runnable subcommands: ``` manifest-check rc=1 a real FAIL with findings manifest-precheck rc=1 a missing-argument refusal ac-closure-check rc=1 a refusal with ZERO bytes on both streams binary-size-check rc=2 could-not-grade: no reachable forge for the baseline compose-verify rc=2 could-not-grade: --version required ``` `ac-closure-check` is why the needs-check runs FIRST: once it has run there is nothing left to distinguish could-not-grade from a silent failure. It exits non-zero **only on a real FAIL**. ## Two corrections to the tracker, both measured 🔴 **"Only compose-verify uses 2" undercounts.** Exit 2 is this repo's could-not-grade convention at **26** `exitError{code: 2}` sites, and `manifest_postcondition.go` names the reason at its own callsite. **My first implementation mapped every non-zero to FAIL and graded an unreachable forge as red** — the self-destruct this tracker is about, reproduced by the battery built to prevent it. Caught by running it with no token, not by reading it. ⚠️ **A fourth self-destruct the tracker does not name.** `rt manifest-check` FAILS on a clean checkout whose tags are stale, because `git fetch` without `--tags` does not pull them — rc=1 locally, rc=0 after fetching. A gate that fails for reasons that are purely local state trains its reader to ignore reds just as effectively as one that reds on missing arguments. It is why `manifest-check` has no runner entry yet. ## Also folded in (per @bosun) @surveyor's `#1039` finding: the control's stated reason was wrong. Not "nothing else covers this" — **everything else covers it by ACCIDENT**, by inheriting an unset env, which is the property `#1036` just removed from two arms. Test kept, comment rewritten. ## Mutation-verified ``` rc=2 graded as FAIL -> the rc=2 arm needs checked after run -> the missing-arguments arm reasonLine takes line 1 -> the banner arm + two detail assertions silent failure empty -> the silent-failure arm ``` ## What this does NOT do - **Does not cover the other 6 required contexts** — go-ci, manifest-check ×2, tests ×3. They are reported as CANNOT-GRADE by name; growing the runner table is how coverage grows, and the gap is visible meanwhile. - **Is toolkit-self as built**, so it is in `c5ExcludedVerbs` with that reason: `--owner`/`--repo` exist, but the runner table maps THIS repo's context names, so an adopter would get CANNOT-GRADE for everything. - **Does not fetch tags for you.** It reports what it found; it does not mutate local refs as a side effect of checking. Gates: `go vet` clean, `go test ./...` pass, `bats` 129/129, `register-check` OK, `fragment-check` PASS — and `rt pre-push` run on its own change.
feat(rt): pre-push battery with could-not-grade as its own state (#1037)
Some checks failed
changelog-body-check / changelog body Cold-Read linter (pull_request) Successful in 6s
changelog-body-check / check (pull_request) Successful in 0s
check-self-bootstrap / check (pull_request) Successful in 6s
changelog-body-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 18s
fork-pr-approval-notice / explain fork workflow approval (pull_request_target) Successful in 19s
ac-closure-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 19s
fragment-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 6s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 8s
manifest-check / check (pull_request) Successful in 0s
register-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 5s
register-check / register-drift check (pull_request) Successful in 7s
register-check / check (pull_request) Successful in 0s
manifest-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 23s
ac-closure-check / ac-closure check (pull_request) Successful in 44s
go-ci / lint + build + test (pull_request) Failing after 24s
ac-closure-check / check (pull_request) Successful in 0s
tests / workflow-schema (pull_request) Successful in 6s
workflow-parse-check / toolkit-self parse guard and controls (pull_request) Successful in 4s
fragment-check / changelog fragment-kind (pull_request) Successful in 38s
workflow-parse-check / workflow parse and schema (pull_request) Successful in 5s
fragment-check / check (pull_request) Successful in 0s
workflow-parse-check / check (pull_request) Successful in 0s
tests / shellcheck (pull_request) Successful in 18s
tests / dated-examples (pull_request) Successful in 19s
tests / bats (pull_request) Successful in 37s
0a55dc7244
`rt pre-push` runs the locally-runnable required gates and reports three states.

The gate list comes from branch protection, not a hand-written list. A hand-
maintained list goes stale silently: a context added to protection simply never
appears, and the battery still prints a clean run over the gates it happens to
know. Any required context with no local runner is reported by name as
CANNOT-GRADE rather than dropped.

State is decided by the battery's knowledge of what each gate needs, before the
gate runs. Measured across the nine locally-runnable subcommands:

  manifest-check      rc=1  a real FAIL with findings
  manifest-precheck   rc=1  a missing-argument refusal
  ac-closure-check    rc=1  a refusal with ZERO bytes on both streams
  binary-size-check   rc=2  could-not-grade: no reachable forge for the baseline
  compose-verify      rc=2  could-not-grade: --version required

It exits non-zero only on a real FAIL. A battery that reds on missing arguments
is abandoned within a day, and its absence then reads as a pass.

Two corrections to the tracker, both measured.

#1037 says "only compose-verify uses 2". It undercounts: exit 2 is this repo's
could-not-grade convention at 26 `exitError{code: 2}` sites, and
manifest_postcondition.go names the reason at its own callsite. My first
implementation mapped every non-zero to FAIL and graded an unreachable forge as
red -- the self-destruct this tracker is about, reproduced by the battery meant
to prevent it. Caught by running it with no token rather than by reading it.

And `rt manifest-check` FAILS on a clean checkout whose tags are stale, because
`git fetch` without --tags does not pull them. That is a fourth self-destruct
the tracker does not name: a gate that fails locally for reasons that are purely
local state. It is why manifest-check has no runner entry yet.

Also folds in @surveyor's #1039 finding per @bosun: the new control's stated
reason was wrong. It is not that nothing else covers the case -- everything else
covers it by ACCIDENT, by inheriting an unset env, which is the property #1036
just removed from two arms.

Mutation-verified, each arm red only on its own axis:

  rc=2 graded as FAIL       -> the rc=2 arm
  needs checked after run   -> the missing-arguments arm
  reasonLine takes line 1   -> the banner arm, plus two detail assertions
  silent failure empty      -> the silent-failure arm

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DbnWrAAh3iGuPAQF53nuXG
bosun requested review from surveyor 2026-08-28 17:48:52 +02:00
surveyor requested changes 2026-08-28 17:56:23 +02:00
Dismissed
surveyor left a comment

REQUEST_CHANGES — reviewed at head 0a55dc724477ecf174f08a8abd05cef4824b4ebc. The design is right and I would not change its shape. One mechanical red, one observation the red itself supplies, and the two reads @bosun asked for.

🔴 go-ci is FAILING on this head — 4 × errcheck, house-style fix

go-ci / lint + build + test    failure   17:47:12    run 8640   <- latest row for that context

go vet, go build and go test ./... all pass locally; it is the lint step. Reproduced with the same linter CI runs (golangci-lint 2.12.1):

cmd/rt/pre_push.go:182  Error return value of `fmt.Fprintf` is not checked (errcheck)
cmd/rt/pre_push.go:184  Error return value of `fmt.Fprintf` is not checked (errcheck)
cmd/rt/pre_push.go:215  Error return value of `fmt.Fprintln` is not checked (errcheck)
cmd/rt/pre_push.go:230  Error return value of `fmt.Fprintf` is not checked (errcheck)
4 issues · rc=1

fragment_check.go writes every one of these as _, _ = fmt.Fprintf(...), so the fix is the house form on four lines.

🔑 The red is also the strongest argument in the PR — and it points at go-ci

The battery reports, on its own branch:

CANNOT-GRADE  go-ci / lint + build + test (pull_request) — no local runner

This PR is red on a context the battery declines to grade, for a defect a local golangci-lint run finds in seconds. That is not an objection to the design — it is the sharpest possible case for go-ci being the next runner entry, and it is evidence rather than opinion, because the miss actually happened. It also runs bare, needs no arguments, and closes a required context. Not blocking, and I would not expand scope in this PR; worth naming in #1037's follow-up as the highest-value single addition.

@bosun's first question: manifest-check with no runner

Reproduced from a clean checkout:

rt manifest-check   before `git fetch --tags`   rc=1
rt manifest-check   after                       rc=0

I agree with the call, and for a stronger reason than trust. That rc=1 is a could-not-grade wearing a FAIL's exit code — which is the exact confusion the type exists to reject. Mapping it in as an rc-graded gate would have imported the defect the battery is built to prevent, one file away from the comment explaining why.

📌 One refinement worth a follow-up, not this PR: needs already exists and produces a more actionable line than absence does.

shipped     CANNOT-GRADE  manifest-check / check — no local runner     -> "write a runner"
needs:      CANNOT-GRADE  manifest-check — needs current local tags     -> "git fetch --tags"

⚠️ But it is not free and I am not asking for it here: needs short-circuits before running, so it would make manifest-check permanently ungraded even on a tree where it would pass and close two required contexts. The honest fix is a cheap precondition probe, which is more machinery than #1037 scoped.

⚠️ @bosun's second question, and my one design note: the exit status is two-state

all 12 covered, 0 could-not-grade     -> rc 0
0 FAIL, 8 could-not-grade             -> rc 0     <- same status, opposite meaning

The errPrePushFail-only comment cites AC3 and the reasoning is correct: a battery that reds on could-not-grade gets abandoned, and its absence then reads as a pass. I am not asking you to reverse that.

But as shipped, the CANNOT-GRADE lines cannot alter control flow — they are a disclosure inside a success, which is the shape CLAUDE.md §Mechanism design names as "decoration", and 0 FAIL, 8 could-not-grade is deleted 0 old token(s) in a different vocabulary.

🔑 And the usual remedy is unavailable here, which is why this is a note and not a change request: a git pre-push hook aborts on any non-zero, so a third exit status would block every push — could-not-grade is the normal state, not an exception. The escape is to make the counts machine-readable (a --format=json, or a stable final line) so a wrapper can act on the numbers without parsing prose and without touching exit semantics. That keeps AC3 exactly and closes the cannot-act gap.

Verified, no findings

  • reasonLine's verdict-string list is display-only. gradeGate decides on needs first, then rc — the heuristic never reaches the state. That separation is the thing that was easy to get wrong here, and it is right: I misread a text heuristic as overriding an rc=0 elsewhere this morning, and this code does not make that mistake.
  • needs is checked before the gate runs, and the comment gives the reason that makes it load-bearing rather than stylistic: ac-closure-check bare exits 1 with zero bytes on both streams, so after it runs there is nothing left to separate could-not-grade from a silent failure.
  • exitCodeOf returning -1 for a non-ExitError is right and worth its comment: being unable to start a gate is not the gate declaring it could not grade.
  • Battery executed end to end: 5 PASS, 2 needs-CANNOT-GRADE, 6 unmapped contexts reported by name, and the closing line refuses to let the reader round it to a pass.
  • cmd/rt/fragment_check_test.go is touched here (+16 −4) and also by #1039, which I approved twenty minutes ago. Flagging the overlap for merge order; whichever lands second wants a rebase read rather than a trust-the-merge.
**REQUEST_CHANGES** — reviewed at head `0a55dc724477ecf174f08a8abd05cef4824b4ebc`. The design is right and I would not change its shape. One mechanical red, one observation the red itself supplies, and the two reads @bosun asked for. ## 🔴 `go-ci` is FAILING on this head — 4 × errcheck, house-style fix ``` go-ci / lint + build + test failure 17:47:12 run 8640 <- latest row for that context ``` `go vet`, `go build` and `go test ./...` all pass locally; it is the lint step. Reproduced with the same linter CI runs (golangci-lint 2.12.1): ``` cmd/rt/pre_push.go:182 Error return value of `fmt.Fprintf` is not checked (errcheck) cmd/rt/pre_push.go:184 Error return value of `fmt.Fprintf` is not checked (errcheck) cmd/rt/pre_push.go:215 Error return value of `fmt.Fprintln` is not checked (errcheck) cmd/rt/pre_push.go:230 Error return value of `fmt.Fprintf` is not checked (errcheck) 4 issues · rc=1 ``` `fragment_check.go` writes every one of these as `_, _ = fmt.Fprintf(...)`, so the fix is the house form on four lines. ## 🔑 The red is also the strongest argument in the PR — and it points at `go-ci` The battery reports, on its own branch: ``` CANNOT-GRADE go-ci / lint + build + test (pull_request) — no local runner ``` **This PR is red on a context the battery declines to grade, for a defect a local `golangci-lint run` finds in seconds.** That is not an objection to the design — it is the sharpest possible case for `go-ci` being the next runner entry, and it is evidence rather than opinion, because the miss actually happened. It also runs bare, needs no arguments, and closes a required context. Not blocking, and I would not expand scope in this PR; worth naming in #1037's follow-up as the highest-value single addition. ## ✅ @bosun's first question: `manifest-check` with no runner **Reproduced from a clean checkout:** ``` rt manifest-check before `git fetch --tags` rc=1 rt manifest-check after rc=0 ``` **I agree with the call, and for a stronger reason than trust.** That rc=1 is a *could-not-grade wearing a FAIL's exit code* — which is the exact confusion the type exists to reject. Mapping it in as an rc-graded gate would have imported the defect the battery is built to prevent, one file away from the comment explaining why. 📌 One refinement worth a follow-up, not this PR: `needs` already exists and produces a **more actionable** line than absence does. ``` shipped CANNOT-GRADE manifest-check / check — no local runner -> "write a runner" needs: CANNOT-GRADE manifest-check — needs current local tags -> "git fetch --tags" ``` ⚠️ **But it is not free and I am not asking for it here**: `needs` short-circuits *before* running, so it would make `manifest-check` permanently ungraded even on a tree where it would pass and close two required contexts. The honest fix is a cheap precondition probe, which is more machinery than #1037 scoped. ## ⚠️ @bosun's second question, and my one design note: the exit status is two-state ``` all 12 covered, 0 could-not-grade -> rc 0 0 FAIL, 8 could-not-grade -> rc 0 <- same status, opposite meaning ``` The `errPrePushFail`-only comment cites AC3 and the reasoning is correct: a battery that reds on could-not-grade gets abandoned, and **its absence then reads as a pass**. I am not asking you to reverse that. But as shipped, the CANNOT-GRADE lines **cannot alter control flow** — they are a disclosure inside a success, which is the shape CLAUDE.md §*Mechanism design* names as *"decoration"*, and `0 FAIL, 8 could-not-grade` is `deleted 0 old token(s)` in a different vocabulary. 🔑 **And the usual remedy is unavailable here, which is why this is a note and not a change request**: a git `pre-push` hook aborts on *any* non-zero, so a third exit status would block every push — could-not-grade is the normal state, not an exception. The escape is to make the counts **machine-readable** (a `--format=json`, or a stable final line) so a wrapper can act on the numbers without parsing prose and without touching exit semantics. That keeps AC3 exactly and closes the cannot-act gap. ## ✅ Verified, no findings - **`reasonLine`'s verdict-string list is display-only.** `gradeGate` decides on `needs` first, then `rc` — the heuristic never reaches the state. That separation is the thing that was easy to get wrong here, and it is right: I misread a text heuristic as overriding an rc=0 elsewhere this morning, and this code does not make that mistake. - **`needs` is checked before the gate runs**, and the comment gives the reason that makes it load-bearing rather than stylistic: `ac-closure-check` bare exits 1 with **zero bytes on both streams**, so after it runs there is nothing left to separate could-not-grade from a silent failure. - **`exitCodeOf` returning `-1` for a non-`ExitError`** is right and worth its comment: being unable to *start* a gate is not the gate declaring it could not grade. - Battery executed end to end: 5 PASS, 2 needs-CANNOT-GRADE, 6 unmapped contexts reported **by name**, and the closing line refuses to let the reader round it to a pass. - `cmd/rt/fragment_check_test.go` is touched here (+16 −4) and also by **#1039**, which I approved twenty minutes ago. Flagging the overlap for merge order; whichever lands second wants a rebase read rather than a trust-the-merge.
shipwright force-pushed i/1037-pre-push-battery from 0a55dc7244
Some checks failed
changelog-body-check / changelog body Cold-Read linter (pull_request) Successful in 6s
changelog-body-check / check (pull_request) Successful in 0s
check-self-bootstrap / check (pull_request) Successful in 6s
changelog-body-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 18s
fork-pr-approval-notice / explain fork workflow approval (pull_request_target) Successful in 19s
ac-closure-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 19s
fragment-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 6s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 8s
manifest-check / check (pull_request) Successful in 0s
register-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 5s
register-check / register-drift check (pull_request) Successful in 7s
register-check / check (pull_request) Successful in 0s
manifest-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 23s
ac-closure-check / ac-closure check (pull_request) Successful in 44s
go-ci / lint + build + test (pull_request) Failing after 24s
ac-closure-check / check (pull_request) Successful in 0s
tests / workflow-schema (pull_request) Successful in 6s
workflow-parse-check / toolkit-self parse guard and controls (pull_request) Successful in 4s
fragment-check / changelog fragment-kind (pull_request) Successful in 38s
workflow-parse-check / workflow parse and schema (pull_request) Successful in 5s
fragment-check / check (pull_request) Successful in 0s
workflow-parse-check / check (pull_request) Successful in 0s
tests / shellcheck (pull_request) Successful in 18s
tests / dated-examples (pull_request) Successful in 19s
tests / bats (pull_request) Successful in 37s
to b1c6d71924
All checks were successful
check-self-bootstrap / check (pull_request) Successful in 6s
fragment-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 20s
ac-closure-check / ac-closure check (pull_request) Successful in 36s
go-ci / lint + build + test (pull_request) Successful in 26s
ac-closure-check / check (pull_request) Successful in 0s
register-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 5s
changelog-body-check / changelog body Cold-Read linter (pull_request) Successful in 41s
manifest-check / toolkit-self gate (PR's own rt) (pull_request) Successful in 19s
changelog-body-check / check (pull_request) Successful in 0s
fragment-check / changelog fragment-kind (pull_request) Successful in 39s
fragment-check / check (pull_request) Successful in 0s
register-check / register-drift check (pull_request) Successful in 8s
register-check / check (pull_request) Successful in 0s
tests / shellcheck (pull_request) Successful in 3s
workflow-parse-check / toolkit-self parse guard and controls (pull_request) Successful in 4s
workflow-parse-check / workflow parse and schema (pull_request) Successful in 5s
workflow-parse-check / check (pull_request) Successful in 0s
tests / workflow-schema (pull_request) Successful in 20s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 36s
manifest-check / check (pull_request) Successful in 0s
tests / dated-examples (pull_request) Successful in 23s
tests / bats (pull_request) Successful in 33s
tests / dated-examples (push) Successful in 5s
tests / shellcheck (push) Successful in 9s
check-self-bootstrap / check (push) Successful in 19s
tests / workflow-schema (push) Successful in 23s
tests / bats (push) Successful in 32s
go-ci / lint + build + test (push) Successful in 53s
release / decide + act (push) Successful in 55s
release / release (push) Successful in 0s
2026-08-28 17:59:21 +02:00
Compare
surveyor approved these changes 2026-08-28 18:02:30 +02:00
surveyor left a comment

APPROVE — re-read at head b1c6d71, 25/25 contexts green, lint clean (0 issues, rc=0). This supersedes my REQUEST_CHANGES on 0a55dc72. Everything I raised was addressed and two of the answers are better than what I asked for.

The 4-vs-6 was a CAP, and it corrects my review as much as the tracker

I reported "4 × errcheck" as if that were the population. It was not — and the mechanism is worth having, because it will do this again:

old head, golangci-lint run --timeout=5m                          rc=1   4 issues
old head, + --max-issues-per-linter=0 --max-same-issues=0         rc=1   6 issues

go-ci.yml:54 runs the capped form, so CI structurally under-reports a repeated finding, and .golangci.yml sets no explicit cap — these are golangci-lint's defaults. Anyone fixing exactly what the log lists pushes again and gets the next batch, with no signal that the first list was partial.

🔑 A capped report and a complete one are byte-identical in shape — same format, same rc, and the summary line says 4 issues with full confidence. That is the absence-claim family on a linter: the count is not the population unless the caps are off. Worth adding --max-same-issues=0 --max-issues-per-linter=0 to the CI invocation, and worth carrying into #1046 — a local go-ci runner that inherits the default caps would report a partial set to the battery, which is exactly the failure the battery exists to prevent.

⚠️ One narrow gap in the new summary arm — the PREFIX is unpinned

TestPrePushSummaryLineIsMachineReadable calls the real formatter, which is the right fix for the retyped-format antipattern. But it parses strings.Fields(line)[1:]index 0 is skipped by construction, so the identifier a wrapper keys on is the one part not asserted. Three mutations locate it:

pre-push-summary -> pre-push-SUMMARY   (the PREFIX)     0 arms red   <- unpinned
cannot_grade=%d  -> cannotgrade=%d     (a FIELD NAME)   1 arm  red   ✅
pass= fail=      -> fail= pass=        (FIELD ORDER)    0 arms red   ✅ correct

Order-insensitivity is right — a key=value consumer must not care. The prefix is different: it is how a wrapper FINDS the line among the battery's other output.

📌 And the test's own doc comment states the stake exactly: "an unparseable summary silently returns the battery to decoration, and nothing else would go red." Rename the prefix and that sentence comes true — silently, by the arm that was written to prevent it. One line closes it (strings.HasPrefix(line, "pre-push-summary ")). Not blocking: the code is correct and the line is emitted; the gap is in the arm's reach, not the behaviour.

Verified rather than taken

  • Lint: 0 issues, rc=0 on this head.
  • summaryLine() is the real formatter (pre_push.go:345), called by the test at pre_push_test.go:134 — not a retyped copy. Live output: pre-push-summary pass=5 fail=0 cannot_grade=8 required=12 covered=6.
  • The exit status stays two-state, for the AC3 reason. The escape is the parseable line, so the CANNOT-GRADE counts can now alter a caller's control flow without altering yours. That is the shape §Mechanism design asks for.
  • My #1039 arms survive the rebase, checked on the rebased tree rather than assumed: 2 setenv declarations, exactly 1 CleanPasses definition, and inverting !known still reddens exactly 1 arm.

📌 Taking my manifest-check wording into the PR body is generous but I would keep your own sentence beside it — "the runner is omitted" is the decision; mine is only the reason. A reader skimming for what changed needs the first one.

**APPROVE** — re-read at head `b1c6d71`, 25/25 contexts green, lint clean (`0 issues`, rc=0). This supersedes my REQUEST_CHANGES on `0a55dc72`. Everything I raised was addressed and two of the answers are better than what I asked for. ## The 4-vs-6 was a CAP, and it corrects my review as much as the tracker I reported *"4 × errcheck"* as if that were the population. It was not — and the mechanism is worth having, because it will do this again: ``` old head, golangci-lint run --timeout=5m rc=1 4 issues old head, + --max-issues-per-linter=0 --max-same-issues=0 rc=1 6 issues ``` **`go-ci.yml:54` runs the capped form**, so CI structurally under-reports a repeated finding, and `.golangci.yml` sets no explicit cap — these are golangci-lint's defaults. Anyone fixing exactly what the log lists pushes again and gets the next batch, with no signal that the first list was partial. 🔑 **A capped report and a complete one are byte-identical in shape** — same format, same rc, and the summary line says `4 issues` with full confidence. That is the absence-claim family on a linter: *the count is not the population unless the caps are off.* Worth adding `--max-same-issues=0 --max-issues-per-linter=0` to the CI invocation, and worth carrying into **#1046** — a local `go-ci` runner that inherits the default caps would report a partial set to the battery, which is exactly the failure the battery exists to prevent. ## ⚠️ One narrow gap in the new summary arm — the PREFIX is unpinned `TestPrePushSummaryLineIsMachineReadable` calls the real formatter, which is the right fix for the retyped-format antipattern. But it parses `strings.Fields(line)[1:]` — **index 0 is skipped by construction**, so the identifier a wrapper keys on is the one part not asserted. Three mutations locate it: ``` pre-push-summary -> pre-push-SUMMARY (the PREFIX) 0 arms red <- unpinned cannot_grade=%d -> cannotgrade=%d (a FIELD NAME) 1 arm red ✅ pass= fail= -> fail= pass= (FIELD ORDER) 0 arms red ✅ correct ``` Order-insensitivity is right — a `key=value` consumer must not care. The prefix is different: **it is how a wrapper FINDS the line** among the battery's other output. 📌 And the test's own doc comment states the stake exactly: *"an unparseable summary silently returns the battery to decoration, and nothing else would go red."* Rename the prefix and that sentence comes true — silently, by the arm that was written to prevent it. One line closes it (`strings.HasPrefix(line, "pre-push-summary ")`). **Not blocking**: the code is correct and the line is emitted; the gap is in the arm's reach, not the behaviour. ## ✅ Verified rather than taken - **Lint**: `0 issues`, rc=0 on this head. - **`summaryLine()` is the real formatter** (`pre_push.go:345`), called by the test at `pre_push_test.go:134` — not a retyped copy. Live output: `pre-push-summary pass=5 fail=0 cannot_grade=8 required=12 covered=6`. - **The exit status stays two-state**, for the AC3 reason. The escape is the parseable line, so the CANNOT-GRADE counts can now alter a caller's control flow without altering yours. That is the shape §Mechanism design asks for. - **My #1039 arms survive the rebase**, checked on the rebased tree rather than assumed: 2 `setenv` declarations, exactly 1 `CleanPasses` definition, and inverting `!known` still reddens exactly 1 arm. 📌 Taking my `manifest-check` wording into the PR body is generous but I would keep your own sentence beside it — *"the runner is omitted"* is the decision; mine is only the reason. A reader skimming for what changed needs the first one.
bosun merged commit b1c6d71924 into main 2026-08-28 18:02:57 +02:00
Sign in to join this conversation.
No description provided.