Introduce new favorite record format. Favourite (config.json) cleanup CLI command. Discovered model persistent in config.json.#408
Conversation
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds discovered-model persistence to setup and discovery flows, updates picker favorites to use provider-qualified keys, and adds config cleanup plus semantic validation for stale favorite-model and provider-model references. ChangesModel persistence and cleanup
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.
|
|
hello @eli-l thank you so much for your contributions. kindly rebase to main and fix conflicts |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Request changes — good ideas, but a silent favorites regression, inert plumbing, and an unscoped bundle
Thanks @eli-l — the provider-qualified favorites idea (#429) is a real improvement, and documenting the Anthropic-SDK transport for the OpenCode-Go Minimax/Qwen models (#428) is sensible. I ran go build, go vet, and the touched-package + TUI picker/wizard tests on Windows — all green. But as bundled this isn't mergeable yet, and I'd ask for a split.
🔴 Blocker — favorites silently break on upgrade (no migration)
picker.go now keys favorites via modelFavoriteKey(item) = <provider>/<model>, and every recent/catalog item now carries OwnerProvider = m.providerName. But existing favorites are stored bare (e.g. deepseek-v4-pro) and loaded as bare map keys, so after upgrade m.favoriteModels["opencode-go/deepseek-v4-pro"] misses — every pre-existing favorite drops out of the Favorites group and lingers as orphaned config. The modelFavoriteKey doc comment claims legacy favorites still match, but that branch only fires when OwnerProvider is empty, which never happens for a real model. The tell: picker_test.go:416 had to rewrite the stored favorite to the new prefixed form to stay green. Fix: add a bare-key fallback in the lookup, or a one-time migration on config load. (Also found independently by @gnanam1990.)
🟡 Major — the "SDK-split" persistence half is inert
config.DiscoveredModel, ProviderProfile.Models, SetProviderDiscoveredModels, and the threading through setup/onboarding/wizard persist a discovered-model list — but nothing in internal/providers/factory.go or internal/modelregistry ever reads it (the type comment itself says APIModel is "not yet wired into the factory"). So it's new config surface + merge/dedup/sort logic for zero active behavior, and the PR title oversells it. Either wire a consumer or drop it and reintroduce it alongside the factory change that uses it. (Credit for already removing the dead PersistTwoProfiles.)
🟡 Major — process / scope
Per CONTRIBUTING.md this needs a linked issue-approved issue (checklist unchecked; @anandh8x already raised this), and the branch is CONFLICTING — a rebase is required before CI is meaningful. Please split into: (1) provider-qualified favorites with migration, (2) the new OpenCode-Go SDK-variant catalog entries gated on the approved issue, (3) persistence deferred until a consumer lands.
Minor
opencode-go-openaireuses the identicalDefaultBaseURLas the mergedopencode-go— resolves by registration order today, latent footgun.- The
qwen/minimaxsubstring allow-list infilter.gois special-cased by provider-ID string with no direct tests; a per-descriptor allow-list field would keep the rule next to the provider.
CI note: fork PR, so Go workflows are maintainer-gated and haven't run — only CodeRabbit did; I verified build/vet/tests locally on Windows. Happy to re-review once the favorites migration is in and the scope is trimmed.
900b285 to
0b48a9f
Compare
|
There is nothing to migrate, as existing favourites are ambiguous and I am not making up the migration, that guesses the provider. My suggestion was to drop the existing favourites if these matches old format. This has not response. Overall feels like AI gen slop review. I strongly suggest human to take a look on this and agree on decisions or otherwise drop, as I have no intention arguing with bots. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
VERDICT: needs-changes
REGRESSION RISK: medium
Re-review of new commits since my prior review (61e3f232 → 0b48a9fd). The new commits address review feedback (fix PR comments, remove unused methods) but introduce a catalog test failure that CI has not caught because the CI checks are in action_required state (not yet approved to run on the latest commits).
BUILD / TEST
go build ./...— passgo vet ./internal/cli/... ./internal/config/... ./internal/providercatalog/... ./internal/tui/...— passgofmt -lon changed files — cleango test ./internal/config/...— passgo test ./internal/providercatalog/...— FAIL.TestListByTransportfails atcatalog_test.go:310: the expectedTransportOpenAICompatlist does not include"opencode-go", but the PR addsopenAICompat("opencode-go", ...)tocatalog.go:142. The test's expected list at line 297 includes"opencode"and"opencode-go-openai"but not"opencode-go". The PR must add"opencode-go"to the expected list in the test.- CI has not run on the latest commit (
0b48a9fd) — the workflow runs are inaction_requiredstate. This failure would be caught by the Ubuntu smoke job once CI is approved.
FINDINGS
- Blocker —
internal/providercatalog/catalog_test.go:297— missing"opencode-go"in expectedTransportOpenAICompatlist. The PR addsopenAICompat("opencode-go", ...)tocatalog.go:142but does not add"opencode-go"to the expected list in theTestListByTransporttest. The test fails withListByTransport("openai-compatible") IDs = [..., "opencode-go", ...], want[..., (no "opencode-go"), ...]. Fix: add"opencode-go"to the expected list at line 297, between"opencode"and"atomic-chat".
BOTTOM LINE
The review feedback cleanup is good, but the catalog test is red because a new provider ID was added without updating the expected test list; CI hasn't caught it yet because the runs are pending approval.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/providermodelcatalog/catalog.go (1)
159-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct test case for
opencode-go-anthropicfiltering.
ModelIDAllowedForProvidersingles outopencode-go-anthropicfor the qwen/minimax restriction — the core behavior this PR introduces — but the provider-scoped test table shown incatalog_test.gocoversollama-cloud,ollama,groq,chatgpt,mistral,minimaxi-cn,zai-cn, and doesn't appear to includeopencode-go-anthropicitself. Adding a case assertingModels(opencode-go-anthropic-descriptor)includesMiniMax-M3/qwen-plusand excludes something likegpt-4.1/claude-sonnet-4.5would directly pin down this new gating rule.🤖 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 159 - 164, Add a direct test for the new `opencode-go-anthropic` filtering path so the core gating rule is covered explicitly. Update `catalog_test.go` to include a case for `opencode-go-anthropic` using the provider descriptor and assert that `Models(opencode-go-anthropic-descriptor)` returns the allowed qwen/minimax entries such as `MiniMax-M3` and `qwen-plus`, while excluding disallowed models like `gpt-4.1` and `claude-sonnet-4.5`. Use the existing `ModelIDAllowedForProvider` and `Models` behavior to keep the test aligned with the provider-scoped filtering logic.
🤖 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.
Nitpick comments:
In `@internal/providermodelcatalog/catalog.go`:
- Around line 159-164: Add a direct test for the new `opencode-go-anthropic`
filtering path so the core gating rule is covered explicitly. Update
`catalog_test.go` to include a case for `opencode-go-anthropic` using the
provider descriptor and assert that `Models(opencode-go-anthropic-descriptor)`
returns the allowed qwen/minimax entries such as `MiniMax-M3` and `qwen-plus`,
while excluding disallowed models like `gpt-4.1` and `claude-sonnet-4.5`. Use
the existing `ModelIDAllowedForProvider` and `Models` behavior to keep the test
aligned with the provider-scoped filtering logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fd77691-8969-49c6-903a-cecd0c7d4dfb
📒 Files selected for processing (3)
internal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/providermodelcatalog/catalog.go
💤 Files with no reviewable changes (1)
- internal/providercatalog/catalog.go
✅ Files skipped from review due to trivial changes (1)
- internal/providercatalog/catalog_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a few issues that need to be addressed before this is ready.
Findings
-
[P1] Use the documented OpenCode Go model IDs
internal/providermodelcatalog/catalog.go:159
The new Anthropic-compatible OpenCode Go catalog entries useMiniMax-M3,MiniMax-M2.7,qwen-plus, andqwen-maxas the model IDs that will be sent to the Anthropic-compatible runtime. The current OpenCode Go endpoint documentation lists the API model IDs as lowercase/versioned IDs such asminimax-m3,minimax-m2.7,qwen3.7-max,qwen3.7-plus, andqwen3.6-plusfor the/v1/messagesendpoint. With the current hardcoded IDs, selecting the new provider sends names the service does not document, so the advertised SDK split can fail before a request reaches the intended model. Please store the documented API IDs in the catalog and keep display names separate if friendly labels are needed. -
[P1] Keep existing favorites working after re-keying
internal/tui/picker.go:373
The picker now looks up favorites withmodelFavoriteKey(item), which becomes<provider>/<model>for real recent/catalog rows because those rows getOwnerProviderset. Existing configs still store bare model IDs, though:favoriteModelSetpreserves"qwen3-coder:480b"as-is, and the updated test only covers the new"ollama-cloud/qwen3-coder:480b"format. After upgrade, every existing bare favorite misses the lookup and disappears from the Favorites group. Please add a bare-key fallback or a one-time config migration so existing favorites survive the new provider-qualified format. -
[P2] Wire persisted discovered models back into model selection
internal/tui/provider_wizard_models.go:11
This PR writes live-discovered models intoProviderProfile.Models, but the saved list is not read when the provider is used later. The wizard still rebuilds options fromprovidermodelcatalog.Models(provider), and provider runtime resolution ignoresprofile.Models, so a model discovered and persisted during setup is lost after restart unless it is also in the curated catalog. That does not match the PR goal of preserving the live-discovered list for later selection. Please either consume the persisted models in the model picker/wizard/runtime path, or remove/defer this config surface until the consumer lands. -
[P2] Preserve live-discovered models that are not already in the catalog
internal/providermodeldiscovery/discovery.go:392
The self-discovery path still drops any live/modelsID that is not already present in the catalog whenevercatalogModelsis non-empty:mergeLiveModelsonly appends a live model when it finds the same ID inbyID, and otherwise continues at line 409. Since setup persists the result ofDiscoverCatalog, newly exposed OpenCode models that are not in the hardcoded/remote catalog never reachProviderProfile.Modelsat all. Please merge allowed live models into the result instead of using the catalog as a strict allow-list for the very providers this PR is trying to self-discover. -
[P2] Reuse the existing OpenCode API key env var
internal/providercatalog/catalog.go:144
The existing OpenCode Zen and OpenCode Go descriptors both useOPENCODE_API_KEY, but the new Anthropic-compatible OpenCode Go descriptor advertises onlyOPENCODE_GO_ANTHROPIC_API_KEY. Setup and the provider wizard only check the descriptor'sAuthEnvVars, so users who already configured the documented OpenCode key will be forced to re-enter or duplicate the same key for the Anthropic SDK variant. Please includeOPENCODE_API_KEYas the primary or fallback env var for this descriptor unless the service actually requires a different credential. -
[P2] Link approved issues before merging this community PR
CONTRIBUTING.md:60
The PR links #428 and #429, but both issues are currently labeledenhancementonly and do not have the requiredissue-approvedlabel. The contribution policy says community PRs must link an existing approved issue and stay focused on that approved scope. Please get the linked issue or issues approved first, then trim the PR to that approved scope if needed.
|
@eli-l — you're right to be annoyed, and the favorites point especially deserved a real answer instead of the same request three times. Let me give you actual decisions on all of it. Favorites — your call is the right one. You're correct that a bare favorite is ambiguous: there's no honest way to migrate Scope — I want two of the three things here, and the third isn't ready. The provider-qualified favorites (#429) and the Anthropic-SDK OpenCode-Go entries (#428) are both genuinely wanted — I've marked both issues The rest that's genuinely real — short, and push back if I've got it wrong:
None of that is a knock on the ideas — the favorites disambiguation is a real improvement and the Anthropic-SDK fallback is sensible. Do the favorites drop, trim the persistence out, fix the IDs/env/test, rebase, and I'll take it the rest of the way myself. Thanks for pushing back — that was the right instinct, and there's a human on it now. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Good direction here and the tests are solid, so I want to land this rather than bounce it — both issues it touches (#428, #429) are approved. Two things hold it back as a single PR.
First, a correctness one: the provider-qualified favorite keys break existing favorites. Favorites are saved as plain model ids today, and since every catalog/recent row now carries an OwnerProvider, the lookup key becomes provider/model and the old stars stop matching. That path needs a migration (or a fallback that actually fires) — and the comment saying legacy favorites still match isn't correct as written, the bare-id fallback never triggers for catalog items.
Second, it's doing a lot at once: a new provider, persisting the discovered model list with an apiModel override that nothing reads yet, the qualified favorites, and flipping the picker/wizard to show raw model ids instead of the friendly names everywhere. Could you split it so the provider and the favorites change land separately, drop the apiModel plumbing until something actually uses it, and leave the friendly-name display alone unless we decide to change that on its own? Happy to review each piece quickly once it's broken up.
|
Hi @Vasanthdev2004 I'll remove it from current PR and keep working on the other changes. |
0ca3f77 to
efbc481
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/tui/picker.go (1)
470-482: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSlash-containing model IDs will be dropped by cleanup
modelFavoriteKeynow persistsprovider/modelID, butvalidFavoriteModelRefrejects any entry whose model part contains/. This already affects catalog IDs likeopenai/gpt-4.1, so those favorites will be written and then removed as invalid. Encode the model component or relax the parser, and add a regression test.🤖 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/picker.go` around lines 470 - 482, The favorite key format in modelFavoriteKey and the validation in validFavoriteModelRef are inconsistent for slash-containing model IDs, so entries like provider/modelID get rejected during cleanup. Fix this by making the stored model component safe for validation (for example, encode/decode it) or by updating validFavoriteModelRef to accept the provider-qualified format used by modelFavoriteKey, then add a regression test covering a slash-containing catalog ID such as openai/gpt-4.1.internal/config/types.go (1)
53-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
ModelsinHasProviderProfile
A profile with only discovered models is still non-empty;HasProviderProfilecurrently returns false in that case, so callers that use it as the emptiness gate can drop a model-only profile. Addlen(profile.Models) > 0to the check.🤖 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/types.go` around lines 53 - 69, HasProviderProfile is missing the discovered-models case, so a profile with only Models can be treated as empty. Update HasProviderProfile in the ProviderProfile helper to include a len(profile.Models) > 0 check alongside the existing field checks, so model-only profiles are preserved.
🧹 Nitpick comments (1)
internal/cli/command_center_test.go (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for the unknown-subcommand branch.
No visible test exercises
zero config <bogus>hitting the newdefault: writeExecUsageError(... "unknown command: %s")path incommand_center.go. Small addition, cheap insurance against a routing regression.🤖 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/cli/command_center_test.go` around lines 47 - 54, Add a test for the unknown-subcommand path in command_center_test.go so zero config <bogus> exercises the default branch in command_center.go that calls writeExecUsageError with “unknown command: %s”. Reuse the existing command center test setup and assert the command exits with the expected failure code and stderr contains the unknown command message, using the same helpers/entry points around command_center.go and the command dispatcher logic.
🤖 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/config/cleanup.go`:
- Around line 10-26: CleanupStaleFavorites currently behaves the same as
CleanupInvalidFavorites and only validates <provider>/<model> format via
cleanupFavoritesForConfigPaths/validFavoriteModelRef, so its name and comment
overpromise stale-provider removal. Update the docs and/or implementation so the
behavior matches the intended meaning: either consolidate both public functions
behind one shared helper and rename the stale-specific API to reflect
format-only cleanup, or add provider-existence checks against cfg.Providers
inside cleanupFavoritesForConfigPaths. Also update the app.go call site to use
the correctly named cleanup entry point.
---
Outside diff comments:
In `@internal/config/types.go`:
- Around line 53-69: HasProviderProfile is missing the discovered-models case,
so a profile with only Models can be treated as empty. Update HasProviderProfile
in the ProviderProfile helper to include a len(profile.Models) > 0 check
alongside the existing field checks, so model-only profiles are preserved.
In `@internal/tui/picker.go`:
- Around line 470-482: The favorite key format in modelFavoriteKey and the
validation in validFavoriteModelRef are inconsistent for slash-containing model
IDs, so entries like provider/modelID get rejected during cleanup. Fix this by
making the stored model component safe for validation (for example,
encode/decode it) or by updating validFavoriteModelRef to accept the
provider-qualified format used by modelFavoriteKey, then add a regression test
covering a slash-containing catalog ID such as openai/gpt-4.1.
---
Nitpick comments:
In `@internal/cli/command_center_test.go`:
- Around line 47-54: Add a test for the unknown-subcommand path in
command_center_test.go so zero config <bogus> exercises the default branch in
command_center.go that calls writeExecUsageError with “unknown command: %s”.
Reuse the existing command center test setup and assert the command exits with
the expected failure code and stderr contains the unknown command message, using
the same helpers/entry points around command_center.go and the command
dispatcher logic.
🪄 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: 461e1069-ad92-4227-b37d-39f0e2d062f5
📒 Files selected for processing (12)
internal/cli/app.gointernal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/config.gointernal/config/cleanup.gointernal/config/cleanup_test.gointernal/config/types.gointernal/config/types_test.gointernal/config/writer.gointernal/config/writer_test.gointernal/tui/picker.gointernal/tui/picker_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/types_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/cleanup_test.go (1)
8-58: 📐 Maintainability & Code Quality | 🔵 TrivialStale test/message names reference the old
CleanupInvalidFavoritesfunction.Test function names (
TestCleanupInvalidFavorites_RemovesInvalidFormatFromAllConfigPaths,TestCleanupFavorites_PreservesOtherConfig) andt.Fatalfmessages ("CleanupInvalidFavorites() error = %v") still reference the pre-rename function even though the calls now useCleanupStaleFavorites. Harmless for correctness (verified the removed-count math is correct in both tests), but confusing when a failure is reported and someone greps for the wrong symbol.✏️ Suggested cleanup
- removed, err := CleanupStaleFavorites(userPath, projectPath) - if err != nil { - t.Fatalf("CleanupInvalidFavorites() error = %v", err) - } + removed, err := CleanupStaleFavorites(userPath, projectPath) + if err != nil { + t.Fatalf("CleanupStaleFavorites() error = %v", err) + }- removed, err := CleanupStaleFavorites(userPath, "") - if err != nil { - t.Fatalf("CleanupInvalidFavorites() error = %v", err) - } + removed, err := CleanupStaleFavorites(userPath, "") + if err != nil { + t.Fatalf("CleanupStaleFavorites() error = %v", err) + }Also applies to: 127-168
🤖 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/cleanup_test.go` around lines 8 - 58, Update the stale test naming and failure messages to match the renamed cleanup API. In the affected tests, rename references from CleanupInvalidFavorites to CleanupStaleFavorites in the test function names and any t.Fatalf messages, so failures point to the current symbol. Keep the assertions and behavior unchanged, just align the identifiers in the cleanup-related tests.
🤖 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.
Nitpick comments:
In `@internal/config/cleanup_test.go`:
- Around line 8-58: Update the stale test naming and failure messages to match
the renamed cleanup API. In the affected tests, rename references from
CleanupInvalidFavorites to CleanupStaleFavorites in the test function names and
any t.Fatalf messages, so failures point to the current symbol. Keep the
assertions and behavior unchanged, just align the identifiers in the
cleanup-related tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0666753-4783-4b8a-a830-325927944912
📒 Files selected for processing (4)
internal/cli/command_center_test.gointernal/cli/config.gointernal/config/cleanup.gointernal/config/cleanup_test.go
💤 Files with no reviewable changes (1)
- internal/config/cleanup.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cli/command_center_test.go
|
Okay, so far this PR is focused on favourites and self-discovery. Now each validation against the /models endpoint is preserved to the config file. This way once model disappears from available models - it disappears from favourited models. This prevents silent fallbacks and potentially long investigations once model is picked from favourites. The |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/providermodeldiscovery/discovery.go (1)
49-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
OnLiveModelsbest-effort
internal/providermodeldiscovery/discovery.go:52-58returnsnil, erron a persistence callback failure, which drops already-fetched live models and skips the catalog fallback. Ignore that callback error here and keep merging/falling back so discovery still succeeds when persistence is unavailable.🤖 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/providermodeldiscovery/discovery.go` around lines 49 - 67, Make OnLiveModels best-effort in Discover: the callback error from options.OnLiveModels should not abort discovery or discard already-fetched liveModels. In the canProbeProvider block, keep the live model merge and catalog fallback flow intact even if OnLiveModels returns an error, and only use liveErr to decide whether to return early when no catalogModels exist.internal/cli/provider_models.go (1)
49-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep discovery output even if persistence fails
internal/cli/provider_models.go:53-55—SetProviderDiscoveredModelsonly updates providers already in the on-disk config, so env-backed providers can discover models successfully and still turn this command into a hard failure before anything is printed. Make the cache write best-effort (warn and continue) instead of returningexitProvider.🤖 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/cli/provider_models.go` around lines 49 - 55, The provider model discovery flow in discoverProviderModels/persistDiscoveredProviderModels currently turns a successful discovery into a hard failure when the cache write fails. Update the post-discovery path so persistence is best-effort: keep the discovered models output and only warn on persist errors instead of returning exitProvider. Use the existing discoverProviderModels, persistDiscoveredProviderModels, and writeAppError flow in provider_models.go to locate the change.
♻️ Duplicate comments (1)
internal/config/validate.go (1)
99-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInherits the slash-in-model-id bug from
cleanup.go.
invalidFavoriteModelRefsdelegates tonewFavoriteModelInventory/validFavoriteModelRef, which rejects model IDs containing "/" — see the detailed comment oninternal/config/cleanup.go(Lines 135-157). Fixing it there fixes this validation path too.🤖 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/validate.go` around lines 99 - 112, The favorite-model validation still inherits the slash-in-model-id bug because invalidFavoriteModelRefs uses newFavoriteModelInventory and validFavoriteModelRef, which reject model IDs containing "/". Update the shared favorite-model validation logic in cleanup.go so slash-containing model IDs are accepted, and keep invalidFavoriteModelRefs relying on that fixed path rather than duplicating special-case handling.
🧹 Nitpick comments (1)
internal/config/validate_test.go (1)
126-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM! Assertions check out against the current
validFavoriteModelRef/invalidFavoriteModelRefsimplementation.Consider adding a case for a gateway-style favorite whose model id itself contains a slash (e.g.
"openrouter/anthropic/claude-3.5-sonnet"with a discovered model{"id": "anthropic/claude-3.5-sonnet"}) — this would have caught the rejection bug flagged incleanup.go.🤖 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/validate_test.go` around lines 126 - 176, Add a regression case in TestValidateFileRejectsFavoriteModelsOutsideProviderInventory for a gateway-style favorite where the model id contains a slash, using ValidateFile and the favoriteModels assertions to verify it is accepted when the provider inventory contains the discovered model id. Update the test fixture to include a provider with a discovered model like anthropic/claude-3.5-sonnet and a favorite ref like openrouter/anthropic/claude-3.5-sonnet, then assert it does not appear in the rejected preferences.favoriteModels issues. This should cover the validFavoriteModelRef/invalidFavoriteModelRefs path that was missed and protects the cleanup.go behavior.
🤖 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/config/cleanup.go`:
- Around line 135-157: The validFavoriteModelRef check is too strict because it
rejects favorite refs whose model portion contains additional slashes, which
breaks provider-qualified IDs like those handled by modelFavoriteKey. Update
favoriteModelInventory.validFavoriteModelRef to keep the existing whitespace and
provider/model split validation, but remove the extra strings.Contains(model,
"/") guard so entries such as openrouter/anthropic/claude-sonnet-4.5 are
preserved during cleanup.
---
Outside diff comments:
In `@internal/cli/provider_models.go`:
- Around line 49-55: The provider model discovery flow in
discoverProviderModels/persistDiscoveredProviderModels currently turns a
successful discovery into a hard failure when the cache write fails. Update the
post-discovery path so persistence is best-effort: keep the discovered models
output and only warn on persist errors instead of returning exitProvider. Use
the existing discoverProviderModels, persistDiscoveredProviderModels, and
writeAppError flow in provider_models.go to locate the change.
In `@internal/providermodeldiscovery/discovery.go`:
- Around line 49-67: Make OnLiveModels best-effort in Discover: the callback
error from options.OnLiveModels should not abort discovery or discard
already-fetched liveModels. In the canProbeProvider block, keep the live model
merge and catalog fallback flow intact even if OnLiveModels returns an error,
and only use liveErr to decide whether to return early when no catalogModels
exist.
---
Duplicate comments:
In `@internal/config/validate.go`:
- Around line 99-112: The favorite-model validation still inherits the
slash-in-model-id bug because invalidFavoriteModelRefs uses
newFavoriteModelInventory and validFavoriteModelRef, which reject model IDs
containing "/". Update the shared favorite-model validation logic in cleanup.go
so slash-containing model IDs are accepted, and keep invalidFavoriteModelRefs
relying on that fixed path rather than duplicating special-case handling.
---
Nitpick comments:
In `@internal/config/validate_test.go`:
- Around line 126-176: Add a regression case in
TestValidateFileRejectsFavoriteModelsOutsideProviderInventory for a
gateway-style favorite where the model id contains a slash, using ValidateFile
and the favoriteModels assertions to verify it is accepted when the provider
inventory contains the discovered model id. Update the test fixture to include a
provider with a discovered model like anthropic/claude-3.5-sonnet and a favorite
ref like openrouter/anthropic/claude-3.5-sonnet, then assert it does not appear
in the rejected preferences.favoriteModels issues. This should cover the
validFavoriteModelRef/invalidFavoriteModelRefs path that was missed and protects
the cleanup.go behavior.
🪄 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: c16397cb-bd06-4e48-859e-3578ce946d6e
📒 Files selected for processing (10)
internal/cli/command_center_test.gointernal/cli/config.gointernal/cli/provider_models.gointernal/cli/provider_models_test.gointernal/config/cleanup.gointernal/config/cleanup_test.gointernal/config/validate.gointernal/config/validate_test.gointernal/providermodeldiscovery/discovery.gointernal/tui/picker.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/config/cleanup_test.go
- internal/cli/command_center_test.go
- internal/cli/config.go
volkkov
left a comment
volkkov
left a comment
There was a problem hiding this comment.
Tested locally and it works. Consider extracting the retry logic into a shared utility — we have similar code in the auth module.
…o prevent introduction of duplicates.
…lidates each favourite model against known providers and models, removing the obsolete/unavailable favourites.
…lti-part models in favorite items.
fcf70c9 to
eac751d
Compare
| return providermodeldiscovery.DiscoverCatalog(ctx, descriptor, p, providermodeldiscovery.Options{ | ||
| OnLiveModels: func(models []providermodeldiscovery.Model) error { | ||
| return m.persistLiveDiscoveredModels(profile.Name, models) | ||
| }, | ||
| }) |
| for _, item := range catalog { | ||
| if item.Value == "" || seen[item.Value] { | ||
| if item.Value == "" || seen[modelFavoriteKey(item)] { | ||
| continue | ||
| } | ||
| item.Favorite = m.favoriteModels[item.Value] | ||
| item.Favorite = m.favoriteModels[modelFavoriteKey(item)] | ||
| result = append(result, item) | ||
| seen[item.Value] = true | ||
| } |
| // Models is the full list of live-discovered models, persisted alongside the | ||
| Models []config.DiscoveredModel |
| "testing" | ||
| ) | ||
|
|
||
| func TestCleanupInvalidFavorites_RemovesInvalidFormatFromAllConfigPaths(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestCleanupFAavorites_PreservesOtherConfig(t *testing.T) { |
- picker.go: dedupe catalog items by (provider, model) by recording seen[fk] after append so two same-Value items from different owners (e.g. openai + custom) both surface - discovery.go: ignore OnLiveModels persistence errors so /model still shows live models when the cache write fails - options.go: complete the SetupSelection.Models doc comment - cleanup_test.go: rename invalid/typo'd test funcs to match the CleanupStaleFavorites API surface and fix the matching t.Fatalf messages
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] Rebase on main and resolve the merge conflicts first
GitHub mergeability
GitHub currently reports this branch asCONFLICTINGwithmergeStateStatus: DIRTY, so the code being reviewed cannot be merged into the current base branch. Please rebase on the latestmainbranch and resolve the conflicts before addressing the remaining findings, so any fixes apply to the code that will actually land. -
[P2] Complete CodeRabbit's request to keep model discovery output when persistence fails
internal/cli/provider_models.go:53
zero providers modelsnow runs live discovery successfully, but then returnsexitProviderbefore printing anything ifpersistDiscoveredProviderModelscannot update the user config. That still happens for resolved providers that are not present in the on-disk user config, such as env-backed providers created byOPENAI_API_KEY/OPENAI_BASE_URL, becauseSetProviderDiscoveredModelsrejects unknown provider names. Please make this cache write best-effort, as CodeRabbit requested, so users still get the discovered model list and only see a warning when persistence is unavailable. -
[P2] Do not collapse saved profiles that share the same catalog provider
internal/tui/picker.go:204
The/modelpicker says it lists every saved provider so selections can switch providers, but it dedupes the saved-provider loop bymodelPickerProviderGroup, which returns the descriptor display name before the profile name. Two saved profiles such aswork-openaiandpersonal-openaiboth resolve to theOpenAIgroup, so the second profile is skipped before its rows ever get anOwnerProvider. The live-discovery path has the same shape: it dedupes bydescriptor.IDand stores discovered models under that ID, so same-descriptor profiles cannot be discovered independently either. That means the new provider-qualified favorite keys cannot disambiguate the exact multi-provider case this PR is meant to support. Please key these paths by the saved profile identity, or otherwise render/discover distinct sections for profiles that share a catalog descriptor, and add a regression test with two saved profiles using the same catalog ID. -
[P3] Update the PR description after splitting out the Anthropic SDK work
PR title/body
The current title and summary still say this PR introduces the OpenCode Go OpenAI/Anthropic SDK distinction and links #428, but those changes were removed from this branch and merged separately in #544. Please update the title/body so this PR describes only the remaining provider-qualified favorites, discovered-model persistence, and cleanup work; otherwise reviewers and future readers will expect behavior that is no longer in this diff.
Zganz404
left a comment
Zganz404
left a comment
There was a problem hiding this comment.
Tested locally and it works. Consider extracting the retry logic into a shared utility — we have similar code in the auth module.
Refines the regression test added in f8a7433 to assert that: 1. The OnLiveModels callback receives the raw live models before any catalog fallback. 2. DiscoverCatalog returns the catalog-fallback models and a nil error even when the callback returns an error. Renames TestDiscoverCatalogIgnoresOnLiveModelsError to TestDiscoverCatalog_OnLiveModelsErrorIsSwallowed for clarity and to match the rest of the file's naming. Addresses the only outstanding review concern on PR Gitlawb#408 that needed test coverage; the other four review items were already addressed in f8a7433.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I read this change end to end — the provider-qualified favorites, the config cleanup command, discovered-model persistence, and the validate additions. The mechanics are consistent and the test coverage is thorough. Three things before merge:
-
The PR title doesn't describe this change. It reads like a leftover from an earlier draft and has nothing to do with what the diff actually does. Please rename it to describe the real change (provider-qualified favorites + config cleanup + discovered-model persistence) so the merge log is searchable.
-
This silently destroys existing users' favorites. The favorite key changed from a bare model ID to "provider/modelID" (modelFavoriteKey), but there's no migration. A user upgrading from current main has favoriteModels like ["gpt-4.1", "claude-sonnet-4"]; after this PR, modelFavoriteKey produces "provider/modelID" so those bare entries never match in the picker (they stop appearing as favorites), and the new config cleanup / validate.go both flag a bare entry as invalid format and delete it. So an upgrade makes a user's favorites vanish and then get wiped from their config file. Add a one-time migration — map legacy bare IDs onto the active provider, or at minimum preserve them under the old key — before the cleanup is allowed to remove them.
-
Discovered-model persistence clobbers hand-curated entries. SetProviderDiscoveredModels (now called from
providers models, the picker's OnLiveModels callback, and setup) fully replaces the provider's Models list with the live /models response, sorted and deduped. Any model the endpoint didn't return — one the user curated by hand, or one the endpoint paginates past or omits — is silently dropped, and a read-style "list models" command now rewrites config.json as a side effect. Consider merging the live results into the existing list (keeping user-added IDs) rather than replacing it, or gate the persistence behind an explicit flag, so the cache can't erase entries the user put there on purpose.
Requesting changes for the destructive favorites migration and the clobbering persistence (plus the title).
eli-l
changed the title
|
@Vasanthdev2004 I'm annoyed with your AI that is drifting expectations in each separate comment. |
- picker.go: dedupe catalog items by (provider, model) by recording seen[fk] after append so two same-Value items from different owners (e.g. openai + custom) both surface - discovery.go: ignore OnLiveModels persistence errors so /model still shows live models when the cache write fails - options.go: complete the SetupSelection.Models doc comment - cleanup_test.go: rename invalid/typo'd test funcs to match the CleanupStaleFavorites API surface and fix the matching t.Fatalf messages
Refines the regression test added in f8a7433 to assert that: 1. The OnLiveModels callback receives the raw live models before any catalog fallback. 2. DiscoverCatalog returns the catalog-fallback models and a nil error even when the callback returns an error. Renames TestDiscoverCatalogIgnoresOnLiveModelsError to TestDiscoverCatalog_OnLiveModelsErrorIsSwallowed for clarity and to match the rest of the file's naming. Addresses the only outstanding review concern on PR Gitlawb#408 that needed test coverage; the other four review items were already addressed in f8a7433.
|
@eli-l So you're using AI to write code, but expecting me to manually review every single change? Even when the AI-generated changes are valid and not hallucinated? It's not practical for me to manually verify every PR every day. AI is there to help everyone work more efficiently it's not something only I'm using. |
|
@Vasanthdev2004 No. I am expecting decisions to be made by humans. My contribution is based on my decisions and involvment - not endless loops. You requested migrations multiple times. These aren't hallucinations - it is lack of timely communication and misdirection. |
|
@eli-l Fair point, and I owe you a straight answer instead of another review comment. You're right that the migration ask kept coming back after you'd already explained it couldn't be done honestly that shouldn't have been repeated, and the scope shouldn't have been re-cut a week later. That's on me for not making a decision up front and letting the review comments drift. "Misdirection" is fair. On the outcome, since the PR is closed: your Anthropic-SDK split-out (#544) merged, and the provider-qualified favorites format is in main now the parts you pushed for did land. The discovered-model persistence and the On the AI point I'm not going to argue it. You asked for human decisions and timely communication, and you're owed both. Going forward I'll consolidate feedback into one decision before asking you for changes, and I'll make the call myself instead of dribbling review notes. If something isn't wanted, I'll say so early. Sorry for the runaround. If you want to pick the favorites/cleanup work back up, I'm happy to scope it with you properly this time if not, no hard feelings. |
|
@Vasanthdev2004 I'm good to take care of it, let me know how I can help. |
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
zero configaddssummaryandcleanupcommands, including--jsonoutput.config cleanupremoves stale/invalid favorite entries from user and project configs.