From 4b260698dda43a9c714d970ab802c4236d20220a Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 17 Jul 2026 22:30:04 +0000 Subject: [PATCH 01/11] fix: clarify effective provider selection Co-authored-by: Pierre Bruno --- internal/cli/command_center.go | 76 +++++++++++++++++++++--- internal/cli/command_center_test.go | 37 ++++++++++++ internal/cli/provider_onboarding.go | 21 ++++++- internal/cli/provider_onboarding_test.go | 63 ++++++++++++++++++++ 4 files changed, 187 insertions(+), 10 deletions(-) diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index a6fab33ec..040499a5e 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -1,8 +1,10 @@ package cli import ( + "encoding/json" "fmt" "io" + "os" "sort" "strconv" "strings" @@ -25,6 +27,12 @@ type providerSummary = zerocommands.ProviderSnapshot type modelSummary = zerocommands.ModelSnapshot type providerCatalogSummary = zerocommands.ProviderCatalogSnapshot +type providerCLISummary struct { + providerSummary + Selectable bool `json:"selectable"` + Source string `json:"source"` +} + func runConfig(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseCommandCenterArgs(args, false, false) if err != nil { @@ -125,15 +133,28 @@ func runProviders(args []string, stdout io.Writer, stderr io.Writer, deps appDep return exitCode } summary := summarizeConfig(resolved) - providers := summary.Providers + userProviderNames, err := loadUserProviderNames(deps) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + providers := make([]providerCLISummary, 0, len(summary.Providers)) + for _, provider := range summary.Providers { + _, selectable := userProviderNames[strings.ToLower(strings.TrimSpace(provider.Name))] + source := "runtime" + if selectable { + source = "user-config" + } + providers = append(providers, providerCLISummary{providerSummary: provider, Selectable: selectable, Source: source}) + } if command == "current" { - providers = []providerSummary{} - for _, provider := range summary.Providers { + current := []providerCLISummary{} + for _, provider := range providers { if provider.Active { - providers = append(providers, provider) + current = append(current, provider) break } } + providers = current } if options.json { if command == "current" { @@ -151,12 +172,35 @@ func runProviders(args []string, stdout io.Writer, stderr io.Writer, deps appDep } return exitSuccess } - if _, err := fmt.Fprintln(stdout, formatProviderSummaries(command, providers)); err != nil { + if _, err := fmt.Fprintln(stdout, formatProviderCLISummaries(command, providers)); err != nil { return exitCrash } return exitSuccess } +func loadUserProviderNames(deps appDeps) (map[string]struct{}, error) { + names := map[string]struct{}{} + path, err := deps.userConfigPath() + if err != nil { + return nil, err + } + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return names, nil + } + if err != nil { + return nil, fmt.Errorf("read config %s: %w", path, err) + } + var cfg config.FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + for _, provider := range cfg.Providers { + names[strings.ToLower(strings.TrimSpace(provider.Name))] = struct{}{} + } + return names, nil +} + func runModels(args []string, stdout io.Writer, stderr io.Writer) int { if len(args) > 0 && (args[0] == "list" || args[0] == "ls") { args = args[1:] @@ -326,6 +370,14 @@ func formatConfigSummary(summary configSummary) string { } func formatProviderSummaries(command string, providers []providerSummary) string { + cliProviders := make([]providerCLISummary, 0, len(providers)) + for _, provider := range providers { + cliProviders = append(cliProviders, providerCLISummary{providerSummary: provider, Selectable: true, Source: "user-config"}) + } + return formatProviderCLISummaries(command, cliProviders) +} + +func formatProviderCLISummaries(command string, providers []providerCLISummary) string { title := "Providers" if command == "current" { title = "Provider" @@ -343,24 +395,32 @@ func formatProviderSummaries(command string, providers []providerSummary) string "model: "+displayCLIValue(provider.Model, "none"), "api model: "+displayCLIValue(provider.APIModel, "unknown"), "base url: "+displayCLIValue(provider.BaseURL, "default"), - "api key: "+providerCredentialState(provider), + "api key: "+providerCredentialState(provider.providerSummary), + fmt.Sprintf("selectable: %t (source: %s)", provider.Selectable, provider.Source), ) if provider.Message != "" { lines = append(lines, "status: "+provider.Status+" - "+provider.Message) } continue } - lines = append(lines, " "+formatProviderLine(provider)) + lines = append(lines, " "+formatProviderCLILine(provider)) } return strings.Join(lines, "\n") } func formatProviderLine(provider providerSummary) string { + return formatProviderCLILine(providerCLISummary{providerSummary: provider, Selectable: true, Source: "user-config"}) +} + +func formatProviderCLILine(provider providerCLISummary) string { marker := " " if provider.Active { marker = "*" } - line := fmt.Sprintf("%s %s [%s] model=%s apiModel=%s api key: %s", marker, displayCLIValue(provider.Name, "none"), displayCLIValue(provider.ProviderKind, "unknown"), displayCLIValue(provider.Model, "none"), displayCLIValue(provider.APIModel, "unknown"), providerCredentialState(provider)) + line := fmt.Sprintf("%s %s [%s] model=%s apiModel=%s api key: %s", marker, displayCLIValue(provider.Name, "none"), displayCLIValue(provider.ProviderKind, "unknown"), displayCLIValue(provider.Model, "none"), displayCLIValue(provider.APIModel, "unknown"), providerCredentialState(provider.providerSummary)) + if !provider.Selectable { + line += " (runtime-only; not selectable/saved)" + } if provider.Message != "" { line += " (" + provider.Status + ": " + provider.Message + ")" } diff --git a/internal/cli/command_center_test.go b/internal/cli/command_center_test.go index 430468c9f..e40f72fef 100644 --- a/internal/cli/command_center_test.go +++ b/internal/cli/command_center_test.go @@ -191,6 +191,43 @@ func TestRunProvidersCurrentJSONIncludesRuntimeMetadata(t *testing.T) { } } +func TestRunProvidersListMarksUserAndRuntimeProfiles(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{{Name: "saved"}}}) + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + profiles := []config.ProviderProfile{{Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, {Name: "runtime", ProviderKind: config.ProviderKindOpenAICompatible, Model: "runtime-model"}} + return config.ResolvedConfig{ActiveProvider: "runtime", Provider: profiles[1], Providers: profiles}, nil + } + + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "list", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("code=%d stderr=%s", code, stderr.String()) + } + var payload struct { + Providers []struct { + Name string `json:"name"` + Selectable bool `json:"selectable"` + Source string `json:"source"` + } `json:"providers"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatal(err) + } + if len(payload.Providers) != 2 || payload.Providers[0].Name != "runtime" || payload.Providers[0].Selectable || payload.Providers[0].Source != "runtime" || !payload.Providers[1].Selectable || payload.Providers[1].Source != "user-config" { + t.Fatalf("unexpected providers: %#v", payload.Providers) + } + + stdout.Reset() + if code := runWithDeps([]string{"providers", "list"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("code=%d stderr=%s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "runtime-only; not selectable/saved") { + t.Fatalf("runtime marker missing: %s", stdout.String()) + } +} + func TestRunProvidersCatalogListsDescriptors(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 0fc4e818f..bc2e97f0f 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -3,6 +3,7 @@ package cli import ( "fmt" "io" + "os" "path/filepath" "strconv" "strings" @@ -55,14 +56,30 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app } cfg, err := config.SetActiveProvider(configPath, options.name) if err != nil { + if strings.Contains(err.Error(), "provider ") && strings.Contains(err.Error(), " not found") { + err = fmt.Errorf("%w; only providers saved in user config are selectable (use zero providers setup or zero providers add first)", err) + } return writeAppError(stderr, err.Error(), exitCrash) } + effectiveProvider := strings.TrimSpace(os.Getenv(config.ActiveProviderEnv)) + overridden := effectiveProvider != "" && effectiveProvider != cfg.ActiveProvider if options.json { - if err := writePrettyJSON(stdout, map[string]any{ + payload := map[string]any{ "activeProvider": cfg.ActiveProvider, "configPath": configPath, - }); err != nil { + } + if overridden { + payload["effectiveProvider"] = effectiveProvider + payload["overriddenBy"] = config.ActiveProviderEnv + } + if err := writePrettyJSON(stdout, payload); err != nil { + return exitCrash + } + return exitSuccess + } + if overridden { + if _, err := fmt.Fprintf(stdout, "Saved active provider: %s\nEffective provider: %s\n%s overrides the saved selection.\nnext: %s\n", cfg.ActiveProvider, effectiveProvider, config.ActiveProviderEnv, providerCheckCommand(effectiveProvider, false)); err != nil { return exitCrash } return exitSuccess diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 9493e81ce..9147402d4 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -75,6 +75,69 @@ func TestRunProvidersUseJSONIncludesActiveProviderAndConfigPath(t *testing.T) { } } +func TestRunProvidersUseReportsZERO_PROVIDEROverride(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{ + {Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + }}) + t.Setenv(config.ActiveProviderEnv, "runtime") + + for _, jsonOutput := range []bool{false, true} { + var stdout, stderr bytes.Buffer + args := []string{"providers", "use", "saved"} + if jsonOutput { + args = append(args, "--json") + } + if code := runWithDeps(args, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("json=%t: code=%d stderr=%s", jsonOutput, code, stderr.String()) + } + for _, want := range []string{"saved", "runtime", config.ActiveProviderEnv} { + if !strings.Contains(stdout.String(), want) { + t.Fatalf("json=%t: output missing %q: %s", jsonOutput, want, stdout.String()) + } + } + if !jsonOutput && !strings.Contains(stdout.String(), "zero providers check runtime") { + t.Fatalf("follow-up check did not target effective provider: %s", stdout.String()) + } + if jsonOutput { + var payload map[string]any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatal(err) + } + if payload["activeProvider"] != "saved" || payload["effectiveProvider"] != "runtime" || payload["overriddenBy"] != config.ActiveProviderEnv { + t.Fatalf("unexpected payload: %#v", payload) + } + } + } +} + +func TestRunProvidersUseMatchingZERO_PROVIDERKeepsNormalOutput(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{{Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}}}) + t.Setenv(config.ActiveProviderEnv, "saved") + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "saved"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("code=%d stderr=%s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "Active provider set to saved") || strings.Contains(stdout.String(), "overrides") { + t.Fatalf("unexpected matching-env output: %s", stdout.String()) + } +} + +func TestRunProvidersUseExplainsRuntimeOnlyProfilesAreNotSelectable(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{{Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}}}) + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "runtime"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitCrash { + t.Fatalf("unexpected code %d", code) + } + for _, want := range []string{"only providers saved in user config are selectable", "providers setup", "providers add"} { + if !strings.Contains(stderr.String(), want) { + t.Fatalf("error missing %q: %s", want, stderr.String()) + } + } +} + func TestRunProvidersUseRejectsUsageErrors(t *testing.T) { cases := []struct { name string From fef6c5d1947eacf08dc1ae2e4f2fd1ef81f4a373 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 22:28:18 +0200 Subject: [PATCH 02/11] fix: address provider selection review --- internal/cli/command_center.go | 2 +- internal/cli/command_center_test.go | 5 +++-- internal/cli/provider_onboarding_test.go | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index 040499a5e..4ab8de512 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -419,7 +419,7 @@ func formatProviderCLILine(provider providerCLISummary) string { } line := fmt.Sprintf("%s %s [%s] model=%s apiModel=%s api key: %s", marker, displayCLIValue(provider.Name, "none"), displayCLIValue(provider.ProviderKind, "unknown"), displayCLIValue(provider.Model, "none"), displayCLIValue(provider.APIModel, "unknown"), providerCredentialState(provider.providerSummary)) if !provider.Selectable { - line += " (runtime-only; not selectable/saved)" + line += " (not selectable via providers use)" } if provider.Message != "" { line += " (" + provider.Status + ": " + provider.Message + ")" diff --git a/internal/cli/command_center_test.go b/internal/cli/command_center_test.go index e40f72fef..79f524bd3 100644 --- a/internal/cli/command_center_test.go +++ b/internal/cli/command_center_test.go @@ -220,11 +220,12 @@ func TestRunProvidersListMarksUserAndRuntimeProfiles(t *testing.T) { } stdout.Reset() + stderr.Reset() if code := runWithDeps([]string{"providers", "list"}, &stdout, &stderr, deps); code != exitSuccess { t.Fatalf("code=%d stderr=%s", code, stderr.String()) } - if !strings.Contains(stdout.String(), "runtime-only; not selectable/saved") { - t.Fatalf("runtime marker missing: %s", stdout.String()) + if !strings.Contains(stdout.String(), "not selectable via providers use") { + t.Fatalf("non-selectable marker missing: %s", stdout.String()) } } diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 9147402d4..69afe57a1 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -12,6 +12,7 @@ import ( ) func TestRunProvidersUseSetsActiveProvider(t *testing.T) { + t.Setenv(config.ActiveProviderEnv, "") var stdout bytes.Buffer var stderr bytes.Buffer configPath := filepath.Join(t.TempDir(), "zero", "config.json") From 9fd27b8dcc2140eec60f2663e1c3dcc7eae98136 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 23:25:40 +0200 Subject: [PATCH 03/11] fix(cli): verify provider overrides actually resolve, not just exist 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 #725. Co-Authored-By: Claude Sonnet 5 --- internal/cli/command_center.go | 10 ++- internal/cli/command_center_test.go | 52 ++++++++++++++ internal/cli/provider_onboarding.go | 30 ++++---- internal/cli/provider_onboarding_test.go | 90 +++++++++++++++++++++++- 4 files changed, 163 insertions(+), 19 deletions(-) diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index eb1628b88..a8f893101 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -151,7 +151,13 @@ func runProviders(args []string, stdout io.Writer, stderr io.Writer, deps appDep } providers := make([]providerCLISummary, 0, len(summary.Providers)) for _, provider := range summary.Providers { - _, selectable := userProviderNames[strings.ToLower(strings.TrimSpace(provider.Name))] + // Resolution merges provider names case-sensitively (a project config + // or provider command can add a "WORK" entry alongside a persisted + // "work"), and `providers use` only ever matches config.json rows by + // their exact stored casing (see config.SetActiveProvider's Resolve() + // path). Folding case here would label a case-variant resolved entry + // selectable even though `providers use` cannot actually select it. + _, selectable := userProviderNames[strings.TrimSpace(provider.Name)] source := providerSourceResolved if selectable { source = providerSourceUserConfig @@ -208,7 +214,7 @@ func loadUserProviderNames(deps appDeps) (map[string]struct{}, error) { return nil, fmt.Errorf("invalid config JSON %s: %w", path, err) } for _, provider := range cfg.Providers { - names[strings.ToLower(strings.TrimSpace(provider.Name))] = struct{}{} + names[strings.TrimSpace(provider.Name)] = struct{}{} } return names, nil } diff --git a/internal/cli/command_center_test.go b/internal/cli/command_center_test.go index b2b7a8270..78de7f7bd 100644 --- a/internal/cli/command_center_test.go +++ b/internal/cli/command_center_test.go @@ -229,6 +229,58 @@ func TestRunProvidersListMarksUserAndRuntimeProfiles(t *testing.T) { } } +// Resolution merges provider names case-sensitively (internal/config/resolver.go +// mergeProvider), so a project config or provider command can add a "WORK" entry +// alongside a persisted "work" as two distinct resolved profiles. `providers use` +// only ever matches config.json rows by their exact stored casing, so it can +// select "work" but has no way to select the case-variant "WORK" entry. Folding +// case when deriving selectable/source would mislabel "WORK" as selectable too. +func TestRunProvidersListDoesNotFoldCaseForSelectability(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{{Name: "work"}}}) + deps := commandCenterDeps(t) + deps.userConfigPath = func() (string, error) { return configPath, nil } + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + profiles := []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + {Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, Model: "other-model"}, + } + return config.ResolvedConfig{ActiveProvider: "work", Provider: profiles[0], Providers: profiles}, nil + } + + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "list", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("code=%d stderr=%s", code, stderr.String()) + } + var payload struct { + Providers []struct { + Name string `json:"name"` + Selectable bool `json:"selectable"` + Source string `json:"source"` + } `json:"providers"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatal(err) + } + if len(payload.Providers) != 2 { + t.Fatalf("unexpected providers: %#v", payload.Providers) + } + for _, provider := range payload.Providers { + switch provider.Name { + case "work": + if !provider.Selectable || provider.Source != providerSourceUserConfig { + t.Fatalf("exact-case persisted entry should be selectable: %#v", provider) + } + case "WORK": + if provider.Selectable || provider.Source != providerSourceResolved { + t.Fatalf("case-variant resolved entry must not be marked selectable: %#v", provider) + } + default: + t.Fatalf("unexpected provider name: %#v", provider) + } + } +} + func TestRunProvidersCatalogListsDescriptors(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 4247ea273..9d1f8dbf4 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -79,7 +79,7 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) // An override only becomes the effective provider if Zero can actually // resolve it; a stale value names nothing and fails the next resolution. - overrideResolves := override != "" && activeProviderEnvOverrideResolves(deps, configPath, override) + overrideResolves := override != "" && activeProviderEnvOverrideResolves(deps, override) if options.json { payload := map[string]any{ "activeProvider": cfg.ActiveProvider, @@ -135,25 +135,23 @@ func activeProviderEnvOverride(getenv func(string) string, selected string) stri } // activeProviderEnvOverrideResolves reports whether the ZERO_PROVIDER value -// names a provider Zero can resolve — a profile saved in the config file this -// command just wrote, or one Resolve() synthesizes from an ambient env var. A -// stale value (a renamed or removed profile) resolves to nothing, so calling it -// the effective provider would point the user at a provider that does not exist -// while the next resolution fails first. -// -// The saved-profile check comes first and reads the same configPath the command -// wrote, so it does not depend on the ambient environment; the resolver pass only -// adds the env-derived profiles. Neither failure is surfaced: `providers use` -// already succeeded, and this only decides which note to print. -func activeProviderEnvOverrideResolves(deps appDeps, configPath string, override string) bool { - if persisted, err := config.ProviderPersisted(configPath, override); err == nil && persisted { - return true - } +// is the provider a subsequent invocation will actually use. A row in +// config.json (or one Resolve() synthesizes from an ambient env var) is not +// enough proof: that profile can still fail normalization — e.g. an +// OpenAI-compatible entry saved without a model — which fails resolution +// before the caller ever gets to use it, or another config layer can leave a +// same-named profile in the list while a different one ends up active. Only +// running the resolver and checking which provider it actually picked proves +// the override works, so this runs the same resolution a following command +// would and compares its ActiveProvider against override. Failure is not +// surfaced: `providers use` already succeeded, and this only decides which +// note to print. +func activeProviderEnvOverrideResolves(deps appDeps, override string) bool { resolved, exitCode := resolveCommandCenterConfig(io.Discard, deps) if exitCode != exitSuccess { return false } - return providerResolvedByName(resolved.Providers, override) + return strings.EqualFold(strings.TrimSpace(resolved.ActiveProvider), override) } func runProvidersSetup(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 4491ee0b4..9ad85ccf9 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "strings" "testing" @@ -103,6 +104,32 @@ func providersUseOverrideConfig(t *testing.T) string { return configPath } +// providersUseOverrideConfigAtDefaultUserPath is providersUseOverrideConfig, +// but written to the exact path config.DefaultUserConfigPath() resolves to +// (via a redirected APPDATA/XDG_CONFIG_HOME). activeProviderEnvOverrideResolves +// proves an override by running the resolver, which loads +// config.DefaultUserConfigPath() directly rather than deps.userConfigPath, so a +// test asserting a real override actually resolves needs the two to agree — +// otherwise the resolver silently reads a different (likely nonexistent) file. +func providersUseOverrideConfigAtDefaultUserPath(t *testing.T) string { + t.Helper() + root := t.TempDir() + if runtime.GOOS == "windows" { + t.Setenv("APPDATA", root) + } else { + t.Setenv("XDG_CONFIG_HOME", root) + } + configPath := filepath.Join(root, "zero", "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "work", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAI, BaseURL: config.OpenAIBaseURL, Model: "gpt-4.1"}, + {Name: "fast", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://api.groq.com/openai/v1", Model: "llama-3.3-70b-versatile"}, + }, + }) + return configPath +} + // The write to config.json still succeeds, but when ZERO_PROVIDER names a // different provider the saved selection is NOT effective, so the command must // warn instead of reporting a silent success (issue #721). @@ -136,7 +163,14 @@ func TestRunProvidersUseWarnsWhenEnvOverrides(t *testing.T) { func TestRunProvidersUseJSONFlagsEnvOverride(t *testing.T) { var stdout, stderr bytes.Buffer - deps := providerSetupDeps(providersUseOverrideConfig(t)) + // activeProviderEnvOverrideResolves runs the resolver to prove the override + // is genuinely effective, and the resolver reads the real process + // environment (config.Resolve falls back to os.Getenv when no Env map is + // injected) — so the override must be set for real, not just mocked via + // deps.getenv, which only feeds the separate "is this an override at all" + // check. + t.Setenv(config.ActiveProviderEnv, "work") + deps := providerSetupDeps(providersUseOverrideConfigAtDefaultUserPath(t)) deps.getenv = func(key string) string { if key == config.ActiveProviderEnv { return "work" @@ -211,6 +245,60 @@ func TestRunProvidersUseFlagsUnresolvableEnvOverride(t *testing.T) { } } +// A ZERO_PROVIDER override that names a persisted profile is still not proof +// the next resolution succeeds: an OpenAI-compatible profile saved without a +// model fails normalization (config.Resolve requires one), so the override +// must not be reported as effective just because a config.json row exists. +func TestRunProvidersUseFlagsBrokenPersistedEnvOverride(t *testing.T) { + root := t.TempDir() + if runtime.GOOS == "windows" { + t.Setenv("APPDATA", root) + } else { + t.Setenv("XDG_CONFIG_HOME", root) + } + configPath := filepath.Join(root, "zero", "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "work", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAI, BaseURL: config.OpenAIBaseURL, Model: "gpt-4.1"}, + {Name: "fast", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://api.groq.com/openai/v1", Model: "llama-3.3-70b-versatile"}, + {Name: "broken", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://api.example.com/v1"}, + }, + }) + t.Setenv(config.ActiveProviderEnv, "broken") + deps := providerSetupDeps(configPath) + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "broken" + } + 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("JSON did not decode: %v\n%s", err, stdout.String()) + } + if _, reported := payload["effectiveProvider"]; reported { + t.Fatalf("a persisted-but-unresolvable override must not be reported as effective: %#v", payload) + } + if resolves, ok := payload["envProviderResolves"].(bool); !ok || resolves { + t.Fatalf("envProviderResolves = %#v, want false", payload["envProviderResolves"]) + } + + stdout.Reset() + stderr.Reset() + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + if strings.Contains(stderr.String(), "stays the active provider") { + t.Fatalf("a persisted-but-unresolvable override must not be called the active provider: %q", stderr.String()) + } +} + // No override note when ZERO_PROVIDER is unset or already names the selection. func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { cases := map[string]func(string) string{ From 0618d546b966471ed1f87de6715d21ff81b4fa76 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 16:48:45 +0200 Subject: [PATCH 04/11] fix(cli): avoid provider command during selection --- internal/cli/command_center_test.go | 6 +- internal/cli/provider_onboarding.go | 80 ++++++++++++----- internal/cli/provider_onboarding_test.go | 108 +++++++++++++++++++++-- internal/config/paths.go | 4 +- internal/config/writer.go | 4 +- internal/config/writer_test.go | 37 +++++++- 6 files changed, 202 insertions(+), 37 deletions(-) diff --git a/internal/cli/command_center_test.go b/internal/cli/command_center_test.go index 78de7f7bd..e07318178 100644 --- a/internal/cli/command_center_test.go +++ b/internal/cli/command_center_test.go @@ -215,7 +215,7 @@ func TestRunProvidersListMarksUserAndRuntimeProfiles(t *testing.T) { if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { t.Fatal(err) } - if len(payload.Providers) != 2 || payload.Providers[0].Name != "runtime" || payload.Providers[0].Selectable || payload.Providers[0].Source != providerSourceResolved || !payload.Providers[1].Selectable || payload.Providers[1].Source != providerSourceUserConfig { + if len(payload.Providers) != 2 || payload.Providers[0].Name != "runtime" || payload.Providers[0].Selectable || payload.Providers[0].Source != "resolved" || !payload.Providers[1].Selectable || payload.Providers[1].Source != "user-config" { t.Fatalf("unexpected providers: %#v", payload.Providers) } @@ -268,11 +268,11 @@ func TestRunProvidersListDoesNotFoldCaseForSelectability(t *testing.T) { for _, provider := range payload.Providers { switch provider.Name { case "work": - if !provider.Selectable || provider.Source != providerSourceUserConfig { + if !provider.Selectable || provider.Source != "user-config" { t.Fatalf("exact-case persisted entry should be selectable: %#v", provider) } case "WORK": - if provider.Selectable || provider.Source != providerSourceResolved { + if provider.Selectable || provider.Source != "resolved" { t.Fatalf("case-variant resolved entry must not be marked selectable: %#v", provider) } default: diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 9d1f8dbf4..8f3027a4d 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -79,7 +79,10 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) // An override only becomes the effective provider if Zero can actually // resolve it; a stale value names nothing and fails the next resolution. - overrideResolves := override != "" && activeProviderEnvOverrideResolves(deps, override) + overrideResolution := activeProviderOverrideAbsent + if override != "" { + overrideResolution = activeProviderEnvOverrideResolution(deps, configPath, override) + } if options.json { payload := map[string]any{ "activeProvider": cfg.ActiveProvider, @@ -89,8 +92,11 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app // A JSON consumer must not read this as an effective switch either. payload["overriddenByEnv"] = config.ActiveProviderEnv payload["envProvider"] = override - payload["envProviderResolves"] = overrideResolves - if overrideResolves { + payload["envProviderResolves"] = overrideResolution.resolves() + if overrideResolution == activeProviderOverrideDeferred { + payload["envProviderResolution"] = "deferred" + } + if overrideResolution == activeProviderOverrideResolved { payload["effectiveProvider"] = override } } @@ -104,7 +110,9 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app } if override != "" { note := fmt.Sprintf("Note: %s=%s is set and overrides config.json, so %s stays the active provider until you unset %s.\n", config.ActiveProviderEnv, override, override, config.ActiveProviderEnv) - if !overrideResolves { + if overrideResolution == activeProviderOverrideDeferred { + note = fmt.Sprintf("Note: %s=%s is set and overrides config.json; %s is also set, so the effective provider will be determined when Zero next resolves configuration.\n", config.ActiveProviderEnv, override, config.ProviderCommandEnv) + } else if overrideResolution != activeProviderOverrideResolved { // Naming it "effective" would be wrong: nothing resolves under that // name, so the next command fails rather than using it. note = fmt.Sprintf("Note: %s=%s is set and overrides config.json, but no provider named %s can be resolved, so Zero cannot start until you unset %s or point it at a saved provider.\n", config.ActiveProviderEnv, override, override, config.ActiveProviderEnv) @@ -116,6 +124,26 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app return exitSuccess } +type activeProviderOverrideResolution uint8 + +const ( + activeProviderOverrideAbsent activeProviderOverrideResolution = iota + activeProviderOverrideResolved + activeProviderOverrideUnresolved + activeProviderOverrideDeferred +) + +func (resolution activeProviderOverrideResolution) resolves() any { + switch resolution { + case activeProviderOverrideResolved: + return true + case activeProviderOverrideUnresolved: + return false + default: + return nil + } +} + // activeProviderEnvOverride returns the ZERO_PROVIDER value when it is set and // names a DIFFERENT provider than the one just selected, meaning the saved // `providers use` selection will NOT be the effective active provider until the @@ -128,30 +156,36 @@ func activeProviderEnvOverride(getenv func(string) string, selected string) stri return "" } override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) - if override == "" || strings.EqualFold(override, strings.TrimSpace(selected)) { + if override == "" || override == strings.TrimSpace(selected) { return "" } return override } -// activeProviderEnvOverrideResolves reports whether the ZERO_PROVIDER value -// is the provider a subsequent invocation will actually use. A row in -// config.json (or one Resolve() synthesizes from an ambient env var) is not -// enough proof: that profile can still fail normalization — e.g. an -// OpenAI-compatible entry saved without a model — which fails resolution -// before the caller ever gets to use it, or another config layer can leave a -// same-named profile in the list while a different one ends up active. Only -// running the resolver and checking which provider it actually picked proves -// the override works, so this runs the same resolution a following command -// would and compares its ActiveProvider against override. Failure is not -// surfaced: `providers use` already succeeded, and this only decides which -// note to print. -func activeProviderEnvOverrideResolves(deps appDeps, override string) bool { - resolved, exitCode := resolveCommandCenterConfig(io.Discard, deps) - if exitCode != exitSuccess { - return false +// activeProviderEnvOverrideResolution checks whether ZERO_PROVIDER resolves +// without running ZERO_PROVIDER_COMMAND. Provider commands are arbitrary +// external programs and `providers use` must remain a config-only operation; +// when one is configured, the final provider is therefore explicitly deferred +// until the next normal resolution instead of being guessed here. +func activeProviderEnvOverrideResolution(deps appDeps, configPath string, override string) activeProviderOverrideResolution { + if deps.getenv != nil && strings.TrimSpace(deps.getenv(config.ProviderCommandEnv)) != "" { + return activeProviderOverrideDeferred + } + workspaceRoot, err := resolveWorkspaceRoot("", deps) + if err != nil { + return activeProviderOverrideUnresolved + } + options, err := config.DefaultResolveOptions(workspaceRoot) + if err != nil { + return activeProviderOverrideUnresolved + } + options.UserConfigPath = configPath + options.ProviderCommand = "" + resolved, err := config.Resolve(options) + if err != nil || strings.TrimSpace(resolved.ActiveProvider) != override { + return activeProviderOverrideUnresolved } - return strings.EqualFold(strings.TrimSpace(resolved.ActiveProvider), override) + return activeProviderOverrideResolved } func runProvidersSetup(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { @@ -561,7 +595,7 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps func providerResolvedByName(providers []config.ProviderProfile, name string) bool { name = strings.TrimSpace(name) for _, provider := range providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if strings.TrimSpace(provider.Name) == name { return true } } diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 9ad85ccf9..ef475f921 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "strings" "testing" @@ -91,6 +92,32 @@ func TestRunProvidersUseExplainsRuntimeOnlyProfilesAreNotSelectable(t *testing.T } } +func TestRunProvidersUseRejectsCaseVariantOfPersistedProvider(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "saved", + Providers: []config.ProviderProfile{ + {Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }) + + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(configPath) + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + profile := config.ProviderProfile{Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"} + return config.ResolvedConfig{ActiveProvider: "saved", Provider: profile, Providers: []config.ProviderProfile{profile}}, nil + } + if code := runWithDeps([]string{"providers", "use", "SAVED"}, &stdout, &stderr, deps); code != exitCrash { + t.Fatalf("exit = %d, want %d", code, exitCrash) + } + if !strings.Contains(stderr.String(), "only providers saved in user config are selectable") { + t.Fatalf("case-variant error did not explain selectability: %q", stderr.String()) + } + if cfg := readFileConfig(t, configPath); cfg.ActiveProvider != "saved" { + t.Fatalf("ActiveProvider = %q, want saved", cfg.ActiveProvider) + } +} + func providersUseOverrideConfig(t *testing.T) string { t.Helper() configPath := filepath.Join(t.TempDir(), "config.json") @@ -105,12 +132,8 @@ func providersUseOverrideConfig(t *testing.T) string { } // providersUseOverrideConfigAtDefaultUserPath is providersUseOverrideConfig, -// but written to the exact path config.DefaultUserConfigPath() resolves to -// (via a redirected APPDATA/XDG_CONFIG_HOME). activeProviderEnvOverrideResolves -// proves an override by running the resolver, which loads -// config.DefaultUserConfigPath() directly rather than deps.userConfigPath, so a -// test asserting a real override actually resolves needs the two to agree — -// otherwise the resolver silently reads a different (likely nonexistent) file. +// but written to the exact path config.DefaultUserConfigPath() resolves to via +// a redirected APPDATA/XDG_CONFIG_HOME. func providersUseOverrideConfigAtDefaultUserPath(t *testing.T) string { t.Helper() root := t.TempDir() @@ -163,7 +186,7 @@ func TestRunProvidersUseWarnsWhenEnvOverrides(t *testing.T) { func TestRunProvidersUseJSONFlagsEnvOverride(t *testing.T) { var stdout, stderr bytes.Buffer - // activeProviderEnvOverrideResolves runs the resolver to prove the override + // activeProviderEnvOverrideResolution runs the resolver to prove the override // is genuinely effective, and the resolver reads the real process // environment (config.Resolve falls back to os.Getenv when no Env map is // injected) — so the override must be set for real, not just mocked via @@ -299,6 +322,65 @@ func TestRunProvidersUseFlagsBrokenPersistedEnvOverride(t *testing.T) { } } +func TestRunProvidersUseDefersOverrideResolutionWhenProviderCommandIsSet(t *testing.T) { + configPath := providersUseOverrideConfig(t) + marker := filepath.Join(t.TempDir(), "provider-command-ran") + t.Setenv(config.ActiveProviderEnv, "work") + t.Setenv("ZERO_TEST_PROVIDER_COMMAND_MARKER", marker) + providerCommand := strconv.Quote(os.Args[0]) + " -test.run=^TestProviderCommandSentinel$" + t.Setenv(config.ProviderCommandEnv, providerCommand) + deps := providerSetupDeps(configPath) + deps.getenv = func(key string) string { + switch key { + case config.ActiveProviderEnv: + return "work" + case config.ProviderCommandEnv: + return providerCommand + 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("JSON did not decode: %v\n%s", err, stdout.String()) + } + if payload["envProviderResolution"] != "deferred" || payload["envProviderResolves"] != nil { + t.Fatalf("provider-command override resolution must be deferred: %#v", payload) + } + if _, reported := payload["effectiveProvider"]; reported { + t.Fatalf("deferred override must not be reported as effective: %#v", payload) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatalf("provider command ran during override reporting: stat error = %v", err) + } + + stdout.Reset() + stderr.Reset() + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + for _, want := range []string{config.ProviderCommandEnv, "determined", "next resolves configuration"} { + if !strings.Contains(stderr.String(), want) { + t.Fatalf("deferred override note missing %q: %q", want, stderr.String()) + } + } +} + +func TestProviderCommandSentinel(t *testing.T) { + marker := os.Getenv("ZERO_TEST_PROVIDER_COMMAND_MARKER") + if marker == "" { + return + } + if err := os.WriteFile(marker, []byte("ran"), 0o600); err != nil { + t.Fatalf("write provider-command marker: %v", err) + } +} + // No override note when ZERO_PROVIDER is unset or already names the selection. func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { cases := map[string]func(string) string{ @@ -325,6 +407,18 @@ func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { } } +func TestActiveProviderEnvOverrideTreatsCaseVariantAsDistinct(t *testing.T) { + getenv := func(key string) string { + if key == config.ActiveProviderEnv { + return "WORK" + } + return "" + } + if override := activeProviderEnvOverride(getenv, "work"); override != "WORK" { + t.Fatalf("activeProviderEnvOverride() = %q, want WORK", override) + } +} + func TestRunProvidersUseSurfacesMalformedConfig(t *testing.T) { var stdout, stderr bytes.Buffer configPath := filepath.Join(t.TempDir(), "config.json") diff --git a/internal/config/paths.go b/internal/config/paths.go index f8c9b34e6..c79ebeb39 100644 --- a/internal/config/paths.go +++ b/internal/config/paths.go @@ -8,6 +8,8 @@ import ( "strings" ) +const ProviderCommandEnv = "ZERO_PROVIDER_COMMAND" + // DefaultResolveOptions builds config resolution inputs from the local process // environment and workspace. func DefaultResolveOptions(workspaceRoot string) (ResolveOptions, error) { @@ -29,7 +31,7 @@ func DefaultResolveOptions(workspaceRoot string) (ResolveOptions, error) { return ResolveOptions{ UserConfigPath: userConfigPath, ProjectConfigPath: projectConfigPath, - ProviderCommand: strings.TrimSpace(os.Getenv("ZERO_PROVIDER_COMMAND")), + ProviderCommand: strings.TrimSpace(os.Getenv(ProviderCommandEnv)), }, nil } diff --git a/internal/config/writer.go b/internal/config/writer.go index 861eb3146..c422fe3d0 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -165,7 +165,7 @@ func SetActiveProvider(path string, name string) (FileConfig, error) { } for _, provider := range cfg.Providers { - if strings.EqualFold(provider.Name, name) { + if strings.TrimSpace(provider.Name) == name { cfg.ActiveProvider = provider.Name if err := writeConfigFile(path, cfg); err != nil { return FileConfig{}, err @@ -197,7 +197,7 @@ func ProviderPersisted(path string, name string) (bool, error) { return false, err } for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if strings.TrimSpace(provider.Name) == name { return true, nil } } diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index c66fc26ba..cf0e5b448 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -31,7 +31,7 @@ func TestSetActiveProviderSwitchesConfiguredProvider(t *testing.T) { }, }, 0o600) - cfg, err := SetActiveProvider(path, " anthropic ") + cfg, err := SetActiveProvider(path, " Anthropic ") if err != nil { t.Fatalf("SetActiveProvider() error = %v", err) } @@ -46,6 +46,41 @@ func TestSetActiveProviderSwitchesConfiguredProvider(t *testing.T) { } } +func TestSetActiveProviderRequiresExactProviderIdentity(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{ + {Name: "work", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }, 0o600) + + _, err := SetActiveProvider(path, "WORK") + if err == nil || !strings.Contains(err.Error(), `provider "WORK" not found`) { + t.Fatalf("SetActiveProvider() error = %v, want exact-case not-found error", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatalf("read config: %v", readErr) + } + if string(after) != string(before) { + t.Fatalf("config was rewritten for case-variant provider\nbefore: %s\nafter: %s", before, after) + } +} + +func TestProviderPersistedRequiresExactProviderIdentity(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}}}, 0o600) + + persisted, err := ProviderPersisted(path, "WORK") + if err != nil { + t.Fatalf("ProviderPersisted() error = %v", err) + } + if persisted { + t.Fatal("ProviderPersisted() = true for case-variant identity, want false") + } +} + func TestSetActiveProviderRejectsUnknownProviderWithoutRewriting(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") before := writeConfigFixture(t, path, FileConfig{ From ccbf9ab8a29b763a7ab11a8ce788c759b4a30237 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 22:15:39 +0200 Subject: [PATCH 05/11] fix(config): match provider identity exactly in every mutator UpsertProvider already merges by exact name, so config.json can hold two rows differing only by case (e.g. "work" saved once, "WORK" saved later). SetActiveProvider/ProviderPersisted were already switched to exact-name matching, but RemoveProvider, RenameProvider, and SetProviderModel still picked the first case-insensitive match: with rows ordered [work, WORK], `providers remove WORK` deleted "work" instead, and the ActiveProvider hand-off/follow logic in Remove/RenameProvider had the same case-folding bug when checking whether the mutated row was the active one. Switch all three to the same exact-identity rule (RenameProvider's newName collision check stays case-insensitive, since the credential store normalizes names and a case-variant rename would silently share/corrupt another row's stored key). Added regression tests for all three functions covering the case-distinct-duplicate scenario, and fixed two existing tests that relied on the old case-folding convenience. Addresses review feedback from jatmn on PR #725. Co-Authored-By: Claude Sonnet 5 --- internal/config/writer.go | 25 ++++++++-- internal/config/writer_test.go | 85 +++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 7 deletions(-) diff --git a/internal/config/writer.go b/internal/config/writer.go index c422fe3d0..f6ab80ee7 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -229,9 +229,13 @@ func RemoveProvider(path string, name string) (FileConfig, error) { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + // Exact match, like ProviderPersisted/SetActiveProvider: two rows can + // coexist that differ only by case (UpsertProvider merges by exact name), + // so folding case here would delete whichever row happens to sort first + // instead of the one the caller actually asked for. index := -1 for i, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if strings.TrimSpace(provider.Name) == name { index = i break } @@ -241,7 +245,7 @@ func RemoveProvider(path string, name string) (FileConfig, error) { } removed := cfg.Providers[index] cfg.Providers = append(cfg.Providers[:index], cfg.Providers[index+1:]...) - if strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(removed.Name)) { + if strings.TrimSpace(cfg.ActiveProvider) == strings.TrimSpace(removed.Name) { cfg.ActiveProvider = "" if len(cfg.Providers) > 0 { cfg.ActiveProvider = cfg.Providers[0].Name @@ -281,10 +285,17 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + // oldName is matched exactly, like ProviderPersisted/SetActiveProvider: + // two rows can coexist that differ only by case (UpsertProvider merges by + // exact name), so folding case here would rename whichever row happens to + // sort first instead of the one the caller actually asked for. newName + // still collides case-insensitively: the credential store normalizes + // names, so renaming into an existing row's case variant would silently + // share (and corrupt) that row's stored key. index := -1 for i, provider := range cfg.Providers { providerName := strings.TrimSpace(provider.Name) - if strings.EqualFold(providerName, oldName) { + if providerName == oldName { index = i continue } @@ -307,7 +318,7 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er } keyMigrated = true } - if strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { + if strings.TrimSpace(cfg.ActiveProvider) == strings.TrimSpace(previousName) { cfg.ActiveProvider = newName } cfg.Providers[index].Name = newName @@ -485,8 +496,12 @@ func SetProviderModel(path string, name string, model string) (FileConfig, error return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + // Exact match, like ProviderPersisted/SetActiveProvider: two rows can + // coexist that differ only by case (UpsertProvider merges by exact name), + // so folding case here would update whichever row happens to sort first + // instead of the one the caller actually asked for. for index := range cfg.Providers { - if strings.EqualFold(cfg.Providers[index].Name, name) { + if strings.TrimSpace(cfg.Providers[index].Name) == name { cfg.Providers[index].Model = model if err := writeConfigFile(path, cfg); err != nil { return FileConfig{}, err diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index cf0e5b448..ec59ea984 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -205,7 +205,7 @@ func TestSetProviderModelUpdatesConfiguredProvider(t *testing.T) { }, }, 0o600) - cfg, err := SetProviderModel(path, " OpenAI ", " gpt-4.1-mini ") + cfg, err := SetProviderModel(path, " openai ", " gpt-4.1-mini ") if err != nil { t.Fatalf("SetProviderModel() error = %v", err) } @@ -252,6 +252,30 @@ func TestSetProviderModelRejectsUnknownProviderWithoutRewriting(t *testing.T) { } } +// Same scenario as RemoveProvider/RenameProvider: two rows differing only by +// case must not let SetProviderModel update the wrong one. +func TestSetProviderModelRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{ + {Name: "work", ProviderKind: ProviderKindOpenAICompatible, Model: "m1"}, + {Name: "WORK", ProviderKind: ProviderKindOpenAICompatible, Model: "m2"}, + }, + }, 0o600) + + cfg, err := SetProviderModel(path, "WORK", "m2-updated") + if err != nil { + t.Fatalf("SetProviderModel() error = %v", err) + } + if cfg.Providers[0].Model != "m1" { + t.Fatalf("unrelated 'work' row changed: %+v", cfg.Providers[0]) + } + if cfg.Providers[1].Model != "m2-updated" { + t.Fatalf("targeted 'WORK' row not updated: %+v", cfg.Providers[1]) + } +} + func TestUpsertProviderTightensExistingConfigFilePermissions(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows does not expose POSIX mode bits reliably") @@ -629,7 +653,7 @@ func TestRemoveProviderDeletesAndHandsOffActive(t *testing.T) { }, }, 0o600) - cfg, err := RemoveProvider(path, " BETA ") + cfg, err := RemoveProvider(path, " beta ") if err != nil { t.Fatalf("RemoveProvider() error = %v", err) } @@ -674,6 +698,34 @@ func TestRemoveProviderKeepsActiveWhenOtherRemoved(t *testing.T) { } } +// UpsertProvider merges by exact name, so a config file can end up with two +// rows that differ only by case (e.g. one saved as "work", another later +// saved as "WORK"). RemoveProvider must delete the exact row the caller +// named, not whichever case-variant sorts first. +func TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{ + {Name: "work", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://a.example.com/v1", Model: "m1"}, + {Name: "WORK", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://b.example.com/v1", Model: "m2"}, + }, + }, 0o600) + + cfg, err := RemoveProvider(path, "WORK") + if err != nil { + t.Fatalf("RemoveProvider() error = %v", err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" { + t.Fatalf("expected only the untouched 'work' row to remain, got %+v", cfg.Providers) + } + // "work" is still active and untouched, so the active pointer must not + // hand off to another provider. + if cfg.ActiveProvider != "work" { + t.Fatalf("active provider changed to %q, want unchanged 'work'", cfg.ActiveProvider) + } +} + func TestRemoveProviderRejectsUnknownWithoutRewriting(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") before := writeConfigFixture(t, path, FileConfig{ @@ -762,6 +814,35 @@ func TestRenameProviderRejectsCollisionAndUnknown(t *testing.T) { } } +// Same scenario as RemoveProvider: two rows differing only by case must not +// let RenameProvider act on the wrong one. +func TestRenameProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{ + {Name: "work", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://a.example.com/v1", Model: "m1"}, + {Name: "WORK", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://b.example.com/v1", Model: "m2"}, + }, + }, 0o600) + + cfg, err := RenameProvider(path, "WORK", "renamed") + if err != nil { + t.Fatalf("RenameProvider() error = %v", err) + } + names := map[string]bool{} + for _, provider := range cfg.Providers { + names[provider.Name] = true + } + if !names["work"] || !names["renamed"] || names["WORK"] { + t.Fatalf("expected 'work' untouched and 'WORK' renamed to 'renamed', got %+v", cfg.Providers) + } + // The active provider is the untouched "work" row, not the renamed one. + if cfg.ActiveProvider != "work" { + t.Fatalf("active provider changed to %q, want unchanged 'work'", cfg.ActiveProvider) + } +} + func TestUpsertProviderPreservesStoredKeyMarkerOnExistingProfile(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") // An env-keyed profile with NO stored-key marker — the shape a provider has From dd353bcbb6836b95eda31c8a82cf7b542d03f81d Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 20:42:00 +0000 Subject: [PATCH 06/11] fix(config): match edited provider identity exactly Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-d742-77fa-b3c2-addbca9602cd Co-authored-by: Pierre Bruno --- internal/config/writer.go | 9 ++++----- internal/config/writer_test.go | 31 ++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/internal/config/writer.go b/internal/config/writer.go index f6ab80ee7..fb3f2e700 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -336,7 +336,7 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er // ProviderEdit is a field-level edit of one saved provider, applied by // EditProvider in a single atomic write. Name is the CURRENT profile name -// (matched case-insensitively); NewName renames (case-only renames included). +// (matched exactly); NewName renames (case-only renames included). // Empty BaseURL/Model/APIKey mean "leave unchanged"; Description is applied // VERBATIM (the editor always knows the full desired text, so clearing works). type ProviderEdit struct { @@ -355,8 +355,7 @@ type ProviderEdit struct { // verbatim description. A single write keeps the operation atomic — the // previous rename+upsert+describe sequence could fail halfway and leave // config.json renamed while every in-memory consumer still held the old name — -// and, unlike UpsertProvider's exact-name merge, the case-insensitive match -// here makes a case-only rename (groq -> Groq) an in-place update instead of +// and a case-only rename (groq -> Groq) remains an in-place update instead of // an appended duplicate profile. func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { path = strings.TrimSpace(path) @@ -384,7 +383,7 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { index := -1 for i, provider := range cfg.Providers { providerName := strings.TrimSpace(provider.Name) - if strings.EqualFold(providerName, oldName) { + if providerName == oldName { index = i continue } @@ -411,7 +410,7 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { } keyMigrated = true } - if renamed && strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { + if renamed && strings.TrimSpace(cfg.ActiveProvider) == strings.TrimSpace(previousName) { cfg.ActiveProvider = newName } diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index ec59ea984..4fe85f806 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -1031,11 +1031,36 @@ func TestEditProviderAppliesRenameFieldsAndDescriptionAtomically(t *testing.T) { } } +func TestEditProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{ + {Name: "WORK", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://upper.example.com/v1", Model: "upper"}, + {Name: "work", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://lower.example.com/v1", Model: "lower"}, + }, + }, 0o600) + + cfg, err := EditProvider(path, ProviderEdit{Name: "WORK", NewName: "renamed", Model: "updated"}) + if err != nil { + t.Fatalf("EditProvider() error = %v", err) + } + if cfg.Providers[0].Name != "renamed" || cfg.Providers[0].Model != "updated" { + t.Fatalf("exact-case target was not edited: %+v", cfg.Providers[0]) + } + if cfg.Providers[1].Name != "work" || cfg.Providers[1].Model != "lower" { + t.Fatalf("case-variant provider changed: %+v", cfg.Providers[1]) + } + if cfg.ActiveProvider != "work" { + t.Fatalf("case-variant active provider changed to %q", cfg.ActiveProvider) + } +} + // TestEditProviderCaseOnlyRenameUpdatesInPlace: the manager previously skipped // RenameProvider on case-insensitively-equal names and fell into UpsertProvider, -// whose case-SENSITIVE merge appended a duplicate profile. EditProvider matches -// case-insensitively, so a case-only rename is an in-place update and the store -// entry (case-normalized) survives. +// whose case-SENSITIVE merge appended a duplicate profile. EditProvider applies +// NewName to the exact current profile, so a case-only rename is an in-place +// update and the store entry (case-normalized) survives. func TestEditProviderCaseOnlyRenameUpdatesInPlace(t *testing.T) { dir := t.TempDir() t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") From d41bd163a6f45dfc546a5c287973be30932433c1 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 28 Jul 2026 10:40:38 +0000 Subject: [PATCH 07/11] fix(providers): enforce credential identity Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno --- internal/cli/app.go | 6 + internal/cli/auth.go | 66 ++++++++-- internal/cli/auth_test.go | 79 ++++++++++++ internal/cli/provider_onboarding.go | 18 ++- internal/cli/provider_onboarding_test.go | 10 +- internal/cli/provider_setup.go | 3 + internal/cli/setup.go | 3 + internal/config/credentials.go | 2 +- internal/config/credentials_test.go | 10 ++ internal/config/resolver.go | 35 +++++- internal/config/resolver_test.go | 89 ++++++++++++++ internal/config/writer.go | 141 +++++++++++++++++++--- internal/config/writer_test.go | 92 +++++++------- internal/oauth/manager.go | 16 +++ internal/tui/oauth_device.go | 7 +- internal/tui/onboarding.go | 22 ++-- internal/tui/provider_manager.go | 10 ++ internal/tui/provider_wizard.go | 60 +++++++-- internal/tui/provider_wizard_discovery.go | 2 +- 19 files changed, 558 insertions(+), 113 deletions(-) diff --git a/internal/cli/app.go b/internal/cli/app.go index 80854beb4..f17293965 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -22,6 +22,7 @@ import ( "github.com/Gitlawb/zero/internal/localcontrol" "github.com/Gitlawb/zero/internal/mcp" "github.com/Gitlawb/zero/internal/modelregistry" + "github.com/Gitlawb/zero/internal/oauth" "github.com/Gitlawb/zero/internal/observability" "github.com/Gitlawb/zero/internal/plugins" "github.com/Gitlawb/zero/internal/providerhealth" @@ -68,6 +69,7 @@ type appDeps struct { discoverProviderModels func(context.Context, config.ProviderProfile) ([]providermodeldiscovery.Model, error) detectLocalRuntimes func(context.Context, provideronboarding.LocalDetectOptions) []provideronboarding.DetectedLocalRuntime openRouterLogin func(context.Context, provideroauth.OpenRouterOptions) (string, error) + chatGPTLogin func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) newSessionStore func() *sessions.Store loadPlugins func(plugins.LoadOptions) (plugins.LoadResult, error) loadHooks func(hooks.LoadOptions) (hooks.LoadResult, error) @@ -155,6 +157,7 @@ func defaultAppDeps() appDeps { discoverProviderModels: defaultDiscoverProviderModels, detectLocalRuntimes: provideronboarding.DetectLocalRuntimes, openRouterLogin: provideroauth.OpenRouterLogin, + chatGPTLogin: provideroauth.ChatGPTLogin, newSessionStore: func() *sessions.Store { return sessions.NewStore(sessions.StoreOptions{}) }, @@ -493,6 +496,9 @@ func fillAppDeps(deps appDeps) appDeps { if deps.openRouterLogin == nil { deps.openRouterLogin = defaults.openRouterLogin } + if deps.chatGPTLogin == nil { + deps.chatGPTLogin = defaults.chatGPTLogin + } if deps.newSessionStore == nil { deps.newSessionStore = defaults.newSessionStore } diff --git a/internal/cli/auth.go b/internal/cli/auth.go index f3ecdcc42..05818c6ca 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -131,6 +131,9 @@ func saveOpenRouterProviderKey(deps appDeps, key string) (string, error) { if err != nil { return "", err } + if err := config.PreflightUserConfig(configPath); err != nil { + return "", err + } ensured, err := config.EnsureCatalogProvider(configPath, "openrouter") if err != nil { return "", err @@ -176,6 +179,14 @@ func runAuthChatGPT(args []string, stdout io.Writer, stderr io.Writer, deps appD if len(args) > 0 { return writeExecUsageError(stderr, fmt.Sprintf("zero auth chatgpt takes no arguments (got %q)", args[0])) } + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + const provider = "chatgpt" + if err := config.PreflightProviderWrite(configPath, provider); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } // Build the same env map the oauth engine reads so the chatgpt preset is // opted into (the preset is off by default to keep third-party OAuth @@ -189,7 +200,7 @@ func runAuthChatGPT(args []string, stdout io.Writer, stderr io.Writer, deps appD } env["ZERO_OAUTH_ALLOW_PRESETS"] = "1" - token, err := provideroauth.ChatGPTLogin(context.Background(), provideroauth.ChatGPTOptions{ + token, err := deps.chatGPTLogin(context.Background(), provideroauth.ChatGPTOptions{ Env: env, HTTPClient: &http.Client{Timeout: 60 * time.Second}, Out: stdout, @@ -212,7 +223,10 @@ func runAuthChatGPT(args []string, stdout io.Writer, stderr io.Writer, deps appD if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - if err := store.Save(oauth.ProviderKey("chatgpt"), token); err != nil { + if err := config.PreflightProviderWrite(configPath, provider); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if err := store.Save(oauth.ProviderKey(provider), token); err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } statuses, err := oauthFormatChatGPTStatus(token) @@ -343,7 +357,7 @@ func validateAuthFlags(sub string, a authArgs) error { // ZERO_OAUTH_TOKENS_PATH (env), so callers/tests can redirect it. Setting // ZERO_OAUTH_STORAGE=encrypted-file selects the AES-256-GCM encrypted-at-rest // backend (a per-user secret is created beside the token file). -func newAuthManager(deps appDeps, out io.Writer) (*oauth.Manager, error) { +func newAuthManager(deps appDeps, out io.Writer, beforeSave func() error) (*oauth.Manager, error) { // Validate ZERO_OAUTH_STORAGE up front: a mistyped value must fail fast rather // than silently change the backend. Empty = default (plaintext 0600 file); // "encrypted-file" = AES-256-GCM; "keyring" = the OS keyring. @@ -372,6 +386,7 @@ func newAuthManager(deps appDeps, out io.Writer) (*oauth.Manager, error) { // `zero auth login ` (e.g. xai) should resolve the baked-in preset // without the operator exporting ZERO_OAUTH_ALLOW_PRESETS first. AllowPresets: true, + BeforeSave: beforeSave, }) } @@ -387,7 +402,7 @@ func runAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps appDep if len(parsed.positional) != 1 { return writeExecUsageError(stderr, "usage: zero auth login [--device] [--scope ]") } - provider := parsed.positional[0] + provider := strings.ToLower(strings.TrimSpace(parsed.positional[0])) // ChatGPT (Codex) requires a fixed redirect_uri (http://localhost:1455/ // auth/callback) and mandatory authorize params (id_token_add_organizations, // codex_cli_simplified_flow, originator) that the generic loopback flow @@ -402,7 +417,16 @@ func runAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps appDep } return runAuthChatGPT(nil, stdout, stderr, deps) } - manager, err := newAuthManager(deps, stdout) + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if err := config.PreflightProviderWrite(configPath, provider); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + manager, err := newAuthManager(deps, stdout, func() error { + return config.PreflightProviderWrite(configPath, provider) + }) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -438,7 +462,27 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe return writeExecUsageError(stderr, "usage: zero auth logout ") } provider := parsed.positional[0] - manager, err := newAuthManager(deps, stdout) + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if err := config.PreflightUserConfig(configPath); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + exact, err := config.ProviderPersisted(configPath, provider) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if !exact { + folded, foldedErr := config.ProviderPersistedCaseInsensitive(configPath, provider) + if foldedErr != nil { + return writeAppError(stderr, foldedErr.Error(), exitCrash) + } + if folded { + return writeAppError(stderr, fmt.Sprintf("provider %q exists with different capitalization; logout requires the exact persisted provider name", provider), exitCrash) + } + } + manager, err := newAuthManager(deps, stdout, nil) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -453,10 +497,8 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe if keyErr != nil { return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) } - if configPath, perr := deps.userConfigPath(); perr == nil { - if _, clearErr := config.ClearProviderKeyStored(configPath, provider); clearErr != nil { - return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) - } + if _, clearErr := config.ClearProviderKeyStored(configPath, provider); clearErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) } removed = removed || keyRemoved if parsed.json { @@ -491,7 +533,7 @@ func runAuthStatus(args []string, stdout io.Writer, stderr io.Writer, deps appDe if len(parsed.positional) > 1 { return writeExecUsageError(stderr, "usage: zero auth status [provider]") } - manager, err := newAuthManager(deps, stdout) + manager, err := newAuthManager(deps, stdout, nil) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -530,7 +572,7 @@ func runAuthRefresh(args []string, stdout io.Writer, stderr io.Writer, deps appD return writeExecUsageError(stderr, "usage: zero auth refresh ") } provider := parsed.positional[0] - manager, err := newAuthManager(deps, stdout) + manager, err := newAuthManager(deps, stdout, nil) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 9b1ba0fb5..ca8c0e702 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -4,6 +4,9 @@ import ( "bytes" "context" "encoding/json" + "io" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -111,6 +114,82 @@ func TestRunAuthLoginUnknownProvider(t *testing.T) { } } +func TestRunAuthLoginRevalidatesConfigImmediatelyBeforeSave(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + initial := `{"providers":[{"name":"demo"}]}` + ambiguous := `{"providers":[{"name":"demo"},{"name":"DEMO"}]}` + if err := os.WriteFile(configPath, []byte(initial), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("demo"), oauth.Token{AccessToken: "unchanged"}); err != nil { + t.Fatal(err) + } + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + mux.HandleFunc("/device", func(w http.ResponseWriter, _ *http.Request) { + _, _ = io.WriteString(w, `{"device_code":"dc","user_code":"code","verification_uri":"https://example.test","expires_in":60,"interval":1}`) + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, _ *http.Request) { + if err := os.WriteFile(configPath, []byte(ambiguous), 0o600); err != nil { + t.Errorf("mutate config: %v", err) + } + _, _ = io.WriteString(w, `{"access_token":"replacement","token_type":"Bearer"}`) + }) + t.Setenv("ZERO_OAUTH_DEMO_CLIENT_ID", "client") + t.Setenv("ZERO_OAUTH_DEMO_TOKEN_URL", server.URL+"/token") + t.Setenv("ZERO_OAUTH_DEMO_DEVICE_URL", server.URL+"/device") + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "login", "demo", "--device"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("exit = %d, stderr = %q; want ambiguous-config failure", code, stderr.String()) + } + token, ok, err := store.Load(oauth.ProviderKey("demo")) + if err != nil || !ok || token.AccessToken != "unchanged" { + t.Fatalf("stored token = %+v, %v, %v; want unchanged", token, ok, err) + } +} + +func TestRunAuthChatGPTRevalidatesConfigImmediatelyBeforeSave(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"chatgpt"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("chatgpt"), oauth.Token{AccessToken: "unchanged"}); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + chatGPTLogin: func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) { + ambiguous := `{"providers":[{"name":"chatgpt"},{"name":"ChatGPT"}]}` + if err := os.WriteFile(configPath, []byte(ambiguous), 0o600); err != nil { + return oauth.Token{}, err + } + return oauth.Token{AccessToken: "replacement"}, nil + }, + }) + if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("exit = %d, stderr = %q; want ambiguous-config failure", code, stderr.String()) + } + token, ok, err := store.Load(oauth.ProviderKey("chatgpt")) + if err != nil || !ok || token.AccessToken != "unchanged" { + t.Fatalf("stored token = %+v, %v, %v; want unchanged", token, ok, err) + } +} + func TestRunAuthRefreshNoToken(t *testing.T) { withAuthStore(t) t.Setenv("ZERO_OAUTH_DEMO_CLIENT_ID", "client") // so config resolves; refresh still fails (no token) diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 8f3027a4d..2524e921d 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -71,9 +71,6 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app } cfg, err := config.SetActiveProvider(configPath, options.name) if err != nil { - if strings.Contains(err.Error(), "provider ") && strings.Contains(err.Error(), " not found") { - err = fmt.Errorf("%w; only providers saved in user config are selectable (use zero providers setup or zero providers add first)", err) - } return writeAppError(stderr, err.Error(), exitCrash) } override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) @@ -595,11 +592,17 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps func providerResolvedByName(providers []config.ProviderProfile, name string) bool { name = strings.TrimSpace(name) for _, provider := range providers { - if strings.TrimSpace(provider.Name) == name { + if strings.TrimSpace(provider.Name) == name || strings.TrimSpace(provider.CatalogID) == name { return true } } - return false + matches := 0 + for _, provider := range providers { + if strings.EqualFold(strings.TrimSpace(provider.Name), name) || strings.EqualFold(strings.TrimSpace(provider.CatalogID), name) { + matches++ + } + } + return matches == 1 } // reportUnpersistedProviderUse handles `zero providers use ` for a @@ -611,6 +614,11 @@ func providerResolvedByName(providers []config.ProviderProfile, name string) boo // reports the situation plainly instead of that confusing error (issue // #707). func reportUnpersistedProviderUse(stdout, stderr io.Writer, deps appDeps, options providerUseOptions, configPath string) (int, bool) { + if persisted, err := config.ProviderPersistedCaseInsensitive(configPath, options.name); err != nil { + return writeAppError(stderr, err.Error(), exitCrash), true + } else if persisted { + return exitSuccess, false + } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { // resolveCommandCenterConfig already wrote its own error to stderr; diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index ef475f921..c83cee00c 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -85,10 +85,8 @@ func TestRunProvidersUseExplainsRuntimeOnlyProfilesAreNotSelectable(t *testing.T if code := runWithDeps([]string{"providers", "use", "runtime"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitCrash { t.Fatalf("unexpected code %d", code) } - for _, want := range []string{"only providers saved in user config are selectable", "providers setup", "providers add"} { - if !strings.Contains(stderr.String(), want) { - t.Fatalf("error missing %q: %s", want, stderr.String()) - } + if !strings.Contains(stderr.String(), `provider "runtime" not found`) { + t.Fatalf("error missing plain not-found: %s", stderr.String()) } } @@ -110,8 +108,8 @@ func TestRunProvidersUseRejectsCaseVariantOfPersistedProvider(t *testing.T) { if code := runWithDeps([]string{"providers", "use", "SAVED"}, &stdout, &stderr, deps); code != exitCrash { t.Fatalf("exit = %d, want %d", code, exitCrash) } - if !strings.Contains(stderr.String(), "only providers saved in user config are selectable") { - t.Fatalf("case-variant error did not explain selectability: %q", stderr.String()) + if !strings.Contains(stderr.String(), `provider "SAVED" not found`) { + t.Fatalf("case-variant error was not plain not-found: %q", stderr.String()) } if cfg := readFileConfig(t, configPath); cfg.ActiveProvider != "saved" { t.Fatalf("ActiveProvider = %q, want saved", cfg.ActiveProvider) diff --git a/internal/cli/provider_setup.go b/internal/cli/provider_setup.go index de13f26ce..dc95bc1d9 100644 --- a/internal/cli/provider_setup.go +++ b/internal/cli/provider_setup.go @@ -53,6 +53,9 @@ func runProvidersAdd(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } // Persist with the key moved into the encrypted credential store (capture flip); // the local profile keeps the key for the verification build below. cfg, err := config.UpsertProvider(configPath, config.SecureProviderProfile(profile, configPath), options.setActive) diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 766cea69b..4ee8c89c3 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -264,6 +264,9 @@ func saveSetupProvider(deps appDeps, selection tui.SetupSelection, options setup if err != nil { return tui.SetupResult{}, err } + if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { + return tui.SetupResult{}, err + } // Persist with the key moved into the encrypted credential store (capture flip); // the returned profile keeps the key for this run's immediate use. if _, err := config.UpsertProvider(configPath, config.SecureProviderProfile(profile, configPath), true); err != nil { diff --git a/internal/config/credentials.go b/internal/config/credentials.go index f9432cfd2..d8a2227ae 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -99,7 +99,7 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { } changed := false for index := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), provider) && cfg.Providers[index].APIKeyStored { + if strings.TrimSpace(cfg.Providers[index].Name) == provider && cfg.Providers[index].APIKeyStored { cfg.Providers[index].APIKeyStored = false changed = true } diff --git a/internal/config/credentials_test.go b/internal/config/credentials_test.go index d627a5cb4..194564642 100644 --- a/internal/config/credentials_test.go +++ b/internal/config/credentials_test.go @@ -157,6 +157,16 @@ func TestClearProviderKeyStored(t *testing.T) { if cleared, _ := ClearProviderKeyStored(path, "nope"); cleared { t.Fatal("unknown provider should report no change") } + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + if cleared, err := ClearProviderKeyStored(path, "WORK"); err != nil || cleared { + t.Fatalf("case-variant clear = %v,%v; want false,nil", cleared, err) + } + cfg = readConfigFixture(t, path) + if !cfg.Providers[0].APIKeyStored { + t.Fatalf("clear must require exact provider identity: %+v", cfg.Providers) + } } func TestProviderProfileAPIKeyStoredRoundTrips(t *testing.T) { diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 0d9b11786..1da50b9e4 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -74,6 +74,9 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) { if err != nil { return ResolvedConfig{}, err } + if err := ValidatePersistedProviderNames(fileConfig); err != nil { + return ResolvedConfig{}, err + } mergeConfig(&cfg, fileConfig) } if options.ProjectConfigPath != "" { @@ -919,26 +922,50 @@ func normalizeProvidersWithOptions(providers []ProviderProfile, activeName strin } if activeName == "" && len(providers) == 1 { - activeName = providers[0].Name + activeName = strings.TrimSpace(providers[0].Name) + } + + // Select the active source row before normalizing anything. An exact name + // always wins; folding is only a fallback when it identifies one row. This + // prevents an invalid case-variant sibling from making an exact target fail. + activeIndex := -1 + if activeName != "" { + for index := range providers { + if strings.TrimSpace(providers[index].Name) == activeName { + activeIndex = index + break + } + } + if activeIndex < 0 { + for index := range providers { + if !strings.EqualFold(strings.TrimSpace(providers[index].Name), activeName) { + continue + } + if activeIndex >= 0 { + return nil, ProviderProfile{}, fmt.Errorf("ambiguous active provider %q: multiple provider names differ only by case", activeName) + } + activeIndex = index + } + } } normalized := make([]ProviderProfile, 0, len(providers)) var active ProviderProfile activeFound := false - for _, provider := range providers { + for index, provider := range providers { next, err := normalizeProvider(provider, env, options) if err != nil { // One unresolvable provider (e.g. a profile referencing a provider preset // this build doesn't ship) must NOT brick the whole app — drop it and keep // the rest. Only the ACTIVE provider failing is fatal, since the run can't // proceed without it. - if strings.TrimSpace(provider.Name) == activeName { + if index == activeIndex { return nil, ProviderProfile{}, err } continue } normalized = append(normalized, next) - if next.Name == activeName { + if index == activeIndex { active = next activeFound = true } diff --git a/internal/config/resolver_test.go b/internal/config/resolver_test.go index 35c8cf872..a1979315a 100644 --- a/internal/config/resolver_test.go +++ b/internal/config/resolver_test.go @@ -2,6 +2,7 @@ package config import ( "errors" + "fmt" "os" "path/filepath" "reflect" @@ -383,6 +384,94 @@ func TestResolveAPIKeyEnvLooksUpEnvOnlyWhenAPIKeyMissing(t *testing.T) { } } +func TestNormalizeProvidersMatchesResolvedActiveNameCaseInsensitively(t *testing.T) { + providers, active, err := normalizeProviders([]ProviderProfile{{ + Name: "EnvProvider", + ProviderKind: ProviderKindOpenAI, + Model: "gpt-4.1", + }}, "envprovider") + if err != nil { + t.Fatalf("normalizeProviders() error = %v", err) + } + if len(providers) != 1 || active.Name != "EnvProvider" { + t.Fatalf("resolved active = %+v from %+v, want EnvProvider", active, providers) + } +} + +func TestNormalizeProvidersSelectsActiveSourceBeforeNormalization(t *testing.T) { + valid := func(name string) ProviderProfile { + return ProviderProfile{Name: name, ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"} + } + t.Run("exact wins over folded invalid sibling", func(t *testing.T) { + providers, active, err := normalizeProviders([]ProviderProfile{ + valid("Target"), + {Name: "target", ProviderKind: "invalid", Model: "broken"}, + }, " Target ") + if err != nil { + t.Fatalf("normalizeProviders() error = %v", err) + } + if active.Name != "Target" || len(providers) != 1 { + t.Fatalf("active = %+v, providers = %+v; want exact Target only", active, providers) + } + }) + + t.Run("unique folded fallback", func(t *testing.T) { + _, active, err := normalizeProviders([]ProviderProfile{valid("Target")}, "target") + if err != nil { + t.Fatalf("normalizeProviders() error = %v", err) + } + if active.Name != "Target" { + t.Fatalf("active.Name = %q, want Target", active.Name) + } + }) + + t.Run("multiple folded matches are ambiguous", func(t *testing.T) { + _, _, err := normalizeProviders([]ProviderProfile{valid("Target"), valid("TARGET")}, "target") + const want = `ambiguous active provider "target": multiple provider names differ only by case` + if err == nil || err.Error() != want { + t.Fatalf("error = %v, want %q", err, want) + } + }) +} + +func TestResolveCrossLayerActiveProviderCaseMatching(t *testing.T) { + valid := func(name string) string { + return fmt.Sprintf(`{"providers":[{"name":%q,"providerKind":"openai","model":"gpt-4.1"}]}`, name) + } + t.Run("exact user active wins over project case variant", func(t *testing.T) { + userPath := writeConfig(t, `{"activeProvider":"Target","providers":[{"name":"Target","providerKind":"openai","model":"gpt-4.1"}]}`) + projectPath := writeConfig(t, valid("target")) + resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath, Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve() error = %v", err) + } + if resolved.ActiveProvider != "Target" { + t.Fatalf("active provider = %q, want exact Target", resolved.ActiveProvider) + } + }) + + t.Run("folded cross-layer target is ambiguous", func(t *testing.T) { + userPath := writeConfig(t, `{"activeProvider":"target","providers":[{"name":"Target","providerKind":"openai","model":"gpt-4.1"}]}`) + projectPath := writeConfig(t, valid("TARGET")) + _, err := Resolve(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath, Env: map[string]string{}}) + const want = `ambiguous active provider "target": multiple provider names differ only by case` + if err == nil || err.Error() != want { + t.Fatalf("Resolve() error = %v, want %q", err, want) + } + }) +} + +func TestResolvePreservesSoleOpenRouterCaseVariant(t *testing.T) { + path := writeConfig(t, `{"activeProvider":"openrouter","providers":[{"name":"OpenRouter","catalogId":"openrouter","providerKind":"openai-compatible","baseURL":"https://openrouter.ai/api/v1","model":"openai/gpt-4.1"}]}`) + resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve() error = %v", err) + } + if resolved.ActiveProvider != "OpenRouter" { + t.Fatalf("active provider name = %q, want preserved OpenRouter", resolved.ActiveProvider) + } +} + func TestResolveAPIKeyEnvRedactsResolvedSecretOnErrors(t *testing.T) { path := writeConfig(t, `{ "activeProvider": "custom", diff --git a/internal/config/writer.go b/internal/config/writer.go index fb3f2e700..bdc2eb966 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -11,6 +11,72 @@ import ( "github.com/Gitlawb/zero/internal/providercatalog" ) +// ValidatePersistedProviderNames rejects user-config rows that share the same +// case-insensitive identity. Credential-store keys are case-insensitive, so +// allowing both rows would make writes and deletes affect a shared secret. +// This validator intentionally applies only to raw persisted user config, not +// to profiles merged from project, environment, or provider-command layers. +func ValidatePersistedProviderNames(cfg FileConfig) error { + seen := make(map[string]string, len(cfg.Providers)) + for _, provider := range cfg.Providers { + name := strings.TrimSpace(provider.Name) + folded := strings.ToLower(name) + if previous, ok := seen[folded]; ok && previous != name { + return fmt.Errorf("ambiguous persisted provider names %q and %q differ only by case; rename or remove one row in config.json", previous, name) + } + seen[folded] = name + } + return nil +} + +// PreflightUserConfig validates existing user config before any command makes +// credential-store side effects. +func PreflightUserConfig(path string) error { + path = strings.TrimSpace(path) + if path == "" { + return fmt.Errorf("config path is required") + } + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return fmt.Errorf("invalid config JSON %s: %w", path, err) + } + return ValidatePersistedProviderNames(cfg) +} + +// PreflightProviderWrite also rejects a new spelling that would share a +// case-insensitive credential key with an existing persisted row. +func PreflightProviderWrite(path, name string) error { + if err := PreflightUserConfig(path); err != nil { + return err + } + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return fmt.Errorf("invalid config JSON %s: %w", path, err) + } + name = strings.TrimSpace(name) + for _, provider := range cfg.Providers { + existing := strings.TrimSpace(provider.Name) + if strings.EqualFold(existing, name) && existing != name { + return fmt.Errorf("provider %q already exists as %q; provider names must be unique case-insensitively", name, existing) + } + } + return nil +} + func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileConfig, error) { path = strings.TrimSpace(path) if path == "" { @@ -29,6 +95,14 @@ func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileC } else if !os.IsNotExist(err) { return FileConfig{}, fmt.Errorf("read config %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return FileConfig{}, err + } + for _, existing := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(existing.Name), profile.Name) && strings.TrimSpace(existing.Name) != profile.Name { + return FileConfig{}, fmt.Errorf("provider %q already exists as %q; provider names must be unique case-insensitively", profile.Name, existing.Name) + } + } mergeProvider(&cfg, profile) // mergeProfile deliberately ignores APIKeyStored — during resolve-time @@ -133,8 +207,11 @@ func MarkProviderAPIKeyStored(path string, provider string) error { if err := json.Unmarshal(data, &cfg); err != nil { return fmt.Errorf("invalid config JSON %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return err + } for index := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), provider) { + if strings.TrimSpace(cfg.Providers[index].Name) == provider { cfg.Providers[index].APIKey = "" cfg.Providers[index].APIKeyEnv = "" cfg.Providers[index].APIKeyStored = true @@ -192,6 +269,11 @@ func ProviderPersisted(path string, name string) (bool, error) { if path == "" || name == "" { return false, nil } + if _, err := os.Stat(path); os.IsNotExist(err) { + return false, nil + } else if err != nil { + return false, fmt.Errorf("stat config %s: %w", path, err) + } cfg, err := loadConfigFile(path) if err != nil { return false, err @@ -204,6 +286,29 @@ func ProviderPersisted(path string, name string) (bool, error) { return false, nil } +// ProviderPersistedCaseInsensitive reports whether any persisted user-config +// row has the same folded name. CLI runtime-only guidance uses this to avoid +// describing a case-variant typo of a saved profile as environment-derived. +func ProviderPersistedCaseInsensitive(path, name string) (bool, error) { + data, err := os.ReadFile(strings.TrimSpace(path)) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return false, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + for _, provider := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(provider.Name), strings.TrimSpace(name)) { + return true, nil + } + } + return false, nil +} + // RemoveProvider deletes the named provider profile from the config at path. // When the removed profile was active, activeProvider hands off to the first // remaining provider (or clears when none remain) so the config never points at @@ -228,11 +333,12 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if err := json.Unmarshal(data, &cfg); err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return FileConfig{}, err + } - // Exact match, like ProviderPersisted/SetActiveProvider: two rows can - // coexist that differ only by case (UpsertProvider merges by exact name), - // so folding case here would delete whichever row happens to sort first - // instead of the one the caller actually asked for. + // Persisted provider identity is exact. Resolution may fold names from + // runtime sources, but config mutations must target the requested row. index := -1 for i, provider := range cfg.Providers { if strings.TrimSpace(provider.Name) == name { @@ -284,14 +390,13 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er if err := json.Unmarshal(data, &cfg); err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return FileConfig{}, err + } - // oldName is matched exactly, like ProviderPersisted/SetActiveProvider: - // two rows can coexist that differ only by case (UpsertProvider merges by - // exact name), so folding case here would rename whichever row happens to - // sort first instead of the one the caller actually asked for. newName - // still collides case-insensitively: the credential store normalizes - // names, so renaming into an existing row's case variant would silently - // share (and corrupt) that row's stored key. + // oldName is matched exactly, like ProviderPersisted/SetActiveProvider. + // newName collides case-insensitively because the credential store retains + // legacy case-insensitive keys. index := -1 for i, provider := range cfg.Providers { providerName := strings.TrimSpace(provider.Name) @@ -379,6 +484,9 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { if err := json.Unmarshal(data, &cfg); err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return FileConfig{}, err + } index := -1 for i, provider := range cfg.Providers { @@ -495,10 +603,8 @@ func SetProviderModel(path string, name string, model string) (FileConfig, error return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } - // Exact match, like ProviderPersisted/SetActiveProvider: two rows can - // coexist that differ only by case (UpsertProvider merges by exact name), - // so folding case here would update whichever row happens to sort first - // instead of the one the caller actually asked for. + // Persisted provider identity is exact. Resolution may fold names from + // runtime sources, but config mutations must target the requested row. for index := range cfg.Providers { if strings.TrimSpace(cfg.Providers[index].Name) == name { cfg.Providers[index].Model = model @@ -750,6 +856,9 @@ func NormalizeRecentModels(entries []RecentModelEntry) []RecentModelEntry { } func writeConfigFile(path string, cfg FileConfig) error { + if err := ValidatePersistedProviderNames(cfg); err != nil { + return err + } dir := filepath.Dir(path) if dir != "." && dir != "" { if err := os.MkdirAll(dir, 0o700); err != nil { diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 4fe85f806..16969fb24 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -1,8 +1,10 @@ package config import ( + "bytes" "encoding/json" "errors" + "fmt" "io/fs" "os" "os/exec" @@ -68,6 +70,21 @@ func TestSetActiveProviderRequiresExactProviderIdentity(t *testing.T) { } } +func TestMarkProviderAPIKeyStoredRequiresExactProviderIdentity(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work", APIKeyEnv: "WORK_KEY"}}}, 0o600) + if err := MarkProviderAPIKeyStored(path, "WORK"); err == nil || !strings.Contains(err.Error(), `provider "WORK" not found`) { + t.Fatalf("MarkProviderAPIKeyStored() error = %v, want exact-case not-found", err) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatal("case-variant mark rewrote config") + } +} + func TestProviderPersistedRequiresExactProviderIdentity(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}}}, 0o600) @@ -256,7 +273,7 @@ func TestSetProviderModelRejectsUnknownProviderWithoutRewriting(t *testing.T) { // case must not let SetProviderModel update the wrong one. func TestSetProviderModelRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") - writeConfigFixture(t, path, FileConfig{ + before := writeConfigFixture(t, path, FileConfig{ ActiveProvider: "work", Providers: []ProviderProfile{ {Name: "work", ProviderKind: ProviderKindOpenAICompatible, Model: "m1"}, @@ -264,16 +281,8 @@ func TestSetProviderModelRequiresExactProviderIdentityAmongCaseVariants(t *testi }, }, 0o600) - cfg, err := SetProviderModel(path, "WORK", "m2-updated") - if err != nil { - t.Fatalf("SetProviderModel() error = %v", err) - } - if cfg.Providers[0].Model != "m1" { - t.Fatalf("unrelated 'work' row changed: %+v", cfg.Providers[0]) - } - if cfg.Providers[1].Model != "m2-updated" { - t.Fatalf("targeted 'WORK' row not updated: %+v", cfg.Providers[1]) - } + _, err := SetProviderModel(path, "WORK", "m2-updated") + assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") } func TestUpsertProviderTightensExistingConfigFilePermissions(t *testing.T) { @@ -704,7 +713,7 @@ func TestRemoveProviderKeepsActiveWhenOtherRemoved(t *testing.T) { // named, not whichever case-variant sorts first. func TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") - writeConfigFixture(t, path, FileConfig{ + before := writeConfigFixture(t, path, FileConfig{ ActiveProvider: "work", Providers: []ProviderProfile{ {Name: "work", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://a.example.com/v1", Model: "m1"}, @@ -712,18 +721,8 @@ func TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing }, }, 0o600) - cfg, err := RemoveProvider(path, "WORK") - if err != nil { - t.Fatalf("RemoveProvider() error = %v", err) - } - if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" { - t.Fatalf("expected only the untouched 'work' row to remain, got %+v", cfg.Providers) - } - // "work" is still active and untouched, so the active pointer must not - // hand off to another provider. - if cfg.ActiveProvider != "work" { - t.Fatalf("active provider changed to %q, want unchanged 'work'", cfg.ActiveProvider) - } + _, err := RemoveProvider(path, "WORK") + assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") } func TestRemoveProviderRejectsUnknownWithoutRewriting(t *testing.T) { @@ -818,7 +817,7 @@ func TestRenameProviderRejectsCollisionAndUnknown(t *testing.T) { // let RenameProvider act on the wrong one. func TestRenameProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") - writeConfigFixture(t, path, FileConfig{ + before := writeConfigFixture(t, path, FileConfig{ ActiveProvider: "work", Providers: []ProviderProfile{ {Name: "work", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://a.example.com/v1", Model: "m1"}, @@ -826,21 +825,8 @@ func TestRenameProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing }, }, 0o600) - cfg, err := RenameProvider(path, "WORK", "renamed") - if err != nil { - t.Fatalf("RenameProvider() error = %v", err) - } - names := map[string]bool{} - for _, provider := range cfg.Providers { - names[provider.Name] = true - } - if !names["work"] || !names["renamed"] || names["WORK"] { - t.Fatalf("expected 'work' untouched and 'WORK' renamed to 'renamed', got %+v", cfg.Providers) - } - // The active provider is the untouched "work" row, not the renamed one. - if cfg.ActiveProvider != "work" { - t.Fatalf("active provider changed to %q, want unchanged 'work'", cfg.ActiveProvider) - } + _, err := RenameProvider(path, "WORK", "renamed") + assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") } func TestUpsertProviderPreservesStoredKeyMarkerOnExistingProfile(t *testing.T) { @@ -1033,7 +1019,7 @@ func TestEditProviderAppliesRenameFieldsAndDescriptionAtomically(t *testing.T) { func TestEditProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { path := filepath.Join(t.TempDir(), "config.json") - writeConfigFixture(t, path, FileConfig{ + before := writeConfigFixture(t, path, FileConfig{ ActiveProvider: "work", Providers: []ProviderProfile{ {Name: "WORK", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://upper.example.com/v1", Model: "upper"}, @@ -1041,18 +1027,22 @@ func TestEditProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T }, }, 0o600) - cfg, err := EditProvider(path, ProviderEdit{Name: "WORK", NewName: "renamed", Model: "updated"}) - if err != nil { - t.Fatalf("EditProvider() error = %v", err) - } - if cfg.Providers[0].Name != "renamed" || cfg.Providers[0].Model != "updated" { - t.Fatalf("exact-case target was not edited: %+v", cfg.Providers[0]) + _, err := EditProvider(path, ProviderEdit{Name: "WORK", NewName: "renamed", Model: "updated"}) + assertAmbiguousConfigUnchanged(t, path, before, err, "WORK", "work") +} + +func assertAmbiguousConfigUnchanged(t *testing.T, path string, before []byte, err error, first, second string) { + t.Helper() + want := fmt.Sprintf("ambiguous persisted provider names %q and %q differ only by case; rename or remove one row in config.json", first, second) + if err == nil || err.Error() != want { + t.Fatalf("error = %v, want %q", err, want) } - if cfg.Providers[1].Name != "work" || cfg.Providers[1].Model != "lower" { - t.Fatalf("case-variant provider changed: %+v", cfg.Providers[1]) + after, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatal(readErr) } - if cfg.ActiveProvider != "work" { - t.Fatalf("case-variant active provider changed to %q", cfg.ActiveProvider) + if !bytes.Equal(after, before) { + t.Fatalf("ambiguous mutation rewrote config\nbefore: %s\nafter: %s", before, after) } } diff --git a/internal/oauth/manager.go b/internal/oauth/manager.go index dbdf7817e..0731eb207 100644 --- a/internal/oauth/manager.go +++ b/internal/oauth/manager.go @@ -35,6 +35,7 @@ type Manager struct { // openBrowser is invoked with the authorization URL for loopback logins. // Tests inject a function that drives the loopback redirect. openBrowser func(authURL string) error + beforeSave func() error // refreshLocks serializes concurrent refreshes per key so parallel callers // don't each spend the single-use refresh token; the loser reuses the rotated // token. refreshMu guards the map (M7). @@ -59,6 +60,10 @@ type ManagerOptions struct { RefreshBuffer time.Duration Out io.Writer OpenBrowser func(authURL string) error + // BeforeSave runs after interactive authorization succeeds and immediately + // before credential mutation. Interactive callers use it to revalidate + // state that may have changed while a browser or device flow was pending. + BeforeSave func() error } // NewManager builds a Manager, filling defaults. @@ -97,6 +102,7 @@ func NewManager(opts ManagerOptions) (*Manager, error) { return &Manager{ store: opts.Store, registry: registry, client: client, env: env, now: now, buffer: buffer, out: out, openBrowser: open, + beforeSave: opts.BeforeSave, }, nil } @@ -143,6 +149,11 @@ func (m *Manager) Login(ctx context.Context, opts LoginOptions) (Status, error) } key := ProviderKey(opts.Provider) + if m.beforeSave != nil { + if err := m.beforeSave(); err != nil { + return Status{}, err + } + } if err := m.store.Save(key, token); err != nil { return Status{}, err } @@ -199,6 +210,11 @@ func (m *Manager) CompleteDeviceLogin(ctx context.Context, provider string, cfg return Status{}, err } key := ProviderKey(provider) + if m.beforeSave != nil { + if err := m.beforeSave(); err != nil { + return Status{}, err + } + } if err := m.store.Save(key, token); err != nil { return Status{}, err } diff --git a/internal/tui/oauth_device.go b/internal/tui/oauth_device.go index 00bdb557c..111e9d67e 100644 --- a/internal/tui/oauth_device.go +++ b/internal/tui/oauth_device.go @@ -59,7 +59,11 @@ func oauthDevicePrepare(name string) (oauth.DeviceAuth, oauth.Config, error) { // oauthDeviceComplete polls for the token authorized via oauthDevicePrepare and // stores it under provider: (phase 2). The runtime resolver then attaches // the refreshable token to model calls. -func oauthDeviceComplete(name string, cfg oauth.Config, auth oauth.DeviceAuth) error { +func oauthDeviceComplete(name string, cfg oauth.Config, auth oauth.DeviceAuth, configPath ...string) error { + path := firstString(configPath) + if err := preflightOAuthUserConfig(path); err != nil { + return err + } store, err := oauth.NewStore(oauth.StoreOptions{}) if err != nil { return err @@ -68,6 +72,7 @@ func oauthDeviceComplete(name string, cfg oauth.Config, auth oauth.DeviceAuth) e Store: store, HTTPClient: &http.Client{Timeout: 60 * time.Second}, AllowPresets: true, // preset config is needed to poll/exchange the device token + BeforeSave: func() error { return preflightOAuthUserConfig(path) }, }) if err != nil { return err diff --git a/internal/tui/onboarding.go b/internal/tui/onboarding.go index ed09f3a87..043048488 100644 --- a/internal/tui/onboarding.go +++ b/internal/tui/onboarding.go @@ -537,10 +537,14 @@ func (m *model) moveSetupMethod(delta int) { // setupOAuthCmd runs the chosen provider's browser OAuth login off the UI // goroutine for first-run setup. Mirrors the /provider wizard's flow. -func setupOAuthCmd(provider providercatalog.Descriptor) tea.Cmd { +func setupOAuthCmd(provider providercatalog.Descriptor, configPath ...string) tea.Cmd { + path := firstString(configPath) switch { case provider.OAuthMintsKey: return func() tea.Msg { + if err := preflightOAuthUserConfig(path); err != nil { + return setupOAuthMsg{providerID: provider.ID, err: err} + } key, err := provideroauth.OpenRouterLogin(context.Background(), provideroauth.OpenRouterOptions{ OpenBrowser: browser.OpenURL, Timeout: 3 * time.Minute, @@ -549,13 +553,13 @@ func setupOAuthCmd(provider providercatalog.Descriptor) tea.Cmd { } case provider.ID == "chatgpt": return func() tea.Msg { - err := runProviderChatGPTLogin() + err := runProviderChatGPTLogin(path) return setupOAuthMsg{tokenLogin: true, providerID: provider.ID, err: err} } default: name := provider.ID return func() tea.Msg { - return setupOAuthMsg{tokenLogin: true, providerID: name, err: runProviderTokenLogin(name)} + return setupOAuthMsg{tokenLogin: true, providerID: name, err: runProviderTokenLogin(name, path)} } } } @@ -587,9 +591,13 @@ func setupDevicePrepareCmd(name string) tea.Cmd { } } -func setupDevicePollCmd(name string, cfg oauth.Config, auth oauth.DeviceAuth) tea.Cmd { +func setupDevicePollCmd(name string, cfg oauth.Config, auth oauth.DeviceAuth, configPath ...string) tea.Cmd { return func() tea.Msg { - return setupOAuthMsg{tokenLogin: true, providerID: name, err: oauthDeviceComplete(name, cfg, auth)} + path := firstString(configPath) + if err := preflightOAuthUserConfig(path); err != nil { + return setupOAuthMsg{tokenLogin: true, providerID: name, err: err} + } + return setupOAuthMsg{tokenLogin: true, providerID: name, err: oauthDeviceComplete(name, cfg, auth, path)} } } @@ -626,7 +634,7 @@ func (m model) applySetupOAuthDeviceCode(msg setupOAuthDeviceMsg) (tea.Model, te } m.setup.deviceUserCode = msg.userCode m.setup.deviceVerificationURI = msg.verifyURL - return m, setupDevicePollCmd(msg.providerID, msg.cfg, msg.auth) + return m, setupDevicePollCmd(msg.providerID, msg.cfg, msg.auth, m.setup.configPath) } // applySetupOAuth folds an OAuth login result into the first-run setup: on success @@ -790,7 +798,7 @@ func (m model) advanceSetup() (tea.Model, tea.Cmd) { m.setup.oauthPending = true m.setup.oauthDevice = false m.setup.oauthErr = "" - return m, setupOAuthCmd(descriptor) + return m, setupOAuthCmd(descriptor, m.setup.configPath) } } if m.setup.stage == setupStageProvider { diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 6816074d7..6f327618c 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -604,6 +604,16 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { wizard.err = "name cannot be empty" return m, nil } + if !strings.EqualFold(newName, oldName) { + if err := config.PreflightProviderWrite(m.userConfigPath, newName); err != nil { + wizard.err = err.Error() + return m, nil + } + } + if err := config.PreflightUserConfig(m.userConfigPath); err != nil { + wizard.err = err.Error() + return m, nil + } edit := config.ProviderEdit{ Name: oldName, NewName: newName, diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index 8f4ee8bb3..c7bb719fd 100644 --- a/internal/tui/provider_wizard.go +++ b/internal/tui/provider_wizard.go @@ -137,7 +137,7 @@ func (m model) applyProviderWizardDeviceCode(msg providerWizardDeviceCodeMsg) (m } m.providerWizard.deviceUserCode = msg.userCode m.providerWizard.deviceVerificationURI = msg.verifyURL - return m, providerWizardDevicePollCmd(msg.providerID, msg.attemptID, msg.cfg, msg.auth) + return m, providerWizardDevicePollCmd(msg.providerID, msg.attemptID, msg.cfg, msg.auth, m.userConfigPath) } // providerWizardSupportsOAuth reports whether the credential step should offer a @@ -155,11 +155,15 @@ func providerWizardSupportsOAuth(provider providercatalog.Descriptor) bool { // from the ID token and stores it on the saved token so the Codex provider can // inject it as a header on every request; other OAuth providers (xAI) run the // generic engine login which stores a refreshable token. -func providerWizardOAuthCmdFor(provider providercatalog.Descriptor, attemptID int) tea.Cmd { +func providerWizardOAuthCmdFor(provider providercatalog.Descriptor, attemptID int, configPath ...string) tea.Cmd { providerID := provider.ID + path := firstString(configPath) switch { case provider.OAuthMintsKey: return func() tea.Msg { + if err := preflightOAuthUserConfig(path); err != nil { + return providerWizardOAuthMsg{providerID: providerID, attemptID: attemptID, err: err} + } key, err := provideroauth.OpenRouterLogin(context.Background(), provideroauth.OpenRouterOptions{ OpenBrowser: browser.OpenURL, Timeout: 3 * time.Minute, @@ -168,12 +172,12 @@ func providerWizardOAuthCmdFor(provider providercatalog.Descriptor, attemptID in } case providerID == "chatgpt": return func() tea.Msg { - err := runProviderChatGPTLogin() + err := runProviderChatGPTLogin(path) return providerWizardOAuthMsg{providerID: providerID, attemptID: attemptID, tokenLogin: true, err: err} } default: return func() tea.Msg { - return providerWizardOAuthMsg{providerID: providerID, attemptID: attemptID, tokenLogin: true, err: runProviderTokenLogin(providerID)} + return providerWizardOAuthMsg{providerID: providerID, attemptID: attemptID, tokenLogin: true, err: runProviderTokenLogin(providerID, path)} } } } @@ -183,7 +187,11 @@ func providerWizardOAuthCmdFor(provider providercatalog.Descriptor, attemptID in // the token's Account field) and persists the resulting token via the oauth // store. The runtime resolver then attaches the bearer to Codex calls and the // Codex provider reads the Account field for the `chatgpt-account-id` header. -func runProviderChatGPTLogin() error { +func runProviderChatGPTLogin(configPath ...string) error { + path := firstString(configPath) + if err := preflightOAuthUserConfig(path); err != nil { + return err + } env := buildOAuthPresetEnv() token, err := provideroauth.ChatGPTLogin(context.Background(), provideroauth.ChatGPTOptions{ Env: env, @@ -198,9 +206,26 @@ func runProviderChatGPTLogin() error { if err != nil { return err } + if err := preflightOAuthUserConfig(path); err != nil { + return err + } return store.Save(oauth.ProviderKey("chatgpt"), token) } +func firstString(values []string) string { + if len(values) == 0 { + return "" + } + return values[0] +} + +func preflightOAuthUserConfig(path string) error { + if strings.TrimSpace(path) == "" { + return nil + } + return config.PreflightUserConfig(path) +} + // appendOAuthLoginProfile mirrors the profile persistOAuthLoginProvider wrote to // config into an in-memory saved-provider list, skipping when a profile already // serves the catalog entry (by name or catalog id). @@ -260,7 +285,11 @@ func buildOAuthPresetEnv() map[string]string { // runProviderTokenLogin runs the generic OAuth engine login for a provider that // has a built-in preset (e.g. xAI), storing a refreshable token under // provider:. The runtime resolver then attaches it to model calls. -func runProviderTokenLogin(name string) error { +func runProviderTokenLogin(name string, configPath ...string) error { + path := firstString(configPath) + if err := preflightOAuthUserConfig(path); err != nil { + return err + } store, err := oauth.NewStore(oauth.StoreOptions{}) if err != nil { return err @@ -273,6 +302,7 @@ func runProviderTokenLogin(name string) error { // into its baked-in preset (e.g. xAI's public client_id); without this the // config never resolves and the browser never opens. AllowPresets: true, + BeforeSave: func() error { return preflightOAuthUserConfig(path) }, }) if err != nil { return err @@ -316,9 +346,13 @@ func providerWizardDevicePrepareCmd(name string, attemptID int) tea.Cmd { // providerWizardDevicePollCmd runs phase 2 (poll for the token + store) off the // UI goroutine and reports completion as a regular OAuth result. -func providerWizardDevicePollCmd(name string, attemptID int, cfg oauth.Config, auth oauth.DeviceAuth) tea.Cmd { +func providerWizardDevicePollCmd(name string, attemptID int, cfg oauth.Config, auth oauth.DeviceAuth, configPath ...string) tea.Cmd { return func() tea.Msg { - return providerWizardOAuthMsg{providerID: name, attemptID: attemptID, tokenLogin: true, err: oauthDeviceComplete(name, cfg, auth)} + path := firstString(configPath) + if err := preflightOAuthUserConfig(path); err != nil { + return providerWizardOAuthMsg{providerID: name, attemptID: attemptID, tokenLogin: true, err: err} + } + return providerWizardOAuthMsg{providerID: name, attemptID: attemptID, tokenLogin: true, err: oauthDeviceComplete(name, cfg, auth, path)} } } @@ -968,7 +1002,7 @@ func (m model) handleProviderWizardKey(msg tea.KeyMsg) (model, tea.Cmd) { if providerWizardSupportsOAuth(m.providerWizard.currentProvider()) { provider := m.providerWizard.currentProvider() attemptID := m.providerWizard.beginOAuthAttempt(false) - return m, providerWizardOAuthCmdFor(provider, attemptID) + return m, providerWizardOAuthCmdFor(provider, attemptID, m.userConfigPath) } return m, nil case keyText(msg) != "": @@ -1261,6 +1295,10 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { nextProvider = built } if strings.TrimSpace(m.userConfigPath) != "" { + if err := config.PreflightProviderWrite(m.userConfigPath, profile.Name); err != nil { + wizard.err = err.Error() + return m, nil + } // Capture flip: move the freshly entered key into the encrypted credential // store before persisting, so config.json never holds the cleartext. The // provider was already built above from runtimeProfile, which has the key. @@ -1329,6 +1367,10 @@ func (m model) applyManageKeyChoice() (model, tea.Cmd) { return m, nil case 2: // Remove if strings.TrimSpace(m.userConfigPath) != "" { + if err := config.PreflightUserConfig(m.userConfigPath); err != nil { + wizard.err = err.Error() + return m, nil + } if store, err := config.ProviderKeyStoreAt(filepath.Dir(m.userConfigPath)); err == nil { _, _ = store.Delete(name) } diff --git a/internal/tui/provider_wizard_discovery.go b/internal/tui/provider_wizard_discovery.go index b0dcada69..e6a8fdaf9 100644 --- a/internal/tui/provider_wizard_discovery.go +++ b/internal/tui/provider_wizard_discovery.go @@ -48,7 +48,7 @@ func (m model) advanceProviderWizard() (model, tea.Cmd) { return m.startProviderDeviceLogin() } attemptID := m.providerWizard.beginOAuthAttempt(false) - return m, providerWizardOAuthCmdFor(provider, attemptID) + return m, providerWizardOAuthCmdFor(provider, attemptID, m.userConfigPath) } // A non-OAuth provider that already has a key in the credential store: offer // keep/replace/remove before re-entering credentials. From 7896ff32fa7f16044d9c22bb9a40b9f5a418e6c6 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 28 Jul 2026 22:46:03 +0200 Subject: [PATCH 08/11] fix(cli): stop treating one provider identity as two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` 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) --- internal/cli/auth.go | 8 +- internal/cli/auth_test.go | 53 +++++++++ internal/cli/provider_onboarding.go | 71 +++++++++++-- internal/cli/provider_onboarding_test.go | 130 +++++++++++++++++++++++ internal/config/writer.go | 60 +++++++++++ 5 files changed, 311 insertions(+), 11 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 05818c6ca..1482b3748 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -184,7 +184,7 @@ func runAuthChatGPT(args []string, stdout io.Writer, stderr io.Writer, deps appD return writeAppError(stderr, err.Error(), exitCrash) } const provider = "chatgpt" - if err := config.PreflightProviderWrite(configPath, provider); err != nil { + if err := config.PreflightCatalogProviderLogin(configPath, provider); err != nil { return writeAppError(stderr, err.Error(), exitCrash) } @@ -223,7 +223,7 @@ func runAuthChatGPT(args []string, stdout io.Writer, stderr io.Writer, deps appD if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - if err := config.PreflightProviderWrite(configPath, provider); err != nil { + if err := config.PreflightCatalogProviderLogin(configPath, provider); err != nil { return writeAppError(stderr, err.Error(), exitCrash) } if err := store.Save(oauth.ProviderKey(provider), token); err != nil { @@ -421,11 +421,11 @@ func runAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps appDep if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - if err := config.PreflightProviderWrite(configPath, provider); err != nil { + if err := config.PreflightCatalogProviderLogin(configPath, provider); err != nil { return writeAppError(stderr, err.Error(), exitCrash) } manager, err := newAuthManager(deps, stdout, func() error { - return config.PreflightProviderWrite(configPath, provider) + return config.PreflightCatalogProviderLogin(configPath, provider) }) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index ca8c0e702..cab7cd480 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -190,6 +190,59 @@ func TestRunAuthChatGPTRevalidatesConfigImmediatelyBeforeSave(t *testing.T) { } } +// TestRunAuthChatGPTAllowsCaseVariantPersistedProfile is the regression test for +// jatmn's #725 finding: preflighting a login as if it were a new provider write +// rejected the very row it was logging into. A config whose sole ChatGPT profile +// is spelled "ChatGPT" made `zero auth chatgpt` fail before the browser flow with +// `provider "chatgpt" already exists as "ChatGPT"` — while the TUI, which only +// validates the file, completed the same login. A login mints no new spelling: +// EnsureCatalogProvider reuses whatever row owns the identity. +func TestRunAuthChatGPTAllowsCaseVariantPersistedProfile(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"ChatGPT"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + chatGPTLogin: func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) { + return oauth.Token{AccessToken: "fresh"}, nil + }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, want success; stderr = %q", code, stderr.String()) + } + if strings.Contains(stderr.String(), "already exists as") { + t.Fatalf("a case-variant re-login must not be treated as a colliding new provider: %q", stderr.String()) + } + token, ok, err := store.Load(oauth.ProviderKey("chatgpt")) + if err != nil || !ok || token.AccessToken != "fresh" { + t.Fatalf("stored token = %+v, %v, %v; want the fresh login saved", token, ok, err) + } + // The ambiguous-config guard is unchanged: a login still refuses to run + // against a file with two case-duplicate rows. + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"chatgpt"},{"name":"ChatGPT"}]}`), 0o600); err != nil { + t.Fatal(err) + } + stdout.Reset() + stderr.Reset() + code = runWithDeps([]string{"auth", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + chatGPTLogin: func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) { + 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()) + } +} + func TestRunAuthRefreshNoToken(t *testing.T) { withAuthStore(t) t.Setenv("ZERO_OAUTH_DEMO_CLIENT_ID", "client") // so config resolves; refresh still fails (no token) diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 2524e921d..dde4b937b 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -71,7 +71,7 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app } cfg, err := config.SetActiveProvider(configPath, options.name) if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) + return writeAppError(stderr, providerMutationError(configPath, options.name, err), exitCrash) } override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) // An override only becomes the effective provider if Zero can actually @@ -179,7 +179,13 @@ func activeProviderEnvOverrideResolution(deps appDeps, configPath string, overri options.UserConfigPath = configPath options.ProviderCommand = "" resolved, err := config.Resolve(options) - if err != nil || strings.TrimSpace(resolved.ActiveProvider) != override { + // Fold case: resolution selects the active row case-insensitively and reports + // the row's canonical persisted spelling, so ZERO_PROVIDER=openrouter against + // a saved "OpenRouter" resolves to "OpenRouter". Comparing exactly called that + // a failed override and told the user Zero could not start with it, which was + // the opposite of true. A fold cannot report a false success here: if the + // active row folds to the override, the override is what selected it. + if err != nil || !strings.EqualFold(strings.TrimSpace(resolved.ActiveProvider), override) { return activeProviderOverrideUnresolved } return activeProviderOverrideResolved @@ -482,7 +488,7 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps } cfg, err := config.RemoveProvider(configPath, name) if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) + return writeAppError(stderr, providerMutationError(configPath, name, err), exitCrash) } // Delete the key from the store BESIDE the config being edited — the same // store setup/rename write to — not the default-path store, so a @@ -568,7 +574,7 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps } cfg, err := config.RenameProvider(configPath, options.names[0], options.names[1]) if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) + return writeAppError(stderr, providerMutationError(configPath, options.names[0], err), exitCrash) } if options.json { if err := writePrettyJSON(stdout, map[string]any{ @@ -605,6 +611,47 @@ func providerResolvedByName(providers []config.ProviderProfile, name string) boo return matches == 1 } +// providerMutationError renders a provider mutator's failure, naming the saved +// profile when the config owns what the user asked for under another spelling. +// The mutators match exactly on purpose — one credential identity, one row — so +// `zero providers use SAVED` against a saved "saved", or a catalog id against a +// row saved under a different name, correctly fails not-found. Saying only that +// leaves the user to guess what they got wrong, and `zero auth logout` already +// explains the same situation; this keeps the provider commands consistent with +// it, and goes one better by naming the spelling that works. +func providerMutationError(configPath, name string, err error) string { + message := err.Error() + if !strings.Contains(message, "not found") { + return message + } + canonical, owned, lookupErr := config.PersistedProviderIdentity(configPath, name) + if lookupErr != nil || !owned || canonical == strings.TrimSpace(name) { + return message + } + return fmt.Sprintf("%s; the saved profile is named %q and provider names are matched exactly", message, canonical) +} + +// configOwnsProviderIdentity reports whether a persisted row already owns name — +// under any capitalization, or as the catalog id of a row saved under a +// different name. Every runtime-only report below must consult it first: its +// message claims the provider exists only because an environment variable is +// set, and that is simply false for a provider the config owns. The caller falls +// through to the command's own not-found handling instead, which is the right +// answer for a mis-addressed saved profile — `zero providers use openrouter` +// against a saved {name: "my-router", catalogId: "openrouter"} is the wrong name +// for a real profile, not an env-derived provider. +// +// failed reports that reading the config failed; the error is already on stderr +// and exit carries the code. owned is meaningful only when failed is false. +func configOwnsProviderIdentity(stderr io.Writer, configPath, name string) (owned bool, exit int, failed bool) { + if _, ok, err := config.PersistedProviderIdentity(configPath, name); err != nil { + return false, writeAppError(stderr, err.Error(), exitCrash), true + } else if ok { + return true, exitSuccess, false + } + return false, exitSuccess, false +} + // reportUnpersistedProviderUse handles `zero providers use ` for a // provider that is not persisted in config.json. If it's not resolvable at // all (an unknown/misspelled name), it returns handled=false so the caller @@ -614,9 +661,9 @@ func providerResolvedByName(providers []config.ProviderProfile, name string) boo // reports the situation plainly instead of that confusing error (issue // #707). func reportUnpersistedProviderUse(stdout, stderr io.Writer, deps appDeps, options providerUseOptions, configPath string) (int, bool) { - if persisted, err := config.ProviderPersistedCaseInsensitive(configPath, options.name); err != nil { - return writeAppError(stderr, err.Error(), exitCrash), true - } else if persisted { + if owned, exit, failed := configOwnsProviderIdentity(stderr, configPath, options.name); failed { + return exit, true + } else if owned { return exitSuccess, false } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) @@ -655,6 +702,11 @@ func reportUnpersistedProviderUse(stdout, stderr io.Writer, deps appDeps, option // name isn't resolvable at all, it returns handled=false so the caller falls // through to RemoveProvider's real "not found" error. func reportUnpersistedProviderRemove(stdout, stderr io.Writer, deps appDeps, name string, jsonOutput bool, configPath string) (int, bool) { + if owned, exit, failed := configOwnsProviderIdentity(stderr, configPath, name); failed { + return exit, true + } else if owned { + return exitSuccess, false + } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { return exitCode, true @@ -686,6 +738,11 @@ func reportUnpersistedProviderRemove(stdout, stderr io.Writer, deps appDeps, nam } func reportUnpersistedProviderRename(stdout, stderr io.Writer, deps appDeps, name string, jsonOutput bool, configPath string) (int, bool) { + if owned, exit, failed := configOwnsProviderIdentity(stderr, configPath, name); failed { + return exit, true + } else if owned { + return exitSuccess, false + } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { return exitCode, true diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index c83cee00c..07ad95e1a 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -151,6 +151,136 @@ func providersUseOverrideConfigAtDefaultUserPath(t *testing.T) string { return configPath } +// TestRunProvidersUseResolvesCaseVariantEnvOverride is the regression test for +// jatmn's #725 review: the override check ran the real resolver but 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=WORK against a saved "work" resolves fine at runtime yet was +// reported as unresolvable — telling the user Zero could not start on an +// override that works. +func TestRunProvidersUseResolvesCaseVariantEnvOverride(t *testing.T) { + var stdout, stderr bytes.Buffer + // As in TestRunProvidersUseJSONFlagsEnvOverride: the resolver reads the real + // process environment, so the override has to be set for real. + t.Setenv(config.ActiveProviderEnv, "WORK") + deps := providerSetupDeps(providersUseOverrideConfigAtDefaultUserPath(t)) + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "WORK" + } + return "" + } + + 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("JSON did not decode: %v\n%s", err, stdout.String()) + } + if resolves, ok := payload["envProviderResolves"].(bool); !ok || !resolves { + t.Fatalf("envProviderResolves = %#v, want true for a case-variant override of a saved profile", payload["envProviderResolves"]) + } + if payload["effectiveProvider"] != "WORK" { + t.Fatalf("effectiveProvider = %#v, want the override reported as effective", payload["effectiveProvider"]) + } + + stdout.Reset() + stderr.Reset() + textDeps := providerSetupDeps(providersUseOverrideConfigAtDefaultUserPath(t)) + textDeps.getenv = deps.getenv + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, textDeps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + if note := stderr.String(); strings.Contains(note, "can be resolved") { + t.Fatalf("a resolvable case-variant override must not be reported as broken: %q", note) + } +} + +// TestRunProvidersUseRejectsCatalogIDOfSavedProfile covers jatmn's #725 finding +// that catalog-id addressing of a SAVED row took the runtime-only path: the row +// is not persisted under that name, but the config plainly owns the identity, so +// the env-derived explanation is false and exiting 0 hides a failed switch. +func TestRunProvidersUseRejectsCatalogIDOfSavedProfile(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "work", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAI, BaseURL: config.OpenAIBaseURL, Model: "gpt-4.1"}, + {Name: "my-router", ProviderKind: config.ProviderKindOpenAICompatible, CatalogID: "openrouter", BaseURL: "https://openrouter.ai/api/v1", Model: "x"}, + }, + }) + + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(configPath) + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + saved := config.ProviderProfile{Name: "my-router", ProviderKind: config.ProviderKindOpenAICompatible, CatalogID: "openrouter", Model: "x"} + return config.ResolvedConfig{ActiveProvider: "work", Provider: saved, Providers: []config.ProviderProfile{saved}}, nil + } + if code := runWithDeps([]string{"providers", "use", "openrouter"}, &stdout, &stderr, deps); code != exitCrash { + t.Fatalf("exit = %d, want %d (stdout %q, stderr %q)", code, exitCrash, stdout.String(), stderr.String()) + } + if strings.Contains(stdout.String(), "environment variable") { + t.Fatalf("a saved profile addressed by catalog id must not be described as environment-derived: %q", stdout.String()) + } + if !strings.Contains(stderr.String(), `provider "openrouter" not found`) { + t.Fatalf("stderr = %q, want the real not-found error", stderr.String()) + } + if !strings.Contains(stderr.String(), `"my-router"`) { + t.Fatalf("stderr = %q, want the saved profile's name named in the hint", stderr.String()) + } + if cfg := readFileConfig(t, configPath); cfg.ActiveProvider != "work" { + t.Fatalf("ActiveProvider = %q, want work (nothing should have switched)", cfg.ActiveProvider) + } +} + +// TestRunProvidersRemoveRenameRejectCaseVariantOfPersistedProvider extends the +// guard `providers use` already had to remove and rename, which jatmn found had +// been left behind: `zero providers remove SAVED` against a saved "saved" exited +// 0 with the environment-variable explanation instead of failing not-found. +func TestRunProvidersRemoveRenameRejectCaseVariantOfPersistedProvider(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {name: "remove", args: []string{"providers", "remove", "SAVED"}}, + {name: "rename", args: []string{"providers", "rename", "SAVED", "renamed"}}, + } { + t.Run(tc.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "saved", + Providers: []config.ProviderProfile{ + {Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }) + + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(configPath) + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + profile := config.ProviderProfile{Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"} + return config.ResolvedConfig{ActiveProvider: "saved", Provider: profile, Providers: []config.ProviderProfile{profile}}, nil + } + if code := runWithDeps(tc.args, &stdout, &stderr, deps); code != exitCrash { + t.Fatalf("exit = %d, want %d (stdout %q, stderr %q)", code, exitCrash, stdout.String(), stderr.String()) + } + if strings.Contains(stdout.String(), "environment variable") { + t.Fatalf("a case variant of a saved profile must not be described as environment-derived: %q", stdout.String()) + } + if !strings.Contains(stderr.String(), `provider "SAVED" not found`) { + t.Fatalf("stderr = %q, want the real not-found error", stderr.String()) + } + if !strings.Contains(stderr.String(), `"saved"`) { + t.Fatalf("stderr = %q, want the persisted spelling named in the hint", stderr.String()) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "saved" { + t.Fatalf("config was mutated by a rejected command: %#v", cfg.Providers) + } + }) + } +} + // The write to config.json still succeeds, but when ZERO_PROVIDER names a // different provider the saved selection is NOT effective, so the command must // warn instead of reporting a silent success (issue #721). diff --git a/internal/config/writer.go b/internal/config/writer.go index bdc2eb966..5edd6cc08 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -50,6 +50,66 @@ func PreflightUserConfig(path string) error { return ValidatePersistedProviderNames(cfg) } +// PreflightCatalogProviderLogin is the preflight for a credential login against +// a catalog provider. It is deliberately weaker than PreflightProviderWrite: +// a login does not mint a new spelling. EnsureCatalogProvider reuses whatever +// row already owns the identity — matching on catalog id OR folded name — so a +// persisted "OpenRouter" is the same credential as a `zero auth login +// openrouter`, not a colliding second row. Preflighting that as a collision +// blocked OAuth outright for anyone whose config used a capitalized spelling, +// while the TUI (which only validates the file) completed the same login. +// +// The collision check still runs when nothing owns the identity, because that is +// the case where a row WILL be created. It is unreachable today given +// EnsureCatalogProvider's own matching, and kept so that if that matching ever +// narrows, this fails fast before the browser flow instead of after it. +func PreflightCatalogProviderLogin(path, catalogID string) error { + if err := PreflightUserConfig(path); err != nil { + return err + } + if _, owned, err := PersistedProviderIdentity(path, catalogID); err != nil { + return err + } else if owned { + return nil + } + return PreflightProviderWrite(path, catalogID) +} + +// PersistedProviderIdentity reports whether a persisted user-config row already +// owns identity — as its name (case-insensitively) or as its catalog id — and +// returns that row's exact name. +// +// Callers use it to answer "is this the config's provider, however it was +// spelled or addressed?", which is a different question from ProviderPersisted's +// "is this the exact key a mutator will match". A row saved as +// {name: "my-router", catalogId: "openrouter"} is owned by both spellings here, +// so `zero providers use openrouter` is a wrong way to address a saved profile +// rather than an environment-derived provider. +func PersistedProviderIdentity(path, identity string) (string, bool, error) { + identity = strings.TrimSpace(identity) + if identity == "" { + return "", false, nil + } + data, err := os.ReadFile(strings.TrimSpace(path)) + if os.IsNotExist(err) { + return "", false, nil + } + if err != nil { + return "", false, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return "", false, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + for _, provider := range cfg.Providers { + name := strings.TrimSpace(provider.Name) + if strings.EqualFold(name, identity) || strings.EqualFold(strings.TrimSpace(provider.CatalogID), identity) { + return name, true, nil + } + } + return "", false, nil +} + // PreflightProviderWrite also rejects a new spelling that would share a // case-insensitive credential key with an existing persisted row. func PreflightProviderWrite(path, name string) error { From 54c1d2ecc1af50fed6da6a49427b768451cb6511 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 29 Jul 2026 14:33:03 +0200 Subject: [PATCH 09/11] fix(cli): finish the identity migration the login preflight started MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` — 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) --- internal/cli/auth.go | 56 +++++++--- internal/cli/auth_test.go | 136 +++++++++++++++++++++++ internal/cli/provider_onboarding.go | 7 +- internal/cli/provider_onboarding_test.go | 26 +++-- internal/cli/provider_setup.go | 7 ++ internal/cli/setup.go | 6 + internal/config/writer.go | 26 +++++ internal/tui/provider_wizard.go | 8 ++ 8 files changed, 249 insertions(+), 23 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 1482b3748..c9a77c419 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -99,6 +99,16 @@ func runAuthOpenRouter(args []string, stdout io.Writer, stderr io.Writer, deps a if len(args) > 0 { return writeExecUsageError(stderr, fmt.Sprintf("zero auth openrouter takes no arguments (got %q)", args[0])) } + // Check the config BEFORE the browser flow, as every other auth entry point + // does. Finding out the file is unusable only after the user has completed a + // PKCE round trip wastes their work and mints a key Zero then cannot store. + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if err := config.PreflightCatalogProviderLogin(configPath, "openrouter"); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } key, err := deps.openRouterLogin(context.Background(), provideroauth.OpenRouterOptions{ Out: stdout, HTTPClient: &http.Client{Timeout: 30 * time.Second}, @@ -111,10 +121,14 @@ func runAuthOpenRouter(args []string, stdout io.Writer, stderr io.Writer, deps a key = strings.TrimSpace(key) line, err := saveOpenRouterProviderKey(deps, key) if err != nil { - if _, writeErr := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key minted, but Zero could not save it: %s\nUse it manually, e.g.:\n export OPENROUTER_API_KEY=%s\n", err, key); writeErr != nil { + // The key is real and the user paid for it with a browser round trip, so + // print it rather than losing it. But nothing was persisted, so this is a + // failure: exiting 0 here reported success for a command that left the + // provider unusable, and a script would have carried on. + if _, writeErr := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key minted, but Zero could not save it.\nUse it manually, e.g.:\n export OPENROUTER_API_KEY=%s\n", key); writeErr != nil { return exitCrash } - return exitSuccess + return writeAppError(stderr, "could not save the OpenRouter API key: "+err.Error(), exitCrash) } if _, err := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key saved.\n%s\n", line); err != nil { return exitCrash @@ -469,18 +483,18 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe if err := config.PreflightUserConfig(configPath); err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - exact, err := config.ProviderPersisted(configPath, provider) - if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) - } - if !exact { - folded, foldedErr := config.ProviderPersistedCaseInsensitive(configPath, provider) - if foldedErr != nil { - return writeAppError(stderr, foldedErr.Error(), exitCrash) - } - if folded { - return writeAppError(stderr, fmt.Sprintf("provider %q exists with different capitalization; logout requires the exact persisted provider name", provider), exitCrash) - } + // Resolve the target the way login does. Login accepts a catalog id or a case + // variant and stores its token under the catalog id, and the TUI tells users + // to run `zero auth logout ` — so refusing that spelling here left + // the OAuth token and any stored API key in place for exactly the command the + // UI documents. The credential store keys stay on what the user typed (that is + // where login put them); only the config mutation below needs the persisted + // spelling, because those mutators match a row exactly. + configProvider := provider + if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { + return writeAppError(stderr, identityErr.Error(), exitCrash) + } else if owned { + configProvider = canonical } manager, err := newAuthManager(deps, stdout, nil) if err != nil { @@ -493,11 +507,23 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // Also drop any stored API key and its marker so `auth logout` clears the whole // credential (OAuth token AND key), not just the OAuth side. Surface deletion // failures rather than reporting success while a credential remains. + // + // 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. Clearing only the spelling + // the user typed would leave the other behind. keyRemoved, keyErr := config.ForgetProviderKey(provider) if keyErr != nil { return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) } - if _, clearErr := config.ClearProviderKeyStored(configPath, provider); clearErr != nil { + if configProvider != provider { + canonicalRemoved, canonicalErr := config.ForgetProviderKey(configProvider) + if canonicalErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(canonicalErr, redaction.Options{}), exitCrash) + } + keyRemoved = keyRemoved || canonicalRemoved + } + if _, clearErr := config.ClearProviderKeyStored(configPath, configProvider); clearErr != nil { return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) } removed = removed || keyRemoved diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index cab7cd480..01e7505f3 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -270,6 +271,141 @@ func TestRunAuthRejectsWrongFlags(t *testing.T) { } } +// TestRunAuthLogoutResolvesCatalogIdentity covers jatmn's #725 finding: login +// accepts a catalog id and stores its token under that key, and the TUI tells +// users to run `zero auth logout chatgpt` — but logout hard-stopped whenever a +// persisted row matched case-insensitively without matching exactly, so the +// documented command left the token and any stored key in place. +func TestRunAuthLogoutResolvesCatalogIdentity(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"ChatGPT"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("chatgpt"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if strings.Contains(stderr.String(), "capitalization") { + t.Fatalf("logout refused the spelling the UI documents: %q", stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("chatgpt")); err != nil || ok { + t.Fatalf("stored token survived logout: ok=%v err=%v", ok, err) + } + if !strings.Contains(stdout.String(), "Logged out") { + t.Fatalf("stdout = %q, want a logout confirmation", stdout.String()) + } +} + +// TestRunAuthOpenRouterPreflightsBeforeTheBrowserFlow covers the second half of +// the same finding: every other auth entry point validates the config before +// opening a browser, and this one minted a key first and only discovered the +// config was unusable when trying to save it. +func TestRunAuthOpenRouterPreflightsBeforeTheBrowserFlow(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"work"},{"name":"WORK"}]}`), 0o600); err != nil { + t.Fatal(err) + } + loginCalled := false + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "openrouter"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + openRouterLogin: func(context.Context, provideroauth.OpenRouterOptions) (string, error) { + loginCalled = true + return "sk-should-not-be-minted", nil + }, + }) + if code == exitSuccess { + t.Fatalf("an unusable config must fail before login; stdout = %q", stdout.String()) + } + if loginCalled { + t.Fatal("the browser flow ran before the config was validated") + } + if !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("stderr = %q, want the config error", stderr.String()) + } +} + +// TestRunAuthOpenRouterFailsWhenTheKeyCannotBeSaved pins the exit code: the +// minted key is still printed so the user does not lose it, but nothing was +// persisted, and reporting success left a script believing the provider was +// configured. +func TestRunAuthOpenRouterFailsWhenTheKeyCannotBeSaved(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[]}`), 0o600); err != nil { + t.Fatal(err) + } + calls := 0 + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "openrouter"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { + calls++ + if calls > 1 { + // The preflight passed; break the path only for the save that follows. + return "", errors.New("config path unavailable") + } + return configPath, nil + }, + openRouterLogin: func(context.Context, provideroauth.OpenRouterOptions) (string, error) { + return "sk-openrouter-test", nil + }, + }) + if code == exitSuccess { + t.Fatal("a failed save must not report success") + } + if !strings.Contains(stdout.String(), "sk-openrouter-test") { + t.Fatalf("stdout = %q, want the minted key printed so it is not lost", stdout.String()) + } + if !strings.Contains(stderr.String(), "could not save") { + t.Fatalf("stderr = %q, want the save failure reported", stderr.String()) + } +} + +// TestProviderSetupAdoptsPersistedCatalogRowCasing covers the third: catalog +// OAuth reused an existing row through EnsureCatalogProvider while setup and the +// wizard still collided with it on write, so re-running setup for a provider +// saved as "OpenRouter" failed after a successful capture. +func TestProviderSetupAdoptsPersistedCatalogRowCasing(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"OpenRouter","catalogId":"openrouter","model":"x"}]}`), 0o600); err != nil { + t.Fatal(err) + } + profile := config.ProviderProfile{Name: "openrouter", CatalogID: "openrouter", Model: "y"} + + adopted, err := config.AdoptPersistedCatalogProviderName(configPath, profile) + if err != nil { + t.Fatalf("AdoptPersistedCatalogProviderName: %v", err) + } + if adopted.Name != "OpenRouter" { + t.Fatalf("adopted name = %q, want the persisted row's spelling", adopted.Name) + } + if err := config.PreflightProviderWrite(configPath, adopted.Name); err != nil { + t.Fatalf("write preflight rejected the adopted name: %v", err) + } + + // A user-chosen name is left alone: colliding with an existing row there is a + // real collision, and silently overwriting it would be worse than the error. + custom := config.ProviderProfile{Name: "openrouter", CatalogID: "anthropic"} + kept, err := config.AdoptPersistedCatalogProviderName(configPath, custom) + if err != nil { + t.Fatalf("AdoptPersistedCatalogProviderName: %v", err) + } + if kept.Name != "openrouter" { + t.Fatalf("name = %q, want a non-catalog-default name left untouched", kept.Name) + } +} + func TestRunAuthOpenRouterRejectsArgs(t *testing.T) { withAuthStore(t) var stdout, stderr bytes.Buffer diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index dde4b937b..d8ed9059a 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -153,7 +153,12 @@ func activeProviderEnvOverride(getenv func(string) string, selected string) stri return "" } override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) - if override == "" || override == strings.TrimSpace(selected) { + // Fold case: resolution selects the active row case-insensitively, so + // ZERO_PROVIDER=WORK against a saved "work" names the same provider the write + // just selected. Warning that the switch "has no effect" there described a + // conflict that does not exist — the runtime lands on exactly the row the user + // asked for. Only a genuinely different provider is an override. + if override == "" || strings.EqualFold(override, strings.TrimSpace(selected)) { return "" } return override diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 07ad95e1a..cdeaa3ab5 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -535,15 +535,27 @@ func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { } } -func TestActiveProviderEnvOverrideTreatsCaseVariantAsDistinct(t *testing.T) { - getenv := func(key string) string { - if key == config.ActiveProviderEnv { - return "WORK" +// TestActiveProviderEnvOverrideFoldsCaseAgainstTheSelection replaces the earlier +// TestActiveProviderEnvOverrideTreatsCaseVariantAsDistinct, which locked in a +// warning jatmn showed to be misleading: resolution selects the active row +// case-insensitively, so ZERO_PROVIDER=WORK against a saved "work" lands on +// exactly the row `providers use work` just selected. Telling the user their +// switch stays overridden described a conflict that does not exist. A genuinely +// different provider must still warn. +func TestActiveProviderEnvOverrideFoldsCaseAgainstTheSelection(t *testing.T) { + getenv := func(value string) func(string) string { + return func(key string) string { + if key == config.ActiveProviderEnv { + return value + } + return "" } - return "" } - if override := activeProviderEnvOverride(getenv, "work"); override != "WORK" { - t.Fatalf("activeProviderEnvOverride() = %q, want WORK", override) + if override := activeProviderEnvOverride(getenv("WORK"), "work"); override != "" { + t.Fatalf("activeProviderEnvOverride() = %q, want no override for a case variant of the selection", override) + } + if override := activeProviderEnvOverride(getenv("fast"), "work"); override != "fast" { + t.Fatalf("activeProviderEnvOverride() = %q, want the genuinely different provider reported", override) } } diff --git a/internal/cli/provider_setup.go b/internal/cli/provider_setup.go index dc95bc1d9..68686727f 100644 --- a/internal/cli/provider_setup.go +++ b/internal/cli/provider_setup.go @@ -53,6 +53,13 @@ func runProvidersAdd(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Retarget a catalog-default name at the row that already owns that catalog + // identity, so re-running setup for a provider saved under different casing + // updates it instead of failing the collision check. + profile, err = config.AdoptPersistedCatalogProviderName(configPath, profile) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { return writeAppError(stderr, err.Error(), exitCrash) } diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 4ee8c89c3..66f75ade9 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -264,6 +264,12 @@ func saveSetupProvider(deps appDeps, selection tui.SetupSelection, options setup if err != nil { return tui.SetupResult{}, err } + // Same identity resolution the login path uses: a catalog-default name yields + // to the row already saved for that catalog provider. + profile, err = config.AdoptPersistedCatalogProviderName(configPath, profile) + if err != nil { + return tui.SetupResult{}, err + } if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { return tui.SetupResult{}, err } diff --git a/internal/config/writer.go b/internal/config/writer.go index 5edd6cc08..713f6d97b 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -75,6 +75,32 @@ func PreflightCatalogProviderLogin(path, catalogID string) error { return PreflightProviderWrite(path, catalogID) } +// AdoptPersistedCatalogProviderName retargets a catalog-named profile at the row +// that already owns its catalog identity, so persisting it UPDATES that row +// instead of colliding with it. +// +// It applies only when the caller took the catalog's own default name (profile +// name == catalog id). That is the case where the two spellings are the same +// provider by construction — a re-setup or re-login of, say, openrouter against +// a row saved as "OpenRouter" — and where EnsureCatalogProvider already reuses +// the row on the login path. A user-chosen name is left alone on purpose: there, +// a case collision with an existing row is a real collision, and silently +// overwriting the other row would be worse than the error. +func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (ProviderProfile, error) { + catalogID := strings.TrimSpace(profile.CatalogID) + if catalogID == "" || !strings.EqualFold(strings.TrimSpace(profile.Name), catalogID) { + return profile, nil + } + canonical, owned, err := PersistedProviderIdentity(path, catalogID) + if err != nil { + return profile, err + } + if owned { + profile.Name = canonical + } + return profile, nil +} + // PersistedProviderIdentity reports whether a persisted user-config row already // owns identity — as its name (case-insensitively) or as its catalog id — and // returns that row's exact name. diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index c7bb719fd..7324c780d 100644 --- a/internal/tui/provider_wizard.go +++ b/internal/tui/provider_wizard.go @@ -1295,6 +1295,14 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { nextProvider = built } if strings.TrimSpace(m.userConfigPath) != "" { + // A catalog-default name yields to the row that already owns that catalog + // identity, matching the CLI login path; a user-chosen name still collides. + adopted, adoptErr := config.AdoptPersistedCatalogProviderName(m.userConfigPath, profile) + if adoptErr != nil { + wizard.err = adoptErr.Error() + return m, nil + } + profile = adopted if err := config.PreflightProviderWrite(m.userConfigPath, profile.Name); err != nil { wizard.err = err.Error() return m, nil From a6bbf74510616f63e216d658e35b979b317e5aad Mon Sep 17 00:00:00 2001 From: Amp Date: Wed, 29 Jul 2026 23:58:55 +0000 Subject: [PATCH 10/11] fix(config): complete provider identity recovery Amp-Thread-ID: https://ampcode.com/threads/T-019fafa2-7d9d-75bc-8801-6d0efc8db1df Co-authored-by: Pierre Bruno --- internal/cli/auth.go | 17 +++-- internal/cli/auth_test.go | 47 +++++++++++++ internal/cli/provider_onboarding.go | 16 ++++- internal/cli/provider_onboarding_test.go | 32 +++++++++ internal/config/writer.go | 28 +++++--- internal/config/writer_test.go | 87 +++++++++++++++++++++++- internal/tui/provider_manager.go | 28 +++++--- 7 files changed, 228 insertions(+), 27 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index c9a77c419..dd4ac170e 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -480,9 +480,7 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - if err := config.PreflightUserConfig(configPath); err != nil { - return writeAppError(stderr, err.Error(), exitCrash) - } + configErr := config.PreflightUserConfig(configPath) // Resolve the target the way login does. Login accepts a catalog id or a case // variant and stores its token under the catalog id, and the TUI tells users // to run `zero auth logout ` — so refusing that spelling here left @@ -491,10 +489,12 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // where login put them); only the config mutation below needs the persisted // spelling, because those mutators match a row exactly. configProvider := provider - if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { - return writeAppError(stderr, identityErr.Error(), exitCrash) - } else if owned { - configProvider = canonical + if configErr == nil { + if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { + configErr = identityErr + } else if owned { + configProvider = canonical + } } manager, err := newAuthManager(deps, stdout, nil) if err != nil { @@ -523,6 +523,9 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe } keyRemoved = keyRemoved || canonicalRemoved } + if configErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(configErr, redaction.Options{}), exitCrash) + } if _, clearErr := config.ClearProviderKeyStored(configPath, configProvider); clearErr != nil { return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) } diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 01e7505f3..4b922f56c 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -308,6 +308,53 @@ func TestRunAuthLogoutResolvesCatalogIdentity(t *testing.T) { } } +func TestRunAuthLogoutCleansCredentialsWhenConfigIsAmbiguous(t *testing.T) { + storePath := withAuthStore(t) + configHome := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configHome) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(configHome, "zero", "config.json") + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + configData := []byte(`{"providers":[{"name":"demo"},{"name":"DEMO","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, configData, 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("demo"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("demo", "stored-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "demo"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("exit = %d stderr = %q, want truthful marker-update failure", code, stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("demo")); err != nil || ok { + t.Fatalf("OAuth credential survived recovery logout: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("demo"); err != nil || ok { + t.Fatalf("API key survived recovery logout: ok=%v err=%v", ok, err) + } + after, err := os.ReadFile(configPath) + if err != nil || !bytes.Equal(after, configData) { + t.Fatalf("invalid config changed during recovery logout: err=%v content=%s", err, after) + } +} + // TestRunAuthOpenRouterPreflightsBeforeTheBrowserFlow covers the second half of // the same finding: every other auth entry point validates the config before // opening a browser, and this one minted a key first and only discovered the diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index d8ed9059a..9ddd3d935 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -498,7 +498,11 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps // Delete the key from the store BESIDE the config being edited — the same // store setup/rename write to — not the default-path store, so a // non-default config path cannot leave the encrypted key behind. - keyRemoved, keyErr := removeStoredProviderKeyAt(configPath, name) + keyRemoved := false + var keyErr error + if !configContainsCaseVariantProvider(cfg, name) { + keyRemoved, keyErr = removeStoredProviderKeyAt(configPath, name) + } if options.json { payload := map[string]any{ "removed": name, @@ -550,6 +554,16 @@ func removeStoredProviderKeyAt(configPath string, provider string) (bool, error) return store.Delete(provider) } +func configContainsCaseVariantProvider(cfg config.FileConfig, name string) bool { + name = strings.TrimSpace(name) + for _, provider := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + return true + } + } + return false +} + // runProvidersRename renames a saved provider profile, migrating its stored // API key and the activeProvider pointer along with it (config.RenameProvider). func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index cdeaa3ab5..612edafc7 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -868,3 +868,35 @@ func TestRunProvidersRemoveDeletesKeyBesideConfig(t *testing.T) { t.Fatalf("stored key must be deleted from the store beside the config") } } + +func TestRunProvidersRemoveCaseDuplicateKeepsSurvivorKey(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + seed := `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK"}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "survivor-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + deps := appDeps{userConfigPath: func() (string, error) { return configPath, nil }} + if code := runWithDeps([]string{"providers", "remove", "WORK", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("remove failed: code=%d stderr=%s", code, stderr.String()) + } + if key, ok, err := store.Get("work"); err != nil || !ok || key != "survivor-key" { + t.Fatalf("surviving provider key = %q, %v, %v; want retained", key, ok, err) + } + var payload struct { + KeyRemoved bool `json:"keyRemoved"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil || payload.KeyRemoved { + t.Fatalf("payload = %s, err = %v; shared key must not be reported removed", stdout.String(), err) + } +} diff --git a/internal/config/writer.go b/internal/config/writer.go index 713f6d97b..38fba63fd 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -419,12 +419,11 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if err := json.Unmarshal(data, &cfg); err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } - if err := ValidatePersistedProviderNames(cfg); err != nil { - return FileConfig{}, err - } // Persisted provider identity is exact. Resolution may fold names from - // runtime sources, but config mutations must target the requested row. + // runtime sources, but config mutations must target the requested row. This + // lookup intentionally precedes validation so an exact removal can repair a + // case-duplicate config; writeConfigFile validates the resulting config. index := -1 for i, provider := range cfg.Providers { if strings.TrimSpace(provider.Name) == name { @@ -435,9 +434,22 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if index < 0 { return FileConfig{}, fmt.Errorf("provider %q not found", name) } - removed := cfg.Providers[index] + activeIndex := -1 + activeFoldedIndex := -1 + activeFoldedMatches := 0 + for i, provider := range cfg.Providers { + providerName := strings.TrimSpace(provider.Name) + if providerName == strings.TrimSpace(cfg.ActiveProvider) { + activeIndex = i + } + if strings.EqualFold(providerName, strings.TrimSpace(cfg.ActiveProvider)) { + activeFoldedIndex = i + activeFoldedMatches++ + } + } + removedWasActive := activeIndex == index || (activeIndex < 0 && activeFoldedMatches == 1 && activeFoldedIndex == index) cfg.Providers = append(cfg.Providers[:index], cfg.Providers[index+1:]...) - if strings.TrimSpace(cfg.ActiveProvider) == strings.TrimSpace(removed.Name) { + if removedWasActive { cfg.ActiveProvider = "" if len(cfg.Providers) > 0 { cfg.ActiveProvider = cfg.Providers[0].Name @@ -509,7 +521,7 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er } keyMigrated = true } - if strings.TrimSpace(cfg.ActiveProvider) == strings.TrimSpace(previousName) { + if strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { cfg.ActiveProvider = newName } cfg.Providers[index].Name = newName @@ -604,7 +616,7 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { } keyMigrated = true } - if renamed && strings.TrimSpace(cfg.ActiveProvider) == strings.TrimSpace(previousName) { + if renamed && strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { cfg.ActiveProvider = newName } diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 16969fb24..7ad84b4c7 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -707,13 +707,47 @@ func TestRemoveProviderKeepsActiveWhenOtherRemoved(t *testing.T) { } } +func TestProviderMutatorsHandOffCaseVariantActiveProvider(t *testing.T) { + tests := []struct { + name string + mutate func(string) (FileConfig, error) + wantActive string + wantName string + }{ + {name: "remove", mutate: func(path string) (FileConfig, error) { return RemoveProvider(path, "work") }}, + {name: "rename", mutate: func(path string) (FileConfig, error) { return RenameProvider(path, "work", "office") }, wantActive: "office", wantName: "office"}, + {name: "edit", mutate: func(path string) (FileConfig, error) { + return EditProvider(path, ProviderEdit{Name: "work", NewName: "office", Model: "updated"}) + }, wantActive: "office", wantName: "office"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ActiveProvider: "WORK", Providers: []ProviderProfile{{Name: "work", Model: "old"}}}, 0o600) + cfg, err := test.mutate(path) + if err != nil { + t.Fatal(err) + } + if cfg.ActiveProvider != test.wantActive { + t.Fatalf("activeProvider = %q, want %q", cfg.ActiveProvider, test.wantActive) + } + if test.wantName == "" && len(cfg.Providers) != 0 { + t.Fatalf("providers = %+v, want none", cfg.Providers) + } + if test.wantName != "" && (len(cfg.Providers) != 1 || cfg.Providers[0].Name != test.wantName) { + t.Fatalf("providers = %+v, want canonical name %q", cfg.Providers, test.wantName) + } + }) + } +} + // UpsertProvider merges by exact name, so a config file can end up with two // rows that differ only by case (e.g. one saved as "work", another later // saved as "WORK"). RemoveProvider must delete the exact row the caller // named, not whichever case-variant sorts first. func TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") - before := writeConfigFixture(t, path, FileConfig{ + writeConfigFixture(t, path, FileConfig{ ActiveProvider: "work", Providers: []ProviderProfile{ {Name: "work", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://a.example.com/v1", Model: "m1"}, @@ -721,8 +755,55 @@ func TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing }, }, 0o600) - _, err := RemoveProvider(path, "WORK") - assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") + cfg, err := RemoveProvider(path, "WORK") + if err != nil { + t.Fatalf("exact removal should repair case duplicates: %v", err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" || cfg.ActiveProvider != "work" { + t.Fatalf("repaired config = %+v", cfg) + } +} + +func TestRemoveProviderRejectsNonExactCaseDuplicateTarget(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "WORK"}}}, 0o600) + _, err := RemoveProvider(path, "WoRk") + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("error = %v, want exact-target not-found error", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil || !bytes.Equal(after, before) { + t.Fatalf("rejected removal rewrote config: readErr=%v", readErr) + } +} + +func TestRemoveProviderRejectsRepairThatRemainsAmbiguous(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "WORK"}, {Name: "Work"}}}, 0o600) + _, err := RemoveProvider(path, "Work") + if err == nil || !strings.Contains(err.Error(), "ambiguous persisted provider names") { + t.Fatalf("error = %v, want resulting-config validation error", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil || !bytes.Equal(after, before) { + t.Fatalf("invalid repair rewrote config: readErr=%v", readErr) + } +} + +func TestRemoveProviderKeepsExactActiveCaseVariant(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{{Name: "alpha"}, {Name: "work"}, {Name: "WORK"}}, + }, 0o600) + + cfg, err := RemoveProvider(path, "WORK") + if err != nil { + t.Fatal(err) + } + if cfg.ActiveProvider != "work" { + t.Fatalf("activeProvider = %q, want exact surviving row work", cfg.ActiveProvider) + } } func TestRemoveProviderRejectsUnknownWithoutRewriting(t *testing.T) { diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 6f327618c..dba19d640 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -364,7 +364,7 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { } activeAfter = cfg.ActiveProvider notes = []string{"Deleted " + name + "."} - cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile) + cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, configContainsCaseVariantProviderAfterRemoval(cfg, name)) } else { // Env-derived providers have no persisted profile or credential to // delete. Keep this path session-only. @@ -406,6 +406,16 @@ func removeSavedProvider(saved []config.ProviderProfile, name string) []config.P return kept } +func configContainsCaseVariantProviderAfterRemoval(cfg config.FileConfig, name string) bool { + name = strings.TrimSpace(name) + for _, provider := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + return true + } + } + return false +} + // providerManagerCleanupMsg reports the off-thread half of a delete: the // stored-key removal outcome and the OAuth-login hint. type providerManagerCleanupMsg struct { @@ -417,17 +427,19 @@ type providerManagerCleanupMsg struct { // reads the token store — blocking work the confirm keypress must not wait on. // A failed key delete is surfaced rather than letting a lingering secret read // as a clean removal. -func providerManagerCleanupCmd(configPath string, profile config.ProviderProfile) tea.Cmd { +func providerManagerCleanupCmd(configPath string, profile config.ProviderProfile, retainStoredKey bool) tea.Cmd { name := profile.Name catalogID := profile.CatalogID return func() tea.Msg { notes := []string{} - keyStore, storeErr := providerKeyStoreForPath(configPath) - if storeErr == nil { - _, storeErr = keyStore.Delete(name) - } - if storeErr != nil { - notes = append(notes, "Warning: its stored API key could not be deleted ("+storeErr.Error()+").") + if !retainStoredKey { + keyStore, storeErr := providerKeyStoreForPath(configPath) + if storeErr == nil { + _, storeErr = keyStore.Delete(name) + } + if storeErr != nil { + notes = append(notes, "Warning: its stored API key could not be deleted ("+storeErr.Error()+").") + } } if login, ok := oauthLoginName(config.ProviderProfile{Name: name, CatalogID: catalogID}); ok { notes = append(notes, "OAuth login kept — remove with `zero auth logout "+login+"`.") From 4749a7b7bc00203ea87ba6807575690ba39799f7 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 30 Jul 2026 09:17:35 +0200 Subject: [PATCH 11/11] fix(cli): delete logout keys from resolved config store Amp-Thread-ID: https://ampcode.com/threads/T-019fb1dc-2159-77ee-b020-f471d016544e Co-authored-by: Amp --- internal/cli/auth.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index dd4ac170e..7c0df38f7 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -512,12 +512,16 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // by provider setup lives under the persisted row's name, while one captured // by a catalog login lives under the catalog id. Clearing only the spelling // the user typed would leave the other behind. - keyRemoved, keyErr := config.ForgetProviderKey(provider) + keyStore, keyErr := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if keyErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) + } + keyRemoved, keyErr := keyStore.Delete(provider) if keyErr != nil { return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) } if configProvider != provider { - canonicalRemoved, canonicalErr := config.ForgetProviderKey(configProvider) + canonicalRemoved, canonicalErr := keyStore.Delete(configProvider) if canonicalErr != nil { return writeAppError(stderr, redaction.ErrorMessage(canonicalErr, redaction.Options{}), exitCrash) }