Opencode supports OpenAI SDK and Anthropic SDK API. Favorites ids are…#408
Opencode supports OpenAI SDK and Anthropic SDK API. Favorites ids are…#408eli-l wants to merge 3 commits into
Conversation
… unique following <provider>/<model> format
There was a problem hiding this comment.
Pull request overview
This PR expands provider/model handling to support additional OpenCode provider variants, makes model favorites provider-qualified (<provider>/<model>), and persists live-discovered model lists into config so they can be reused across sessions.
Changes:
- Add new provider catalog entries for OpenCode-Go OpenAI SDK and Anthropic SDK variants, plus curated model lists and provider-specific model filtering.
- Persist live-discovered models into config (
ProviderProfile.Models) and thread discovered models through setup flows. - Update TUI model picker and provider wizard UI behavior to prefer raw model IDs and provider-qualified favorites.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/provider_wizard.go | Persist discovered models after saving provider; refresh savedProviders; adjust model label/secondary text rendering; add mergeSavedProvider. |
| internal/tui/provider_wizard_test.go | Update assertions to match new model label/secondary text behavior. |
| internal/tui/picker.go | Provider-qualified favorite keys, updated picker labeling, and favorite toggling keyed by provider/model. |
| internal/tui/picker_test.go | Update picker tests for provider-qualified favorites and raw ID rendering. |
| internal/tui/options.go | Add SetupSelection.Models to carry discovered models through setup. |
| internal/tui/onboarding.go | Include discovered models in setup save request. |
| internal/providermodeldiscovery/discovery.go | Filter discovered/catalog models through provider-specific allow/block logic. |
| internal/providermodelcatalog/filter.go | Add provider-aware model ID allow/block and filtering helpers. |
| internal/providermodelcatalog/catalog.go | Add curated models for new OpenCode-Go providers; apply provider-specific filtering. |
| internal/providercatalog/catalog.go | Register new OpenCode-Go OpenAI/Anthropic SDK provider descriptors. |
| internal/providercatalog/catalog_test.go | Update expected catalog IDs and per-transport ordering. |
| internal/config/writer.go | Add persistence/merge logic for discovered models; add profile-splitting helper; add dedupe helper. |
| internal/config/writer_test.go | Add tests for discovered model persistence/merge behavior and validation. |
| internal/config/types.go | Introduce DiscoveredModel; add ProviderProfile.Models and unmarshal support. |
| internal/config/types_test.go | Add JSON round-trip tests for DiscoveredModel and ProviderProfile.Models. |
| internal/cli/setup.go | Persist discovered models after provider setup (non-fatal on failure). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func dedupDiscoveredModels(models []DiscoveredModel) []DiscoveredModel { | ||
| seen := map[string]bool{} | ||
| out := make([]DiscoveredModel, 0, len(models)) | ||
| for _, m := range models { | ||
| if seen[m.ID] { | ||
| continue | ||
| } | ||
| seen[m.ID] = true | ||
| out = append(out, m) | ||
| } | ||
| return out | ||
| } |
|
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 (1)
💤 Files with no reviewable changes (1)
WalkthroughAdds discovered-model persistence through setup and wizard flows, extends provider and model catalogs with OpenCode-Go entries and provider-specific filtering, and changes picker favorites and labels to use provider-qualified model IDs. ChangesDiscovered model persistence and provider-qualified UI
Estimated code review effort: 4 (Complex) | ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/provider_wizard.go (1)
906-934: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-fatal warning is set but immediately discarded — never reaches the user.
wizard.erris set on line 930 whenSetProviderDiscoveredModelsfails, butapplyProviderWizardunconditionally executesm.providerWizard = nilat line 958 before returning, in the same call and with no render step in between. Sincewizardism.providerWizard(a pointer, confirmed by the nil-check at function entry), setting it tonildiscards the entire wizard state — including the just-seterr— before it can ever be displayed. The intended non-fatal warning is effectively silent, defeating the purpose of setting it at all.Store the warning somewhere that survives past
m.providerWizard = nil(e.g. a persistent status/toast field onmodel), or surface it before nilling the wizard.🐛 Illustrative fix (adjust to the model's actual status-message mechanism)
if len(discovered) > 0 { if _, err := config.SetProviderDiscoveredModels(m.userConfigPath, profile.Name, discovered); err != nil { - // Non-fatal: the provider was already saved. Log but don't block. - // (Redact the path in case it contains identifying info.) - wizard.err = strings.TrimSpace("saved provider but could not persist models") + // Non-fatal: the provider was already saved. Surface via a field + // that outlives the wizard, since m.providerWizard is nilled below. + m.postSetupWarning = "saved provider but could not persist models" } }🤖 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/provider_wizard.go` around lines 906 - 934, The non-fatal warning assigned in applyProviderWizard is lost because m.providerWizard is cleared before the UI can render it. Update the applyProviderWizard flow in provider_wizard.go so the SetProviderDiscoveredModels failure message is stored in a model-level status/toast field or otherwise surfaced before m.providerWizard is set to nil. Keep the existing warning text generation near the wizard.err assignment, but move delivery of that message to a state that survives beyond the wizard pointer reset.
🧹 Nitpick comments (3)
internal/providermodelcatalog/catalog.go (1)
169-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the repeated
FilterModelsForProviderwrapping.All three return branches now separately call
FilterModelsForProvider(provider.ID, dedupeModels(...)). Collapsing this into a single filtered return at the end reduces duplication and removes the risk that a future new branch forgets to wrap the result in the filter.♻️ Proposed refactor
func Models(provider providercatalog.Descriptor) []Model { - if models, ok := curatedModels[provider.ID]; ok { - return FilterModelsForProvider(provider.ID, dedupeModels(provider.DefaultModel, models)) - } - models := registryModels(provider) - if len(models) > 0 { - return FilterModelsForProvider(provider.ID, dedupeModels(provider.DefaultModel, models)) - } - return FilterModelsForProvider(provider.ID, dedupeModels(provider.DefaultModel, nil)) + models, ok := curatedModels[provider.ID] + if !ok { + models = registryModels(provider) + } + return FilterModelsForProvider(provider.ID, dedupeModels(provider.DefaultModel, models)) }🤖 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/providermodelcatalog/catalog.go` around lines 169 - 178, The Models function repeats the same FilterModelsForProvider(provider.ID, dedupeModels(...)) wrapping in every branch, so refactor it to compute the models slice once from curatedModels or registryModels and then apply the filter in a single return at the end. Keep using provider.DefaultModel with dedupeModels and preserve the existing fallback to nil when no registry models are found, but centralize the final FilterModelsForProvider call to avoid duplicated logic in Models.internal/providermodelcatalog/filter.go (1)
3-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded provider allow-list logic will get harder to maintain as more providers need filtering.
opencode-go-anthropicis special-cased directly by ID string inside the switch. If another provider needs a similar allow-list later, this pattern requires another hardcoded case here, disconnected from the provider's own descriptor/catalog entry. Consider making the allow-list rule part of the provider data (e.g., anAllowedModelSubstrings/AllowedModelPatternfield on the catalog descriptor or curated model set) so the constraint lives next to the provider definition instead of in a separate switch statement.Also, this new exported logic (
ModelIDAllowedForProvider/FilterModelsForProvider) doesn't appear to have its own dedicated unit tests in this diff — onlycatalog_test.go's ID/order checks exercise it indirectly. A few direct test cases (e.g., assertingqwen/minimaxpass and other models are rejected foropencode-go-anthropic, and that all models pass through for unrelated providers) would guard against regressions in this filter 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/providermodelcatalog/filter.go` around lines 3 - 24, The hardcoded provider-specific allow-list in ModelIDAllowedForProvider should be moved out of the switch and into the provider’s catalog data so filtering rules live with the provider descriptor instead of being special-cased by normalized provider ID. Update the provider catalog/descriptors to carry an allow-list field (or equivalent curated model rule) and have ModelIDAllowedForProvider consult that data rather than strings like opencode-go-anthropic directly. Also add dedicated unit tests for this filter contract covering qwen/minimax acceptance, rejection of other models for opencode-go-anthropic, and passthrough behavior for unrelated providers.internal/config/writer.go (1)
184-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for
PersistTwoProfiles/persistProfileGroup.
writer_test.goonly adds tests forSetProviderDiscoveredModels; the new profile-splitting logic (classifier partitioning, single-vs-both-format no-op behavior, API key vs. APIKeyEnv branch) has no tests in this diff, despite being the mechanism that implements the PR's core "OpenAI SDK + Anthropic SDK" split.🤖 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/config/writer.go` around lines 184 - 235, Add tests for PersistTwoProfiles and persistProfileGroup to cover the new split logic: verify the classifier partitions models into OpenAI-compatible vs Anthropic-compatible groups, that the function is a no-op when only one format is present, and that both profiles are persisted only when both groups exist. Use the existing symbols PersistTwoProfiles, persistProfileGroup, and the provider profile helpers to assert the APIKey vs APIKeyEnv branch and the correct profile names/format values are written.
🤖 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/cli/setup.go`:
- Around line 271-279: The empty error branch in the SetProviderDiscoveredModels
call is triggering SA9003 and hides useful diagnostics. Update the
selection.Models persistence block in setup.go so the err from
config.SetProviderDiscoveredModels is actually handled, ideally by logging a
non-fatal warning with profile.Name and configPath while still allowing setup to
continue. Keep the existing non-blocking behavior, but replace the empty if body
with an explicit use of err so failures are visible.
In `@internal/tui/picker.go`:
- Around line 350-369: The dedup logic in the picker assembly is using the wrong
key and never applies to catalog items, so the same provider+model can be
emitted twice. Update the recent/catalog merge in the function that builds the
picker list to use modelFavoriteKey(item) consistently for both seen checks and
writes, and apply the same dedup check before appending catalog entries while
still allowing cross-provider duplicates. Add a regression test around the
picker logic to cover an item appearing in both recent and catalog for the same
provider.
---
Outside diff comments:
In `@internal/tui/provider_wizard.go`:
- Around line 906-934: The non-fatal warning assigned in applyProviderWizard is
lost because m.providerWizard is cleared before the UI can render it. Update the
applyProviderWizard flow in provider_wizard.go so the
SetProviderDiscoveredModels failure message is stored in a model-level
status/toast field or otherwise surfaced before m.providerWizard is set to nil.
Keep the existing warning text generation near the wizard.err assignment, but
move delivery of that message to a state that survives beyond the wizard pointer
reset.
---
Nitpick comments:
In `@internal/config/writer.go`:
- Around line 184-235: Add tests for PersistTwoProfiles and persistProfileGroup
to cover the new split logic: verify the classifier partitions models into
OpenAI-compatible vs Anthropic-compatible groups, that the function is a no-op
when only one format is present, and that both profiles are persisted only when
both groups exist. Use the existing symbols PersistTwoProfiles,
persistProfileGroup, and the provider profile helpers to assert the APIKey vs
APIKeyEnv branch and the correct profile names/format values are written.
In `@internal/providermodelcatalog/catalog.go`:
- Around line 169-178: The Models function repeats the same
FilterModelsForProvider(provider.ID, dedupeModels(...)) wrapping in every
branch, so refactor it to compute the models slice once from curatedModels or
registryModels and then apply the filter in a single return at the end. Keep
using provider.DefaultModel with dedupeModels and preserve the existing fallback
to nil when no registry models are found, but centralize the final
FilterModelsForProvider call to avoid duplicated logic in Models.
In `@internal/providermodelcatalog/filter.go`:
- Around line 3-24: The hardcoded provider-specific allow-list in
ModelIDAllowedForProvider should be moved out of the switch and into the
provider’s catalog data so filtering rules live with the provider descriptor
instead of being special-cased by normalized provider ID. Update the provider
catalog/descriptors to carry an allow-list field (or equivalent curated model
rule) and have ModelIDAllowedForProvider consult that data rather than strings
like opencode-go-anthropic directly. Also add dedicated unit tests for this
filter contract covering qwen/minimax acceptance, rejection of other models for
opencode-go-anthropic, and passthrough behavior for unrelated providers.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd6fb509-077f-48f2-a054-4b6dcd4ef28b
📒 Files selected for processing (16)
internal/cli/setup.gointernal/config/types.gointernal/config/types_test.gointernal/config/writer.gointernal/config/writer_test.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/providermodelcatalog/catalog.gointernal/providermodelcatalog/filter.gointernal/providermodeldiscovery/discovery.gointernal/tui/onboarding.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for this — the request path and existing provider-ID resolution are safe, but two things need addressing before merge.
1. Saved favorites are silently invalidated on upgrade (no migration). This re-keys favorites from bare <model> to <provider>/<model> (modelFavoriteKey in picker.go). Existing users' favorites are stored bare, so after upgrade m.favoriteModels[fk] misses — every favorite silently drops out of the Favorites group and lingers as orphaned config. The tell is that picker_test.go:416 had to change the stored favorite to the new key format to stay green. Please add a bare-key fallback in the lookup (or a one-time migration on load) so existing favorites survive.
2. The advertised SDK-split looks like inert plumbing. PersistTwoProfiles (config/writer.go) has no callers, the IsAnthropicModel classifier it expects isn't in the tree, and the new ProviderProfile.Models field is written but never read. If shipping it unwired is intentional, please note that in the PR description — as-is the title oversells what's actually active. (It being inert does mean it can't misroute existing providers.)
Minor: opencode-go-openai reuses the identical DefaultBaseURL as the existing opencode-go (catalog.go) — resolves fine today by registration order, but it's a latent footgun if descriptors get reordered.
build / vet / test are otherwise green. Happy to re-review once the favorites migration is in.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Thanks for the PR. Per CONTRIBUTING.md, community pull requests must be tied to an approved issue with the issue-approved label. This PR:
- Has no linked issue (
Fixes #is blank) - Has the checklist entirely unchecked
- Has no description of what changed and why
Please open an issue first describing the problem you want to solve, and once it gets the issue-approved label, send the PR referencing it. The linked issue also helps scope the change — right now this PR mixes catalog entries, favorites re-keying, model persistence, and a dead PersistTwoProfiles, which is hard to review as one change.
|
This PR addresses a few problems:
FYI @gnanam1990 it does not make sense to migrate old identifiers, as config might already contain multiple providers for the same model and it won't be possible to determine which provider was used for favourited item. The only viable solution I see here is to drop favourite records entirely, to prevent dangling items.
|
Summary
Introduce distinction for Opencode Go models, as per documentation some of these shall be used via Anthropic-like connector (SDK) as per documentation https://opencode.ai/docs/go/#endpoints (Minimax and Qwen family)
These are still preserved over OpenAI-like connector (SDK) as these are working (undocumented). It provides clear indication how models are used and at least - a built-in fallback in case side effect (undocumented) OpenAI-like access stop working for these models.
/models API does not provide details of these, so filtering had to be added (hardcoded)
Opencode provides /model endpoint, that allows self-discovery. It sounds reasonable to preserve a list of provided models and make these available to be chosen once discovered, rather than adding models one by one.
Configuring models as provider-model configuration entry requires separate naming and prevents effective key management. Re-keying basically means all entries must be updated.
Multiple providers might provide the same models. In such setup current approach to favourite entries becomes unambiguous. This PR introduces / format, making clear which provider will be used for the favourite model. Failing to pick proper provider may cause unwanted costs for the users.
Linked issue
#428
#429
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests