Remediate lint findings surfaced by the standard .golangci.yml #61
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/vaultik#61
Reference in New Issue
Block a user
The org-standard
.golangci.yml(adopted per #59) surfaces 2,990findings under golangci-lint. Per the decision on #59, the config lands
first and the cleanup is tracked here as its own effort rather than being
bundled into #59.
Per-linter breakdown (from
make checkat time of adoption):wsl_v5nilnilnlreturngocognitnoinlineerrcontainedctxrevivenestifparalleltesterrorlinterr113gosmopolitanmndcontextchecklllexhaustivegosecnonamedreturnsfuncorderpreallocgoconstrecvcheckcyclopgodoclinttagliatelleunparamtestpackagenilnesserrmodernizethelperfunlenunconverttestifylintforbidigointrangegoprintffuncnamegochecknoglobalsnoctxduplperfsprintembeddedstructfieldcheckforcetypeassertgocriticireturnusetestingDefinition of done
no test deletions, no weakening of assertions. Per-finding
//nolintcomments (with a reason) are acceptable where a code change would be
worse than the suppression.
.golangci.ymlis NOT modified — it is org-standard and only therepo owner may change it.
branches/PRs), mechanical linters first (
wsl_v5,nlreturn,noinlineerr,intrange,modernize), judgment-heavy ones(
gosec,err113,cyclop,ireturn) last.make teststays green after every chunk.make checkis fully green onmainwith the standardconfig in place, at which point #59 can also be closed.
Starting the first chunk: the three mechanical whitespace/return-style
linters —
wsl_v5(1050),nlreturn(378),noinlineerr(373) —which together are ~60% of all findings.
Plan:
script/lint-fixentrypoint (with amake lint-fixshim)that runs the linter's own autofixer, so mechanical remediation is
repo tooling rather than ad-hoc invocations.
.golangci.ymlitselfis not touched.
make lint-fixand land the autofixed changes in per-packagecommits, keeping
make testgreen after every commit.noinlineerr)gets fixed by hand, behavior-preservingly, also in per-package
commits.
wsl_v5,nlreturn, andnoinlineerrreport zero findings, the total drops below ~1,200,and
make testis green. Then merge tomain, push, and post theupdated per-linter count here.
Judgment-heavy linters (
gosec,err113,cyclop,revive, etc.)remain for later chunks per the definition of done above.
Chunk 1 is done and merged to
main(23d22a0..b87b72d, 22 commits).Total findings: 2,990 → 1,077. The three mechanical target linters
are all at zero:
wsl_v5: 1050 → 0nlreturn: 378 → 0noinlineerr: 373 → 0How it was done:
script/lint-fixentrypoint plus amake lint-fixMakefileshim (documented in the README Entrypoints section), which runs the
linter's own autofixer, so mechanical remediation is repo tooling.
make lint-fixhandled all ofwsl_v5andnlreturnplus about200 of the
noinlineerrsites, and incidentally cleared severalother autofixable linters (
errorlint,intrange, most ofmodernize,perfsprint, most oftestifylint,embeddedstructfieldcheck,nilnesserr). The autofixer's rewritesleft ~25 files missing
errors/strings/heximports and about 40:=redeclarations, all repaired by hand.noinlineerrsites (no autofix available) werefixed by hand across 41 files: each inline
if err := f(); err != nilsplit into a standalone assignment plus check, using
=whereerrwas already in scope. Strictly behavior-preserving — no logic,
message, or control-flow changes, no
//nolintsuppressions needed.The work landed as reviewable per-package commits ("Apply linter
autofixes: ..." then "Fix noinlineerr findings: ..."). The full test
suite is green after the final state (verified via
make test; alsogreen at each pushed boundary).
Remaining 1,077 findings by linter (top of the list):
paralleltest137,
revive142,err11396,mnd93,lll79,gosec78,funcorder68,goconst55,cyclop52,tagliatelle48,testpackage34,funlen36, plus a long tail. Suggested chunk 2:the still-mechanical group —
paralleltest,funcorder,testpackage,lll— before the judgment-heavy ones.Note:
make testin a fresh environment now takes ~19s ininternal/database, close to the 20s repo policy target; worthwatching but not acted on here.
Manager note — picking this back up.
Found chunk 2 of the remediation sitting only in a local checkout,
never pushed: branch
golangci-v2.12.2, commits6cf9211,7ae470e,and a merge of
main. I have pushed it and opened PR #63 so it is nolonger at risk.
Verification of that branch with
make checkin a clean worktree:make test: green, all 14 packagesokmake fmt-check: greenmake lint: red, 80 findings —wsl_v560,sqlclosecheck10,gosec4,prealloc3,revive3Commit
7ae470e's message claims "make check is green". That claim isincorrect as of the branch tip (
make checkexits 2). Flagging itexplicitly so the record is not misleading — the branch is real progress
(2,990 → 1,077 → 80) but it is not done.
Definition of done for this issue is unchanged and still:
make checkexits 0 under the canonical
.golangci.yml(sha256021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb), withall fixes behavior-preserving. Remaining work is tracked on PR #63, which
carries
needs-checksuntil lint is clean.Note on #59: both files in its definition of done are already on
mainand byte-identical to canonical. #59 is blocked purely on this issue
making
make checkgreen, so PR #63 closes both.Correction to my previous comment — I got the diagnosis partly wrong and
want the record straight.
I said chunk 2 was "unpushed work at risk on
golangci-v2.12.2". That waswrong. The branch and
mainhave identical tree hashes(
0f36f0b99599b63018da6cd3b0a62e2046eb3c16), so the branch had no uniquecontent — chunk 2 had already landed on
mainvia PR #62 (cc58583). Iwas misled by a three-dot (merge-base) diff showing 126 files; the
two-dot diff of the tips is empty. Nothing was ever at risk of being
lost.
What does not change, and is the thing that matters:
mainis red right now. The 80 findings I measured were measured oncontent byte-identical to
main, somake checkfails onmaintoday —and with it
.gitea/workflows/check.ymlanddocker build .:wsl_v560,sqlclosecheck10,gosec4,prealloc3,revive3make testgreen (all 14 packages),make fmt-checkgreen,make lintred
The claim in
7ae470e's commit message that "make check is green" isstill incorrect, and
TODO.mdonmainstill records this work ascomplete with
make checkgreen. Both need correcting.PR #63 is closed as empty and
golangci-v2.12.2will be deleted.Remediation now proceeds on
lint-remediation-final, branched frommain, with a fresh PR closing this issue and #59.Of the remaining 80, the 10
sqlclosecheckfindings ininternal/database/{files,snapshots,chunks,chunks_ext,blobs,blob_chunks}.goare the ones worth attention: they are unclosed
sql.Rows, i.e. realresource leaks in a long-running backup process, not style nits. Those
are being fixed properly rather than suppressed.
Remediation of the final 80 findings is up as PR #77, branched from
mainatcc58583(branchlint-remediation-final).make checkexits 0 there:
make test14 packages ok (run with the Go test cachedisabled so they genuinely executed),
make lint0 issues,make fmt-checkclean..golangci.ymlis unmodified and still hashesto
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.Breakdown:
wsl_v560 (blank lines, viamake lint-fix),sqlclosecheck10,prealloc3,gosec4 andrevive3 (per-site//nolintwith justifications; the package-rename question behind thereviveones is now issue #76).One correction for the record. My earlier comment said the 10
sqlclosecheckfindings were "unclosedsql.Rows, i.e. real resourceleaks". That was wrong. All ten sites already had
defer CloseRows(rows), andCloseRowscallsrows.Close()— the rowswere being closed.
sqlclosecheckonly recognises aClosecall on therows value inside the function that produced it, so handing
rowsto ahelper reads as unhandled; that is also why 8 other
defer CloseRows(rows)sites in the same package were never flagged(they pass
rowsto a scan helper as their last use, which the analyzeraccepts). No leak existed, and no behavior changed: the PR removes the
helper and defers a closure calling
rows.Close()directly at all 18sites, preserving the fatal-on-close-error path exactly.
Correcting myself again, and this one is my error rather than a stale
record.
I wrote earlier that the 10
sqlclosecheckfindings were "unclosedsql.Rows, i.e. real resource leaks in a long-running backup process,not style nits". That was wrong. Every one of those sites already had
defer CloseRows(rows), andCloseRows(formerly ininternal/database/errors.go) calledrows.Close(). The rows were beingclosed. There was no leak.
What the linter was actually reporting is a limitation of its own
analysis:
sqlclosecheckonly recognises aCloseperformed on the rowsvalue inside the function that produced it, so passing
rowsto a helperreads as unhandled. The tell I should have noticed is that 8 other
identical
defer CloseRows(rows)sites in the same package were neverflagged — those pass
rowsto a scan helper as their last use, whichthe analyzer accepts. A real leak would not be inconsistent like that.
I asserted a data-integrity problem from a linter category name without
reading the call sites. Flagging it prominently so nobody plans work on
the belief that vaultik was leaking database cursors — it was not.
The fix in PR #77 is still the right change: the helper hid the close
from the analyzer, the codebase already used the inline deferred-close
idiom elsewhere, and the helper is now gone with all 18 sites (not just
the 10 flagged) converted to a deferred inline close preserving the
fatal-on-close-error path. But it is a readability and lint-conformance
fix, not a bug fix, and the changelog should describe it that way.
Everything else in my earlier comments stands:
mainis red today, andPR #77 is what makes
make checkexit 0.Closed by PR #77, merged to
mainase496aa3.I re-ran the gate against the merged
mainmyself rather than relying onthe PR's result, because this issue produced two false green claims:
golangci-lint v2.12.2(digest-pinned image):0 issues.make fmt-check: passmake test: 14 packagesok, 0(cached), 0FAIL.golangci.ymlstill hashes to021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcbmainis green..gitea/workflows/check.ymlanddocker build .areunblocked for the first time since the canonical config landed.
Final arc: 2,990 → 1,077 → 80 → 0.
Two corrections belong in the permanent record, since both were asserted
as fact on this issue and both were wrong:
sqlclosecheckfindings were realsql.Rowsleaks.They were not — every site already had
defer CloseRows(rows). Thelinter cannot see a close performed via a helper. The fix that landed
is a readability and lint-conformance change, not a bug fix, and
should be described that way in any release notes.
7ae470eand an earlierTODO.mdentry claimedmake checkwas green when lint was red. The root cause turned out not to be
carelessness:
script/lintresolvesgolangci-lintfromPATH(2.10.1 here) while CI pins v2.12.2 by digest, and the two disagree.
Two independent agents were misled by it. That is now tracked as
#78, and until it lands
script/cibuildis the only trustworthygate in this repo —
make checkalone must not be used to claim achange is green.
Suppressions that landed: five
//nolint:revive,nolintlintdirectives onthe package clauses of
internal/log,internal/crypto, andinternal/types, deferring a repo-wide naming decision to #76 (withsneak). Nogosecsuppressions were needed under the pinned linter, andnothing in the config,
Dockerfile,Makefile,.gitea/, orscript/was weakened — all byte-identical to
main.