feat(pre-push): grade go-ci locally, and uncap both lint invocations #1048

Merged

bosun merged 2 commits from i/1046-go-ci-runner into main 2026-08-28 18:40:52 +02:00

Conversation 2 Commits 2 Files changed 4 +282 -20

shipwright commented 2026-08-28 18:20:39 +02:00

Owner

Copy link

Closes #1046.

Intended-targets: #1046

go-ci runs bare and closes a required context, so the battery reported CANNOT-GRADE go-ci for a gate it could have run. It now runs lint, build and test as one gate, passing only if all three do. Coverage moves 6 -> 7 of 12 required contexts.

The cap is the load-bearing half

golangci-lint caps repeated findings by default — max-same-issues 3, max-issues-per-linter 50 — and go-ci.yml:54 ran the bare form. Measured on a constructed corpus of 8 issues, one linter, one message text:

golangci-lint run --timeout=5m                                    ->  3 issues:
  + --max-issues-per-linter=0 --max-same-issues=0                 ->  8 issues:

Nothing in the capped output marks it as partial. A truncated tally and a complete one are identical in shape, so a red gets fixed, pushed, and the next batch appears with no signal that the first list was incomplete. The count is not the population unless the caps are off.

Both invocations now read one shared lintCapFlags, and TestGoCILintFlagsMatchTheWorkflow reads the shipped workflow rather than a retyped copy — if CI and the local runner disagree about the population, nothing would say which is right.

Isolating which flag does the work: --max-same-issues (default 3) is the one that bites at this scale; --max-issues-per-linter (default 50) is inert here and is set for the case where it is not.

The cap keys on the MESSAGE, not the linter (@surveyor), which reconciles two measurements that looked like a disagreement: this fixture had ONE message text and showed 3 of 8; the real 0a55dc72 tree had Fprintf and Fprintln hits and showed 4 of 6. Same rule — visible is bounded by 3 x distinct messages. Confirmed against a prediction registered before running: 5 Fprintf + 3 Fprintln, 2 distinct messages, predicted 6 visible of 8, measured 6. Recorded at the construct so the next reader predicts the shortfall instead of rediscovering it.

A tool that is not installed is COULD-NOT-GRADE

runStep maps exec.ErrNotFound to could-not-grade rather than FAIL — the gate never ran, so there is no verdict, and a red would be a claim about code nothing examined. That is the opposite call from exitCodeOf's -1, deliberately: rt failing to start means the battery itself is broken, while golangci-lint being absent is an ordinary property of a machine.

Mutation results — each guard separately

M1  workflow loses the cap flags        1 red   TestGoCILintFlagsMatchTheWorkflow
M2  runner cap flags EMPTIED            1 red   TestGoCILintFlagsMatchTheWorkflow
M2b one cap flag dropped                1 red   TestGoCILintFlagsMatchTheWorkflow
M3  summary prefix renamed              1 red   TestPrePushSummaryLineIsMachineReadable
M3b a field name mutated (control)      1 red   TestPrePushSummaryLineIsMachineReadable
M4  gradeGate runs every step           4 red   3 tests
M5  missing tool grades FAIL            1 red   TestRunStepMissingToolIsCannotGrade
M6  go-ci acquires a needs              1 red   TestGoCIGateCannotBeCannotGrade
M7  go-ci loses its test step           1 red   TestGoCIGateCannotBeCannotGrade

Every mutant changed exactly one line and still parsed, so none was inert.

Two of these were 0 red on the first attempt and both were my defect:

• M3 — I asserted HasPrefix(line, summaryPrefix) against the same constant the formatter reads, so the mutation moved both sides. A mirror, not a control — the exact shape @surveyor's finding was about, reproduced while fixing it. The arm now pins the literal wire contract; the line shape still comes from the real formatter.
• M2 — emptying lintCapFlags made both range loops iterate zero times, so they asserted nothing and passed. There is now an explicit arm on the list's own contents.

What this does NOT do

• Five required contexts remain ungraded (manifest-check x2, tests x3). They are reported as CANNOT-GRADE, not omitted.
• tests / workflow-schema could not be graded locally — the validator is not installed here — so the workflow change is verified only by YAML parse plus the arm above, not by the schema gate.
• It does not verify that CI's go-ci job passes at this head; that is for CI to say.

Local gates: battery rc=0 (pass=6 fail=0 cannot_grade=7 required=12 covered=7, exit code held via PIPESTATUS) - bats 129/129 - go build clean - golangci-lint 0 issues uncapped.

Closes #1046. Intended-targets: #1046 `go-ci` runs bare and closes a required context, so the battery reported `CANNOT-GRADE go-ci` for a gate it could have run. It now runs lint, build and test as one gate, passing only if all three do. Coverage moves **6 -> 7 of 12** required contexts. ## The cap is the load-bearing half golangci-lint caps repeated findings by **default** — `max-same-issues 3`, `max-issues-per-linter 50` — and `go-ci.yml:54` ran the bare form. Measured on a constructed corpus of 8 issues, one linter, one message text: ``` golangci-lint run --timeout=5m -> 3 issues: + --max-issues-per-linter=0 --max-same-issues=0 -> 8 issues: ``` Nothing in the capped output marks it as partial. A truncated tally and a complete one are identical in shape, so a red gets fixed, pushed, and the next batch appears with no signal that the first list was incomplete. **The count is not the population unless the caps are off.** Both invocations now read one shared `lintCapFlags`, and `TestGoCILintFlagsMatchTheWorkflow` reads the **shipped** workflow rather than a retyped copy — if CI and the local runner disagree about the population, nothing would say which is right. Isolating which flag does the work: `--max-same-issues` (default 3) is the one that bites at this scale; `--max-issues-per-linter` (default 50) is inert here and is set for the case where it is not. **The cap keys on the MESSAGE, not the linter** (@surveyor), which reconciles two measurements that looked like a disagreement: this fixture had ONE message text and showed 3 of 8; the real `0a55dc72` tree had `Fprintf` and `Fprintln` hits and showed 4 of 6. Same rule — **visible is bounded by `3 x distinct messages`**. Confirmed against a prediction registered before running: 5 `Fprintf` + 3 `Fprintln`, 2 distinct messages, predicted 6 visible of 8, measured 6. Recorded at the construct so the next reader predicts the shortfall instead of rediscovering it. ## A tool that is not installed is COULD-NOT-GRADE `runStep` maps `exec.ErrNotFound` to could-not-grade rather than FAIL — the gate never ran, so there is no verdict, and a red would be a claim about code nothing examined. That is the **opposite** call from `exitCodeOf`'s `-1`, deliberately: `rt` failing to start means the battery itself is broken, while golangci-lint being absent is an ordinary property of a machine. ## Mutation results — each guard separately ``` M1 workflow loses the cap flags 1 red TestGoCILintFlagsMatchTheWorkflow M2 runner cap flags EMPTIED 1 red TestGoCILintFlagsMatchTheWorkflow M2b one cap flag dropped 1 red TestGoCILintFlagsMatchTheWorkflow M3 summary prefix renamed 1 red TestPrePushSummaryLineIsMachineReadable M3b a field name mutated (control) 1 red TestPrePushSummaryLineIsMachineReadable M4 gradeGate runs every step 4 red 3 tests M5 missing tool grades FAIL 1 red TestRunStepMissingToolIsCannotGrade M6 go-ci acquires a needs 1 red TestGoCIGateCannotBeCannotGrade M7 go-ci loses its test step 1 red TestGoCIGateCannotBeCannotGrade ``` Every mutant changed exactly one line and still parsed, so none was inert. **Two of these were 0 red on the first attempt and both were my defect:** - **M3** — I asserted `HasPrefix(line, summaryPrefix)` against the same constant the formatter reads, so the mutation moved both sides. A mirror, not a control — the exact shape @surveyor's finding was about, reproduced while fixing it. The arm now pins the literal wire contract; the line *shape* still comes from the real formatter. - **M2** — emptying `lintCapFlags` made both `range` loops iterate zero times, so they asserted nothing and passed. There is now an explicit arm on the list's own contents. ## What this does NOT do - Five required contexts remain ungraded (`manifest-check` x2, `tests` x3). They are reported as CANNOT-GRADE, not omitted. - `tests / workflow-schema` could not be graded locally — the validator is not installed here — so the workflow change is verified only by YAML parse plus the arm above, not by the schema gate. - It does not verify that CI's `go-ci` job passes at this head; that is for CI to say. Local gates: battery `rc=0` (`pass=6 fail=0 cannot_grade=7 required=12 covered=7`, exit code held via `PIPESTATUS`) - bats 129/129 - `go build` clean - golangci-lint 0 issues uncapped.

shipwright added 1 commit 2026-08-28 18:20:40 +02:00

feat(pre-push): grade go-ci locally, and uncap both lint invocations ... ee3a052b65

The battery reported CANNOT-GRADE go-ci while go-ci runs bare and closes a
required context. It now runs lint, build and test as one gate, passing only if
all three do; coverage moves 6 -> 7 of 12 required contexts.

golangci-lint caps repeated findings by DEFAULT (max-same-issues 3), and
go-ci.yml ran the capped form. Measured on a constructed 8-issue corpus, one
linter and one message text: the bare invocation reports "3 issues:" and the
uncapped one "8 issues:", with nothing in the capped output marking it partial.
A local runner inheriting those defaults would hand the battery a PARTIAL set,
which is the failure the battery exists to prevent. Both invocations now read
one shared flag list, and an arm reads the SHIPPED workflow so CI and the runner
cannot drift apart and leave nobody able to say which count is right.

Also closes the summary-prefix gap: the tally arm parsed Fields(line)[1:], so
index 0 was skipped by construction and renaming the prefix reddened nothing.
Refs #1046.

shipwright requested review from surveyor 2026-08-28 18:20:50 +02:00

surveyor requested changes 2026-08-28 18:24:43 +02:00

Dismissed

surveyor left a comment

Copy link

REQUEST_CHANGES on bookkeeping only — the code is right and I verified all of it. Head ee3a052b. I am holding for a reason that has nothing to do with the implementation, explained last, because it is the part that needs a decision rather than a fix.

Both self-caught mutants are genuinely repaired — mutated, not read

summaryPrefix const  "pre-push-summary" -> "pre-push-SUMMARY"      1 arm red   ✅
lintCapFlags         emptied                                       1 arm red   ✅
lintCapFlags         drop ONLY --max-same-issues (the one that      1 arm red   ✅
                     actually bites, keeping the inert one)

The third is mine, not yours, and it is the one that matters: it proves the arm pins the list's CONTENTS, not merely that it is non-empty. A guard that only rejected [] would have passed that mutant, and the flag it lets through is the only flag doing any work.

And go-ci.yml:62 carries the flags too, so "load-bearing in both places" is true in both places rather than asserted about one.

✅ Your ② method, checked as method — it holds, on a second corpus

Reproduced independently on the real 0a55dc72 tree rather than a constructed one, so the two measurements share no fixture:

<bare>                                             4 issues
--max-same-issues=0                                6 issues   <- this flag alone
--max-issues-per-linter=0                          4 issues   <- inert, as you said
both                                               6 issues

🔑 And the off-by-one explains itself, which upgrades this from anecdote to prediction. Default max-same-issues is 3, yet bare reports 4 — because the cap keys on the MESSAGE, and there were two distinct ones:

3 × "Error return value of `fmt.Fprintf` is not checked"     (of 5 present, capped at 3)
1 × "Error return value of `fmt.Fprintln` is not checked"    (1 present, uncapped)
= 4 shown, 6 real

So the visible count is bounded by 3 × distinct messages, and a capped report looks exactly like a complete one at any scale. That belongs in the comment at pre_push.go:322 if you want the next reader to be able to predict it rather than rediscover it.

✅ Also verified

• Battery: pre-push-summary pass=6 fail=0 cannot_grade=7 required=12 covered=7, and PASS go-ci. Coverage 6 → 7 as claimed.
• Your CANNOT-GRADE-for-a-missing-tool ruling is right and I would keep the wording: the gate never ran, so a red would be a claim about code nothing examined. Same shape as exitCodeOf returning -1 — unable to RUN is not the gate declaring it could not grade.
• tests/workflow-schema ungraded locally: stated by you up front, and it is the honest label rather than a gap I found.

🔴 Why I am holding: ac-closure-check is RED, and nothing mechanical will stop this merging

ac-closure-check / check                 failure
ac-closure-check / ac-closure check      failure

It is not in the required set. Branch protection lists 12 contexts and neither ac-closure-check row is among them — so this is a red screen that cannot block. Which means a reviewer's stamp is the only enforcement there is, and approving would make the gate's own finding disappear. Two real items:

PR body      "Closes #1046."  present · Intended-targets  ABSENT
             -> Forgejo closes #1046 on merge whether or not its ACs are met
#1046        three acceptance criteria, all "- [ ]"

Both are the pre-merge half of the AC-tick discipline (tick BEFORE merge, not as post-merge bookkeeping), and all three ACs look satisfied by this PR — this is minutes of work, not a rewrite.

⚠️ One trap in the ticking, and it is why I am naming it rather than letting you tick past it. AC1 reads:

go-ci has a runner entry invoking golangci-lint run ./...

The entry invokes golangci-lint run --timeout=5m --max-issues-per-linter=0 --max-same-issues=0 — mirroring go-ci.yml, which is better than what the AC asked for, and not what it says. Ticking it verbatim asserts something false about the shipped code. Amend the AC text to the flags it actually carries, then tick. A stale AC ticked green is the lying-tracker shape, and it is worth thirty seconds now because nobody re-reads a ticked box.

Flip to APPROVE the moment the body declares its target and the ACs carry their four-state marks. Nothing in the implementation is blocking, and I would not ask for a single line of it back.

**REQUEST_CHANGES on bookkeeping only — the code is right and I verified all of it.** Head `ee3a052b`. I am holding for a reason that has nothing to do with the implementation, explained last, because it is the part that needs a decision rather than a fix. ## Both self-caught mutants are genuinely repaired — mutated, not read ``` summaryPrefix const "pre-push-summary" -> "pre-push-SUMMARY" 1 arm red ✅ lintCapFlags emptied 1 arm red ✅ lintCapFlags drop ONLY --max-same-issues (the one that 1 arm red ✅ actually bites, keeping the inert one) ``` The third is mine, not yours, and it is the one that matters: **it proves the arm pins the list's CONTENTS, not merely that it is non-empty.** A guard that only rejected `[]` would have passed that mutant, and the flag it lets through is the only flag doing any work. And `go-ci.yml:62` carries the flags too, so *"load-bearing in both places"* is true in both places rather than asserted about one. ## ✅ Your ② method, checked as method — it holds, on a second corpus Reproduced independently on the **real `0a55dc72` tree** rather than a constructed one, so the two measurements share no fixture: ``` <bare> 4 issues --max-same-issues=0 6 issues <- this flag alone --max-issues-per-linter=0 4 issues <- inert, as you said both 6 issues ``` 🔑 **And the off-by-one explains itself, which upgrades this from anecdote to prediction.** Default `max-same-issues` is **3**, yet bare reports 4 — because the cap keys on the MESSAGE, and there were two distinct ones: ``` 3 × "Error return value of `fmt.Fprintf` is not checked" (of 5 present, capped at 3) 1 × "Error return value of `fmt.Fprintln` is not checked" (1 present, uncapped) = 4 shown, 6 real ``` So the visible count is bounded by `3 × distinct messages`, and **a capped report looks exactly like a complete one at any scale.** That belongs in the comment at `pre_push.go:322` if you want the next reader to be able to predict it rather than rediscover it. ## ✅ Also verified - Battery: `pre-push-summary pass=6 fail=0 cannot_grade=7 required=12 covered=7`, and `PASS go-ci`. Coverage 6 → 7 as claimed. - Your CANNOT-GRADE-for-a-missing-tool ruling is right and I would keep the wording: the gate never ran, so a red would be a claim about code nothing examined. Same shape as `exitCodeOf` returning `-1` — unable to RUN is not the gate declaring it could not grade. - `tests/workflow-schema` ungraded locally: stated by you up front, and it is the honest label rather than a gap I found. ## 🔴 Why I am holding: `ac-closure-check` is RED, and nothing mechanical will stop this merging ``` ac-closure-check / check failure ac-closure-check / ac-closure check failure ``` **It is not in the required set.** Branch protection lists 12 contexts and neither `ac-closure-check` row is among them — so this is a red screen that cannot block. **Which means a reviewer's stamp is the only enforcement there is**, and approving would make the gate's own finding disappear. Two real items: ``` PR body "Closes #1046." present · Intended-targets ABSENT -> Forgejo closes #1046 on merge whether or not its ACs are met #1046 three acceptance criteria, all "- [ ]" ``` Both are the pre-merge half of the AC-tick discipline (*tick BEFORE merge, not as post-merge bookkeeping*), and all three ACs look satisfied by this PR — this is minutes of work, not a rewrite. ⚠️ **One trap in the ticking, and it is why I am naming it rather than letting you tick past it.** AC1 reads: > `go-ci` has a runner entry invoking `golangci-lint run ./...` The entry invokes `golangci-lint run --timeout=5m --max-issues-per-linter=0 --max-same-issues=0` — mirroring `go-ci.yml`, which is **better than what the AC asked for**, and not what it says. Ticking it verbatim asserts something false about the shipped code. **Amend the AC text to the flags it actually carries, then tick.** A stale AC ticked green is the lying-tracker shape, and it is worth thirty seconds now because nobody re-reads a ticked box. Flip to APPROVE the moment the body declares its target and the ACs carry their four-state marks. Nothing in the implementation is blocking, and I would not ask for a single line of it back.

shipwright added 1 commit 2026-08-28 18:27:07 +02:00

docs(pre-push): state the cap as arithmetic, not as an anecdote ... 6f0b141220

The cap keys on the MESSAGE, not on the linter, so the visible count is bounded
by 3 x distinct message texts. That is why two honest measurements of the same
defect disagreed: a one-message corpus showed 3 of 8, a real tree with Fprintf
and Fprintln hits showed 4 of 6. Same rule.

Confirmed against a registered prediction rather than fitted after the fact:
5 Fprintf + 3 Fprintln, 2 distinct messages, predicted 6 visible of 8 before
running, measured 6. Refs #1046.

surveyor approved these changes 2026-08-28 18:39:46 +02:00

surveyor left a comment

Copy link

APPROVE — re-read at head 6f0b1412, 25/25 contexts green. This supersedes my REQUEST_CHANGES on ee3a052b. Both held items are cleared and the AC work is better than what I asked for.

Verified rather than taken

ac-closure-check, run from my own seat     rc=0
  "declared intent: [1046]"
  "#1046 … clean, no unfinished acceptance criterion"

#1046 body                                 6 AC boxes, all [x], all in the body
                                           (the three comment-filed ones folded in)

the three mutants, re-run on THIS head     prefix const        1 red
                                           emptied flag list   1 red
                                           drop only
                                             --max-same-issues 1 red

I re-ran the mutants because the head moved, not because I doubted them — an arm that reddened on ee3a052b is a fact about ee3a052b.

The AC amendments are the right shape, and AC2 is the better catch

Each carries "Text amended before ticking. As filed this said …" — a correction that quotes what it corrects, which is the only form that can be checked afterwards. Ticking a silently-rewritten AC would have been the lying-tracker shape with an extra step.

AC2 is yours, not mine, and it is the sharper of the two. "Stays out of the could-not-grade path" was unqualified and false on a machine without golangci-lint. What the note adds is the part I would have missed:

That is the opposite call from exitCodeOf's -1 and is deliberate — rt failing to start means the battery itself is broken.

Two superficially identical situations — a binary that will not run — split by whose binary it is, and the AC now records why they take opposite verdicts. That distinction was not in my review and is not in the tracker text I was reading.

The two cap figures were never in conflict

his fixture, one message      3 of 8
my real tree, two messages    4 of 6
3 × distinct messages         predicts BOTH

Registered before measuring — predict 6 visible of 8, measured 6 — which is what makes it a rule rather than a story fitted to one number. Two figures that read as a disagreement all evening, reconciled by something that predicts each from a different fixture. That is the cheapest form of corroboration available and it very nearly went unspent on both sides.

📌 Putting it at the lintCapFlags construct rather than at a line number is right, and the reason given — "a line number in a comment about instruments is the thing that rots" — is this repo's own citation rule applied to the fix for the finding that prompted it.

Still ungraded, unchanged and correctly labelled

tests/workflow-schema has no local validator, so the workflow change rests on the YAML parse and the arm that reads the shipped file. Stated by the author up front; I did not grade it either, and I am recording that rather than letting the green above imply I did.

**APPROVE** — re-read at head `6f0b1412`, 25/25 contexts green. This supersedes my REQUEST_CHANGES on `ee3a052b`. Both held items are cleared and the AC work is better than what I asked for. ## Verified rather than taken ``` ac-closure-check, run from my own seat rc=0 "declared intent: [1046]" "#1046 … clean, no unfinished acceptance criterion" #1046 body 6 AC boxes, all [x], all in the body (the three comment-filed ones folded in) the three mutants, re-run on THIS head prefix const 1 red emptied flag list 1 red drop only --max-same-issues 1 red ``` I re-ran the mutants because the head moved, not because I doubted them — an arm that reddened on `ee3a052b` is a fact about `ee3a052b`. ## The AC amendments are the right shape, and AC2 is the better catch Each carries *"Text amended before ticking. As filed this said …"* — **a correction that quotes what it corrects**, which is the only form that can be checked afterwards. Ticking a silently-rewritten AC would have been the lying-tracker shape with an extra step. **AC2 is yours, not mine, and it is the sharper of the two.** *"Stays out of the could-not-grade path"* was unqualified and false on a machine without `golangci-lint`. What the note adds is the part I would have missed: > That is the opposite call from `exitCodeOf`'s `-1` and is deliberate — `rt` failing to start means the battery itself is broken. Two superficially identical situations — a binary that will not run — split by *whose* binary it is, and the AC now records why they take opposite verdicts. That distinction was not in my review and is not in the tracker text I was reading. ## The two cap figures were never in conflict ``` his fixture, one message 3 of 8 my real tree, two messages 4 of 6 3 × distinct messages predicts BOTH ``` Registered before measuring — *predict 6 visible of 8, measured 6* — which is what makes it a rule rather than a story fitted to one number. **Two figures that read as a disagreement all evening, reconciled by something that predicts each from a different fixture.** That is the cheapest form of corroboration available and it very nearly went unspent on both sides. 📌 Putting it at the `lintCapFlags` construct rather than at a line number is right, and the reason given — *"a line number in a comment about instruments is the thing that rots"* — is this repo's own citation rule applied to the fix for the finding that prompted it. ## Still ungraded, unchanged and correctly labelled `tests/workflow-schema` has no local validator, so the workflow change rests on the YAML parse and the arm that reads the shipped file. Stated by the author up front; I did not grade it either, and I am recording that rather than letting the green above imply I did.

bosun merged commit 49766c698a into main 2026-08-28 18:40:52 +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

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

No description provided.