chore: fix golangci-lint findings (dead code, ineffassign, staticcheck)#527
Conversation
- Remove unused safeRequestedPrefix, safeShellCommandTokens (internal/agent/command_prefix.go), and truncateHeadTail (internal/tools/bash.go): each was a thin wrapper with no remaining callers. - Drop an ineffectual assignment in stringListArgWebSearch: every return path after it already sets the named return explicitly. - Simplify a negated-and condition in partition_cache_stable_test.go via De Morgan's law. - Use strings.Contains instead of strings.Index(...) < 0 in edit_replacers.go. - Fix error string style (ST1005): lowercase "unsafe redirect URL" and drop the trailing period from webFetchPublicOnlyHint. Skipped the errcheck findings on this pass (unchecked defer Close()/Write() errors): those are stylistic, not correctness bugs, and this repo's CI only runs go vet, not golangci-lint.
|
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 (7)
💤 Files with no reviewable changes (3)
WalkthroughThis PR removes unused helper functions (safeRequestedPrefix, safeShellCommandTokens, truncateHeadTail), simplifies a test conditional and a fuzzy-match check, adjusts message casing/punctuation in web_fetch.go with corresponding test updates, and removes a redundant assignment in web_search.go. ChangesCleanup and minor fixes
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 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] Tie this cleanup to an approved parent issue or maintainer-approved scope
CONTRIBUTING.md:45
This PR is from aCONTRIBUTORaccount and does not link an approved parent issue, but the project contribution policy says all community PRs require an approved parent issue with theissue-approvedlabel before implementation starts. The change is also a cleanup/refactor pass driven by localgolangci-lintoutput rather than a linked approved bug or feature, which falls under the same scope rule. Please link the approved issue for this cleanup, or get explicit maintainer approval for taking this contributor cleanup outside the issue-first flow.
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 #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 #527) without blocking in-flight PRs on a big-bang adoption.
Summary
Small cleanup pass from running
golangci-lintlocally (installed it after CodeRabbit'struncateHeadTailnitpick on #476 turned out to have no equivalent check in this repo's actual CI, which only runsgo vet).Changes
unusedlinter:safeRequestedPrefix,safeShellCommandTokens(internal/agent/command_prefix.go), andtruncateHeadTail(internal/tools/bash.go). Each was a thin wrapper with no remaining callers.stringListArgWebSearch(internal/tools/web_search.go): every return path after it already sets the named return explicitly, so the assignment never took effect.partition_cache_stable_test.govia De Morgan's law.strings.Containsinstead ofstrings.Index(...) < 0inedit_replacers.go."unsafe redirect URL"and drop the trailing period fromwebFetchPublicOnlyHintinweb_fetch.go, updating the one test that asserted on the old casing.Deliberately left the
errcheckfindings (uncheckeddefer file.Close()/response.Body.Close()/Write()errors) alone. Those are stylistic, not correctness bugs, and this repo's CI doesn't run golangci-lint at all, so there's no actual gate to satisfy there.Test plan
go build ./...go vet ./...(clean, matches actual CI)go test ./internal/agent/... ./internal/tools/...(pass)golangci-lint run ./internal/agent/... ./internal/tools/... --enable-only unused,ineffassign,staticcheck(0 issues, down from 6)Summary by CodeRabbit
Bug Fixes
Refactor