fix(lint): resolve static analysis baseline#769
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request resolves the repository’s existing unused, ineffassign, and staticcheck baseline by applying safe mechanical refactors, removing confirmed-dead helpers, and adding targeted lint suppressions where error-string text must remain stable for users.
Changes:
- Simplifies boolean/control-flow expressions to address
staticcheckquick-fix findings while preserving behavior. - Removes unused helpers and assignments flagged by
unused/ineffassign. - Preserves established user-facing error text by adding targeted
//nolint:staticchecksuppressions and by using non-nil contexts in tests/commands.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/usercommands/usercommands.go | Simplifies rune validation logic for validCommandName. |
| internal/update/update.go | Adds //nolint:staticcheck to preserve established Termux error text. |
| internal/update/apply.go | Replaces OS if/else with switch to satisfy staticcheck quick-fix guidance. |
| internal/update/apply_test.go | Mirrors apply.go OS switch structure to keep tests aligned and lint-clean. |
| internal/tui/transcript_selection.go | Removes unused layout helper and consolidates viewport-start computation path. |
| internal/tui/streaming_fade.go | Removes an unused helper for age-tick rescheduling. |
| internal/tui/specialist_card.go | Simplifies switch for tool call summaries. |
| internal/tui/rendering.go | Applies De Morgan simplification to collapse-footer gating logic. |
| internal/tui/provider_wizard_test.go | Removes an ineffectual assignment while retaining the type assertion. |
| internal/tui/onboarding.go | Converts stage checks to switch statements for readability and staticcheck fixes. |
| internal/tui/model.go | Removes unused scrollableTranscriptView wrapper. |
| internal/tui/model_test.go | Removes an ineffectual assignment while preserving call/type-check intent. |
| internal/tui/loop.go | Replaces Sprintf+WriteString with Fprintf into a builder. |
| internal/tui/image_attach_test.go | Uses Fprintf into a buffer to avoid intermediate allocations flagged by linters. |
| internal/tui/flush.go | Simplifies negated boolean expression for “settled row” determination. |
| internal/tui/files_git_sweep_test.go | Passes non-nil contexts (context.TODO()) to satisfy staticcheck. |
| internal/tui/command_center.go | Simplifies credential-check boolean expression. |
| internal/specialist/exec.go | Adjusts signal description extraction per staticcheck quick-fix guidance. |
| internal/sandbox/windows_network.go | Adds //nolint:staticcheck to keep exported sentinel error text unchanged. |
| internal/sandbox/runner.go | Removes unused sandbox helper functions flagged by unused. |
| internal/sandbox/landlock_other.go | Adds //nolint:staticcheck to keep exported sentinel error text unchanged. |
| internal/sandbox/engine.go | Simplifies backend capability boolean checks. |
| internal/sandbox/command_prefix.go | Simplifies if/return into a single return expression. |
| internal/sandbox/adapters.go | Applies De Morgan simplification to backend marker checks. |
| internal/repomap/prompt.go | Uses Fprintf into a builder to avoid intermediate string formatting. |
| internal/provideronboarding/localruntime_test.go | Simplifies slice nil/len checks using len(nil)==0 behavior. |
| internal/providerhealth/providerhealth.go | Simplifies endpoint safety boolean checks while preserving semantics. |
| internal/lsp/navigate_test.go | Passes non-nil contexts (context.TODO()) to satisfy staticcheck. |
| internal/imageinput/pdf_test.go | Uses Fprintf into buffers to avoid intermediate allocations flagged by linters. |
| internal/dictation/transcriber_deepgram.go | Adds //nolint:staticcheck to preserve established Deepgram error text. |
| internal/config/mcp_merge.go | Simplifies merge logic boolean condition without semantic change. |
| internal/cli/usage.go | Uses Fprintf into a builder for report formatting. |
| internal/cli/provider_detect.go | Simplifies argument parsing switch to match staticcheck recommendations. |
| internal/cli/mcp_config.go | Removes ineffectual assignments flagged by ineffassign. |
| internal/cli/agent_eval.go | Uses variadic append to simplify list accumulation. |
| internal/agentinit/agentinit.go | Uses Fprintf into a builder to avoid intermediate strings. |
| internal/agenteval/score.go | Uses variadic append to simplify list accumulation. |
| internal/agent/profile_controller.go | Simplifies boolean condition for escalation gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Review limit reached
Next review available in: 21 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR resolves existing lint findings across agent, CLI, configuration, provider health, sandbox, TUI, and update code. It also replaces intermediate string formatting, removes unused helpers, simplifies equivalent control flow, and updates affected tests. ChangesLint baseline cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Preserve the id-invariant invalid-session recovery path
internal/tools/exec_command.go:778
Missing, nil, non-integer, and< 1IDs now return distinct validation errors instead ofUnknownExecSessionError. The agent’s repeated-failure guard keys its streak on the first 80 characters of the error, so a model can alternate malformed IDs (or transition from one to a guessed unknown ID) and reset the streak rather than receiving the recovery hint and six-failure halt. This reintroduces the probing/thrashing behavior fixed by the immediate base; keep every no-live-session input on the shared recovery error and restore coverage for the invalid forms.
…gci-lint-baseline The three conflicts were all the same shape — this branch deleted some dead helpers, main deleted a superset — so each resolves to main's side: - internal/sandbox/runner.go: main moved seatbeltCompatibilityPermissionProfile and sandboxEnvironment into export_test.go and dropped existingBubblewrapMounts, which is what this branch had removed. - internal/tui/model.go and transcript_selection.go: main removed scrollableTranscriptView and transcriptViewportStartForLayout outright. Re-ran the pinned baseline against the merged tree and cleared the five findings main introduced since the branch was cut: - Three helpers main moved into internal/tui/export_test.go (scrollableTranscriptView, ensureAgeTickReschedule, transcriptViewportStartForLayout) are unused there too; removed, along with the now-unused bubbletea import. - tuiTheme.onPerm has no callers; removed. - A new QF1001 in transcript_selection.go, simplified the same way the branch simplified the earlier one. internal/tools/ is byte-identical to main, so the id-invariant invalid-session recovery path (Gitlawb#768) is untouched — see the review reply for the verification.
|
Merged current [P1] Preserve the id-invariant invalid-session recovery path — I want to be precise here, because I think this one is an artifact of the stale branch rather than a change in the diff: this branch does not touch
What produced the finding is the same thing that made GitHub report The three real conflicts were all one shape — this branch deleted some dead helpers,
Baseline re-run, which was the point of the PR. The pinned linter reported 5 new findings on the merged tree that
Result: On the toolchain limitation in the PR body — it is worked around rather than accepted. That is worth folding into the repo's lint invocation, since the same mismatch will hit anyone whose default toolchain resolves below the module's Go version. Validation (Windows host, Go 1.26.5): the lint command above reports 0 issues; @jatmn — ready for another look, in particular the @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD / merge-base / internal-tools diff =='
git status --short
git rev-parse HEAD
git merge-base HEAD upstream/main
git diff --stat upstream/main...HEAD -- internal/tools || true
git diff --name-status upstream/main...HEAD
echo '== Changed Go-file outlines (focused candidates) =='
for file in \
internal/specialist/exec.go \
internal/providerhealth/providerhealth.go \
internal/config/mcp_merge.go \
internal/cli/mcp_config.go \
internal/update/apply.go \
internal/tui/rendering.go \
internal/tools/exec_command.go
do
if [ -f "$file" ]; then
echo "-- $file"
ast-grep outline "$file" --items all --type function,method --view compact || true
fi
done
echo '== Focused diffs =='
git diff --unified=35 upstream/main...HEAD -- \
internal/specialist/exec.go \
internal/providerhealth/providerhealth.go \
internal/config/mcp_merge.go \
internal/cli/mcp_config.go \
internal/update/apply.go \
internal/tui/rendering.go \
internal/tools/exec_command.goLength of output: 1468 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Available refs =='
git branch -a
git show-ref --heads --remotes || true
echo '== PR `#769` metadata =='
gh pr view 769 --repo Gitlawb/zero \
--json number,state,mergeable,baseRefName,baseRefOid,headRefName,headRefOid,url \
--jq '{number,state,mergeable,baseRefName,baseRefOid,headRefName,headRefOid,url}'
base_oid="$(gh pr view 769 --repo Gitlawb/zero --json baseRefOid --jq .baseRefOid)"
head_oid="$(gh pr view 769 --repo Gitlawb/zero --json headRefOid --jq .headRefOid)"
echo "base=$base_oid"
echo "head=$head_oid"
if git cat-file -e "${base_oid}^{commit}" 2>/dev/null && git cat-file -e "${head_oid}^{commit}" 2>/dev/null; then
range="${base_oid}...${head_oid}"
else
echo 'PR base/head objects are not both available locally; showing GitHub file list instead.'
gh pr view 769 --repo Gitlawb/zero --json files --jq '.files[] | [.path,.additions,.deletions] | `@tsv`'
exit 0
fi
echo '== internal/tools change relative to PR base =='
git diff --stat "$range" -- internal/tools
git diff --name-status "$range" -- internal/tools
echo '== All changed files =='
git diff --name-status "$range"
echo '== Focused diffs =='
git diff --unified=35 "$range" -- \
internal/specialist/exec.go \
internal/providerhealth/providerhealth.go \
internal/config/mcp_merge.go \
internal/cli/mcp_config.go \
internal/update/apply.go \
internal/tui/rendering.go \
internal/tools/exec_command.goLength of output: 27370 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="a4a805a6c82c51cb255e909bd87ca183c8805cf2"
head="d35fa3921008a6e52cef8d35e5e6f266e4f79fb3"
echo '== Full changed-line summary =='
git diff --stat "$base...$head"
echo '== Merged-main cleanup diff and surrounding declarations =='
git diff --unified=45 "$base...$head" -- \
internal/tui/export_test.go \
internal/tui/theme.go \
internal/tui/transcript_selection.go \
internal/tui/model_test.go \
internal/tui/provider_wizard_test.go \
internal/tui/onboarding.go
echo '== References to removed test seams / onPerm (including tests) =='
rg -n -C 3 \
'\b(scrollableTranscriptView|ensureAgeTickReschedule|transcriptViewportStartForLayout|onPerm)\b' \
internal/tui || true
echo '== Context-sensitive changed functions and tests =='
for file in internal/tui/transcript_selection.go internal/tui/onboarding.go internal/tui/theme.go internal/tui/export_test.go; do
echo "-- $file"
ast-grep outline "$file" --items all --type function,method --view compact || true
done
echo '== All additions/removals excluding direct lint-only mechanical patterns =='
git diff --unified=12 "$base...$head" -- \
':!internal/agent/profile_controller.go' \
':!internal/agenteval/score.go' \
':!internal/agentinit/agentinit.go' \
':!internal/cli/agent_eval.go' \
':!internal/cli/provider_detect.go' \
':!internal/cli/usage.go' \
':!internal/imageinput/pdf_test.go' \
':!internal/lsp/navigate_test.go' \
':!internal/repomap/prompt.go' \
':!internal/sandbox/adapters.go' \
':!internal/sandbox/command_prefix.go' \
':!internal/sandbox/engine.go' \
':!internal/sandbox/landlock_other.go' \
':!internal/sandbox/runner.go' \
':!internal/sandbox/windows_network.go' \
':!internal/tui/command_center.go' \
':!internal/tui/files_git_sweep_test.go' \
':!internal/tui/flush.go' \
':!internal/tui/image_attach_test.go' \
':!internal/tui/loop.go' \
':!internal/tui/specialist_card.go' \
':!internal/update/apply_test.go' \
':!internal/update/update.go' \
':!internal/usercommands/usercommands.go'Length of output: 65028
No new actionable findings from this review. I could not independently execute the Go validation commands in this sandbox, but the diff inspection aligns with your reported validation. ✅ Action performedReview finished.
|
Summary
unused,ineffassign, andstaticcheckbaselineValidation
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 run --enable-only unused,ineffassign,staticcheck ./...(0 issues)go vet ./...go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokego run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...Closes #743
Summary by CodeRabbit
Bug Fixes
Refactor