mirror of https://github.com/gastownhall/beads.git synced 2026-09-14 20:17:24 +08:00

Files

T

Julian Knutsen cff62bd631 Make the pre-commit lint gate trustworthy, and stop one main-side lint word from reddening every PR (#5538 )

* fix(githooks): build golangci-lint with the repo's Go, not golangci-lint's

The pre-commit hook ran `go run golangci-lint@v2.10.1`. For a pkg@version
build the go command honours the TARGET module's toolchain directive, so
GOTOOLCHAIN=auto downgraded to go1.25.12 — and golangci-lint built by
go1.25 refuses this repo's `go 1.26.5` at config load:

go: github.com/golangci/golangci-lint/v2@v2.10.1 requires go >= 1.25.0; switching to go1.25.12
Error: can't load config: the Go language version (go1.25) used to build golangci-lint is lower than the targeted Go version (1.26.5)

Every Go commit failed there, so the hook taught six-plus contributors to
reach for --no-verify — the one habit a lint gate must never train.

CI never hit it because actions/setup-go installs the go.mod version and
exports GOTOOLCHAIN=local, so its `go install ...@v2.10.1` compiles the
linter with go1.26.5. Pinning GOTOOLCHAIN to the go directive reproduces
that resolution on any host, including one whose base toolchain is older
than the repo's target (where a bare GOTOOLCHAIN=local would pin the
linter to that older toolchain instead).

Verified on a real staged Go change: red before with the message above,
`0 issues.` after, and still exit 1 on a planted errcheck violation.

Bead: ga-2ltro.13

* ci: lint PRs for what they introduce, sweep the whole tree on main

Both PR lint gates linted the whole tree: the `lint` job left
only-new-issues at its false default, and pr-lint-wrapper ran
`make ci-pr-lint` over ./... . So a single lint word landing on main —
twice this wave, once a prose misspell — turned EVERY open PR red for a
change it did not make, and each author had to either fix someone else's
line or wait.

The tradeoff, recorded beside the job: a pre-existing issue in untouched
code no longer blocks a PR, and because "new" is measured off the diff, a
violation carried in by a MOVE or a rename does. What it buys is that a
main-side word now reds main's own run, where it belongs. main.yml
already runs both jobs unscoped on every push, so whole-tree coverage —
including the GOOS=windows cross-lint — is unchanged; only its blast
radius moves.

The wrapper takes its scope from BD_LINT_NEW_FROM_MERGE_BASE, set only
by pr.yml, so `make ci-pr-lint` run by hand still sweeps the tree. That
makes the local wrapper STRICTER than the PR gate rather than equal to
it, which is what LINTING.md and CI_CLEANUP_PLAN.md said before and no
longer would: both now state the two-lane contract and say the local
run's extra strictness is deliberate. Their freshness markers move with
the workflow and wrapper files they are pinned to.

Bead: ga-uwix0.2

---------

Co-authored-by: Julian Knutsen <ci@beads.test>

2026-08-10 04:55:55 -07:00

2.8 KiB

Raw Permalink Blame History

Linting Policy

Last reviewed: 2026-08-10

Freshness source: .golangci.yml, scripts/ci/pr-lint.sh, Makefile, .github/workflows/pr.yml, and .github/workflows/main.yml.

This document explains the required Go lint gate for this codebase.

Current Status

Lint is a required CI gate, and it runs in TWO LANES over the same pinned golangci-lint v2.10.1 release and the same .golangci.yml. Each must pass with zero issues in its own scope.

The PR lane reports only what the PR introduces. Both of its jobs are scoped to the diff against the merge base with main: the lint job passes only-new-issues, and pr-lint-wrapper runs the repository-owned ci-pr-lint wrapper with BD_LINT_NEW_FROM_MERGE_BASE. A finding in code the PR did not touch does not block it.
The main lane sweeps the whole tree. Both jobs run unscoped on every push to main, so a finding that lands there reds main's own run rather than every open PR.

Run the wrapper locally with:

make ci-pr-lint

That is the MAIN lane's contract, not the PR lane's: with BD_LINT_NEW_FROM_MERGE_BASE unset it sweeps the whole tree, so it is deliberately STRICTER than the gate your PR has to clear and may report findings you are not required to fix. Fix the ones that are yours; the PR gate is the authority on what blocks a merge, and main's own run is where the rest is answered.

The wrapper runs:

make fmt-check;
golangci-lint with .golangci.yml, readonly module downloads, a five-minute timeout, the gms_pure_go build tag, and --new-from-merge-base when BD_LINT_NEW_FROM_MERGE_BASE names a ref; and
a second non-CGO Windows cross-lint pass, on the same scope, when the native host does not already cover that target.

Policy

Treat new lint findings as defects to fix before merge. Do not add a tolerated failing baseline, and do not configure CI with --issues-exit-code=0.

When a linter reports an intentional or false-positive pattern:

Prefer a narrow .golangci.yml exclusion tied to a path, linter, and message.
Use //nolint:<linter> only when the reason is local to a specific line and the comment explains why the warning is not actionable.
Keep broad linter disables as a last resort.

The current configuration already encodes accepted exclusions for intentional patterns such as deferred cleanup errors, controlled subprocess execution, test-fixture file reads, and documented security false positives.

CI Cleanup Decision

pr-lint stays separate from pr-policy and pr-core so failures are easy to identify and rerun. Its repository-owned wrapper is scripts/ci/pr-lint.sh, exposed as make ci-pr-lint.

See CI_CLEANUP_PLAN.md for the full CI tier policy.

Future Work

Periodically audit .golangci.yml exclusions and remove entries that are no longer needed.

2.8 KiB Raw Permalink Blame History