refactor: extract openai_compat, fix bugs, and modernize Go idioms#7
Merged
refactor: extract openai_compat, fix bugs, and modernize Go idioms#7
Conversation
- Move ToolError and generateRequestID from edit.go to util.go so all tools in the package share a single definition rather than relying on cross-file visibility - Add panic guard to Registry.Register for empty tool names (programming error caught at startup) - Fix ImageRefPattern to use (?i) flag so uppercase extensions (.PNG, .JPG) are matched correctly - Add file path context to image load errors for easier debugging - Mark PermissionAllow/Deny/Ask/PermissionResult aliases as Deprecated via godoc comments - Modernize extractBashCommands to use strings.SplitSeq Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SetTokenLimit was modifying ModelInfo slice elements via a range-copy of ModelCache. The range copy shares the underlying array with the stored data, so concurrent readers holding a slice from GetCachedModels could observe partial writes without holding any lock. Fix: copy the slice into a fresh allocation before mutation, then store the new slice back. Also modernize GetConnections to use maps.Copy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dead code removal: - Delete unused containsIgnoreCase in cmd/gen/agent.go (and the strings import) - Delete unused getPlanSeparatorStyle/getPlanTitleStyle in prompt_plan.go - Remove unused installScopes/installScopeIdx fields from plugin model Code quality: - Replace empty if-err branch with _ = in plugin/integration.go - Simplify Extra loop to append(parts, s.Extra...) in system.go - Inline blank-identifier assignment in askuser.go - Use UpdateMsg(u) type conversion in progress.go (same underlying type) - Fix capitalized error string in exa.go - Convert if-else chains to switch in header.go and skill/loader.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
MCP client:
- Explicitly discard transport.Close() errors in Connect() error paths
(cleanup is best-effort; original error is already being returned)
- Fix handleNotification goroutine to check ListTools error and return
early rather than silently skipping the onToolsChanged callback
- Replace interface{} with any in helper functions (Go 1.18+ idiom)
- Explicitly discard Disconnect() errors in RemoveServer/DisconnectAll
and in ConnectServers cleanup (cleanup-path, errors are non-actionable)
- Explicitly discard io.Copy drain errors in HTTP/SSE transports
- Use defer func() { _ = r.Close() }() for deferred close in SSE readLoop
Other:
- Explicitly discard rand.Read error in cron/store.go generateID
- Explicitly discard json.Unmarshal/os.WriteFile in mcp/config.go
- Explicitly discard os.Remove in image/clipboard.go temp file cleanup
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…check
Permission constants:
- Replace config.PermissionAllow/Deny/Ask with config.Allow/Deny/Ask
in app/tool/run.go (following the Deprecated: godoc annotations)
Unchecked errors (best-effort cleanups):
- hooks/engine.go: discard stdinPipe.Close() errors (cleanup path)
- mcp/transport/stdio.go: discard stdin/stdout Close, Signal, and Kill
errors during server shutdown (best-effort termination)
- app/mcp/commands.go: discard Close/Remove errors on tmpFile and
use defer func(){_ = os.Remove(...)}() for cleanup defers
- app/mcp/model.go: discard Disconnect error in HandleDisconnect
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace empty if-err {} branches with _ = ... in plugin code
- Remove redundant embedded field name StylePrimitive in markdown.go
glamour style configuration (can use promoted fields directly)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Discard config.Reload() returns with _, _ = (reload is best-effort)
- Discard os.Remove errors in todostore.go cleanup paths
- Discard os.Remove error in cmd/gen/mcp.go editor failure cleanup
- Use _, _ = fmt.Fprintln and _ = w.Flush() in history.go write path
- Explicitly discard tty.WriteString, tty.Close, and exec.Run in
handler_command.go /clear command (terminal ops, errors non-actionable)
- Use defer func() { _ = log.Sync() }() in main.go
- Use defer func() { _ = resp.Body.Close() }() in webfetch.go
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- New internal/provider/openai_compat package eliminates ~600 lines of
duplication across openai/moonshot/alibaba providers:
ConvertMessages, ConvertTools, MapFinishReason, ExtractReasoningContent
are shared; each provider injects only its provider-specific behavior
(e.g. reasoning_content) via a pluggable convertAssistant function.
- New param helpers in internal/tool/util.go (requireString, getString,
getBool, getFloat64, getInt) applied across all 20+ tool files to
replace repetitive type-assertion boilerplate and float64/int dual
checks from JSON unmarshaling.
- Replace remaining interface{} with any in types, MCP transport, and
tool interface definitions.
- Add zap.Error logging to compactAndReplace (was silently swallowing
compaction failures).
- Minor modernizations: bytes.IndexByte for binary detection in read.go,
max(min(...)) for textarea height clamping, min() for timeout clamping
in bash.go and taskoutput.go.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on, modernize sorts Session store: - loadWithoutLock() now delegates to loadFromFile() — single canonical JSONL parser instead of two nearly-identical 50-line functions - Extract applyMetadata() helper to copy EntryMetadata_ fields into SessionMetadata, eliminating the duplicated field-by-field block - Replace sort.Slice with slices.SortFunc (time.Compare) for List() and listByScanning() Anthropic provider: - Extract convertAnthropicTools() and anyStrings() helpers; the tool conversion loop is now 5 lines instead of 25 - anyStrings() normalises the JSON Schema "required" field which may arrive as []string or []any depending on the caller Core package: - Add godoc to Messages(), SetMessages(), Tokens(), AddUser(), AddToolResult() - Clarify DecideCompletion() precedence: tool calls always execute even when stop_reason == "max_tokens" Provider sorts: - Replace sort.Slice/sort.Strings with slices.SortFunc / slices.Sorted across alibaba, moonshot, openai, google providers and streamutil (AddToolCallsSorted, AddToolCallsByKey now use maps.Keys + slices.Sorted) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 tasks
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (85)
📝 WalkthroughWalkthroughThis PR applies systematic code quality improvements across the codebase: converting Changes
Sequence Diagram(s)No sequence diagrams generated. Changes consist primarily of type alias updates, error handling standardization, parameter parsing consolidation, and utility refactoring—without significant new control flow or multi-component interactions warranting visualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In internal/config/permission.go, extractBashCommands now uses strings.SplitSeq with range (
for part := range strings.SplitSeq(...)), but the standard library strings package has no SplitSeq and range over a function call like this is invalid; this will fail to compile unless there is a custom strings implementation, so either restore strings.Split or implement/rename the intended helper.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In internal/config/permission.go, extractBashCommands now uses strings.SplitSeq with range (`for part := range strings.SplitSeq(...)`), but the standard library strings package has no SplitSeq and range over a function call like this is invalid; this will fail to compile unless there is a custom strings implementation, so either restore strings.Split or implement/rename the intended helper.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
1 participant
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.
Summary
Cherry-picked the valuable parts of PR #5 (originally
refactor/deep-cleanup) onto the new main after PR #6 merged:internal/provider/openai_compat— new shared package eliminating ~600 lines of duplication across openai/moonshot/alibaba providers (ConvertMessages,ConvertTools,MapFinishReason,ExtractReasoningContent)internal/tool/util.go— param helpers (requireString,getString,getBool, etc.) applied across 20+ tool files to replace repetitive type-assertion boilerplatestore.SetTokenLimit— concurrent readers could observe partial writes without holding a lockconfig.PermissionAllow/Denywithconfig.Allow/DenycontainsIgnoreCase, unused style functions,installScopeIdxfieldloadWithoutLock()delegates toloadFromFile(), single canonical JSONL parserconvertAnthropicTools()helper; tool conversion loop now 5 lines instead of 25sort.Slice→slices.SortFuncacross providers and session storeSkipped: the original first commit (
ed0adcearchitecture cleanup) which was entirely superseded by PR #6.Test plan
go build ./...passesgo test ./...passesSummary by Sourcery
Extract shared OpenAI-compatible message/tool conversion into a reusable helper package, apply new parameter utility helpers across tools, fix concurrency and error-handling issues, and modernize assorted Go patterns and APIs.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Chores