chore(lint): enable 19 new linters and address violations
Summary
Enables 19 new golangci-lint linters that catch issues the existing config did not, and addresses all surfaced violations (auto-fix where possible, hand-fix the rest).
This is the companion to !3346 (merged) (the MR review-instructions expansion): many of the rules synthesized from @timofurrer's review feedback into that YAML are now enforced deterministically at edit time, freeing Duo Code Review to focus on judgment calls instead of repeating mechanical findings.
Linters added
Code-quality and bug-class:
testifylint— testify-specific hygiene:requirebefore indexing,assert.Emptyoverassert.Equal(t, "", x), norequirein HTTP handlers, JSON unmarshal asserts.errorlint— wraps error comparisons witherrors.Is/As; catcheserr == sentinelstyle.bodyclose— HTTP response bodies must be closed.contextcheck— context propagation through call chains.misspell— common typos in comments and string literals.forcetypeassert— flags unchecked type assertions (panics on mismatch).nilerr— flagsif err != nil { return nil }(bug class).fatcontext— detects nested context creation in loops.asasalint— catchesfunc(...any)accidentally invoked with a[]anyslice.gochecksumtype— exhaustive switch on sum-type interfaces.
Hygiene and safety:
gocheckcompilerdirectives— guards//go:directive syntax.exptostd— flagsx/exppackages now in the stdlib.gomoddirectives—go.moddirective hygiene.nolintlint—//nolintdirectives must be well-formed and used.reassign— flags reassignment of top-level package variables.nosprintfhostport— flags unsafehost:portURL construction (IPv6 bug class).asciicheck— bans non-ASCII identifiers (Unicode homoglyphs).bidichk— bans dangerous bidi unicode (CVE-2021-42574).
Plus the previously-disabled staticcheck check ST1005 is
re-enabled, which enforces Go's standard error-string style
(lowercase first letter, no trailing punctuation; see
https://go.dev/wiki/CodeReviewComments#error-strings).
What this changes in the codebase
| Type | Count |
|---|---|
| Files touched | ~250 |
Auto-fixed by golangci-lint --fix |
~165 files (mostly testifylint + errorlint) |
| Hand-fixed | ~85 files |
| Test files updated to match new error strings | ~25 |
A handful of files received small real improvements as a side effect of the linter probes:
internal/commands/ci/ciutils/utils.go::getPipelineIdnow accepts acontext.Context(was previously callingcontext.Background()mid-stack).internal/commands/ci/lint/lint.go::runnow threadscmd.Context()through its HTTP request.internal/commands/api/api.go::runnow closes paginated HTTP response bodies per iteration.- 24 unchecked type assertions across 12 files now use the
comma-ok form (or
require.Truein tests). - One real
nilerrbug fixed inagent_update_kubeconfig.go(the function previously swallowedos.Executable()errors and continued with an empty path). Four othernilerrhits are intentional fallback paths and carry//nolint:nilerrwith a reason.
What was NOT included (deliberately)
noctx: enforcing it surfaces a structural issue ininternal/git,internal/browser, and parts ofinternal/iostreamsthat intentionally do not carrycontext.Context. Threading ctx through those packages is its own refactor; the bare exec / interactive launchers don't benefit from cancellation semantics. Tried, reverted; capture as a future follow-up if we want to enforce it.
Coordination with !3346 (merged)
!3346 (merged) (the review-instructions expansion) has been updated to drop the ~14 rules now enforced deterministically by linters here, so the two MRs are complementary rather than overlapping. Each remaining rule in !3346 (merged)'s YAML now has a one-line note pointing at which linter handles the basics, so Duo's review comments cite the right authority.
Other linters probed (not included, reported for follow-up)
| Linter | Violations | Recommendation |
|---|---|---|
gosec |
73 | Needs careful triage; security findings should not be batch-fixed |
predeclared |
45 | Hard — most violations are package names (delete, list, etc.) that conflict with predeclared identifiers. Renaming packages is high-blast-radius. |
gocritic |
33 | Opinionated; would need disabled-checks tuning. Mostly autofixable cosmetics. |
goprintffuncname |
2 | IOStreams.StartSpinner / StopSpinner would need to be renamed to *Spinnerf. Breaking internal-API change. |
durationcheck |
1 | False positive at internal/utils/utils.go:134 (m * time.Minute is the remainder-to-nanos idiom). Not worth a nolint. |
Test plan
- CI lint job passes with the new
.golangci.yml. - CI test suite passes (locally:
go test ./...is green). - @timofurrer review on whether ST1005 error-string changes feel
consistent with the codebase's voice (many were
Capital. Sentence.before; now they'relowercase phrase). - Run Duo Code Review on this MR itself to confirm it doesn't flag the same things the linters now catch.
Depends-on: !3346 (merged)