Introduce new favorite record format. Favourite (config.json) cleanup CLI command. Discovered model are persistent in config.json.#593
Open
eli-l wants to merge 20 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Zero’s model favorites and provider model discovery so favorites are unambiguous (provider+model) and discovered model lists can be persisted into config.json, with a new zero config cleanup command to remove stale/invalid favorite entries.
Changes:
- Switch favorites handling to provider-qualified keys (
provider/modelID) and adjust model picker grouping/deduping accordingly. - Persist live-discovered provider model lists into
config.json(wizard, onboarding/setup flow, model picker discovery, andzero providers models). - Add config validation for stale provider model selections / invalid favorites, plus a
zero config cleanupcommand and tests.
Reviewed changes
Copilot reviewed 24 out of 24 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 a provider; refresh in-memory provider list. |
| internal/tui/provider_wizard_test.go | Update wizard rendering expectations for model display. |
| internal/tui/picker.go | Persist live-discovered models; change favorites to provider-qualified keys; adjust picker assembly/dedup. |
| internal/tui/picker_test.go | Add coverage for provider-qualified favorites and slash-containing model IDs; update expectations. |
| internal/tui/options.go | Extend setup selection payload to include discovered models list for persistence. |
| internal/tui/onboarding.go | Include discovered models list in setup save call. |
| internal/swarm/scheduler_test.go | Tighten scheduler test assertion around run counting. |
| internal/providermodeldiscovery/discovery.go | Add OnLiveModels callback and ToDiscoveredModels helper. |
| internal/providermodeldiscovery/discovery_test.go | Verify OnLiveModels errors don’t break discovery flow. |
| internal/config/writer.go | Add SetProviderDiscoveredModels and discovered-model dedup/sort helper. |
| internal/config/writer_test.go | Add tests for SetProviderDiscoveredModels behavior and error cases. |
| internal/config/validate.go | Add semantic validation for provider model vs discovered list and favorites inventory refs. |
| internal/config/validate_test.go | Add tests for invalid favorites and stale provider model selection. |
| internal/config/types.go | Introduce DiscoveredModel and add Models to provider profiles. |
| internal/config/types_test.go | Add JSON round-trip tests for discovered models and provider profile models. |
| internal/config/local_control_test.go | Update resolve options construction to include explicit Env map. |
| internal/config/cleanup.go | Add stale/invalid favorites cleanup logic based on user-config inventory. |
| internal/config/cleanup_test.go | Add coverage for cleanup behavior across user + project config paths. |
| internal/cli/setup.go | Persist discovered models during setup (non-fatal warning on failure). |
| internal/cli/provider_models.go | Persist discovered models after providers models discovery. |
| internal/cli/provider_models_test.go | Add persistence test and seed named provider in selection test. |
| internal/cli/config.go | Add zero config summary and zero config cleanup subcommand implementations. |
| internal/cli/command_center.go | Route zero config to summary/cleanup subcommands; update help text. |
| internal/cli/command_center_test.go | Add tests for config cleanup output and JSON payload; seed config in deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1133
to
+1138
| // Refresh savedProviders so /model shows the newly added / updated provider | ||
| // without requiring a restart. The profile was just persisted (above) and is | ||
| // guaranteed usable (key stored / env var set / local), so it qualifies. | ||
| if strings.TrimSpace(profile.Name) != "" { | ||
| m.savedProviders = mergeSavedProvider(m.savedProviders, profile) | ||
| } |
Comment on lines
+1969
to
+1984
| // mergeSavedProvider replaces or appends a profile in the saved list by name. | ||
| // This lets the provider wizard refresh m.savedProviders so /model sees newly | ||
| // added providers without requiring a restart. | ||
| func mergeSavedProvider(profiles []config.ProviderProfile, profile config.ProviderProfile) []config.ProviderProfile { | ||
| name := strings.TrimSpace(profile.Name) | ||
| if name == "" { | ||
| return profiles | ||
| } | ||
| for i, p := range profiles { | ||
| if strings.EqualFold(strings.TrimSpace(p.Name), name) { | ||
| profiles[i] = profile | ||
| return profiles | ||
| } | ||
| } | ||
| return append(profiles, profile) | ||
| } |
Comment on lines
+53
to
+55
| if err := persistDiscoveredProviderModels(deps, profile.Name, models); err != nil { | ||
| return writeAppError(stderr, err.Error(), exitProvider) | ||
| } |
Comment on lines
+10
to
+12
| // CleanupStaleFavorites removes favorite model entries from the user and | ||
| // project configs that do not reference a provider/model available from the | ||
| // global user config. |
|
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 skipped from review due to trivial changes (1)
WalkthroughAdds persisted discovered-model storage, a ChangesDiscovered models, cleanup, and favorites
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (providers models / setup)
participant Discovery as providermodeldiscovery
participant ConfigWriter as config.SetProviderDiscoveredModels
participant UserConfig as user config.json
User->>CLI: run providers models or setup
CLI->>Discovery: DiscoverCatalog(Options{OnLiveModels})
Discovery->>Discovery: fetch live /models
Discovery->>CLI: OnLiveModels(liveModels)
CLI->>ConfigWriter: SetProviderDiscoveredModels(path, provider, models)
ConfigWriter->>ConfigWriter: trim, dedupe, sort models
ConfigWriter->>UserConfig: write updated provider.Models
ConfigWriter-->>CLI: success or error
CLI-->>User: model list output
ChangesCatalog defaults and test/runtime adjustments
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
🧹 Nitpick comments (3)
internal/tui/provider_wizard.go (2)
1968-1984: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing saved-profile upsert helper
mergeSavedProvideris the same replace-or-append-by-name logic asupsertSavedProviderProfile, and the surrounding code already guarantees a non-emptyprofile.Name. Drop the duplicate helper and call the existing one instead.🤖 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 1968 - 1984, The mergeSavedProvider helper duplicates the existing replace-or-append-by-name logic already implemented by upsertSavedProviderProfile, and the caller context already ensures profile.Name is non-empty. Remove mergeSavedProvider and update the provider wizard flow to reuse upsertSavedProviderProfile instead, keeping the saved profile refresh behavior in provider_wizard.go consistent through the existing helper.
1130-1139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the duplicate
m.savedProvidersupdate
mergeSavedProviderandupsertSavedProviderProfileboth replace-or-append by name, so this path updates the same slice twice. Keep one call here and remove the now-unused helper if nothing else depends on it.🤖 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 1130 - 1139, The saved providers slice is being updated twice for the same profile, once via mergeSavedProvider and again via upsertSavedProviderProfile, causing redundant replace-or-append behavior. In provider_wizard.go, keep only one savedProviders update in this commit path around the profile persistence flow and remove the now-unused helper if nothing else references it; use the mergeSavedProvider and upsertSavedProviderProfile symbols to verify the duplicate path is eliminated.internal/providermodeldiscovery/discovery.go (1)
52-54: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueIntentional error swallowing is correct but consider logging.
The
_ = options.OnLiveModels(liveModels)pattern silently discards persistence failures. The test confirms this is by design — discovery results should not be blocked by cache write errors. However, a failed persist means the cleanup feature won't have fresh data, and the operator has no signal. Consider a debug-level log when the callback returns an error.🤖 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 52 - 54, The OnLiveModels callback in discovery should still ignore persistence failures, but it currently discards the error without any signal. Update the discovery flow around options.OnLiveModels in discovery.go so that when the callback returns an error it is captured and emitted as a debug-level log with enough context to identify the live-model persistence attempt, while still allowing discovery to continue unchanged.
🤖 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/provider_models.go`:
- Around line 53-55: The `ListProviderModels` flow is treating
`persistDiscoveredProviderModels` failure as fatal, which prevents the
discovered models from being shown. Update the error handling around
`persistDiscoveredProviderModels(deps, profile.Name, models)` so persistence
errors are reported to stderr but do not return `writeAppError` or exit early.
Keep the discovery results flowing through `writeProviderModels`/the model
display path even when persistence fails, and only use the persistence failure
as a non-blocking warning.
In `@internal/config/cleanup.go`:
- Around line 135-154: `validFavoriteModelRef` handles empty provider model maps
differently from nil maps, which causes cleanup to reject all favorites for
providers whose discovered `Models` collapse to an empty set. Update the logic
in `favoriteModelInventory.validFavoriteModelRef` so an empty non-nil model map
is treated the same as nil (allow all), matching the behavior already used by
`invalidProviderModelIssues` in `validate.go`. Keep the provider/model lookup
and normalization intact, but change the final availability check to skip
enforcement when the provider’s model map has no entries.
---
Nitpick comments:
In `@internal/providermodeldiscovery/discovery.go`:
- Around line 52-54: The OnLiveModels callback in discovery should still ignore
persistence failures, but it currently discards the error without any signal.
Update the discovery flow around options.OnLiveModels in discovery.go so that
when the callback returns an error it is captured and emitted as a debug-level
log with enough context to identify the live-model persistence attempt, while
still allowing discovery to continue unchanged.
In `@internal/tui/provider_wizard.go`:
- Around line 1968-1984: The mergeSavedProvider helper duplicates the existing
replace-or-append-by-name logic already implemented by
upsertSavedProviderProfile, and the caller context already ensures profile.Name
is non-empty. Remove mergeSavedProvider and update the provider wizard flow to
reuse upsertSavedProviderProfile instead, keeping the saved profile refresh
behavior in provider_wizard.go consistent through the existing helper.
- Around line 1130-1139: The saved providers slice is being updated twice for
the same profile, once via mergeSavedProvider and again via
upsertSavedProviderProfile, causing redundant replace-or-append behavior. In
provider_wizard.go, keep only one savedProviders update in this commit path
around the profile persistence flow and remove the now-unused helper if nothing
else references it; use the mergeSavedProvider and upsertSavedProviderProfile
symbols to verify the duplicate path is eliminated.
🪄 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: c846310c-aa1e-41a2-8b67-40fadc0317b0
📒 Files selected for processing (24)
internal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/config.gointernal/cli/provider_models.gointernal/cli/provider_models_test.gointernal/cli/setup.gointernal/config/cleanup.gointernal/config/cleanup_test.gointernal/config/local_control_test.gointernal/config/types.gointernal/config/types_test.gointernal/config/validate.gointernal/config/validate_test.gointernal/config/writer.gointernal/config/writer_test.gointernal/providermodeldiscovery/discovery.gointernal/providermodeldiscovery/discovery_test.gointernal/swarm/scheduler_test.gointernal/tui/onboarding.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
… unique following <provider>/<model> format
…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.
- 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
force-pushed
the
feat/opencode-self-discovery
branch
from
fc0db3a to
1f74d10
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 (1)
internal/providercatalog/catalog_test.go (1)
43-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
opencode-go-anthropic-compatibleinTransportAnthropicCompat
internal/providercatalog/catalog_test.go:295-296should listopencode-go-anthropic-compatibleunderTransportAnthropicCompattoo;catalog.gomarks it asanthropic-compatible, soListByTransport(TransportAnthropicCompat)should return it.🤖 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/providercatalog/catalog_test.go` around lines 43 - 47, Update the provider catalog mapping so `opencode-go-anthropic-compatible` is included in `TransportAnthropicCompat` as well as its current transport group; adjust the relevant setup in `catalog.go` and the expectations in `catalog_test.go` around `ListByTransport` so the provider is returned for `TransportAnthropicCompat`.
🧹 Nitpick comments (1)
internal/providermodelcatalog/catalog.go (1)
174-183: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCollapse the redundant fallback in
Models.dedupeModels(provider.DefaultModel, registryModels(provider))already covers the empty-registry case, so thelen(models)check andnilreturn can be removed.🤖 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 174 - 183, Collapse the redundant fallback in Models by removing the explicit len(models) branch and nil return; Models should return dedupeModels(provider.DefaultModel, curatedModels[provider.ID]) when curated data exists, otherwise directly return dedupeModels(provider.DefaultModel, registryModels(provider)). Keep the function name Models and the dedupeModels helper as the main touchpoints when updating this 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/tui/picker.go`:
- Around line 509-520: modelFavoriteKey is still using the raw OwnerProvider
casing, which can cause favorites lookups to miss for the same provider/model
pair. Update modelFavoriteKey to normalize the provider portion to lower-case
before building the provider-qualified key, matching the canonicalization
already used by NormalizeRecentModels and pickerItemDedupKey. Keep the existing
trimming behavior for item.Value and OwnerProvider, and ensure the returned
favorite key is generated consistently regardless of provider name casing.
---
Outside diff comments:
In `@internal/providercatalog/catalog_test.go`:
- Around line 43-47: Update the provider catalog mapping so
`opencode-go-anthropic-compatible` is included in `TransportAnthropicCompat` as
well as its current transport group; adjust the relevant setup in `catalog.go`
and the expectations in `catalog_test.go` around `ListByTransport` so the
provider is returned for `TransportAnthropicCompat`.
---
Nitpick comments:
In `@internal/providermodelcatalog/catalog.go`:
- Around line 174-183: Collapse the redundant fallback in Models by removing the
explicit len(models) branch and nil return; Models should return
dedupeModels(provider.DefaultModel, curatedModels[provider.ID]) when curated
data exists, otherwise directly return dedupeModels(provider.DefaultModel,
registryModels(provider)). Keep the function name Models and the dedupeModels
helper as the main touchpoints when updating this 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: 81d60cdb-434a-459b-87d3-c6f9310fc967
📒 Files selected for processing (27)
internal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/config.gointernal/cli/provider_models.gointernal/cli/provider_models_test.gointernal/cli/setup.gointernal/config/cleanup.gointernal/config/cleanup_test.gointernal/config/local_control_test.gointernal/config/types.gointernal/config/types_test.gointernal/config/validate.gointernal/config/validate_test.gointernal/config/writer.gointernal/config/writer_test.gointernal/providercatalog/catalog_test.gointernal/providermodelcatalog/catalog.gointernal/providermodelcatalog/filter.gointernal/providermodeldiscovery/discovery.gointernal/providermodeldiscovery/discovery_test.gointernal/swarm/scheduler_test.gointernal/tui/onboarding.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/providermodelcatalog/filter.go
🚧 Files skipped from review as they are similar to previous changes (23)
- internal/tui/options.go
- internal/swarm/scheduler_test.go
- internal/config/writer_test.go
- internal/config/writer.go
- internal/config/types.go
- internal/config/types_test.go
- internal/providermodeldiscovery/discovery.go
- internal/cli/provider_models_test.go
- internal/config/cleanup.go
- internal/cli/provider_models.go
- internal/cli/command_center.go
- internal/config/local_control_test.go
- internal/config/cleanup_test.go
- internal/config/validate.go
- internal/cli/setup.go
- internal/cli/command_center_test.go
- internal/tui/onboarding.go
- internal/tui/provider_wizard_test.go
- internal/tui/provider_wizard.go
- internal/providermodeldiscovery/discovery_test.go
- internal/cli/config.go
- internal/tui/picker_test.go
- internal/config/validate_test.go
Comment on lines
+267
to
+271
| // For the restricted provider (opencode-go-anthropic) a | ||
| // non-allowed model returns false immediately, without reaching the | ||
| // IsKnownNonCodingModelID, Local, or LooksLikeCodingModelID checks. | ||
| restricted := providercatalog.Descriptor{ | ||
| ID: "opencode-go-anthropic-compatible", | ||
| ID: "opencode-go-anthropic", |
| TransportBedrock: {"bedrock"}, | ||
| TransportVertex: {"vertex"}, | ||
| TransportAnthropicCompat: {"minimax", "minimaxi-cn", "opencode-go-anthropic-compatible", "custom-anthropic-compatible"}, | ||
| TransportAnthropicCompat: {"minimax", "minimaxi-cn", "custom-anthropic-compatible"}, |
Comment on lines
+54
to
+56
| if err := persistDiscoveredProviderModels(deps, profile.Name, models); err != nil { | ||
| return writeAppError(stderr, err.Error(), exitProvider) | ||
| } |
Comment on lines
+516
to
+518
| if owner := strings.TrimSpace(item.OwnerProvider); owner != "" { | ||
| return owner + "/" + id | ||
| } |
Persist failures during `providers models` are now stderr warnings, not fatal exits (Copilot + CodeRabbit). CleanupStaleFavorites treats empty model inventories the same as nil (CodeRabbit); its doc comment also reflects the invalid-format cleanup it actually performs (Copilot). Provider-casing normalization in modelFavoriteKey and favoriteModelSet matches pickerItemDedupKey and NormalizeRecentModels (CodeRabbit). The provider wizard no longer calls the redundant mergeSavedProvider helper that duplicated upsertSavedProviderProfile (Copilot + CodeRabbit). Test fixes the reviewers missed: TransportAnthropicCompat expectation restored for the still-registered opencode-go-anthropic-compatible catalog entry; the discovery-gate test now uses the matching provider id the gate actually fires on; the providers-models test seeds the temp config file with the profile its resolveConfig override returns; and the favorites picker test is rewritten for the per-provider/model semantics instead of the pre-Gitlawb#593 per-Value format. go test ./... green (75 packages, 0 FAIL). gofmt, vet, build clean.
Contributor
|
hello @eli-l thanks for your work on this. kindly rebase to main and fix conflcits |
govulncheck ./... is clean on 1.26.5 (no vulnerabilities). go build ./... passes. CI reads toolchain via go-version-file: go.mod so all three OS runners pick this up automatically.
…code health" This reverts commit 3ff7189.
anandh8x
requested changes
Jul 9, 2026
anandh8x
left a comment
anandh8x
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for taking on #429. The core problem is real: favorites are still bare model ids, while RecentModels is already provider-qualified, so multi-provider setups can resolve the wrong provider.
What works in the direction of this PR:
- Making favorites provider-aware matches what we already do for recents
- A migration path for old bare-id favorites is needed either way
Where I'd push back is scope. Caching full discovered model lists on every provider in config.json, plus a new zero config cleanup command, is a bigger product surface than the favorites ambiguity needs. A tighter cut would be:
- Store favorites the same way as RecentModels (provider + model), not a "provider/model" string split
- Migrate or drop old bare-id favorites on read/write
- Defer full model-list persistence and the cleanup command unless there's a stronger reason than validation inventory
A few concrete issues on the current tip:
- Branch is conflicting with main (
mergeStateStatus: DIRTY) — needs a rebase before review can finish - Checklist still has go build / go test / gofmt unchecked
zero providers modelsfailing when the discovered-models write fails is inconsistent with the wizard, which treats that as best-effort; listing should still succeedmergeSavedProviderlooks like a second path next toupsertSavedProviderProfile— worth consolidating- Splitting favorites on the first
/is fragile if a provider name ever contains/; structured fields avoid that
Happy to re-review a narrower favorites-only revision. The multi-provider favorite fix is worth landing; the rest can follow if we still need it.
gnanam1990
requested changes
Jul 9, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Review
Verdict: Request changes. Agree with @anandh8x on scope and structured favorites.
Process / merge
- Branch is still CONFLICTING with main (
mergeStateStatus: DIRTY) — rebase/merge main before this can land. I hit a conflict ininternal/config/local_control_test.golocally. - Checklist still has go build / go test / gofmt unchecked.
- Parent issue #429 is
issue-approved— good.
What improved on tip (prior feedback)
zero providers modelsnow warns on persist failure instead of failing the list (good).mergeSavedProvideris gone; wizard usesupsertSavedProviderProfile(good).
Blockers
-
Bare-id favorites break silently, then cleanup deletes them.
favoriteModelSetkeeps old entries like"gpt-4.1";modelFavoriteKeyalways uses"provider/model"whenOwnerProvideris set. Favorites vanish after upgrade.zero config cleanuptreats bare IDs as invalid and removes them — data loss, not migration. -
First-
/encoding is wrong for slashy model IDs.
Legacy favorites are bare model IDs. IDs likeopenai/gpt-4.1(OpenRouter-style) get misread asprovider=openai. Structured{provider, model}fields (same asRecentModelEntry) avoid this. Do not encode identity in a single slash-separated string. -
Scope is still too large for the favorites fix.
Caching full discovered model lists +zero config cleanup+ inventory-based validation circularly justify each other. A tighter cut:- Store favorites like RecentModels (provider + model fields)
- Migrate or drop bare-id favorites on read/write
- Defer model-list persistence and the cleanup command
Other notes
- Listing models still rewrites config as a side effect and persists before catalog filtering.
OnLiveModelserrors are swallowed (_ =); picker can fail persist with no UI warning.- Cleanup inventory is user-config only — project-scoped providers get purged.
- New validate requiring
provider.Model ∈ provider.Modelsis a separate semantic change and will false-positive before discovery. providermodelcatalog.Models()droppedFilterModelsForProvider— looks unrelated; restore or justify separately.
Ask
Happy to re-review a favorites-only revision on a rebased tip: structured provider+model favorites + real migration for bare (and slash-containing) legacy IDs. Model cache / cleanup can follow in a separate PR if still needed.
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Favorites now use provider/model format to reduce unambiguity.
config cleanupcommand to cleanup stale favorite configs (old format).For cleanup needs discovered models are saved in config.json for each provider.
Linked issue
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit
New Features
config summaryandconfig cleanupcommands, including machine-readable--jsonoutput.Bug Fixes
Tests