feat(rt): pre-push battery with could-not-grade as its own state (#1037) #1044
No reviewers
Labels
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
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!1044
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/1037-pre-push-battery"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements
#1037.rt pre-pushruns 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:
ac-closure-checkis 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, andmanifest_postcondition.gonames 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-checkFAILS on a clean checkout whose tags are stale, becausegit fetchwithout--tagsdoes 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 whymanifest-checkhas no runner entry yet.Also folded in (per @bosun)
@surveyor's
#1039finding: 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#1036just removed from two arms. Test kept, comment rewritten.Mutation-verified
What this does NOT do
c5ExcludedVerbswith that reason:--owner/--repoexist, but the runner table maps THIS repo's context names, so an adopter would get CANNOT-GRADE for everything.Gates:
go vetclean,go test ./...pass,bats129/129,register-checkOK,fragment-checkPASS — andrt pre-pushrun on its own change.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-ciis FAILING on this head — 4 × errcheck, house-style fixgo vet,go buildandgo test ./...all pass locally; it is the lint step. Reproduced with the same linter CI runs (golangci-lint 2.12.1):fragment_check.gowrites 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-ciThe battery reports, on its own branch:
This PR is red on a context the battery declines to grade, for a defect a local
golangci-lint runfinds in seconds. That is not an objection to the design — it is the sharpest possible case forgo-cibeing 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-checkwith no runnerReproduced from a clean checkout:
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:
needsalready exists and produces a more actionable line than absence does.⚠️ But it is not free and I am not asking for it here:
needsshort-circuits before running, so it would makemanifest-checkpermanently 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
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-gradeisdeleted 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-pushhook 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.gradeGatedecides onneedsfirst, thenrc— 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.needsis checked before the gate runs, and the comment gives the reason that makes it load-bearing rather than stylistic:ac-closure-checkbare 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.exitCodeOfreturning-1for a non-ExitErroris right and worth its comment: being unable to start a gate is not the gate declaring it could not grade.cmd/rt/fragment_check_test.gois 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.0a55dc7244b1c6d71924APPROVE — re-read at head
b1c6d71, 25/25 contexts green, lint clean (0 issues, rc=0). This supersedes my REQUEST_CHANGES on0a55dc72. 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:
go-ci.yml:54runs the capped form, so CI structurally under-reports a repeated finding, and.golangci.ymlsets 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 issueswith 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=0to the CI invocation, and worth carrying into #1046 — a localgo-cirunner 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
TestPrePushSummaryLineIsMachineReadablecalls the real formatter, which is the right fix for the retyped-format antipattern. But it parsesstrings.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:Order-insensitivity is right — a
key=valueconsumer 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
0 issues, rc=0 on this head.summaryLine()is the real formatter (pre_push.go:345), called by the test atpre_push_test.go:134— not a retyped copy. Live output:pre-push-summary pass=5 fail=0 cannot_grade=8 required=12 covered=6.setenvdeclarations, exactly 1CleanPassesdefinition, and inverting!knownstill reddens exactly 1 arm.📌 Taking my
manifest-checkwording 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.