fix(lint): 248 -> 0, uncapped and site by site - #24
Conversation
SA1012 says do not pass a nil Context, and for 32 of the 36 sites in this
sweep that was right. For four it was wrong, and mechanically applying it
did real damage:
* authz_helpers_test.go x2 and ide/bridge_test.go x1 failed loudly.
Bridge.Start has no nil guard, so context.WithCancel(nil) panics —
the test asserts exactly that, and a real Context removed the case.
* notify_test.go's TestNotificationMethods_Good_NilContext did NOT
fail, which is worse. That test has no assertions at all; its only
mechanism is "these four notification methods do not panic on a nil
ctx". With context.TODO() it passed while checking nothing — green,
and testing air.
The last one is the reason for this commit rather than a quiet fixup. A
linter's advice is about the general case; a test named for its input is
the specific case, and the two disagree here. Each site now carries a
nolint with the reason at the site, so the next sweep does not redo this.
Found by auditing the enclosing test name of all 36 substitutions rather
than trusting a green suite, since the failing three had already been
caught and only the silent one was left.
Co-Authored-By: Virgil <virgil@lethean.io>
cmd/mcpcmd/cmd_mcp_test.go imported the same package twice — once dotted, once as core — so half the file read core.New and the other half read a bare New that was the identical function. cmd/openbrain-mcp did the same with one dotted import. Every symbol was found by removing the import and reading the compiler's undefined list rather than by eye, so nothing was qualified on a guess. Co-Authored-By: Virgil <virgil@lethean.io>
Six SA1019 are the MCP logging surface, deprecated by SEP-2577 as of protocol 2026-07-28. There is no successor call — the feature is being removed outright, not replaced — so there is nothing to migrate to and inventing a replacement would be worse than the deprecation. Each site carries a dated deferral instead: functional for a window of at least 12 months, revisit by 2027-07-28. The other two are real and fixed: a two-arm if/else on host becomes a tagged switch, and `_ = <-errCh` becomes `<-errCh`. Co-Authored-By: Virgil <virgil@lethean.io>
…site Not a sweep. The unchecked returns split three ways by what the silence actually costs: Real bugs, now surfaced (17 sites). Seven json Decode calls discarded the error and carried on with zero values: createIssue reported a created issue as number 0, listOrgRepos returned "no repos" as success, and generateTodo would have written a TODO with an empty title for an agent to work from. Four return the error; three cannot, so they warn and stop rather than hand back a confident zero. Three EnsureDir calls could leave the workspace root or the agent's kb/ and specs/ context dirs absent — the root is now fatal, the context dirs warn. Two c.Command registrations could drop a command, which surfaces much later as "command not found" a long way from the cause. Three cmd.Wait() calls discarded the agent's exit status while the very next lines report the run as "completed". A crashed agent has been reported as success for as long as this code has existed. Fixing the verdict is a behaviour change I cannot verify from here, so this pass makes the crash loud and leaves the status semantics alone — flagged for its own decision, not smuggled into a lint commit. Genuinely nothing lost (33 sites). Read-side body closes after the body is consumed or the request already failed, /dev/null closes, and closes on paths that are already returning an error. These take `_ =`, which states the intent in the code rather than in a linter directive. The three close-after-Wait calls are in the middle group, not this one: that file holds the agent's entire transcript, and a failed close means a truncated log — the only record of what the agent did. Uncapped count for this repo: 248 -> 0. Full suite green. Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 28 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 (38)
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
Closes the mcp lint debt the same way agent's 109 was closed: uncapped, adjudicated per site, silence only where failure genuinely loses nothing.
The measurement was wrong before the work started
This repo had no
.golangci.yml, so policy lived in a CI argument where it was both wrong and invisible:max-issues-per-linter: 50andmax-same-issues: 3. The gate reported "71 issues" for as long as anyone looked. Uncapped, the same code reports 248. A capped number is not a measurement — fixing findings can move the total by zero as suppressed ones surface to replace them.--tests=falseskipped test files entirely, which fails in both directions: production symbols read as dead when their only callers are test seams, and dead scaffolding inside the test files stays hidden.Both fixed here. Tests are linted;
errcheckis excluded for them instead, which is the narrower cut.What the 248 actually were
Real bugs (17). Seven
json.Decodecalls discarded the error and continued with zero values —createIssuereported a created issue as number 0,listOrgReposreturned "no repos" as success,generateTodowould write a TODO with an empty title for an agent to act on. ThreeEnsureDircalls could leave the workspace root or the agent'skb//specs/context absent. Twoc.Commandregistrations could silently drop a command.A silent failure left deliberately half-fixed (3). Three
cmd.Wait()calls discard the agent's exit status while the next lines report"completed". A crashed agent has been reported as success for as long as this code has existed. Changing that verdict is a behaviour change not verifiable from a lint pass, so the crash is now loud and the status semantics are untouched — flagged for its own decision rather than smuggled in here.Deprecations with no successor (6). MCP logging is deprecated by SEP-2577 with nothing to migrate to — the feature is being removed, not replaced. Each site carries a dated deferral: revisit by 2027-07-28.
Genuinely nothing lost (33). Read-side body closes,
/dev/nullcloses, closes on already-failing paths. These take_ =so the intent lives in the code, not in a linter directive.Dead scaffolding (rest). Including
pkg/mcp/service_test.go— 170 lines, 15 tests of the shapesubject := NewService; if subject == nil { t.FailNow() }. A func value is never nil, so it asserted nothing; all five surfaces are covered by real tests inregister_test.go,tools_process_ci_test.goandipc_test.go, verified before deleting.One thing worth reading
The
SA1012(nil Context) findings were not safe to apply mechanically, and I got this wrong first. Four of 36 sites were tests whose declared subject was the nil itself. Three failed loudly. One did not:TestNotificationMethods_Good_NilContexthas no assertions at all — its only mechanism is "these four do not panic on a nil ctx", so a real Context left it passing while testing air.Found by auditing every substitution against its enclosing test name rather than trusting a green suite. Every deliberate nil now carries the reason at the site.
Verification
go test ./...: green🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io