ci: expand the lint config, make make fix converge - #60
Merged
Merged
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
min0625
force-pushed
the
ci/lint-config-and-fix-target
branch
from
September 1, 2026 11:21
84b4ec8 to
8569431
Compare
Re-add gci. #59 dropped it as covered by gofumpt+goimports, but those two only sort within the groups a file already has: a third-party import stranded in the stdlib block is moved out into a group of its own rather than merged into the existing third-party block. gci is what collapses it back to the canonical two groups. Replace gomodguard_v2's local-replace-directives with the gomoddirectives linter. gomodguard only sees directly imported modules, so an indirect dep's replace slipped through; gomoddirectives forbids every replace by default -- checked against a resolvable non-local one, not just a `=> ../local`. Retire depguard. Its one rule (github.com/pkg/errors) moves into the gomodguard_v2 blocklist so the recommendation text sits next to the other deprecated modules. mitchellh/mapstructure and gopkg.in/yaml (prefix match, so both majors) join it. Enable bidichk, makezero and reassign. reassign gets patterns: ".*", which is upstream's own recommendation -- the default only guards EOF and Err*, leaving os.Args and http.DefaultClient open. Tests are excluded from it: they borrow os.Stdin/os.Args and restore them afterwards. Turn on errcheck.check-type-assertions and prealloc.for-loops (modernize rewrites 3-clause loops into the range form prealloc polices, so with the default the finding only surfaces after --fix), and turn off perfsprint.concat-loop, which modernize rewrites better by reusing the variable instead of inventing one. Exclude gosec's G104. It fires on bare call statements only, never on defer/go, and honors a whitelist -- a strict subset of errcheck, which also names the offending function; with uniq-by-line: false both would print on the same line. Narrowing errcheck (exclude-functions, std-error-handling) would reopen the gap. Correct the check-rev rationale in the Makefile and AGENTS.md. #58 documented golangci-lint as only warning and exiting 0 on an unresolvable --new-from-rev, which would let CI pass having linted nothing. It does not: it warns, reports every issue in the repo, and exits 1. The guard stays, but it buys a readable error message, not safety. `make fix` now runs tidy -> --fix -> tidy -> lint. --fix is not a fixpoint: its edits can trip a linter the fixing run never saw, and can add or remove imports. The new ordering was exercised on a clean tree, so it confirms the plumbing runs, not that the second pass caught a real fixpoint miss. Also document in AGENTS.md that .golangci.yaml is not self-contained -- whole-files: true is silently inert without one of the --new* modes, so the file only works alongside the Makefile's --new-from-rev harness -- and that formatter findings are the exception to the ratchet: apply `golangci-lint fmt ./...` repo-wide rather than letting a half-formatted tree contradict its own config. `golangci-lint run ./...` reports 0 issues repo-wide under this config, so the whole-files ratchet takes on no new debt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
min0625
force-pushed
the
ci/lint-config-and-fix-target
branch
from
September 1, 2026 11:35
8569431 to
6132aa6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Second pass over the lint setup after #59, plus a
make fixthat actuallyconverges.
Formatters — re-add
gci. #59 dropped it as covered bygofumpt+goimports, but those two only sort within the groups a filealready has. Checked on
cmd/mint/main.gowithcobramoved into the stdlibblock:
gofumpt+goimportscobraends up in a third group of its own+ gciSo it earns its slot; #59's reasoning was wrong on that point.
replaceguard — swapgomodguard_v2.local-replace-directivesfor thegomoddirectiveslinter.gomodguardonly sees directly imported modules, soan indirect dep's
replaceslipped through. Verifiedgomoddirectivesrejectsa resolvable non-local replace by default, not just
=> ../local:depguardretired — its single rule (github.com/pkg/errors) moves intothe
gomodguard_v2blocklist, next to the other deprecated modules.mitchellh/mapstructureandgopkg.in/yaml(prefix match, both majors) joinit.
New linters —
bidichk,makezero,reassign.reassigngetspatterns: [".*"]per upstream's own recommendation; the default only guardsEOFandErr*, leavingos.Argsandhttp.DefaultClientopen. Tests areexcluded from it — they borrow
os.Stdin/os.Argsand restore them.Settings —
errcheck.check-type-assertionson;prealloc.for-loopson (modernizerewrites 3-clause loops into the rangeform
preallocpolices, so with the default the finding only surfaces after--fix);perfsprint.concat-loopoff (modernizerewrites the same patternbetter, reusing the variable instead of inventing one).
gosecG104 excluded — it fires on bare call statements only (neverdefer/go) and honors a whitelist, so it is a strict subset oferrcheck,which also names the offending function. With
uniq-by-line: falseboth wouldprint on the same line. Narrowing
errcheck(exclude-functions,std-error-handling) would reopen the gap — noted in the config.make fix— nowtidy → --fix → tidy → lint.--fixis not a fixpoint:its own edits can trip a linter the fixing run never saw, and can add or remove
imports.
check-revrationale corrected — #58 documentedgolangci-lintas onlywarning and exiting 0 on an unresolvable
--new-from-rev, which would let CIpass having linted nothing. It does not: it warns, reports every issue in the
repo, and exits 1. The guard stays, but the Makefile comment and
AGENTS.mdnow say what it actually buys — a readable error message, not safety.
Docs —
AGENTS.mdalso gains a note that.golangci.yamlis notself-contained:
whole-files: trueis silently inert without one of the--new*modes, so the file only works alongside the Makefile's--new-from-revharness. And that formatter findings are the exception to theratchet — apply
golangci-lint fmt ./...repo-wide rather than leave ahalf-formatted tree contradicting its own config.
Verification
golangci-lint config verify— cleangolangci-lint run ./...(no--new-from-rev) — 0 issues repo-wide, sothe
whole-filesratchet takes on no new debtmake check(full prek suite, the CI gate) — all hooks passmake fix— exits 0, leaves the tree unchangedNote on scope: the
make fixrun above exercised the newtidy → --fix → tidy → lintplumbing on a clean tree. It confirms the orderingruns, not that the second pass caught a real fixpoint miss.
🤖 Generated with Claude Code