Enable more linters and fix existing issues#1627
Merged
dgageot merged 15 commits intodocker:mainfrom Feb 7, 2026
Merged
Conversation
Enable the following new linters: - asasalint: Check for pass []any as any in variadic func(...any) - asciicheck: Check identifiers for non-ASCII symbols - bidichk: Check for dangerous unicode character sequences - decorder: Check declaration order and count - dogsled: Check assignments with too many blank identifiers - durationcheck: Check for two durations multiplied together - exptostd: Detect functions from golang.org/x/exp/ replaceable by std - fatcontext: Detect nested contexts in loops and function literals - gocheckcompilerdirectives: Check go compiler directive comments - gochecksumtype: Run exhaustiveness checks on sum types - gomodguard: Allow/blocklist for direct Go module dependencies - goprintffuncname: Check printf-like functions are named with f - grouper: Analyze expression groups - iface: Detect incorrect use of interfaces - loggercheck: Check key-value pairs for common logger libraries - predeclared: Find code that shadows predeclared identifiers - reassign: Check that package variables are not reassigned - sloglint: Ensure consistent code style with log/slog - testableexamples: Check if examples are testable - usetesting: Report uses of functions with replacement in testing Also alphabetically sort the linter list for maintainability. Assisted-By: cagent
Detects SELECT * in SQL queries, encouraging explicit column selection. Assisted-By: cagent
Fix: use builder.Write(formatted) instead of builder.WriteString(string(formatted)) to avoid unnecessary allocation in transcript.go. Assisted-By: cagent
Fix unnecessary trailing newline in filesystem.go and unnecessary leading newline in mcp.go. Assisted-By: cagent
Reports constructs that check for err != nil but return a different nil value error. Assisted-By: cagent
Fix wasted assignments in oauth.go (use var instead of := for variables that are immediately reassigned) and fast_renderer.go (same pattern). Assisted-By: cagent
Add parameter name to interface method Update(msg tea.Msg) in layout.go Model interface. Assisted-By: cagent
Rename error types to follow Go convention (ErrXxx for values, XxxError for types): - ErrAutoModelFallback -> AutoModelFallbackError - ErrKeychainNotAvailable -> KeychainNotAvailableError - ErrPassNotAvailable -> PassNotAvailableError Assisted-By: cagent
Use net.JoinHostPort instead of fmt.Sprintf for host:port URL construction in DMR fallback URLs. This ensures correct handling of IPv6 addresses. Assisted-By: cagent
Add rows.Err() checks after row iteration loops in: - memory/database/sqlite/sqlite.go (GetMemories) - session/migrations.go (GetAppliedMigrations) - session/store.go (GetSessionSummaries) These are correctness bugs: without checking rows.Err(), I/O errors during iteration would be silently ignored. Assisted-By: cagent
Configure replace-allow-list to permit the intentional github.com/charmbracelet/ultraviolet replacement. Assisted-By: cagent
recvcheck flags types that mix pointer and value receivers. The 5 flagged types (DeferConfig, RAGStrategyConfig in v2/v3/latest) inherently require mixed receivers: MarshalYAML/MarshalJSON must use value receivers so they're callable on slice elements, while UnmarshalYAML/UnmarshalJSON must use pointer receivers to mutate. Added //nolint:recvcheck with explanatory comments on these types. Assisted-By: cagent
All 11 cases are deliberate patterns where err is checked but nil is returned: - Context cancellation treated as normal completion (acp, fake, mcp) - Fallback to partial/default result on non-critical error (html, fsx) - WalkDir callbacks skipping unreadable entries (skills, filesystem) - Script errors returned as data, not Go errors (codemode) - Non-JSON output skipping toon encoding (teamloader) Assisted-By: cagent
Fix three categories of error handling issues: - Use errors.Is instead of == for error comparison (6 fixes) io.EOF, sql.ErrNoRows, http.ErrServerClosed, context.Canceled - Use errors.As instead of type assertion on errors (4 fixes) *exec.ExitError, *environment.RequiredEnvError, cli.RuntimeError - Use %w instead of %v in fmt.Errorf for proper wrapping (8 fixes) api.go, eval.go, store.go, sqliteutil.go Assisted-By: cagent
All 11 issues were intentional patterns, requiring 11 //nolint directives with no actual code improvement. Assisted-By: cagent
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Review Complete - No Issues Found
This PR successfully enables 31 new linters and fixes all reported issues. After thorough analysis of the 700+ line diff across 34 files, no bugs were detected in the changed code.
Changes Verified
Error Handling (errorlint) ✓
- All type assertions correctly converted to
errors.Is()anderrors.As() - Proper error comparison for
io.EOF,sql.ErrNoRows,http.ErrServerClosed,context.Canceled, etc.
Error Type Renames (errname) ✓
ErrAutoModelFallback→AutoModelFallbackErrorErrKeychainNotAvailable→KeychainNotAvailableErrorErrPassNotAvailable→PassNotAvailableError- All references consistently updated throughout the codebase
Database Operations (rowserrcheck) ✓
- Added
rows.Err()checks after iteration in all SQL query functions - Proper error handling in
sqlite.go,store.go, andmigrations.go
Error Wrapping ✓
- Changed
%vto%winfmt.Errorf()calls for better error chains - Applied in
eval.go,api.go,store.go,sqlite.go
Network Operations (nosprintfhostport) ✓
- Correctly switched to
net.JoinHostPort()for URL construction indmr/client.go
Code Quality ✓
- Removed unnecessary string conversions (mirror)
- Cleaned up wasted assignments (wastedassign)
- Removed extra blank lines (whitespace)
- Added parameter names to interface methods (inamedparam)
- Added appropriate nolint comments for recvcheck linter
Summary
All linter fixes are correctly implemented with no functional regressions. The changes follow Go best practices and improve code quality, error handling, and maintainability.
Great work! 🎉
krissetto
approved these changes
Feb 6, 2026
| const Version = "0" | ||
|
|
||
| // Toolset represents a tool configuration | ||
| // Toolset represents a tool configuration. |
Contributor
There was a problem hiding this comment.
lol this is really pedantic tbh
Member
Author
There was a problem hiding this comment.
yeah, I need to take a look again. I want to catch issues. not this
Member
Author
There was a problem hiding this comment.
Only a couple of comments were invalid. Let's try to live with it and revisit
Contributor
There was a problem hiding this comment.
Review Summary
Analyzed 34 files with linter fixes. Found 1 issue worth noting:
Nested error wrapping pattern: The code uses multiple %w verbs in a single fmt.Errorf call, which doesn't work as intended in Go. While this doesn't cause functional issues in the current usage, it's semantically incorrect for Go's error handling model.
Overall, the linter fixes look good - the error handling improvements (using errors.Is/errors.As instead of direct comparisons) are solid improvements.
| // Return the original error if backup failed | ||
| slog.Error("Failed to backup database for recovery", "error", backupErr) | ||
| return nil, fmt.Errorf("migration failed: %w (backup also failed: %v)", err, backupErr) | ||
| return nil, fmt.Errorf("migration failed: %w (backup also failed: %w)", err, backupErr) |
Contributor
There was a problem hiding this comment.
Issue: Multiple %w verbs in fmt.Errorf
The code uses fmt.Errorf("migration failed: %w (backup also failed: %w)", err, backupErr) but Go's fmt.Errorf only wraps the last error when multiple %w verbs are used.
This means:
errors.Unwrap()will only returnbackupErr, noterrerrors.Is(returnedErr, err)will return false- The migration error (
err) cannot be introspected via the error chain
While both error messages appear in the string (so user-visible info is preserved), this violates Go's error wrapping semantics.
Suggested fix:
return nil, fmt.Errorf("migration failed: %w (backup also failed: %v)", err, backupErr)Or if you need to wrap both, create a custom error type or log one error separately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Newly Enabled Linters (31 added, 24 -> 55 total)