fix(lint): measure the Go lint debt honestly, and clear the production half - #25
Conversation
…n half The reported debt was never the debt. golangci-lint defaults to max-issues-per-linter: 50 and max-same-issues: 3, so every count taken from it here — 101 with --tests=false, 90 without — was a display cap. Uncapped, the same code reports 839. Fixing eleven errcheck findings moved the total by zero, because eleven suppressed ones surfaced to replace them, which is what exposed it. A capped number is not a measurement, so the config now sets both to 0. Policy moves out of a CI argument into .golangci.yml, and --tests=false goes. That flag was doing two harmful things at once. It made `unused` report 21 production symbols as dead because their only callers live in _test.go — syncPull, mcpInitialize, mcpCall, readSSEData, newCoreAgent, pokeCh and the rest, every one a deliberate test-injection seam, since a linter cannot see a caller it has been told not to read. And it hid the dead scaffolding actually inside the test files, which is where all 28 real `unused` findings live. errcheck is excluded for tests instead: the narrower, honest cut, because an unchecked Close() in test setup is noise while an unchecked write-close in production loses data. Two of those production cases were losing data. chathistory's CopyTo and ExportJSONL both closed their destination file with a bare defer, so a close that failed to flush returned nil and reported a complete copy of a truncated file. Both now close explicitly on the success path and report the error, with the defer kept as the net for early returns. The rest are decided per site rather than silenced in bulk. Read cursors and response bodies say so and drop the error. Notifications — Core.ACTION, NotifySession, ResourceUpdated, session.Log — are best-effort by design, and say why: a listener that has gone away must not fail the work that produced the event. Status writes get the opposite treatment and now report through core.Warn, the idiom already used in those files, because the status file is what the monitor polls and a silent failure leaves a workspace looking stuck forever. Same for the runner losing track of a workspace, and for a shutdown that cannot complete. Two signatures I had assumed wrong and the compiler caught: core.WriteString returns a Result, not (int, error), and ServiceShutdown returns a Result too. Remaining, and now countable: 59 production errcheck, 22 staticcheck (three in production, all SA1019 deprecations — session.Log against MCP SEP-2577, and httputil Director against Go 1.26), 28 unused, every one of them dead scaffolding in test files. 109 real findings, against a number that used to read 90 and meant nothing. go build, go vet and go test all pass; gofmt clean. Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
The reported debt was never the debt
golangci-lint defaults to
max-issues-per-linter: 50andmax-same-issues: 3. Every count taken from it in this repo was a display cap — 101 with--tests=false, 90 without. Uncapped, the same code reports 839.I found it because fixing eleven errcheck findings moved the total by zero: eleven suppressed ones surfaced to replace them. A capped number is not a measurement, so the config now sets both to 0.
--tests=falsewas doing two harmful things at once_test.go—syncPull,mcpInitialize,mcpCall,readSSEData,newCoreAgent,pokeCh, every one a deliberate test-injection seam. A linter cannot see a caller it's been told not to read.unusedfindings turn out to liveTests are now linted; errcheck is excluded for them instead. That's the narrower, honest cut: an unchecked
Close()in test setup is noise, while an unchecked write-close in production loses data.Policy moves out of a CI argument into a committed
.golangci.yml, so it's visible and reviewable.Two production cases were losing data
chathistory'sCopyToandExportJSONLboth closed their destination file with a baredefer. A close that fails to flush returnednil— reporting a complete copy of a truncated file. Both now close explicitly on the success path and report the error, with the defer kept as the net for early returns.The rest, decided per site rather than silenced in bulk
Core.ACTION,NotifySession,ResourceUpdated,session.Log) — best-effort by design, and now say why: a listener that has gone away must not fail the work that produced the eventcore.Warn, the idiom already in those files, because the status file is what the monitor polls and a silent failure leaves a workspace looking stuck forever. Same for the runner losing track of a workspace, and a shutdown that cannot complete.Two signatures I assumed wrong and the compiler caught:
core.WriteStringreturns aResult, not(int, error), and so doesServiceShutdown.Remaining, and now countable
session.Log(MCP SEP-2577),httputilDirector(Go 1.26)109 real findings, against a number that used to read 90 and meant nothing.
go build,go vetandgo testall pass;gofmtclean.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io