fix(providers): stop "provider not found" for env-derived profiles#716
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a mismatch between the resolved provider list (which can include env-derived providers synthesized at runtime) and the persisted provider list in config.json, which previously caused confusing provider "openai" not found errors when users tried to remove/use/update models for env-derived providers (issue #707).
Changes:
- Adds
config.ProviderPersisted(path, name)to distinguish providers that exist on disk vs. providers that only exist in-memory due to environment variables. - Updates TUI provider deletion and model persistence paths to avoid treating env-derived providers as “not found” when there’s nothing to mutate on disk.
- Updates CLI
zero providers use/removeto provide clearer handling for unpersisted-but-resolved provider names.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/tui/provider_manager.go | Avoids “not found” on deleting env-derived providers by removing them from the session list with an explanatory message. |
| internal/tui/command_center.go | Skips persisting model selection for providers that aren’t persisted in config.json. |
| internal/config/writer.go | Introduces ProviderPersisted helper to detect whether a provider exists as a config row. |
| internal/cli/provider_onboarding.go | Handles providers use/remove for unpersisted providers by resolving config and emitting a non-error explanation when appropriate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func ProviderPersisted(path string, name string) bool { | ||
| path = strings.TrimSpace(path) | ||
| name = strings.TrimSpace(name) | ||
| if path == "" || name == "" { | ||
| return false | ||
| } | ||
| cfg, err := loadConfigFile(path) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| for _, provider := range cfg.Providers { | ||
| if strings.EqualFold(strings.TrimSpace(provider.Name), name) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
| message := fmt.Sprintf( | ||
| "Provider %q is not saved in config.json (likely set via an environment variable), so there is no saved profile to switch to.\nIt is already used automatically whenever its environment variable is set; unset it to stop Zero from detecting it automatically.", | ||
| options.name, | ||
| ) |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughProvider operations now distinguish persisted providers from environment-derived providers. CLI commands report unpersisted providers, while TUI model persistence, editing, and deletion avoid invalid config writes and update session state appropriately. ChangesProvider persistence handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Provider CLI
participant Config as ProviderPersisted
participant Center as Command center resolution
CLI->>Config: Check provider persistence
Config-->>CLI: Return persisted status
CLI->>Center: Resolve unpersisted provider
Center-->>CLI: Return provider match
CLI-->>CLI: Emit text or JSON status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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/writer.go`:
- Around line 195-197: Update the config-loading flow around loadConfigFile to
return (bool, error) instead of collapsing load failures into false. Propagate
missing, unreadable, and malformed configuration errors to callers before the
unpersisted path, and update each caller to handle the returned error without
treating the operation as an env-derived provider.
In `@internal/tui/provider_manager.go`:
- Around line 361-371: The unpersisted provider branch in
providerManagerCleanupCmd must remain session-only and must not delete
credential-store data. Restrict the same-named credential cleanup to the
persisted-removal path, while preserving the existing informational notes and
session-list removal for env-derived 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: 39727400-8876-4ca6-9194-694f471df6d9
📒 Files selected for processing (4)
internal/cli/provider_onboarding.gointernal/config/writer.gointernal/tui/command_center.gointernal/tui/provider_manager.go
There was a problem hiding this comment.
The core fix is right ProviderPersisted cleanly separates "not on disk" from "doesn't exist anywhere," the four call sites you guarded are correct, the /model picker still applies the model in-session for env-derived providers, and the typo fall-through still hits the real "not found". But the class fix is incomplete two more reachable name-keyed mutators have the identical env-derived gap, and the providers remove --json shape has a type inconsistency.
-
zero providers rename(provider_onboarding.go:455) callsconfig.RenameProviderwith noProviderPersistedguard, sorename openai myopenaion an env-derived openai still failsprovider "openai" not foundthe same bug you closed foruse/remove. It needs the same persisted-check + explanatory message (or fall-through) as the siblings. -
The TUI
/providersedit-save path (saveManagerEdit, provider_manager.go:611) callsconfig.EditProviderwith no guard. You guarded the sibling delete path (deleteManagerSelection) in this same file, but pressingeon an env-derived row, editing a field, and saving hitsprovider "openai" not foundthe same way. Env-derived rows do reach the editor —savedProvidersis seeded fromusableSavedProviders(resolved.Providers), so the env-derived profile is in the manager list and editable. Needs the sameProviderPersistedbranch as delete. -
providers remove --jsonemitsremovedas a STRING on success (line 389, the name) but a BOOLEAN on the env-derived path (line 547,false) same key, different JSON type, both exit 0. A typed consumer breaks on the env-derived path; the repo's own test struct at provider_onboarding_test.go:263 unmarshalsremovedinto a string, so that pattern would fail to parse{removed: false}. Pick one type e.g. keepremovedas the name-string (empty when nothing removed) and convey "not persisted" via the existingmessage/persistedfields.
Non-blocking: the env-derived use/remove --json payloads also omit configPath/keyRemoved that the success payload always includes worth aligning the field set or documenting the env-derived shape as a distinct variant.
Requesting changes for 1–3.
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/cli/provider_onboarding.go (1)
514-536: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude
configPathin the JSON payload.The success path for
providers use --jsonincludesconfigPath. The unpersisted path should include it as well to maintain a predictable JSON schema. Update the function signature to acceptconfigPathand add it to the JSON response.🐛 Proposed fix
-func reportUnpersistedProviderUse(stdout, stderr io.Writer, deps appDeps, options providerUseOptions) (int, bool) { +func reportUnpersistedProviderUse(stdout, stderr io.Writer, deps appDeps, options providerUseOptions, configPath string) (int, bool) { resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { // resolveCommandCenterConfig already wrote its own error to stderr; // stop here instead of letting the caller try SetActiveProvider too. return exitCode, true } if !providerResolvedByName(resolved.Providers, options.name) { return exitCode, false } message := fmt.Sprintf( "Provider %q is not saved in config.json (likely set via an environment variable), so there is no saved profile to switch to.\nIt is available whenever its environment variable is set, but is only active when selected (for example via ZERO_PROVIDER); unset its environment variable to stop Zero from detecting it automatically.", options.name, ) if options.json { if err := writePrettyJSON(stdout, map[string]any{ "activeProvider": resolved.ActiveProvider, "persisted": false, + "configPath": configPath, "message": message, }); err != nil {Then, update the call site in
runProvidersUse:- if exit, handled := reportUnpersistedProviderUse(stdout, stderr, deps, options); handled { + if exit, handled := reportUnpersistedProviderUse(stdout, stderr, deps, options, configPath); handled {🤖 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_onboarding.go` around lines 514 - 536, Update reportUnpersistedProviderUse to accept configPath, update its call site in runProvidersUse to pass configPath, and include configPath in the providers use --json payload alongside activeProvider, persisted, and message.
🤖 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_onboarding.go`:
- Around line 593-596: Update the success-path response built by writePrettyJSON
so the renamed field uses nil instead of false, preserving the expected
object-or-null JSON type while leaving the other fields unchanged.
---
Outside diff comments:
In `@internal/cli/provider_onboarding.go`:
- Around line 514-536: Update reportUnpersistedProviderUse to accept configPath,
update its call site in runProvidersUse to pass configPath, and include
configPath in the providers use --json payload alongside activeProvider,
persisted, and message.
🪄 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: 517764e2-8f5e-4fcc-bcdc-5b5e6738d5f0
📒 Files selected for processing (5)
internal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.gointernal/config/writer.gointernal/tui/command_center.gointernal/tui/provider_manager.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/command_center.go
|
fix coderabbit changes @PierrunoYT |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Really close now — all three of my asks landed. The rename guard with the typo fall-through, the edit-save guard in the provider manager, and the remove --json payload keeping removed a string plus picking up configPath/keyRemoved all look right, and the schema test on the remove payload is a nice touch. The malformed-config propagation and keeping the credential cleanup on the persisted-delete path only are also correct.
One thing left, and it's the same trap we just closed on remove: the new env-derived rename path emits renamed: false while the success path emits renamed as a {from, to} object (provider_onboarding.go:593). Same key, two JSON types, both exit 0 — a typed consumer parses one and chokes on the other. Emit null (or an empty object) there and I'm good to approve. The use payload omitting configPath is still take-it-or-leave-it from my side.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on the current head (807c826), and all three of my earlier asks landed cleanly:
- The
renamepath now has theProviderPersistedguard with the same typo fall-through asuse/remove, so a genuinely unknown name still hits the real "not found". - The TUI edit-save path (
saveManagerEdit) is guarded too, rejecting an env-derived row with a clear "no saved profile to edit" message instead of the bogus not-found. - The
--jsonschema is consistent now:removedis a string on both paths (name on success, "" on the env path) andrenamedis an object on success andnull(notfalse) on the env path, so a typed consumer parses both. The env-derivedusepayload also picks upconfigPath.
I walked every call site of the five name-keyed config mutators (RemoveProvider, SetActiveProvider, SetProviderModel, RenameProvider, EditProvider) to be sure the class fix is actually complete: all six reachable ones are guarded, the credential-store cleanup still runs only on the persisted delete path, and the persisted paths are unchanged. Build, vet, and the config/cli/tui tests are green, including the four new env-derived onboarding tests.
One optional, non-blocking note for later (nothing to do here): switchProviderModel in command_center.go makes two name-keyed persist calls that are safe today only because they discard the error with _, _ =, unlike persistSelectedModel which you guarded explicitly. It behaves correctly for env-derived providers (the persist just no-ops), so it is not a bug, but if a future change ever starts checking those errors the same not-found would resurface. Worth a guard for symmetry someday, not in this PR.
Nice work seeing this through the rounds. Approving.
…#767) Closes #721. `zero providers use <name>` wrote activeProvider to config.json and reported "Active provider set to <name>" even when ZERO_PROVIDER was set, which applyEnv makes win over config.json unconditionally. So the switch was reported as a success while `providers current` still showed the env-pinned provider, a silent no-op. Detect the override and say so: the text path prints a note to stderr naming the env var and the provider that stays active, and the --json path adds effectiveProvider / overriddenByEnv fields so a machine consumer is not misled either. The config write still lands unchanged; only the reporting is honest. The env read is injected via a new appDeps.getenv (os.Getenv in production, left nil by fillAppDeps like exportActiveProvider) so tests stay hermetic against an ambient ZERO_PROVIDER. The related env-derived-profile "not found" case the issue also mentions is already handled by the unpersisted-provider path (#707) and #716.
Fixes #707
Problem
RemoveProvider/SetActiveProvider/SetProviderModelonly ever look at rows persisted inconfig.json. But a provider can appear in the resolved provider list (TUI picker,zero providers list) purely becauseResolve()synthesized it in-memory from an ambient env var (e.g.OPENAI_API_KEY) without ever writing it to disk.That mismatch caused a confusing
provider "openai" not founderror when a user tried to:d)/modelpicker) — showednot saved (provider "openai" not found)zero providers use <name>orzero providers remove <name>from the CLIFix
Added
config.ProviderPersisted(path, name)to distinguish "not on disk" from "doesn't exist anywhere," and used it at all four call sites so an env-derived provider gets an accurate, non-error message instead of a bogus not-found failure:internal/tui/provider_manager.go— provider manager deleteinternal/tui/command_center.go—/modelpicker persistenceinternal/cli/provider_onboarding.go—zero providers use/zero providers removeinternal/config/writer.go— the new sharedProviderPersistedhelperGenuinely unknown/misspelled provider names still fall through to the original "not found" error.
Testing
go build ./...go vet ./...go test ./internal/cli/... ./internal/config/... ./internal/tui/...Summary by CodeRabbit
Bug Fixes
Improvements
--jsonoutput for env-derived use/remove/rename, including consistentpersistedandconfigPathreporting.