Clarify saved and effective provider selection - #725
Conversation
Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
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:
WalkthroughProvider listings now distinguish persisted and runtime-resolved profiles. Provider selection reports environment override resolution, while configuration mutations require exact persisted names and reject ambiguous duplicates. Authentication, setup, and TUI flows preflight configuration before persistence. ChangesProvider configuration and selection
Provider CLI behavior
Authentication and persistence preflight
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProvidersUse
participant UserConfig
participant Resolver
ProvidersUse->>UserConfig: save requested provider
ProvidersUse->>Resolver: resolve environment override
Resolver-->>ProvidersUse: return resolved, unresolved, or deferred state
ProvidersUse-->>ProvidersUse: report effectiveProvider and envProviderResolves
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.
🧹 Nitpick comments (1)
internal/cli/command_center_test.go (1)
194-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset
stderralongsidestdoutfor clean failure messages.While this test passes correctly, it is a good practice to reset both output buffers before reusing them. This ensures that if the second command fails,
stderr.String()only contains the relevant error from the second execution, preventing confusingt.Fatalferror messages containing leftover artifacts.♻️ Proposed refactor
} stdout.Reset() + stderr.Reset() if code := runWithDeps([]string{"providers", "list"}, &stdout, &stderr, deps); code != exitSuccess { t.Fatalf("code=%d stderr=%s", code, stderr.String())🤖 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 194 - 229, Reset stderr alongside stdout before the second runWithDeps invocation in TestRunProvidersListMarksUserAndRuntimeProfiles, so failure reporting reflects only the second command’s output.
🤖 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/cli/command_center_test.go`:
- Around line 194-229: Reset stderr alongside stdout before the second
runWithDeps invocation in TestRunProvidersListMarksUserAndRuntimeProfiles, so
failure reporting reflects only the second command’s output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86a322da-ff12-4cef-bf1f-5f77bcde8327
📒 Files selected for processing (4)
internal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.go
There was a problem hiding this comment.
Pull request overview
This PR improves clarity around provider selection by distinguishing the provider saved in user config from the provider that is actually effective at runtime (especially when ZERO_PROVIDER is set), and by enriching provider listing output so UIs can tell which entries are user-selectable vs runtime-derived.
Changes:
- Update
providers useto report whenZERO_PROVIDERoverrides the newly saved active provider, and include effective/override metadata in JSON output. - Enhance
providers list/currentoutput (human + JSON) withselectableandsourcemetadata to distinguish user-config providers from runtime-only entries. - Add tests covering the override messaging/JSON payload and the selectable/source metadata in provider lists.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cli/provider_onboarding.go | Adds override detection for ZERO_PROVIDER to clarify saved vs effective provider; enriches JSON output and error messaging for non-selectable providers. |
| internal/cli/provider_onboarding_test.go | Adds coverage for override messaging/JSON and for improved error text when selecting runtime-only providers. |
| internal/cli/command_center.go | Adds provider list metadata (selectable, source) and updates formatting to mark runtime-only/non-selectable entries. |
| internal/cli/command_center_test.go | Adds coverage for new selectable/source JSON fields and for the runtime-only marker in human output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
please rebase. to main and fix conflicts |
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
-
[P2] Rebase this stale, superseded branch before merging.
internal/cli/provider_onboarding.go
The current head is not descended frommainand has content conflicts in both this file and its test. The resulting base-to-head comparison spans 227 files rather than the PR's four-file merge-base diff. Main already contains #716's runtime-only-provider handling and #767's fix for #721; resolving these conflicts by taking this branch would remove both. Please rebase onto currentmain, retain the existing fixes, and submit the resolved diff for review rather than merging this head. -
[P2] Do not report an unresolvable
ZERO_PROVIDERvalue as the effective provider.
internal/cli/provider_onboarding.go:64
A nonemptyZERO_PROVIDERis reported as the effective provider without checking that it resolves. For example, a staleZERO_PROVIDER=runtimewhen noruntimeprofile exists printsEffective provider: runtimeand suggestszero providers check runtime, but the next resolution fails before that check can run. Validate the override against the resolved provider list, or report it as invalid and direct the user to fix or unset it.
|
Heads-up @PierrunoYT: #767 merged and covers the core of this (the warning when ZERO_PROVIDER overrides a providers-use selection, #721). Your PR goes further though, and the extra parts are genuinely useful: the JSON effective-provider/override-source fields and the user-config-vs-runtime-only list marking are not in #767, so this is not fully superseded. To carry it forward it would need a rebase onto main (post-#767) and a trim down to just the net-new parts (the JSON output, the list marking, and the runtime-only explanation), since the warning itself is now landed. Happy to review that, or if you would rather, fold those into a fresh focused PR. Let us know which you prefer. |
…lection-overrides Keeps main's Gitlawb#767 override reporting and drops this branch's duplicate of it, so the branch reduces to what main does not have yet: - providers list/current: per-entry selectable + source metadata (JSON) and a "(not selectable via providers use)" marker in the human output. - providers use: an actionable error when the requested name is only resolvable at runtime and cannot be saved. The override plumbing is main's: activeProviderEnvOverride reading the injected deps.getenv, not a direct os.Getenv, so tests stay hermetic. The branch's duplicate `providers use` override tests are dropped in favour of main's; its runtime-only-selection test is kept. Also addresses the two open review points: - An unresolvable ZERO_PROVIDER is no longer reported as the effective provider. activeProviderEnvOverrideResolves checks the saved profile in the config file just written, then the resolver for env-derived profiles; when neither matches, the note says the override resolves to nothing and the JSON payload omits effectiveProvider (adding envProvider/envProviderResolves). - Non-user-config entries are labelled source "resolved" rather than "runtime": project config and provider commands persist profiles too, just not where `providers use` writes, so the only claim the label makes is that the entry cannot be selected with that command.
|
Merged current [P2] Rebase this stale, superseded branch — done, and resolved away from this branch where
Net diff against [P2] Do not report an unresolvable
@copilot: "runtime-only; not selectable/saved" over-claims for other persisted sources — valid, and it applies to the @coderabbitai: reset Validation (Windows host, Go 1.26.5): @jatmn @Vasanthdev2004 — the resolved diff is ready for another look; I can't use the reviewer-request button on this repo, hence the mention. @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/provider_onboarding_test.go (1)
163-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a companion test for the resolver-fallback-resolves-true case.
This test nicely covers the fully-unresolvable override. Consider adding a sibling test where
ZERO_PROVIDERnames a provider that is not persisted but is resolvable viaresolveCommandCenterConfig(e.g. set an ambientOPENAI_API_KEYand point the override at the env-derived provider name while switching to a different saved profile). That exercises the second branch ofactiveProviderEnvOverrideResolves(lines 148-156 in provider_onboarding.go), which currently only sees coverage via the persisted-check branch and the fully-unresolvable branch.As per path instructions, "
**/*_test.go: ... add regression tests for behavior changes."🧪 Suggested additional test sketch
func TestRunProvidersUseFlagsResolvableEnvOverrideViaResolver(t *testing.T) { t.Setenv("OPENAI_API_KEY", "sk-env") configPath := providersUseOverrideConfig(t) deps := providerSetupDeps(configPath) deps.getenv = func(key string) string { switch key { case config.ActiveProviderEnv: return "openai" case "OPENAI_API_KEY": return "sk-env" default: return "" } } var stdout, stderr bytes.Buffer if code := runWithDeps([]string{"providers", "use", "fast", "--json"}, &stdout, &stderr, deps); code != exitSuccess { t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) } var payload map[string]any if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { t.Fatalf("decode JSON: %v\n%s", err, stdout.String()) } if payload["effectiveProvider"] != "openai" { t.Fatalf("expected env-derived override to resolve via resolver fallback, got %#v", payload) } }🤖 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_test.go` around lines 163 - 213, Add a sibling regression test next to TestRunProvidersUseFlagsUnresolvableEnvOverride that covers an environment override naming a provider absent from persisted profiles but resolvable by resolveCommandCenterConfig, such as “openai” with an injected OPENAI_API_KEY. Invoke providers use with --json and assert successful execution reports that provider as effectiveProvider, exercising the resolver-fallback branch of activeProviderEnvOverrideResolves while retaining the existing unresolvable test.Source: Path instructions
internal/cli/provider_onboarding.go (1)
137-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolver-fallback branch of
activeProviderEnvOverrideResolvesis untested.This function has two independent resolution paths: the persisted-config check (line 149) and the resolver-fallback via
resolveCommandCenterConfig/providerResolvedByName(lines 152-156). Only the persisted-true case ("work") and the fully-unresolvable case ("removed-profile") are covered by tests; the case whereZERO_PROVIDERnames an env-derived-but-unpersisted provider (e.g. an ambientOPENAI_API_KEY-derived profile) is not tested, leaving one of the two logical branches unverified.See companion comment on
internal/cli/provider_onboarding_test.gofor the suggested 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/cli/provider_onboarding.go` around lines 137 - 158, Add a regression test for activeProviderEnvOverrideResolves covering an env-derived provider that is not persisted, such as one synthesized from an ambient OPENAI_API_KEY. Configure the test so the persisted-profile check does not match, resolveCommandCenterConfig succeeds, and providerResolvedByName finds the override, asserting the function returns true while preserving existing persisted and unresolvable cases.
🤖 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/cli/provider_onboarding_test.go`:
- Around line 163-213: Add a sibling regression test next to
TestRunProvidersUseFlagsUnresolvableEnvOverride that covers an environment
override naming a provider absent from persisted profiles but resolvable by
resolveCommandCenterConfig, such as “openai” with an injected OPENAI_API_KEY.
Invoke providers use with --json and assert successful execution reports that
provider as effectiveProvider, exercising the resolver-fallback branch of
activeProviderEnvOverrideResolves while retaining the existing unresolvable
test.
In `@internal/cli/provider_onboarding.go`:
- Around line 137-158: Add a regression test for
activeProviderEnvOverrideResolves covering an env-derived provider that is not
persisted, such as one synthesized from an ambient OPENAI_API_KEY. Configure the
test so the persisted-profile check does not match, resolveCommandCenterConfig
succeeds, and providerResolvedByName finds the override, asserting the function
returns true while preserving existing persisted and unresolvable cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f848d4aa-1f56-4478-ba84-ea66efece4ee
📒 Files selected for processing (4)
internal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/command_center_test.go
- internal/cli/command_center.go
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
-
[P2] Determine the effective override from the resolved active profile
internal/cli/provider_onboarding.go:149
A persisted row, or merely a matching entry inresolved.Providers, is not proof thatZERO_PROVIDERis the provider a subsequent invocation will use. For example, a saved OpenAI-compatible profile without a model passes the persisted fast path, soproviders use fast --jsonreportsenvProviderResolves: trueandeffectiveProvider: "broken"; the next resolution instead fails because the activebrokenprofile requires a model. Likewise, a provider command can leave the env-named profile in the list while setting a different active provider. Run the resolver for every override and require the successfully resolvedActiveProviderto match the override before emitting the effective-provider fields/note. -
[P3] Do not case-fold provider identities when labeling individual list entries selectable
internal/cli/command_center.go:154
Resolution merges provider names case-sensitively, but this map collapsesworkandWORK. If user config hasworkand project config (or a provider command) contributesWORK, both resolved entries are labeledselectable: true, source: "user-config".providers use WORKonly updates the user-configworkrow, so it cannot select the displayedWORKentry. Preserve the concrete persisted identity when deriving the metadata (or explicitly reject/handle case-only collisions) and add a regression test.
activeProviderEnvOverrideResolves no longer treats a config.json row or a resolved-list match as proof ZERO_PROVIDER is effective. It now always runs the resolver and requires the resolved ActiveProvider to match the override, so a persisted-but-broken profile (e.g. missing a required model) is reported as unresolvable instead of falsely "effective". providers list/current also stopped case-folding provider names when deriving selectable/source metadata. Resolution merges providers case-sensitively, so a project config or provider command can add a "WORK" entry alongside a persisted "work"; `providers use` can only ever select the exact persisted casing, so the case-variant entry must not be labeled selectable too. Addresses review feedback from jatmn on PR Gitlawb#725. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/command_center_test.go`:
- Around line 271-276: Update the assertions in the provider cases of the test
to compare Source directly against the documented JSON strings "user-config" and
"resolved" instead of production constants, while preserving the existing
Selectable checks and failure messages.
🪄 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 Plus
Run ID: aa142389-f1f1-48db-8ae1-a5138ccf7608
📒 Files selected for processing (4)
internal/cli/command_center.gointernal/cli/command_center_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/provider_onboarding.go
- internal/cli/command_center.go
- internal/cli/provider_onboarding_test.go
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
-
[P2] Avoid executing the provider command just to render the override note
internal/cli/provider_onboarding.go:150
activeProviderEnvOverrideResolvesnow performs a full config resolution afterproviders usehas already writtenconfig.json. WhenZERO_PROVIDER_COMMANDis configured, that resolution runs the configured shell command (and can wait up to five seconds); its side effects or failure are then discarded and reported as an unresolvedZERO_PROVIDER. This makes a previously config-only selection unexpectedly execute an external command after committing the change. Determine the override status without loading the provider command, or surface and handle that resolution failure before claiming a successful selection. -
[P2] Resolve the case-variant provider identity ambiguity in list selectability
internal/cli/command_center.go:160
The new exact-case lookup correctly treats a resolvedWORKprofile as distinct from persistedwork, butProviderPersistedandSetActiveProviderstill match case-insensitively. Copying the displayedWORKintozero providers use WORKtherefore exits successfully while silently activating the different persistedworkprofile. Case-only persisted duplicates have the inverse problem: both are reported selectable although the mutator always selects the first match. Align the mutation and listing identity rules (or reject ambiguous case variants) so a selected list entry cannot resolve to a different provider.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-d742-77fa-b3c2-addbca9602cd Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Final follow-up on current head
The PR is mergeable. PierrunoYT requests fresh reviewer approval to supersede the previous |
…lection-overrides
|
Merged current The still-valid
All four inline review threads are resolved; the latest CodeRabbit review approved the implementation. Validation in the Linux orb with Go 1.26.5:
Skipped: none. Post-push GitHub checks are expected to rerun on @jatmn @Vasanthdev2004 — please re-review the current head when convenient so the stale |
|
Post-push update for |
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.
The prior review round's rebase/supersession, unresolvable ZERO_PROVIDER reporting, resolver-backed override proof, provider-command deferral, list selectability case folding, and exact-identity edits for SetActiveProvider/ProviderPersisted/RemoveProvider/RenameProvider/SetProviderModel/EditProvider are all addressed on e1d1e42. The remaining problems are new providers use error routing introduced here, a regression in env-derived name matching, and an incomplete credential-identity migration that this branch makes user-visible.
Findings
-
[P2] Do not label every
providers usenot-found as a runtime-only profile
internal/cli/provider_onboarding.go:74
This PR adds a suffix to anySetActiveProvidererror whose text containsproviderandnot found. Onmain, a typo such asproviders use fasttwhenfastis saved simply returnedprovider "fastt" not found; here it also says to runproviders setuporproviders add. That guidance is wrong when the name does not exist in the resolved provider list at all. Only append the selectability hint when the name is absent from user config but present in the resolved list (the same conditionreportUnpersistedProviderUsealready uses), and leave plain unknown-name failures as a simple not-found error. -
[P3] Wrong-case env-synthesized names regressed away from the #707 path
internal/cli/provider_onboarding.go:595
providerResolvedByNamechanged from case-insensitive to exact match in this PR. With onlyOPENAI_API_KEYset,providers use openaistill reachesreportUnpersistedProviderUse(exit 0,persisted:false), butproviders use OpenAInow skips that path and falls through to the misleading not-found wrapper above even thoughopenaiis resolvable for the session. Keep exact identity for persistedconfig.jsonrows, but restore case-insensitive matching for env/catalog-derived profile names before deciding whether a provider is resolvable-but-unpersisted. -
[P2] Finish the exact-identity migration in the credential marker helpers
internal/config/writer.go:137,internal/config/credentials.go:102
The main config mutators now use exact, case-sensitive identity, and this branch adds regression tests for coexistingwork/WORKrows, butMarkProviderAPIKeyStoredandClearProviderKeyStoredstill useEqualFold(they were already case-folding before this PR; the gap is now inconsistent and user-visible). Afterzero author key removal targets"WORK", theworkrow can get theapiKeyStoredmarker set or cleared instead. Apply the same exact-name rule here and add the same case-variant regression coverage used for the other writer mutators. -
[P2] Case-variant config rows still share one credential-store entry
internal/credstore/credstore.go:267,internal/cli/provider_onboarding.go:493
Lowercasing store keys predates this PR, but this branch now treats distinctworkandWORKconfig rows as a supported layout.providers remove WORKdeletes only theWORKrow yetremoveStoredProviderKeyAtdeletes the sharedworkkey, leaving the survivingworkprofile without its stored secret. Either reject ambiguous case-variant rows up front or make stored-key read/write/delete use the same exact identity as the config writers.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the remaining provider identity findings in
Validation: affected config, CLI, and TUI tests pass with |
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.
The prior review round's rebase/supersession work, unresolvable ZERO_PROVIDER reporting, resolver-backed override proof, provider-command deferral, list selectability case folding, exact-identity config mutators, credential-marker exact matching, and the providers use typo-vs-runtime-only routing fix from d41bd16 all look addressed on the current head. The items below are verified against d41bd163; they are new regressions or incomplete follow-through from that commit, not stale CI or pre-existing drift.
Findings
-
[P2]
zero auth login/zero auth chatgptnow block OAuth for mixed-case persisted profile names
internal/cli/auth.go:405,internal/cli/auth.go:424,internal/cli/auth.go:187
d41bd16adds bothstrings.ToLoweron the login target andPreflightProviderWriteagainst that lowercased spelling. Onmain, login neither lowercased nor preflighted. A config with a sole legitimate case-variant row such as"OpenRouter"or"ChatGPT"(TestResolvePreservesSoleOpenRouterCaseVariant) now fails before any browser/device flow withprovider "openrouter" already exists as "OpenRouter". TUI OAuth still only callsPreflightUserConfig, so the same on-disk config can complete OAuth in the wizard while the CLI path is blocked. Resolve the login target against the persisted row (or catalog id) before preflighting, or allowPreflightProviderWriteto accept a case-variant re-login of the same credential identity. -
[P2]
ZERO_PROVIDERoverride resolution false-negatives when env casing differs from the resolved profile name
internal/cli/provider_onboarding.go:182
activeProviderEnvOverrideResolutionrunsconfig.Resolveto prove the override works, but comparesresolved.ActiveProviderto the raw env string with exact equality. Resolution selects the active row case-insensitively and returns the canonical persisted name (normalizeProvidersWithOptionsininternal/config/resolver.go), so a valid override such asZERO_PROVIDER=openrouterwith a saved"OpenRouter"profile resolves successfully at runtime yet is reported asenvProviderResolves: false, omitseffectiveProvider, and prints the “cannot be resolved / Zero cannot start” note. Compare case-insensitively (or compare against the env-applied active name) and add a regression test for this mismatch. -
[P2]
providers usetreats catalog-id addressing of a saved profile as an env-only provider
internal/cli/provider_onboarding.go:595,internal/cli/provider_onboarding.go:628
This PR adds an exactCatalogIDmatch toproviderResolvedByName's first loop;mainonly case-foldedName. A persisted user-config row such as{name:"my-router", catalogId:"openrouter"}appears in the resolved list, butzero providers use openrouterskipsSetActiveProvider, exits 0, and prints the runtime-only/env guidance becauseProviderPersisted("openrouter")is false whileproviderResolvedByNamematches the saved row by catalog id. Only genuinely env-synthesized names should take the unpersisted path. Restrict the catalog-id branch to env-derived rows, or require that no persisted row already owns that catalog identity. -
[P2]
providers remove/providers renamedid not get the same case-variant guard asproviders use
internal/cli/provider_onboarding.go:657,internal/cli/provider_onboarding.go:688
reportUnpersistedProviderUsenow checksProviderPersistedCaseInsensitiveand falls through to the real not-found path when a case-variant persisted row exists (provider_onboarding.go:617), butreportUnpersistedProviderRemoveandreportUnpersistedProviderRenamedo not.zero providers remove SAVED/rename SAVED …against a saved"saved"row therefore exit 0 with the env-only message instead of the same plain not-found behaviorproviders usenow has. Apply the same persisted-case guard on remove/rename.
Needs maintainer decision @Vasanthdev2004 @gnanam1990
- Rejecting ambiguous case-duplicate user config is intentional, but there is no in-app repair path
internal/config/resolver.go:77,internal/config/writer.go:336,internal/config/writer.go:859
ValidatePersistedProviderNamesfailingResolve()and every mutator — includingRemoveProvider— forwork/WORKduplicates is deliberate (TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariantsexpects the mutation to be blocked). That is not a mistaken finding. The remaining problem is operational: the error tells users to “rename or remove one row,” but no CLI/TUI command can delete one duplicate while the file is still invalid, so pre-existing ambiguous configs require manual JSON surgery before Zero starts. Please either accept that as a one-time breaking migration, or add a repair path (for example, allow exact-name removal of one duplicate without requiring the file to already pass validation, or ship a one-shot migration/command) and adjust the error text to match whichever policy you choose.
Findings (lower priority)
- [P3] Case-mismatch guidance is inconsistent across commands
internal/cli/provider_onboarding.go:617,internal/cli/auth.go:481
zero providers use SAVEDagainst a persisted"saved"row correctly avoids the runtime-only suffix (perTestRunProvidersUseRejectsCaseVariantOfPersistedProvider) but still returns onlyprovider "SAVED" not found, whilezero auth logout SAVEDexplains that a differently cased row exists. If exact casing is required everywhere, consider returning the same capitalization hint onproviders useand on the remove/rename case-variant paths above. Separately,auth logoutcould print the canonical persisted spelling (for example"saved") instead of only saying the exact name is required.
Four places drew the identity line in the wrong spot. A login is not a new provider write. Preflighting it as one rejected the very row it was logging into: a config whose sole ChatGPT profile is spelled "ChatGPT" failed `zero auth chatgpt` before the browser flow with "provider \"chatgpt\" already exists as \"ChatGPT\"", while the TUI completed the same login. EnsureCatalogProvider reuses whatever row owns the identity, so PreflightCatalogProviderLogin now checks that instead, and keeps the collision check for the case where a row would actually be created. The ZERO_PROVIDER override check ran the resolver to prove the override works, then compared its result to the raw env string exactly. Resolution matches the active row case-insensitively and reports the row's persisted spelling, so ZERO_PROVIDER=openrouter against a saved "OpenRouter" resolved fine yet was reported as unresolvable — the opposite of true. `providers use <catalog-id>` against a row saved under a different name took the runtime-only path and exited 0 with an environment-variable explanation that was simply false for a provider the config owns. Every runtime-only report now asks config first, and remove/rename get the same persisted-case guard use already had rather than the env-only message on a case variant of a saved row. Falling through to a bare "not found" leaves the user to guess what they got wrong, so the provider mutators now name the saved profile — as `auth logout` already did for capitalization, and one better by naming the spelling that works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed all four P2s and the P3 in All four turned out to be the same mistake in different places: treating one provider identity as two because the spelling or the addressing differed. [P2] [P2] [P2] Catalog-id addressing of a saved row. Went with your second option — require that no persisted row owns the identity. New [P2] remove/rename guard. Same helper, same guard, now on all three commands rather than just [P3] Case-mismatch guidance. That covers the catalog-id case too ( Regressions — four new tests, each verified to fail with the source changes stashed and the tests kept, reproducing exactly the symptoms you described:
Not addressed — maintainer decision. I deliberately left the case-duplicate repair path alone: whether pre-existing ambiguous configs get a one-time breaking migration or an in-app repair command is a product call, and the error text should follow whichever is chosen rather than the other way round. Happy to implement either once @Vasanthdev2004 / @gnanam1990 decide. Nothing in this commit changes that behavior. Validation
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/auth.go (1)
465-485: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLogout's capitalization-mismatch error doesn't tell the user what to type.
When a case-variant is detected, the error only echoes the user's own input back at them — it never surfaces the actual persisted spelling, so the user has no easy way to discover the exact string required. A one-line addition pointing them at
providers list(or embedding the actual persisted name, if cheaply available) would make this actionable rather than just a dead end.🛠️ Proposed fix
if folded { - return writeAppError(stderr, fmt.Sprintf("provider %q exists with different capitalization; logout requires the exact persisted provider name", provider), exitCrash) + return writeAppError(stderr, fmt.Sprintf("provider %q exists with different capitalization; logout requires the exact persisted provider name (run \"zero providers list\" to see it)", provider), exitCrash) }🤖 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/auth.go` around lines 465 - 485, Update the case-insensitive provider mismatch error in the logout flow around ProviderPersistedCaseInsensitive to tell users how to discover the exact persisted spelling, preferably by directing them to run “providers list”; retain the existing exact-name requirement and error 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.
Nitpick comments:
In `@internal/cli/auth.go`:
- Around line 465-485: Update the case-insensitive provider mismatch error in
the logout flow around ProviderPersistedCaseInsensitive to tell users how to
discover the exact persisted spelling, preferably by directing them to run
“providers list”; retain the existing exact-name requirement and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6658f0cb-5c9d-4cbb-a41c-d2fd219fa4d6
📒 Files selected for processing (19)
internal/cli/app.gointernal/cli/auth.gointernal/cli/auth_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.gointernal/cli/provider_setup.gointernal/cli/setup.gointernal/config/credentials.gointernal/config/credentials_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/writer.gointernal/config/writer_test.gointernal/oauth/manager.gointernal/tui/oauth_device.gointernal/tui/onboarding.gointernal/tui/provider_manager.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/writer_test.go
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.
The prior review round's rebase/supersession work, unresolvable ZERO_PROVIDER reporting, resolver-backed override proof, provider-command deferral, list selectability case folding, exact-identity config mutators, credential-marker exact matching, providers use typo-vs-runtime-only routing, catalog-id addressing of saved profiles, case-variant env-override resolution, and PreflightCatalogProviderLogin for CLI OAuth login all look addressed on 7896ff32fa7f16044d9c22bb9a40b9f5a418e6c6. The items below were re-verified against that head (not drift from an earlier SHA or merge-base mismatch).
Findings
-
[P2]
zero auth logoutblocks the logout path the TUI documents for case-variant profile names
internal/cli/auth.go:472
Login now accepts catalog-id / case-variant addressing viaPreflightCatalogProviderLoginand stores OAuth tokens under the catalog id (e.g.chatgpt).oauthLoginNamereports that stored key back to users (zero auth logout chatgptinprovider_manager.go), butrunAuthLogoutstill hard-stops whenProviderPersistedCaseInsensitivematches and the exact name does not (internal/cli/auth.go:476-482), somanager.Logout,ForgetProviderKey, andClearProviderKeyStorednever run.Repro: config with a sole ChatGPT row saved as
"ChatGPT"(any setup that persisted a capitalized name).zero auth chatgptsucceeds;zero auth logout chatgpt(the string the TUI emits) fails with the capitalization error and leaves the OAuth token and any stored API key in place.zero auth logout ChatGPTwould pass the guard but is not what the UI documents when the token lives under the catalog id.Please resolve the logout target the same way login does —
PersistedProviderIdentityand/or the stored OAuth key — instead of treating a catalog-id or case-variant invocation as a hard error. -
[P2]
zero auth openrouterstill runs the browser flow before config preflight
internal/cli/auth.go:102
runAuthChatGPTandrunAuthLogincallPreflightCatalogProviderLoginbefore opening the browser. The TUI OpenRouter wizard at least runsPreflightUserConfigviapreflightOAuthUserConfigbeforeOpenRouterLogin(internal/tui/provider_wizard.go:164).runAuthOpenRouteris the outlier: it callsdeps.openRouterLoginfirst and only reachesPreflightUserConfiginsidesaveOpenRouterProviderKeyafter the key is minted (internal/cli/auth.go:134).Repro: invalid or ambiguous
config.json(anythingPreflightUserConfigrejects). The user completes the browser/PKCE flow, then save fails. Worse, a save failure still returns exit 0 with a manualexport OPENROUTER_API_KEY=...fallback (internal/cli/auth.go:112-117), so the command can look successful while nothing was persisted. Preflight before starting PKCE, matching the other auth entry points, and consider a non-zero exit when persistence fails. -
[P2] Setup and wizard finalize still use write-time collision checks for catalog re-login
internal/tui/provider_wizard.go:1298,internal/cli/setup.go:267,internal/tui/onboarding.go:876
CLI catalog OAuth reuses existing rows throughEnsureCatalogProvider/PreflightCatalogProviderLogin, but finalize paths still callPreflightProviderWriteon the catalog-derived profile name (typically lowercased, e.g.openrouterfromproviderWizardDisplayName/setupProfileName). When a legitimately saved row already owns that identity under different casing (e.g."OpenRouter"), finalize fails withprovider "openrouter" already exists as "OpenRouter"after a successful OpenRouter OAuth/key capture, whilezero auth openroutersucceeds viaEnsureCatalogProvider.This is narrower than the logout gap (re-setup or wizard re-finalize with a case-variant row already on disk) but it is the same partial migration: identity-aware preflight on login, collision preflight on persist. Use
PreflightCatalogProviderLogin/EnsureCatalogProvider(or derive the persisted profile name before writing) on these finalize paths.
Findings (lower priority)
-
[P3]
ZERO_PROVIDERoverride warnings fire when only the spelling differs
internal/cli/provider_onboarding.go:155
This PR correctly fixed resolution (activeProviderEnvOverrideResolutionusesEqualFold; seeTestRunProvidersUseResolvesCaseVariantEnvOverride), but the warning trigger still compares raw strings inactiveProviderEnvOverride(TestActiveProviderEnvOverrideTreatsCaseVariantAsDistinctlocks that in).zero providers use workwithZERO_PROVIDER=WORKtherefore prints an override note and JSON override fields even though resolution proves the env value selects the same saved row. Runtime behavior is correct; the warning is misleading. Compare normalized identity, or suppress the note when resolution proves the override names the same provider. -
[P3]
auth logoutcase-mismatch errors do not name the persisted spelling
internal/cli/auth.go:482
If exact casing remains required on logout, the error should name the saved profile the wayproviderMutationErrornow does forproviders use/remove/rename(internal/cli/provider_onboarding.go:622). Today it only says capitalization differs. Include the canonical persisted name (viaPersistedProviderIdentity) or direct the user toproviders list. This becomes moot if the P2 logout fix resolves identity instead of rejecting.
Needs maintainer decision @Vasanthdev2004 @gnanam1990
- Rejecting ambiguous case-duplicate user config is intentional, but there is still no in-app repair path
internal/config/resolver.go:77,internal/config/writer.go:396
ValidatePersistedProviderNamescorrectly blockswork/WORKduplicates at resolution time, and every mutator includingRemoveProvidervalidates before acting, so the error text "rename or remove one row" cannot be followed through shipped commands. Pre-existing ambiguous configs require manual JSON surgery before Zero starts. Please either accept that as a one-time breaking migration, or add a repair path (for example, allow exact-name removal of one duplicate without requiring the file to already pass validation) and align the error text with whichever policy you choose.
Making login identity-aware left the surrounding paths on write-time collision checks, so one provider identity was still two in three places. Logout hard-stopped whenever a persisted row matched case-insensitively without matching exactly. Login accepts a catalog id and stores its token under that key, and the TUI tells users to run `zero auth logout <catalog id>` — so the documented command failed and left the OAuth token and any stored API key in place. Logout now resolves the target the way login does: credential keys stay on the spelling the user typed, where login put them, while the config mutation uses the persisted row's name. Both spellings are cleared from the key store when they differ, since a setup-captured key and a login-captured one live under different names. `zero auth openrouter` ran the browser flow before checking the config, so a user could complete a PKCE round trip only to have the save fail — and it then exited 0, reporting success for a command that persisted nothing. It now preflights first like every other auth entry point, and a failed save is a failure. The minted key is still printed, because it is real and the user paid a browser round trip for it. Setup and the wizard collided with the row that catalog login reuses. When a caller takes the catalog's own default name, it now yields to the row already holding that catalog identity. A user-chosen name is deliberately left alone: there a case collision is a real collision, and silently overwriting the other row would be worse than the error. Also fold case in the ZERO_PROVIDER override warning. Resolution already selects the active row case-insensitively, so ZERO_PROVIDER=WORK against a saved "work" lands on exactly the row the user just selected; warning that the switch stays overridden described a conflict that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three P2s and both P3s addressed in [P2] One thing that fell out while writing it: the key store is asked for both spellings when they differ. A key captured by provider setup lives under the persisted row's name while one captured by a catalog login lives under the catalog id, so clearing only what the user typed would have left the other behind. [P3, folded in] [P2] I also took the "consider" on the exit code, because I think the current behavior is worse than it looks: a failed save returned 0, so a script would carry on believing the provider was configured. It now exits non-zero. The minted key is still printed — it is real and the user paid a browser round trip for it — but printing a key is not the same as persisting one, and only the exit code says which happened. [P2] Setup and wizard finalize adopt the persisted row. New
[P3] Not addressed, still yours: the case-duplicate repair path. Unchanged from last round — whether that is a one-time breaking migration or an in-app repair command is a product call, and the error text should follow the decision. Tests: four new, plus one replaced. Validation: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/auth_test.go (1)
229-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the rejected ChatGPT attempt neither logs in nor overwrites the token.
This branch only checks stderr. Track callback invocation and verify the existing
freshtoken remains stored after rejection.Proposed test assertions
stdout.Reset() stderr.Reset() + loginCalled := false code = runWithDeps([]string{"auth", "chatgpt"}, &stdout, &stderr, appDeps{ userConfigPath: func() (string, error) { return configPath, nil }, chatGPTLogin: func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) { + loginCalled = true return oauth.Token{AccessToken: "should-not-save"}, nil }, }) if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { t.Fatalf("exit = %d, stderr = %q; want the ambiguous-config failure preserved", code, stderr.String()) } + if loginCalled { + t.Fatal("login ran despite ambiguous persisted provider names") + } + token, ok, err = store.Load(oauth.ProviderKey("chatgpt")) + if err != nil || !ok || token.AccessToken != "fresh" { + t.Fatalf("stored token = %+v, %v, %v; want fresh token unchanged", token, ok, err) + }As per coding guidelines, “
**/*_test.go: … add a regression test for behavior changes.”🤖 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/auth_test.go` around lines 229 - 245, The ambiguous-provider rejection test around runWithDeps must verify that the rejected ChatGPT request neither invokes chatGPTLogin nor overwrites the existing fresh token. Track callback invocation, seed the configuration with a fresh token, then assert the callback was not called and the persisted token remains unchanged after the command fails.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@internal/cli/auth_test.go`:
- Around line 229-245: The ambiguous-provider rejection test around runWithDeps
must verify that the rejected ChatGPT request neither invokes chatGPTLogin nor
overwrites the existing fresh token. Track callback invocation, seed the
configuration with a fresh token, then assert the callback was not called and
the persisted token remains unchanged after the command fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e51aa94-a48d-4675-8885-9737fc02816c
📒 Files selected for processing (8)
internal/cli/auth.gointernal/cli/auth_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.gointernal/cli/provider_setup.gointernal/cli/setup.gointernal/config/writer.gointernal/tui/provider_wizard.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cli/auth.go
- internal/config/writer.go
- internal/cli/provider_onboarding.go
- internal/tui/provider_wizard.go
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.
The branch is mergeable and all required GitHub checks are green, but GitHub still shows the pull request as blocked on my earlier CHANGES_REQUESTED review. Please dismiss or supersede that review after the items below are fixed.
The prior review items on exact mutators, runtime-only routing, env-override resolution, auth login/logout/openrouter identity, and setup/wizard catalog adoption look addressed on 54c1d2e. What remains is one verified regression from the exact-identity migration and two policy gaps this branch introduces alongside the new ambiguous-config validation.
Findings
- [P2] Keep
activeProviderin sync when the removed or renamed row is the active profile
internal/config/writer.go:440,internal/config/writer.go:512,internal/config/writer.go:607
This is a real regression introduced here, not drift frommain.RemoveProvider,RenameProvider, andEditProvidernow updateactiveProvideronly on case-sensitive equality with the mutated row, butmainusedEqualFoldfor that hand-off and resolution still selects the active row case-insensitively. You do not need ambiguouswork/WORKduplicates to hit this: a single saved"work"row with project config settingactiveProvider: "WORK"is enough.Resolve()works,zero providers remove workdeletes the profile, andactiveProviderstays"WORK"pointing at nothing. Please clear or repointactiveProviderwhenever the mutated row is the active profile under case-insensitive identity, and add regression coverage for remove/rename/edit with mismatchedactiveProvidercasing.
Findings (lower priority)
- [P3] Decide whether
zero auth logoutshould clear credentials whenconfig.jsonfails validation
internal/cli/auth.go:483
PreflightUserConfigon logout is also new in this branch (d41bd16);maindid not gate logout on config validity. With the newValidatePersistedProviderNameschecks, an ambiguouswork/WORKfile now blocks logout beforemanager.LogoutorForgetProviderKeyrun, so users cannot clear OAuth/API keys and then hand-edit JSON the way they could before. That is a deliberate extension of “reject bad config everywhere” unless you want logout to stay a recovery escape hatch. If recovery matters, run OAuth and credential-store cleanup without requiring the file to pass validation, and only gate theClearProviderKeyStoredwrite on a readable config; if not, say so in the error text and fold this into the maintainer decision below.
Needs maintainer decision @Vasanthdev2004 @gnanam1990
-
Rejecting ambiguous case-duplicate user config is intentional, but there is still no in-app repair path
internal/config/resolver.go:77,internal/config/writer.go:396
ValidatePersistedProviderNamesis new in this pull request. Blockingwork/WORKduplicates atResolve()and in every mutator is deliberate, and the error text tells users to rename or remove one row. No CLI/TUI command can delete one duplicate while the file is still invalid, so pre-existing ambiguous configs require manual JSON surgery before Zero starts. Please either accept that as a one-time breaking migration, or add a repair path (for example, allow exact-name removal of one duplicate without requiring the file to already pass validation) and align the error text with whichever policy you choose. -
Whether credential cleanup should work on an otherwise-invalid config
internal/cli/auth.go:483
This overlaps the P3 item above. If the answer to the repair-path question is “manual JSON only,” please confirm that blockingzero auth logouton invalid config is intentional too, so users are not told to fix config while every shipped command that could help is refused.
Amp-Thread-ID: https://ampcode.com/threads/T-019fafa2-7d9d-75bc-8801-6d0efc8db1df Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the remaining provider-identity and recovery findings in
Validation passed: @jatmn @Vasanthdev2004 — PierrunoYT requests a fresh review of the current head. The account does not have permission to use GitHub’s reviewer-request button. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/auth.go (1)
483-527: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRecovery logout leaves API keys under alternate case-variant spellings. An ambiguous persisted config prevents canonical identity resolution, so cleanup only deletes the typed spelling even though provider-setup keys may be stored under a persisted spelling.
internal/cli/auth.go#L483-L527: for an ambiguity error, enumerate and deduplicate all case-insensitive persisted provider names, delete each corresponding key, then return the preflight error.internal/cli/auth_test.go#L328-L350: seed API keys under alternate persisted spellings such asDemoandDEMO, and assert recovery logout removes them.🤖 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/auth.go` around lines 483 - 527, Update the logout cleanup in internal/cli/auth.go lines 483-527 to handle ambiguous PersistedProviderIdentity errors by enumerating all persisted provider names matching provider case-insensitively, deduplicating them, deleting each corresponding key, and then returning the original preflight error; preserve existing cleanup for unambiguous identities. Extend the recovery logout test in internal/cli/auth_test.go lines 328-350 to seed keys under alternate spellings such as Demo and DEMO and assert both are removed.
🤖 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.
Outside diff comments:
In `@internal/cli/auth.go`:
- Around line 483-527: Update the logout cleanup in internal/cli/auth.go lines
483-527 to handle ambiguous PersistedProviderIdentity errors by enumerating all
persisted provider names matching provider case-insensitively, deduplicating
them, deleting each corresponding key, and then returning the original preflight
error; preserve existing cleanup for unambiguous identities. Extend the recovery
logout test in internal/cli/auth_test.go lines 328-350 to seed keys under
alternate spellings such as Demo and DEMO and assert both are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a74457e6-0225-4d1a-8e42-eda4a774a66a
📒 Files selected for processing (7)
internal/cli/auth.gointernal/cli/auth_test.gointernal/cli/provider_onboarding.gointernal/cli/provider_onboarding_test.gointernal/config/writer.gointernal/config/writer_test.gointernal/tui/provider_manager.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cli/provider_onboarding_test.go
Summary
ZERO_PROVIDERoverrides a provider saved byproviders useproviders useValidation
go test ./...go vet ./...govulncheck ./...Repository-wide golangci-lint still reports existing findings unrelated to this change.
Fixes #721
Summary by CodeRabbit
providers listnow labels profiles with case-sensitive selectable status and source (user-configvsresolved) in both text and--json.