feat(opencode): load custom providers in model picker#946
Open
mauricioalfarodev wants to merge 2 commits into
Open
feat(opencode): load custom providers in model picker#946mauricioalfarodev wants to merge 2 commits into
mauricioalfarodev wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds JSONC comment-stripping and trailing-comma removal utilities to the config loader, refactors ChangesEffective Config Provider Loading
Sequence Diagram(s)sequenceDiagram
participant TUI as NewModelPickerState
participant Loader as LoadEffectiveConfigProvidersForDir
participant Global as globalConfigPaths
participant Project as projectConfigPaths
participant Parser as loadConfigProvidersData
participant Merger as mergeConfigProviders
TUI->>Loader: settingsPath (+ os.Getwd() as cwd)
Loader->>Global: settingsPath → opencode.json, opencode.jsonc
Loader->>Project: cwd → walk to .git, collect opencode.json/jsonc/.opencode/opencode.json
loop each discovered config path
Loader->>Parser: path / content
Parser->>Parser: stripJSONC + removeTrailingJSONCommas (if .jsonc or OPENCODE_CONFIG_CONTENT)
Parser-->>Loader: providers map
Loader->>Merger: accumulated + new providers
Merger-->>Loader: merged map (project overrides global)
end
Loader-->>TUI: merged providers, firstErr
TUI->>TUI: merge into picker state, set ConfigWarning if firstErr != nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/screens/model_picker.go (1)
88-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate warning text to reflect effective config sources.
After switching to
LoadEffectiveConfigProviders, the warning still saysopencode.json, which is misleading for env/project/global-source failures.💡 Proposed fix
var configWarning string if configErr != nil { - configWarning = fmt.Sprintf("Could not load custom providers from opencode.json: %v", configErr) + configWarning = fmt.Sprintf("Could not load custom providers from OpenCode config sources: %v", configErr) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/screens/model_picker.go` around lines 88 - 110, The configWarning message in the model_picker.go file incorrectly references only opencode.json as the configuration source, but the function LoadEffectiveConfigProviders loads configuration from multiple effective sources (environment, project, global configs, etc.). Update the warning message that is assigned to configWarning when configErr is not nil to accurately reflect that it could be failing to load custom providers from any of the effective configuration sources rather than just the opencode.json file specifically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/opencode/models_test.go`:
- Around line 663-676: The test setup for validating project inclusion is
missing a .git marker in projectDir, which means it cannot properly test
git-root boundary behavior. Add code to create a .git directory in projectDir
using os.Mkdir or os.MkdirAll after the workDir is created and before the
WriteFile call for opencode.json. This ensures the test matches the intended
discovery contract by establishing a clear git repository boundary at
projectDir.
In `@internal/opencode/models.go`:
- Around line 460-481: The `mergeConfigProviders` function initializes
`existing` as an empty struct and only copies the `Name` and `Models` fields,
which causes other ConfigProvider fields to be lost when processing new provider
IDs. Fix this by copying the full `incoming` provider to `existing` first when
the provider ID is new to the base map, then merge specific fields like `Models`
on top if needed, ensuring all provider metadata and options are preserved in
the merged configuration.
- Around line 435-446: The projectConfigPaths function continues traversing to
the filesystem root when no .git directory is found, which allows
ancestor-directory opencode.json files to be unexpectedly merged. Fix this by
ensuring the traversal stops at the Git repository root: once .git is found and
the loop breaks, return only the directories collected up to that point (those
within the Git repository). If no .git is found before reaching the filesystem
root, limit the results to prevent merging configuration files from outside the
intended project scope.
---
Outside diff comments:
In `@internal/tui/screens/model_picker.go`:
- Around line 88-110: The configWarning message in the model_picker.go file
incorrectly references only opencode.json as the configuration source, but the
function LoadEffectiveConfigProviders loads configuration from multiple
effective sources (environment, project, global configs, etc.). Update the
warning message that is assigned to configWarning when configErr is not nil to
accurately reflect that it could be failing to load custom providers from any of
the effective configuration sources rather than just the opencode.json file
specifically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c95c3e1-cef9-4c1e-808b-ee44af0f51b5
📒 Files selected for processing (4)
internal/opencode/models.gointernal/opencode/models_test.gointernal/tui/screens/model_picker.gointernal/tui/screens/model_picker_test.go
Comment on lines
+663
to
+676
| projectDir := filepath.Join(dir, "repo") | ||
| workDir := filepath.Join(projectDir, "subdir") | ||
| if err := os.MkdirAll(workDir, 0o755); err != nil { | ||
| t.Fatalf("create project dir: %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(projectDir, "opencode.json"), []byte(`{ | ||
| "provider": { | ||
| "lite-llm": {"name": "LITE-LLM", "models": {"proxy-model": {"name": "Proxy", "tool_call": true}}}, | ||
| "shared": {"name": "Project Shared", "models": {"project-model": {"name": "Project", "tool_call": true}}} | ||
| } | ||
| }`), 0o644); err != nil { | ||
| t.Fatalf("write project config: %v", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Strengthen this test by adding a .git marker.
This case currently validates project inclusion without creating .git, so it won’t catch regressions in git-root boundary behavior. Add .git in projectDir so the test matches the intended discovery contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/opencode/models_test.go` around lines 663 - 676, The test setup for
validating project inclusion is missing a .git marker in projectDir, which means
it cannot properly test git-root boundary behavior. Add code to create a .git
directory in projectDir using os.Mkdir or os.MkdirAll after the workDir is
created and before the WriteFile call for opencode.json. This ensures the test
matches the intended discovery contract by establishing a clear git repository
boundary at projectDir.
Comment on lines
+435
to
+446
| func projectConfigPaths(cwd string) []string { | ||
| var dirs []string | ||
| for dir := filepath.Clean(cwd); ; dir = filepath.Dir(dir) { | ||
| dirs = append(dirs, dir) | ||
| if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { | ||
| break | ||
| } | ||
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bound project discovery strictly to a detected Git root.
Current traversal appends every ancestor until / when .git is absent, so parent-directory opencode.json* files can be merged unexpectedly. That breaks the intended .git-scoped project resolution behavior.
💡 Proposed fix
func projectConfigPaths(cwd string) []string {
var dirs []string
+ foundGit := false
for dir := filepath.Clean(cwd); ; dir = filepath.Dir(dir) {
dirs = append(dirs, dir)
if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil {
+ foundGit = true
break
}
parent := filepath.Dir(dir)
if parent == dir {
break
}
}
+ if !foundGit {
+ return nil
+ }
paths := make([]string, 0, len(dirs)*3)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func projectConfigPaths(cwd string) []string { | |
| var dirs []string | |
| for dir := filepath.Clean(cwd); ; dir = filepath.Dir(dir) { | |
| dirs = append(dirs, dir) | |
| if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { | |
| break | |
| } | |
| parent := filepath.Dir(dir) | |
| if parent == dir { | |
| break | |
| } | |
| } | |
| func projectConfigPaths(cwd string) []string { | |
| var dirs []string | |
| foundGit := false | |
| for dir := filepath.Clean(cwd); ; dir = filepath.Dir(dir) { | |
| dirs = append(dirs, dir) | |
| if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { | |
| foundGit = true | |
| break | |
| } | |
| parent := filepath.Dir(dir) | |
| if parent == dir { | |
| break | |
| } | |
| } | |
| if !foundGit { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/opencode/models.go` around lines 435 - 446, The projectConfigPaths
function continues traversing to the filesystem root when no .git directory is
found, which allows ancestor-directory opencode.json files to be unexpectedly
merged. Fix this by ensuring the traversal stops at the Git repository root:
once .git is found and the loop breaks, return only the directories collected up
to that point (those within the Git repository). If no .git is found before
reaching the filesystem root, limit the results to prevent merging configuration
files from outside the intended project scope.
Comment on lines
+460
to
+481
| func mergeConfigProviders(base, override map[string]ConfigProvider) map[string]ConfigProvider { | ||
| if len(override) == 0 { | ||
| return base | ||
| } | ||
| if base == nil { | ||
| base = map[string]ConfigProvider{} | ||
| } | ||
| for id, incoming := range override { | ||
| existing := base[id] | ||
| if incoming.Name != "" { | ||
| existing.Name = incoming.Name | ||
| } | ||
| if len(incoming.Models) > 0 { | ||
| if existing.Models == nil { | ||
| existing.Models = map[string]ConfigModel{} | ||
| } | ||
| for modelID, model := range incoming.Models { | ||
| existing.Models[modelID] = model | ||
| } | ||
| } | ||
| base[id] = existing | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve full provider structs when merging new provider IDs.
mergeConfigProviders initializes existing := base[id] and only copies Name/Models. For first-time IDs, other parsed fields (e.g., provider metadata like npm/options) are dropped, corrupting merged config data.
💡 Proposed fix
func mergeConfigProviders(base, override map[string]ConfigProvider) map[string]ConfigProvider {
if len(override) == 0 {
return base
}
if base == nil {
base = map[string]ConfigProvider{}
}
for id, incoming := range override {
- existing := base[id]
+ existing, exists := base[id]
+ if !exists {
+ // Preserve full provider payload on first insert.
+ base[id] = incoming
+ continue
+ }
if incoming.Name != "" {
existing.Name = incoming.Name
}
if len(incoming.Models) > 0 {
if existing.Models == nil {
existing.Models = map[string]ConfigModel{}
}
for modelID, model := range incoming.Models {
existing.Models[modelID] = model
}
}
base[id] = existing
}
return base
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func mergeConfigProviders(base, override map[string]ConfigProvider) map[string]ConfigProvider { | |
| if len(override) == 0 { | |
| return base | |
| } | |
| if base == nil { | |
| base = map[string]ConfigProvider{} | |
| } | |
| for id, incoming := range override { | |
| existing := base[id] | |
| if incoming.Name != "" { | |
| existing.Name = incoming.Name | |
| } | |
| if len(incoming.Models) > 0 { | |
| if existing.Models == nil { | |
| existing.Models = map[string]ConfigModel{} | |
| } | |
| for modelID, model := range incoming.Models { | |
| existing.Models[modelID] = model | |
| } | |
| } | |
| base[id] = existing | |
| } | |
| func mergeConfigProviders(base, override map[string]ConfigProvider) map[string]ConfigProvider { | |
| if len(override) == 0 { | |
| return base | |
| } | |
| if base == nil { | |
| base = map[string]ConfigProvider{} | |
| } | |
| for id, incoming := range override { | |
| existing, exists := base[id] | |
| if !exists { | |
| // Preserve full provider payload on first insert. | |
| base[id] = incoming | |
| continue | |
| } | |
| if incoming.Name != "" { | |
| existing.Name = incoming.Name | |
| } | |
| if len(incoming.Models) > 0 { | |
| if existing.Models == nil { | |
| existing.Models = map[string]ConfigModel{} | |
| } | |
| for modelID, model := range incoming.Models { | |
| existing.Models[modelID] = model | |
| } | |
| } | |
| base[id] = existing | |
| } | |
| return base | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/opencode/models.go` around lines 460 - 481, The
`mergeConfigProviders` function initializes `existing` as an empty struct and
only copies the `Name` and `Models` fields, which causes other ConfigProvider
fields to be lost when processing new provider IDs. Fix this by copying the full
`incoming` provider to `existing` first when the provider ID is new to the base
map, then merge specific fields like `Models` on top if needed, ensuring all
provider metadata and options are preserved in the merged configuration.
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.
Linked Issue
Closes #TODO
PR Type
What kind of change does this PR introduce?
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)Summary
opencode.json,opencode.jsonc,.opencode.json,.opencode.jsonc, and inline OpenCode config content with explicit precedence.opencode.jsoncwere skipped whenopencode.jsonalready existed.Changes
internal/opencode/models.gointernal/opencode/models_test.go.jsoncglobal config regression.internal/tui/screens/model_picker.gointernal/tui/screens/model_picker_test.goTest Plan
Unit Tests
go test ./internal/opencodeLocal Verification
go test ./internal/opencode)cd e2e && ./docker-test.sh)Notes:
git diff --checkpassed locally before the latest commit.go test ./internal/opencodecould not be run in this terminal becausegois not available in PATH.Automated Checks
The following checks run automatically on this PR:
additions + deletions) or usesize:exceptionCloses/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*label must be appliedgo test ./...must passcd e2e && ./docker-test.shmust passContributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-BytrailersNotes for Reviewers
Replace
#TODOwith the approved issue number before creating the PR, and add thetype:featurelabel after opening it.This PR is 551 changed lines against
Gentleman-Programming/gentle-ai:main, so it needs either a maintainer-appliedsize:exceptionlabel or a split into smaller PRs before the policy check can pass.Summary by CodeRabbit