ci: add golangci-lint as an advisory check#528
Conversation
go vet, gofmt, govulncheck, and deadcode don't catch unused private functions/assignments reachable within a package but never called, or staticcheck-style correctness/readability issues. deadcode's whole-program reachability analysis specifically missed several of these; golangci-lint's truncateHeadTail nitpick on Gitlawb#476 assumed it would fail CI, but nothing in this repo actually ran it. Scoped to unused, ineffassign, and staticcheck rather than the full default battery, and continue-on-error like deadcode, so this surfaces the existing findings across the repo (being cleaned up incrementally per Gitlawb#527) without blocking in-flight PRs on a big-bang adoption.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdated the CI workflow's toolchain-pinning comment to reference golangci-lint, and added a new non-blocking "golangci-lint (advisory)" step running golangci-lint v2.12.2 with continue-on-error, limited to unused, ineffassign, and staticcheck analyzers. ChangesCI Advisory Lint Step
Estimated code review effort: 1 (Trivial) | ~3 minutes Related Issues: None referenced. Related PRs: None referenced. Suggested labels: ci, chore Suggested reviewers: None specified. Since it's advisory and won't block, one nit worth confirming: pin the version consistently and double check Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jatmn
left a comment
There was a problem hiding this comment.
Findings
- [P2] Link an approved parent issue or get maintainer confirmation for this exception
CONTRIBUTING.md:19
The workflow change itself is consistent with the PR description, but this PR does not link an approved parent issue.CONTRIBUTING.mdsays community PRs must be tied to an existing issue with theissue-approvedlabel, and the PR author only has read permission on the repository, so the team-member exception does not clearly apply. Please link an approved issue for this CI change, or get an explicit maintainer confirmation that this PR should proceed under an exception to the community-PR policy.
Summary
CI currently runs
go vet,gofmt -l, tests,govulncheck(hard gate), and an advisorydeadcodecheck (unreachable-from-cmd/* analysis). None of these catch unused private functions/assignments that are locally reachable but never called, or staticcheck-style correctness/readability issues.Concretely: CodeRabbit's review on #476 flagged
truncateHeadTailininternal/tools/bash.goas dead code that "would fail CI", but nothing in this repo actually runs golangci-lint, so it wouldn't have. Running it locally turned up the same finding plus several more that had been sitting onmainuncaught, fixed in #527, plus more elsewhere in the repo (internal/sandbox,internal/tui,internal/update) that #527 didn't touch.deadcodedidn't catch any of these because it's a different kind of analysis: whole-program reachability fromcmd/*entrypoints, not per-package unused-function detection.Changes
Add a
golangci-lint (advisory)step to thesecurityjob, right afterdeadcode:unused,ineffassign, andstaticcheckrather than the full default battery, so this doesn't immediately dump the entire default linter set's noise into CI.continue-on-error: true(same asdeadcode), so it's visible but non-blocking while the existing findings across the repo get cleaned up incrementally instead of all landing as a wall of failures on unrelated in-flight PRs.v2.12.2, invoked viago runthe same waygovulncheck/deadcodealready are, and added to the toolchain-pin comment/step for the same reason (its own module's toolchain resolution can drop below what go.mod requires).Test plan
main:go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 run --enable-only unused,ineffassign,staticcheck ./..., confirms it resolves and runs cleanly (42 pre-existing findings surfaced, which is expected and why this lands advisory first).deadcodestep exactly.Summary by CodeRabbit