From 0b8d0efe2f7c192cdf1abb4154e8a66897369427 Mon Sep 17 00:00:00 2001 From: E Ismail Date: Mon, 5 Oct 2026 14:06:34 +0300 Subject: [PATCH 1/5] ci: the linter runs on every leg, and lint refuses to skip there No leg of the matrix had golangci-lint, so the lint target printed its skip line inside every make ci step and lint ran nowhere. Nothing said so except that line. The workflow now installs the linter on every leg before make ci, at an exact version, through the Go toolchain: the install is checked against the checksum database, and the linter is built by the same Go the leg tests with. The step prints the version it installed. On CI the target treats an absent linter as a failure that names it, told apart by the same CI variable the branch guard reads. On a machine without the linter it still skips, with the skip line unchanged word for word, because that line is what a CI log is searched for. With the linter present it prints every finding: its caps per linter and per message, and its rule of one finding per line, are turned off. With the linter present, lint fails ci on any finding, which is what this target has always done when the linter was there. No row in the suite holds the CI check; it was exercised both ways, and with the check disabled, by running the target. --- .github/workflows/ci.yml | 31 +++++++++++++++++++++++++++++++ Makefile | 34 +++++++++++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 073ee44..594d0d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,6 +124,37 @@ jobs: shell: pwsh run: choco install make --version=4.4.1 -y --no-progress + # golangci-lint, AT AN EXACT VERSION, on every leg, because the lint + # target is a step of make ci and make ci is what every leg runs. + # Until this step existed no leg had the linter: the target's skip + # line was printed inside every make ci step, and lint ran nowhere. + # The target now refuses that skip on CI, so a leg this step does not + # reach fails at lint rather than passing it. + # + # THROUGH THE GO TOOLCHAIN rather than a release download. An install + # at an exact module version is checked against the checksum database + # this job's Go is configured with, so the version below pins the + # content and not only the name. The linter is then built by the same + # Go this leg tests with, and that matters in one direction: a linter + # built by an older Go cannot type-check a newer one's standard + # library, and fails on files nobody touched. What it costs is a build + # of the linter on every run of every leg. + # + # THE VERSION MOVES BY HAND. The scheduled update policy bumps action + # references and cannot see a version written in a line of shell. + # + # THE VERSION IT PRINTS is the record of which linter the leg ran: the + # lint target runs whichever golangci-lint is first on PATH, and this + # line says which one that was. Built without cgo, as everything this + # module builds is, so no leg's C toolchain takes part. + - name: install golangci-lint + shell: bash + env: + CGO_ENABLED: "0" + run: | + go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.14.0 + golangci-lint --version + - name: make ci shell: bash run: make ci diff --git a/Makefile b/Makefile index 88b9af2..d2c3634 100644 --- a/Makefile +++ b/Makefile @@ -223,11 +223,39 @@ fmt: exit 1; \ fi -# lint is a no-op until a linter is actually configured for this repo — -# it must never fail ci for the reason "no linter is installed". +# lint runs golangci-lint over the whole module. +# +# ON CI IT IS INSTALLED, SO AN ABSENT LINTER IS A FAILURE. This comment +# used to say the opposite: that lint must never fail ci for the reason +# "no linter is installed". While that held, no workflow installed one, +# every leg printed the skip line below inside its make ci step, and lint +# ran nowhere with nothing to say so but that line. The workflow now +# installs the linter at an exact version on every leg before make ci, so +# on CI there is nothing to tolerate: a skip there would be this target +# reporting that it ran when it did not. CI is told apart the way +# guard-a-branch-to-work-on tells it apart, by a CI variable that is set +# and not empty. +# +# ON A MACHINE WITHOUT IT, it still skips, and says so. The linter is not +# a build dependency of this module, and a contributor has no reason to +# hold it. The skip line is kept word for word because it is what a CI log +# is searched for to show the linter did not run, and a reworded skip +# would make that search find nothing whether or not it ran. +# +# EVERY FINDING IS PRINTED. By default the linter caps how many findings +# it prints per linter and how many share one message, and keeps one +# finding per line, so a new finding can land inside a group it has +# already collapsed and the count it prints does not move. A report that +# drops findings cannot be read as a list of them; these flags turn all +# three reductions off. lint: @if command -v golangci-lint >/dev/null 2>&1; then \ - golangci-lint run ./...; \ + golangci-lint run --max-issues-per-linter=0 --max-same-issues=0 --uniq-by-line=false ./...; \ + elif [ -n "$$CI" ]; then \ + echo "golangci-lint is not installed, and on CI that is a failure rather than a skip."; \ + echo "The workflow installs it at an exact version before make ci runs, so if it"; \ + echo "is missing here, that step did not run or did not put it on PATH."; \ + exit 1; \ else \ echo "golangci-lint not installed; skipping lint"; \ fi From f90ed5028503611deb3a0141f6d4ce1226d40c5f Mon Sep 17 00:00:00 2001 From: E Ismail Date: Mon, 5 Oct 2026 18:46:26 +0300 Subject: [PATCH 2/5] tests: four dead declarations go, and a guard reads its package file by file The linter's dead-code check and its correctness checks found five things worth changing in this tree, and these are they. A guard that holds every git invocation to a deadline read its own package with parser.ParseDir, which is deprecated. It now lists the directory and parses each non-test file, so every file is still read whatever its build constraint. The replacement the deprecation names, a package loader, would see only the host's build and would add a dependency. The guard still reds on a second caller of the undeadlined call, including one in a file only another platform builds. Four declarations nothing used are gone: a citation-pattern loader the citation guard stopped calling when it moved to the shared engine, an ordering helper whose only call was replaced by a structured assertion, a wrapper every caller went around (its one sentence of contract now opens the doc of the function they call), and a struct field nothing read or wrote. --- internal/guard/failurecontract_test.go | 16 +++------ internal/guard/guard_test.go | 34 ------------------ internal/pack/limits_test.go | 17 --------- tools/leakscan/spawnbound_test.go | 50 ++++++++++++++++---------- 4 files changed, 36 insertions(+), 81 deletions(-) diff --git a/internal/guard/failurecontract_test.go b/internal/guard/failurecontract_test.go index 287f48d..721f0e8 100644 --- a/internal/guard/failurecontract_test.go +++ b/internal/guard/failurecontract_test.go @@ -88,7 +88,6 @@ type obligation struct { where string field string expr string - chain string values []string // resolved possibilities; empty means unresolved problem string // a structurally forbidden write or address-taking binding string // reaching definitions for an obligation-ledger expression @@ -1251,16 +1250,11 @@ func TestBuildTargetsKeepTheirOwnTypeInfoAndIncludeArm64(t *testing.T) { } } -// resolve reduces one argument expression to the finite set of values it -// can carry, or to nothing at all — which is UNRESOLVED and fails. -func resolve(site, field string, e ast.Expr, text func(parsedFile, ast.Node) string, - p parsedFile, kind string) obligation { - return resolveIn(site, field, e, text, p, kind, nil, 0) -} - -// resolveIn carries the enclosing function, so a local name can be traced -// to the statement that defined it, and a depth, so a cycle stops rather -// than recursing forever. RECURSION FAILS CLOSED: past the bound the +// resolveIn reduces one argument expression to the finite set of values it +// can carry, or to nothing at all — which is UNRESOLVED and fails. It +// carries the enclosing function, so a local name can be traced to the +// statement that defined it, and a depth, so a cycle stops rather than +// recursing forever. RECURSION FAILS CLOSED: past the bound the // obligation is unresolved, which is a visible failure. func resolveIn(site, field string, e ast.Expr, text func(parsedFile, ast.Node) string, p parsedFile, kind string, within *ast.FuncDecl, depth int) obligation { diff --git a/internal/guard/guard_test.go b/internal/guard/guard_test.go index 3fcda5a..296e350 100644 --- a/internal/guard/guard_test.go +++ b/internal/guard/guard_test.go @@ -846,40 +846,6 @@ func TestNamedURLConstantsStayUnderTheCeiling(t *testing.T) { // Guard 3: no private citation in a source comment. // --------------------------------------------------------------------- -// loadCitationPatterns reads and compiles every pattern in the committed -// manifest. It genuinely reads the file on every run rather than caching -// a copy of its contents in this package: the required proof for this -// guard is that deleting a line from the manifest measurably changes what -// the guard can see, and a guard that read its own hardcoded copy of the -// patterns would fail that proof while still going green on the day -// nobody expects it to. -func loadCitationPatterns(t *testing.T, root string) []*regexp.Regexp { - t.Helper() - path := filepath.Join(root, "scripts", "citation-patterns.txt") - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("reading %s: %v", path, err) - } - - declared, err := rulefile.Parse("scripts/citation-patterns.txt", string(data)) - if err != nil { - t.Fatal(err) - } - var patterns []*regexp.Regexp - for _, rule := range declared { - re, err := regexp.Compile(rule.Text) - if err != nil { - t.Fatalf("scripts/citation-patterns.txt:%d: %s is not a pattern this guard can "+ - "compile: %v", rule.Line, rule.ID, err) - } - patterns = append(patterns, re) - } - if len(patterns) == 0 { - t.Fatal("scripts/citation-patterns.txt contains no patterns — this guard would silently pass") - } - return patterns -} - // loadVendorTerms reads the provider and service vocabulary this // repository may not name in authored text. Read on every run, no cached // copy, for the reason every rule file here is: deleting a line has to diff --git a/internal/pack/limits_test.go b/internal/pack/limits_test.go index 6ed49fe..6d1a2cc 100644 --- a/internal/pack/limits_test.go +++ b/internal/pack/limits_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "reflect" - "sort" "strings" "testing" @@ -1549,22 +1548,6 @@ func wholeFinding(f check.Finding) string { return strings.Join([]string{f.Message, f.What, f.Why, f.Next}, "\n") } -// listedInOrder reports the first pair of names that appear in s out of -// the order given, or the empty string when they all agree. -func listedInOrder(s string, want []string) string { - positions := make([]int, len(want)) - for i, w := range want { - positions[i] = strings.Index(s, w) - if positions[i] < 0 { - return fmt.Sprintf("%s is missing", w) - } - } - if !sort.IntsAreSorted(positions) { - return fmt.Sprintf("positions %v", positions) - } - return "" -} - func rowFor(t *testing.T, res check.Results, id string) check.Status { t.Helper() for _, row := range res.Manifest { diff --git a/tools/leakscan/spawnbound_test.go b/tools/leakscan/spawnbound_test.go index db52199..d7863c3 100644 --- a/tools/leakscan/spawnbound_test.go +++ b/tools/leakscan/spawnbound_test.go @@ -179,32 +179,44 @@ func TestEveryGitInvocationGoesThroughTheDeadline(t *testing.T) { const wrapped = "runInputNow" const wrapper = "runInput" + // EVERY NON-TEST FILE IN THE DIRECTORY IS PARSED, whatever its build + // constraint, so a caller in a file built only for another platform is + // still seen. A package loader would answer for the host's build alone, + // and would add a dependency this module does not otherwise need. fset := token.NewFileSet() - pkgs, err := parser.ParseDir(fset, ".", func(fi os.FileInfo) bool { - return !strings.HasSuffix(fi.Name(), "_test.go") - }, 0) + entries, err := os.ReadDir(".") if err != nil { - t.Fatalf("parsing this package: %v", err) + t.Fatalf("listing this package: %v", err) + } + var files []*ast.File + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parsing %s: %v", name, err) + } + files = append(files, file) } callers := map[string]int{} scanned := 0 - for _, pkg := range pkgs { - for _, file := range pkg.Files { - var enclosing string - ast.Inspect(file, func(n ast.Node) bool { - if fd, ok := n.(*ast.FuncDecl); ok { - enclosing = fd.Name.Name - } - sel, ok := n.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != wrapped { - return true - } - scanned++ - callers[enclosing]++ + for _, file := range files { + var enclosing string + ast.Inspect(file, func(n ast.Node) bool { + if fd, ok := n.(*ast.FuncDecl); ok { + enclosing = fd.Name.Name + } + sel, ok := n.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != wrapped { return true - }) - } + } + scanned++ + callers[enclosing]++ + return true + }) } if scanned == 0 { From 070fbaa1ff4e2ca9074c017b1d41d48bf4518a7e Mon Sep 17 00:00:00 2001 From: E Ismail Date: Mon, 5 Oct 2026 18:54:34 +0300 Subject: [PATCH 3/5] tests: two lines the linter would change say on the line what they prove Two of the findings in the set the linter now gates on are lines that exist to prove something, and changing them as it asks would weaken the proof. Each carries a //nolint for that one linter, with its reason: - a %s of a whole config through an any, in the unknown-fields leak test. The verb is wrong for the type on purpose: a debug print through an any is how a secret would leak, and the row proves it does not. - the stall-window floor for a leg with nothing recorded. Nothing uses it, by design: a refusal tells a caller to pass it, and a guard polices that name. --- internal/config/unknown_test.go | 2 +- internal/flow/upload_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/unknown_test.go b/internal/config/unknown_test.go index 4a68bf3..065f292 100644 --- a/internal/config/unknown_test.go +++ b/internal/config/unknown_test.go @@ -126,7 +126,7 @@ func TestUnknownFieldsRenderKeysAndNeverValues(t *testing.T) { // ...any parameter, which is exactly where vet cannot look either. var asAny any = loaded renderings := map[string]string{ - "%s of the config": fmt.Sprintf("%s", asAny), + "%s of the config": fmt.Sprintf("%s", asAny), //nolint:staticcheck // the wrong verb is the point: a debug print through an any is how a secret would leak, vet cannot see it, and this row proves it does not leak "%v of the config": fmt.Sprintf("%v", loaded), "%+v of the config": fmt.Sprintf("%+v", loaded), "%v of the value": fmt.Sprintf("%v", *loaded), diff --git a/internal/flow/upload_test.go b/internal/flow/upload_test.go index 090b989..7e1e754 100644 --- a/internal/flow/upload_test.go +++ b/internal/flow/upload_test.go @@ -1081,7 +1081,7 @@ const ( // caller passes its own and says why; this value is what a caller // passes when its leg has nothing measured to reason from, and passing // it is a statement about the RECORD rather than about the row. - unmeasuredLegFloor = 6 << 20 + unmeasuredLegFloor = 6 << 20 //nolint:unused // unused by design: the floor for a leg with nothing recorded, which blockPointFloor's refusal tells a caller to pass and TestNoRowInheritsAFloorItDidNotState polices by this name // drainTail is how much body is left after the pacing stops, to be // drained at full speed. It matters for the reason the paced phase From b73656692c8d8f6ffc272e01cabe70356ceb169a Mon Sep 17 00:00:00 2001 From: E Ismail Date: Mon, 5 Oct 2026 19:02:22 +0300 Subject: [PATCH 4/5] lint: the linter gates over dead code and staticcheck's correctness checks .golangci.yml enables unused and staticcheck's correctness checks, and nothing else. The linter's default set, with every finding printed, reported the same 110 on all three legs. errcheck gave 93, and every one was false: writes to a terminal or a report stream the program cannot act on, and closes of handles opened only for reading. staticcheck's style families gave 10: nine rewrites of code that already reads clearly, and one that would have removed the %s path from a redaction test's positive control. The two classes kept gave 7, all signal, fixed or annotated in the two commits before this one, and with this configuration the tree reports none. CLAUDE.md records the posture: why these two, why a gate inside make ci rather than advice, what the two annotations protect, and that an unchecked error is not linted at all. --- .golangci.yml | 45 +++++++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..7666163 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,45 @@ +# golangci-lint configuration, in the version 2 schema. +# +# LINT GATES, OVER TWO CLASSES OF FINDING AND NO OTHERS: declarations +# nothing uses (unused), and staticcheck's correctness checks (its SA +# family). Every other linter is off, and the reason is a measurement +# rather than a preference. +# +# Measured 2026-10-05 with golangci-lint 2.14.0: the linter's default set +# was run over this tree with every finding printed, and all three CI legs +# reported the same 110. +# +# - errcheck: 93, and every one was false. Each was a write to a +# terminal or a report stream that the program cannot act on when it +# fails, where the exit status already carries the verdict, or a Close +# on something opened only for reading. errcheck cannot tell a Close +# that could lose a write from one that cannot, by the name it reads, +# so there is no narrower form of it to keep. +# - staticcheck's style families (S1, ST and QF): 10. Nine were rewrites +# of code that already reads clearly. The tenth would have done damage: +# it asked for a fmt.Sprintf("%s", value) to become the value itself, +# inside the positive control of a redaction test, where formatting +# through that verb is the very path the control exists to run. +# - unused and the SA family: 7, and all signal. Four dead declarations +# and one deprecated call were fixed. Two lines that exist to prove +# something stay, each with an annotation on the line saying what it +# proves and why the linter is wrong about it. +# +# So the classes that are off produced nothing worth changing and one +# change that would have weakened a test; the classes that are on produced +# nothing else. govet and ineffassign, the rest of the default set, +# reported nothing; vet runs anyway, as its own step of make ci. +# +# WHAT THIS DOES NOT SEE: an unchecked error is not linted at all. +version: "2" + +linters: + default: none + enable: + - staticcheck + - unused + settings: + staticcheck: + # The correctness checks only, for the reason above. + checks: + - "SA*" diff --git a/CLAUDE.md b/CLAUDE.md index 3db160a..69fd4df 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -331,6 +331,51 @@ YAML is a step nobody can run before pushing. - `make fmt` `make vet` `make test` `make build` — `ci` runs them in that order, formatting first, so a formatting failure is not discovered after a five-minute suite. +- **`make lint` GATES, over two classes of finding and no others, and + that is a ruling rather than a default** (2026-10-05). The workflow + installs golangci-lint at an exact version on every leg, through the Go + toolchain, and `lint` is a step of `make ci`, so a finding reds every + leg. On CI an absent linter is a failure, not a skip; on a machine + without it the target skips and says so. `.golangci.yml` enables + `unused` and staticcheck's correctness checks, and nothing else. + + **Why those two.** The linter's default set was run over this tree with + every finding printed, and all three legs reported the same 110. + errcheck gave 93, and every one was false: a write to a terminal or a + report stream the program cannot act on when it fails, where the exit + status already carries the verdict, or a Close on something opened only + for reading. staticcheck's style families gave 10: nine rewrites of code + that already reads clearly, and one that would have done damage, asking + for a `fmt.Sprintf("%s", value)` inside a redaction test's positive + control to become the value itself, which would stop the control + running the path it exists for. The two classes kept gave 7, and every + one was signal: four dead declarations and a deprecated call, which + were fixed, and two lines that exist to prove something. + + **Why a gate rather than advice.** `lint` runs inside `make ci`, on + every leg of every push. A step there whose exit code decides nothing + is a decoration, by this file's own rule: it spends its runtime on every + run, and while it is green its output is a log nobody reads. Advice is + honest where a person runs the linter and reads it; inside the gate it + is a cost with no consequence. + + **The two annotations, and what they protect.** Each is a + `//nolint: // ` on the line it excuses, so adding a + third is a reviewable change with its reason beside it, never a quiet + widening of the configuration: + + - the `%s` of a whole config, through an `any`, in the unknown-fields + leak test. The verb is wrong for the type on purpose: a debug print + through an `any` is how a secret would leak, vet cannot see that path, + and the row exists to prove it does not leak; + - the shared floor for a stall-window leg with nothing recorded. + Nothing uses it today, by design: a refusal tells a caller to pass it, + and a guard polices that name, so deleting it would leave both naming + nothing. + + **What it does not see:** an unchecked error is not linted at all. + errcheck cannot tell a Close that could lose a write from one that + cannot by the name it reads, so that class stays with review. - **`make surface-check` is the one check `ci` cannot carry, and the reason is the shape of its subject rather than its cost.** There is no push range in a working copy: a checkout is one state, and that check is From 656faac4412fcd39610c8a32a00dd3c2ca79f9ed Mon Sep 17 00:00:00 2001 From: E Ismail Date: Mon, 5 Oct 2026 19:12:16 +0300 Subject: [PATCH 5/5] docs: the merge queue refuses a head commit with a failing required run Observed 2026-10-05: a pull request's head commit carried two runs of the required check ci (windows-latest), one from the push event and one from the pull request, and with the first red and the second green the queue refused to enqueue the pull request, naming that check as failing. CLAUDE.md's record of the queue now says so, beside the way a pull request is enqueued here. --- CLAUDE.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 69fd4df..026e8e1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -425,6 +425,14 @@ YAML is a step nobody can run before pushing. invites is `--admin`, which would merge past the very queue this ruleset exists to enforce. + **The queue refuses to enqueue a pull request whose head commit carries + a failing run of a required check, even when another run of that check + on the same commit is green** — observed 2026-10-05: the push-event run + and the pull-request run both report `ci (windows-latest)`, and with the + first red and the second green the enqueue was refused with *"Pull + request has failing required statuses and Pull request Required status + check "ci (windows-latest)" is failing"*. + **The seven required checks, spelled exactly as GitHub names them:** | required check |