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/.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..026e8e1 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 @@ -380,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 | 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 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 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 {