From 4b260698dda43a9c714d970ab802c4236d20220a Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 17 Jul 2026 22:30:04 +0000 Subject: [PATCH 01/17] 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/17] 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/17] 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/17] 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/17] 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/17] 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/17] 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/17] 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/17] 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/17] 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/17] 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) } From 0479e9f667fa4aff4cb3a7e002e2b7e65ebc6423 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 30 Jul 2026 21:32:05 +0200 Subject: [PATCH 12/17] fix(providers): keep case-variant credentials reachable across logout/remove - providers remove / TUI delete: transfer the apiKeyStored marker to a surviving case-variant row instead of orphaning the still-shared credential-store secret when the row that owned the marker is removed - auth logout: delete every OAuth login candidate (profile name, canonical persisted name, catalog id), not just the typed spelling, so a catalog-id-addressed login is fully cleared - auth logout / TUI remove-key: clear the apiKeyStored marker on every case-variant row sharing the deleted credential-store secret, and say explicitly when an ambiguous config blocks clearing a stale marker - TUI provider manager: align removeSavedProvider with RemoveProvider's exact-name identity so deleting one case-variant row can no longer evict its surviving sibling from the in-memory list Addresses jatmn's #725 review round on 4749a7b. --- internal/cli/auth.go | 52 ++++++++++++++++++--- internal/cli/auth_test.go | 34 ++++++++++++++ internal/cli/provider_onboarding.go | 32 +++++++++++-- internal/cli/provider_onboarding_test.go | 48 +++++++++++++++++++ internal/config/credentials.go | 37 +++++++++++++++ internal/config/writer.go | 57 +++++++++++++++++++++++ internal/tui/provider_manager.go | 36 ++++++++++++--- internal/tui/provider_manager_test.go | 59 ++++++++++++++++++++++++ internal/tui/provider_wizard.go | 6 ++- 9 files changed, 342 insertions(+), 19 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 7c0df38f7..be5193665 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "strings" "time" @@ -463,6 +464,18 @@ func runAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps appDep return exitSuccess } +// appendUniqueOAuthCandidate appends candidate to candidates if it is +// non-blank and not already present. The OAuth token store is a +// case-sensitive map, so comparison here is exact, matching +// ProviderProfile.OAuthLoginCandidates. +func appendUniqueOAuthCandidate(candidates []string, candidate string) []string { + candidate = strings.TrimSpace(candidate) + if candidate == "" || slices.Contains(candidates, candidate) { + return candidates + } + return append(candidates, candidate) +} + func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { parsed, err := parseAuthArgs("logout", args) if err != nil { @@ -500,9 +513,28 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - removed, err := manager.Logout(provider) - if err != nil { - return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + // Delete every candidate the runtime bearer resolver would have tried + // (ProviderProfile.OAuthLoginCandidates: profile name, then catalog id) — + // not just the spelling the user typed. A profile saved as + // {name:"my-xai", catalogId:"xai"} logged in via `zero auth login xai` + // stores its token under "xai"; `zero auth logout my-xai` must delete that + // too, or the login silently survives. + oauthCandidates := []string{provider} + if configProvider != provider { + oauthCandidates = append(oauthCandidates, configProvider) + } + if configErr == nil { + if row, found, rowErr := config.ProviderRow(configPath, configProvider); rowErr == nil && found { + oauthCandidates = appendUniqueOAuthCandidate(oauthCandidates, row.CatalogID) + } + } + removed := false + for _, candidate := range oauthCandidates { + candidateRemoved, err := manager.Logout(candidate) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + removed = removed || candidateRemoved } // 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 @@ -528,9 +560,17 @@ 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 { + // Credentials are already gone at this point, but an invalid persisted + // config (e.g. ambiguous case-duplicate rows) blocks every config write, + // including clearing a stale apiKeyStored marker — writeConfigFile + // re-validates on every call, so nothing here could succeed either. + // Say so explicitly rather than leaving the marker silently stale. + return writeAppError(stderr, redaction.ErrorMessage(configErr, redaction.Options{})+"; any stale apiKeyStored marker in config.json must be corrected by hand alongside this", exitCrash) + } + // Clear the marker on every case-variant row, not just configProvider's exact + // spelling: the credential store keys secrets case-folded, so a sibling row + // could have pointed at the same now-deleted secret. + if _, clearErr := config.ClearProviderKeyStoredCaseVariants(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 4b922f56c..04854cba6 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -308,6 +308,40 @@ func TestRunAuthLogoutResolvesCatalogIdentity(t *testing.T) { } } +// TestRunAuthLogoutDeletesCatalogIDToken covers jatmn's #725 follow-up +// finding: a profile addressed by its own name but logged in under its +// catalog id (e.g. {name:"my-xai", catalogId:"xai"} via `zero auth login +// xai`) left the "xai" OAuth token behind when logged out as "my-xai", +// because logout only ever deleted the exact spelling the user typed. +func TestRunAuthLogoutDeletesCatalogIDToken(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"my-xai","catalogId":"xai"}]}`), 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("xai"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "my-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("xai")); err != nil || ok { + t.Fatalf("catalog-id OAuth 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()) + } +} + func TestRunAuthLogoutCleansCredentialsWhenConfigIsAmbiguous(t *testing.T) { storePath := withAuthStore(t) configHome := t.TempDir() diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 9ddd3d935..b252ece56 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -491,6 +491,15 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exit } } + // Read the row BEFORE removal so we know whether it owned the stored-key + // marker — RemoveProvider's returned cfg no longer has it, and the + // credential store's secret is keyed case-folded, so removing a keyed row + // while a case-variant sibling survives must carry the marker over + // instead of silently orphaning the still-shared secret. + before, hadBefore, err := config.ProviderRow(configPath, name) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } cfg, err := config.RemoveProvider(configPath, name) if err != nil { return writeAppError(stderr, providerMutationError(configPath, name, err), exitCrash) @@ -500,7 +509,17 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps // non-default config path cannot leave the encrypted key behind. keyRemoved := false var keyErr error - if !configContainsCaseVariantProvider(cfg, name) { + if survivor, ok := caseVariantProviderName(cfg, name); ok { + if hadBefore && before.APIKeyStored { + if survivorRow, foundSurvivor, err := config.ProviderRow(configPath, survivor); err != nil { + keyErr = err + } else if foundSurvivor && !survivorRow.APIKeyStored { + if _, err := config.TransferProviderAPIKeyStoredMarker(configPath, survivor); err != nil { + keyErr = err + } + } + } + } else { keyRemoved, keyErr = removeStoredProviderKeyAt(configPath, name) } if options.json { @@ -554,14 +573,17 @@ func removeStoredProviderKeyAt(configPath string, provider string) (bool, error) return store.Delete(provider) } -func configContainsCaseVariantProvider(cfg config.FileConfig, name string) bool { +// caseVariantProviderName returns the exact name of a row in cfg that folds +// to name (a case-only variant), and whether one was found. +func caseVariantProviderName(cfg config.FileConfig, name string) (string, bool) { name = strings.TrimSpace(name) for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { - return true + providerName := strings.TrimSpace(provider.Name) + if strings.EqualFold(providerName, name) { + return providerName, true } } - return false + return "", false } // runProvidersRename renames a saved provider profile, migrating its stored diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 612edafc7..b6d1a18ab 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -900,3 +900,51 @@ func TestRunProvidersRemoveCaseDuplicateKeepsSurvivorKey(t *testing.T) { t.Fatalf("payload = %s, err = %v; shared key must not be reported removed", stdout.String(), err) } } + +// TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant covers the +// inverse of TestRunProvidersRemoveCaseDuplicateKeepsSurvivorKey: the removed +// row (not the survivor) owns apiKeyStored. Without transferring the marker, +// the still-shared credential-store secret becomes unreachable through the +// surviving row even though it was never deleted. +func TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant(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", "shared-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 != "shared-key" { + t.Fatalf("shared 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) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + var cfg config.FileConfig + if err := json.Unmarshal(after, &cfg); err != nil { + t.Fatal(err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "WORK" || !cfg.Providers[0].APIKeyStored { + t.Fatalf("survivor must inherit apiKeyStored marker, got %+v", cfg.Providers) + } +} diff --git a/internal/config/credentials.go b/internal/config/credentials.go index d8a2227ae..47596aa35 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -110,6 +110,43 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { return true, writeConfigFile(path, cfg) } +// ClearProviderKeyStoredCaseVariants unsets the APIKeyStored marker on every +// row whose name folds to provider, not just an exact-spelling match. The +// credential store keys its secrets case-folded, so deleting the shared +// secret for one case-variant row (e.g. "WORK") must also clear the marker +// on any sibling row ("work") that pointed at the same now-gone entry — +// leaving it set would claim a key is available when ApplyStoredAPIKey's +// store lookup will always miss. +func ClearProviderKeyStoredCaseVariants(path, provider string) (bool, error) { + path = strings.TrimSpace(path) + provider = strings.TrimSpace(provider) + if path == "" || provider == "" { + return false, nil + } + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return false, 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) + } + changed := false + for index := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), provider) && cfg.Providers[index].APIKeyStored { + cfg.Providers[index].APIKeyStored = false + changed = true + } + } + if !changed { + return false, nil + } + return true, writeConfigFile(path, cfg) +} + // MigratePlaintextProviderKeys moves any inline plaintext API key in the config at // path into the credential store, marking the profile APIKeyStored and stripping // the inline secret — but ONLY after the store write succeeds, so a failed Set diff --git a/internal/config/writer.go b/internal/config/writer.go index 38fba63fd..4dfe4a118 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -372,6 +372,63 @@ func ProviderPersisted(path string, name string) (bool, error) { return false, nil } +// ProviderRow returns the persisted user-config row whose Name matches name +// exactly, and whether one was found. Unlike ProviderPersisted's yes/no, this +// hands back the row itself (CatalogID, APIKeyStored, ...) so callers that +// need more than presence — e.g. deciding whether a stored key marker must be +// carried over to a surviving case-variant row before it is removed — don't +// each re-implement FileConfig parsing. +func ProviderRow(path string, name string) (ProviderProfile, bool, error) { + path = strings.TrimSpace(path) + name = strings.TrimSpace(name) + if path == "" || name == "" { + return ProviderProfile{}, false, nil + } + if _, err := os.Stat(path); os.IsNotExist(err) { + return ProviderProfile{}, false, nil + } else if err != nil { + return ProviderProfile{}, false, fmt.Errorf("stat config %s: %w", path, err) + } + cfg, err := loadConfigFile(path) + if err != nil { + return ProviderProfile{}, false, err + } + for _, provider := range cfg.Providers { + if strings.TrimSpace(provider.Name) == name { + return provider, true, nil + } + } + return ProviderProfile{}, false, nil +} + +// TransferProviderAPIKeyStoredMarker sets the APIKeyStored marker on the row +// named to, without touching any other field on it. The credential store keys +// its secrets case-folded, so removing a case-variant row that owned the +// marker leaves the shared secret orphaned unless a surviving case-variant +// row is marked to take over reading it. No-op (false, nil) when to isn't +// found or already carries the marker. +func TransferProviderAPIKeyStoredMarker(path string, to string) (bool, error) { + path = strings.TrimSpace(path) + to = strings.TrimSpace(to) + if path == "" || to == "" { + return false, nil + } + cfg, err := loadConfigFile(path) + if err != nil { + return false, err + } + for index := range cfg.Providers { + if strings.TrimSpace(cfg.Providers[index].Name) == to { + if cfg.Providers[index].APIKeyStored { + return false, nil + } + cfg.Providers[index].APIKeyStored = true + return true, writeConfigFile(path, cfg) + } + } + return false, fmt.Errorf("provider %q not found", to) +} + // 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. diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index dba19d640..edf45f8e1 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -364,7 +364,21 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { } activeAfter = cfg.ActiveProvider notes = []string{"Deleted " + name + "."} - cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, configContainsCaseVariantProviderAfterRemoval(cfg, name)) + retainStoredKey := false + if survivor, ok := caseVariantProviderNameAfterRemoval(cfg, name); ok { + retainStoredKey = true + // The removed row owned the shared secret's marker; carry it over to + // the surviving case-variant so it stays reachable, mirroring the CLI + // (`zero providers remove`) fix for the same case. + if row.profile.APIKeyStored { + if survivorRow, foundSurvivor, err := config.ProviderRow(m.userConfigPath, survivor); err == nil && foundSurvivor && !survivorRow.APIKeyStored { + if _, err := config.TransferProviderAPIKeyStoredMarker(m.userConfigPath, survivor); err != nil { + notes = append(notes, "Warning: could not keep its stored API key reachable ("+err.Error()+").") + } + } + } + } + cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, retainStoredKey) } else { // Env-derived providers have no persisted profile or credential to // delete. Keep this path session-only. @@ -394,11 +408,15 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { return next, tea.Batch(cmd, cleanup) } -// removeSavedProvider drops one profile from the in-memory saved list. +// removeSavedProvider drops one profile from the in-memory saved list. Uses +// exact-name matching to agree with config.RemoveProvider, which the caller +// just invoked: with case-variant rows now a supported layout (e.g. "work" +// and "WORK"), a fold-based match here could evict the surviving row from the +// UI list while it remains in config.json. func removeSavedProvider(saved []config.ProviderProfile, name string) []config.ProviderProfile { kept := saved[:0] for _, profile := range saved { - if strings.EqualFold(strings.TrimSpace(profile.Name), strings.TrimSpace(name)) { + if strings.TrimSpace(profile.Name) == strings.TrimSpace(name) { continue } kept = append(kept, profile) @@ -406,14 +424,18 @@ func removeSavedProvider(saved []config.ProviderProfile, name string) []config.P return kept } -func configContainsCaseVariantProviderAfterRemoval(cfg config.FileConfig, name string) bool { +// caseVariantProviderNameAfterRemoval returns the exact name of a row in cfg +// that folds to name (a case-only variant surviving the removal), and +// whether one was found. +func caseVariantProviderNameAfterRemoval(cfg config.FileConfig, name string) (string, bool) { name = strings.TrimSpace(name) for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { - return true + providerName := strings.TrimSpace(provider.Name) + if strings.EqualFold(providerName, name) { + return providerName, true } } - return false + return "", false } // providerManagerCleanupMsg reports the off-thread half of a delete: the diff --git a/internal/tui/provider_manager_test.go b/internal/tui/provider_manager_test.go index ddc2429fd..8e16ef340 100644 --- a/internal/tui/provider_manager_test.go +++ b/internal/tui/provider_manager_test.go @@ -152,6 +152,65 @@ func TestProviderManagerDeleteConfirmsAndRemoves(t *testing.T) { } } +// TestProviderManagerDeleteTransfersKeyMarkerToSurvivingCaseVariant mirrors +// the CLI's TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant: +// deleting the case-variant row that owns apiKeyStored must carry the marker +// to the surviving sibling, since the credential store's secret is keyed +// case-folded and stays put either way. +func TestProviderManagerDeleteTransfersKeyMarkerToSurvivingCaseVariant(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(home, "oauth-tokens.json")) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + seed := config.FileConfig{ + ActiveProvider: "WORK", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v1", APIKeyStored: true, Model: "work-model"}, + {Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v2", Model: "work-model-2"}, + }, + } + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatalf("encode seed config: %v", err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatalf("write seed config: %v", err) + } + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "shared-key"); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "WORK", + ModelName: "work-model-2", + Provider: &fakeProvider{}, + ProviderProfile: seed.Providers[1], + SavedProviders: seed.Providers, + UserConfigPath: configPath, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m.width = 120 + m.height = 40 + m, _ = m.openProviderManager() + + m = managerKey(t, m, testKeyText("d")) // select "work" (row 0) + next, _ := m.handleProviderWizardKey(testKeyText("y")) + + persisted := readManagerConfig(t, next.userConfigPath) + if len(persisted.Providers) != 1 || persisted.Providers[0].Name != "WORK" || !persisted.Providers[0].APIKeyStored { + t.Fatalf("survivor must inherit apiKeyStored marker, got %+v", persisted.Providers) + } + if key, ok, err := store.Get("work"); err != nil || !ok || key != "shared-key" { + t.Fatalf("shared provider key = %q, %v, %v; want retained", key, ok, err) + } +} + func TestProviderManagerEditModelPersists(t *testing.T) { m := managerTestModel(t) m = managerKey(t, m, testKeyText("e")) diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index 7324c780d..6195f1e78 100644 --- a/internal/tui/provider_wizard.go +++ b/internal/tui/provider_wizard.go @@ -1382,7 +1382,11 @@ func (m model) applyManageKeyChoice() (model, tea.Cmd) { if store, err := config.ProviderKeyStoreAt(filepath.Dir(m.userConfigPath)); err == nil { _, _ = store.Delete(name) } - _, _ = config.ClearProviderKeyStored(m.userConfigPath, name) + // The credential store keys secrets case-folded, so a case-variant + // sibling row (e.g. "work" beside the deleted key's "WORK") pointed at + // the same now-gone entry — clear the marker on every folded match, + // not just the exact row the user picked. + _, _ = config.ClearProviderKeyStoredCaseVariants(m.userConfigPath, name) } else { _, _ = config.ForgetProviderKey(name) } From 780a21968ca0ed9b5c5dc92e3390fa37e5661bad Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 30 Jul 2026 22:30:08 +0200 Subject: [PATCH 13/17] fix(providers): finish credential-candidate coverage on auth logout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - auth logout: delete API keys under every OAuth login candidate (typed name, canonical persisted name, catalog id), not just the first two — a key stored under the catalog id previously survived logout - auth logout: resolve identity and expand OAuth/API-key candidates regardless of PreflightUserConfig's result, since PersistedProviderIdentity and ProviderRow only read+parse raw JSON and never validate case-duplicate names; only the final apiKeyStored marker write still requires a valid config - TUI provider manager: mirror the on-disk apiKeyStored marker transfer into the in-memory savedProviders list on case-variant delete, so the manager UI doesn't show "no credential" for a survivor whose key is actually reachable until the process restarts - providers list (text): include the source (user-config/resolved) next to the not-selectable marker, matching the JSON output and `providers current` text Addresses jatmn's #725 review round on 0479e9f. --- internal/cli/auth.go | 54 +++++++++--------- internal/cli/auth_test.go | 81 +++++++++++++++++++++++++++ internal/cli/command_center.go | 2 +- internal/tui/provider_manager.go | 17 ++++++ internal/tui/provider_manager_test.go | 8 +++ 5 files changed, 136 insertions(+), 26 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index be5193665..122caa894 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -501,13 +501,21 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // 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. + // + // Identity resolution and candidate expansion run regardless of configErr: + // PersistedProviderIdentity/ProviderRow only read+parse the raw JSON, they + // never call ValidatePersistedProviderNames, so an unrelated ambiguous + // case-duplicate row elsewhere in the file must not suppress deleting every + // credential this logout can find for the requested provider. Only the + // config WRITE below (clearing the apiKeyStored marker) needs a valid config + // and is gated on configErr. configProvider := provider - if configErr == nil { - if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { + if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { + if configErr == nil { configErr = identityErr - } else if owned { - configProvider = canonical } + } else if owned { + configProvider = canonical } manager, err := newAuthManager(deps, stdout, nil) if err != nil { @@ -518,18 +526,17 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // not just the spelling the user typed. A profile saved as // {name:"my-xai", catalogId:"xai"} logged in via `zero auth login xai` // stores its token under "xai"; `zero auth logout my-xai` must delete that - // too, or the login silently survives. - oauthCandidates := []string{provider} + // too, or the login silently survives. The same candidate set is used below + // for the API key store, which has the identical asymmetry. + credentialCandidates := []string{provider} if configProvider != provider { - oauthCandidates = append(oauthCandidates, configProvider) + credentialCandidates = append(credentialCandidates, configProvider) } - if configErr == nil { - if row, found, rowErr := config.ProviderRow(configPath, configProvider); rowErr == nil && found { - oauthCandidates = appendUniqueOAuthCandidate(oauthCandidates, row.CatalogID) - } + if row, found, rowErr := config.ProviderRow(configPath, configProvider); rowErr == nil && found { + credentialCandidates = appendUniqueOAuthCandidate(credentialCandidates, row.CatalogID) } removed := false - for _, candidate := range oauthCandidates { + for _, candidate := range credentialCandidates { candidateRemoved, err := manager.Logout(candidate) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -540,24 +547,21 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // 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. + // The key store is asked for every OAuth candidate spelling too: 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 others behind. 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 := keyStore.Delete(configProvider) - if canonicalErr != nil { - return writeAppError(stderr, redaction.ErrorMessage(canonicalErr, redaction.Options{}), exitCrash) + keyRemoved := false + for _, candidate := range credentialCandidates { + candidateRemoved, candidateErr := keyStore.Delete(candidate) + if candidateErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(candidateErr, redaction.Options{}), exitCrash) } - keyRemoved = keyRemoved || canonicalRemoved + keyRemoved = keyRemoved || candidateRemoved } if configErr != nil { // Credentials are already gone at this point, but an invalid persisted diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 04854cba6..20e286380 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -342,6 +342,87 @@ func TestRunAuthLogoutDeletesCatalogIDToken(t *testing.T) { } } +// TestRunAuthLogoutDeletesCatalogIDAPIKey covers jatmn's second #725 follow-up +// finding: logout's OAuth-token deletion covers the profile name, canonical +// persisted name, and catalog id, but API-key deletion only covered the first +// two — a key stored under the catalog id (e.g. captured via `zero auth +// openrouter`-style catalog flows) survived `zero auth logout my-xai`. +func TestRunAuthLogoutDeletesCatalogIDAPIKey(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"my-xai","catalogId":"xai","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "catalog-id-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "my-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := keyStore.Get("xai"); err != nil || ok { + t.Fatalf("catalog-id API key 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()) + } +} + +// TestRunAuthLogoutResolvesCandidatesDespiteUnrelatedAmbiguousConfig covers +// jatmn's third #725 follow-up finding: identity resolution and OAuth/API-key +// candidate expansion were gated on PreflightUserConfig succeeding, even +// though PersistedProviderIdentity/ProviderRow only read+parse raw JSON and +// never validate case-duplicate names. An unrelated ambiguous pair elsewhere +// in the file (demo/DEMO) must not suppress deleting every credential for the +// unambiguous profile actually being logged out — only the final marker-write +// should fail on that unrelated validation error. +func TestRunAuthLogoutResolvesCandidatesDespiteUnrelatedAmbiguousConfig(t *testing.T) { + storePath := withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + configData := []byte(`{"providers":[{"name":"demo"},{"name":"DEMO"},{"name":"my-xai","catalogId":"xai","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("xai"), 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("xai", "catalog-id-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "my-xai"}, &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 the unrelated ambiguity surfaced as a truthful marker-update failure", code, stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("xai")); err != nil || ok { + t.Fatalf("catalog-id OAuth token survived logout despite the unrelated ambiguous config: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("xai"); err != nil || ok { + t.Fatalf("catalog-id API key survived logout despite the unrelated ambiguous config: ok=%v err=%v", ok, err) + } +} + func TestRunAuthLogoutCleansCredentialsWhenConfigIsAmbiguous(t *testing.T) { storePath := withAuthStore(t) configHome := t.TempDir() diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index a8f893101..4003d57b4 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -440,7 +440,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 += " (not selectable via providers use)" + line += fmt.Sprintf(" (not selectable via providers use; source: %s)", provider.Source) } if provider.Message != "" { line += " (" + provider.Status + ": " + provider.Message + ")" diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index edf45f8e1..3b2393a61 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -365,6 +365,7 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { activeAfter = cfg.ActiveProvider notes = []string{"Deleted " + name + "."} retainStoredKey := false + markerTransferredTo := "" if survivor, ok := caseVariantProviderNameAfterRemoval(cfg, name); ok { retainStoredKey = true // The removed row owned the shared secret's marker; carry it over to @@ -374,11 +375,27 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { if survivorRow, foundSurvivor, err := config.ProviderRow(m.userConfigPath, survivor); err == nil && foundSurvivor && !survivorRow.APIKeyStored { if _, err := config.TransferProviderAPIKeyStoredMarker(m.userConfigPath, survivor); err != nil { notes = append(notes, "Warning: could not keep its stored API key reachable ("+err.Error()+").") + } else { + markerTransferredTo = survivor } } } } cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, retainStoredKey) + // reloadProviderManagerRows (below, via the caller) rebuilds the manager + // list from savedProviders, so the on-disk marker transfer above must be + // mirrored here too — otherwise the survivor reads APIKeyStored: false in + // memory until the process restarts, and providerManagerCredState (which + // gates store lookups on that field) reports "no credential" for a + // profile whose key is actually reachable. + if markerTransferredTo != "" { + for index := range m.savedProviders { + if strings.TrimSpace(m.savedProviders[index].Name) == markerTransferredTo { + m.savedProviders[index].APIKeyStored = true + break + } + } + } } else { // Env-derived providers have no persisted profile or credential to // delete. Keep this path session-only. diff --git a/internal/tui/provider_manager_test.go b/internal/tui/provider_manager_test.go index 8e16ef340..77c12b500 100644 --- a/internal/tui/provider_manager_test.go +++ b/internal/tui/provider_manager_test.go @@ -209,6 +209,14 @@ func TestProviderManagerDeleteTransfersKeyMarkerToSurvivingCaseVariant(t *testin if key, ok, err := store.Get("work"); err != nil || !ok || key != "shared-key" { t.Fatalf("shared provider key = %q, %v, %v; want retained", key, ok, err) } + // The marker transfer must be mirrored into the in-memory savedProviders + // too — reloadProviderManagerRows rebuilds the manager list from it, and + // providerManagerCredState gates store lookups on APIKeyStored, so a stale + // false here would show "no credential" for the survivor until restart + // even though config.json and the credstore both already agree. + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "WORK" || !next.savedProviders[0].APIKeyStored { + t.Fatalf("in-memory savedProviders must inherit apiKeyStored too, got %+v", next.savedProviders) + } } func TestProviderManagerEditModelPersists(t *testing.T) { From 64f186f706ba4789569be703cc7c50a286ab279a Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 31 Jul 2026 15:52:42 +0000 Subject: [PATCH 14/17] fix(auth): surface OAuth setup failures Amp-Thread-ID: https://ampcode.com/threads/T-019fb8cf-a980-7568-80f1-fe34faaaecae Co-authored-by: Pierre Bruno --- internal/cli/provider_onboarding.go | 9 +++- internal/tui/onboarding.go | 23 +++++++-- internal/tui/onboarding_test.go | 55 ++++++++++++++++++++++ internal/tui/provider_wizard.go | 16 ++++++- internal/tui/provider_wizard_oauth_test.go | 23 +++++++++ 5 files changed, 118 insertions(+), 8 deletions(-) diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index b252ece56..4cc2f9943 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -509,13 +509,16 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps // non-default config path cannot leave the encrypted key behind. keyRemoved := false var keyErr error + markerTransferFailed := false if survivor, ok := caseVariantProviderName(cfg, name); ok { if hadBefore && before.APIKeyStored { if survivorRow, foundSurvivor, err := config.ProviderRow(configPath, survivor); err != nil { keyErr = err + markerTransferFailed = true } else if foundSurvivor && !survivorRow.APIKeyStored { if _, err := config.TransferProviderAPIKeyStoredMarker(configPath, survivor); err != nil { keyErr = err + markerTransferFailed = true } } } @@ -542,7 +545,11 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exitCrash } if keyErr != nil { - if _, err := fmt.Fprintf(stderr, "warning: its stored API key could not be deleted and remains in the credential store: %v\n", keyErr); err != nil { + warning := "its stored API key could not be deleted and remains in the credential store" + if markerTransferFailed { + warning = "the stored API key marker could not be transferred to the surviving case-variant provider, so the shared key may be unreachable" + } + if _, err := fmt.Fprintf(stderr, "warning: %s: %v\n", warning, keyErr); err != nil { return exitCrash } } else if keyRemoved { diff --git a/internal/tui/onboarding.go b/internal/tui/onboarding.go index 043048488..0537779f9 100644 --- a/internal/tui/onboarding.go +++ b/internal/tui/onboarding.go @@ -575,8 +575,11 @@ type setupOAuthDeviceMsg struct { err error } -func setupDevicePrepareCmd(name string) tea.Cmd { +func setupDevicePrepareCmd(name string, configPath ...string) tea.Cmd { return func() tea.Msg { + if err := preflightOAuthUserConfig(firstString(configPath)); err != nil { + return setupOAuthDeviceMsg{providerID: name, err: err} + } auth, cfg, err := oauthDevicePrepare(name) if err != nil { return setupOAuthDeviceMsg{providerID: name, err: err} @@ -612,7 +615,7 @@ func (m model) startSetupDeviceLogin(descriptor providercatalog.Descriptor) (tea m.setup.oauthErr = "" m.setup.deviceUserCode = "" m.setup.deviceVerificationURI = "" - return m, setupDevicePrepareCmd(descriptor.ID) + return m, setupDevicePrepareCmd(descriptor.ID, m.setup.configPath) } // applySetupOAuthDeviceCode handles phase 1 of device-code login: show the code, @@ -660,9 +663,12 @@ func (m model) applySetupOAuth(msg setupOAuthMsg) (tea.Model, tea.Cmd) { if msg.tokenLogin { // Early persist on the Update goroutine (see persistOAuthLoginProvider's // threading contract) so quitting setup after the login doesn't lose it; - // completeSetup persists the full profile with the chosen model anyway, - // so a failure here is recoverable and not fatal to setup. - _ = persistOAuthLoginProvider(m.setup.configPath, msg.providerID) + // do not advance when that write fails or the stored token would have no + // reachable provider profile after setup exits. + if err := persistOAuthLoginProvider(m.setup.configPath, msg.providerID); err != nil { + m.setup.oauthErr = "Signed in, but the provider profile could not be saved: " + redaction.ErrorMessage(err, redaction.Options{}) + return m, nil + } } m.setup.oauthErr = "" m.setup.err = "" @@ -882,6 +888,9 @@ func (m model) completeSetup() (tea.Model, tea.Cmd) { }) if err != nil { m.setup.err = err.Error() + if m.setup.oauthMode && m.setupProviderDescriptor().OAuthMintsKey && strings.TrimSpace(apiKey) != "" { + m.setup.err = oauthMintedKeySaveError(m.setup.err, apiKey) + } return m, nil } @@ -911,6 +920,10 @@ func (m model) completeSetup() (tea.Model, tea.Cmd) { return m.exitSetupToChat() } +func oauthMintedKeySaveError(message, apiKey string) string { + return fmt.Sprintf("%s\nThe minted API key was not saved. Copy it now before leaving Zero:\n%s", message, apiKey) +} + func (m *model) resetSetupModels() { option := m.setupProvider() provider := setupProviderDescriptor(option) diff --git a/internal/tui/onboarding_test.go b/internal/tui/onboarding_test.go index 1f70616fb..7d247f45d 100644 --- a/internal/tui/onboarding_test.go +++ b/internal/tui/onboarding_test.go @@ -2066,6 +2066,61 @@ func TestApplySetupOAuthSuccessAdvancesToModel(t *testing.T) { } } +func TestApplySetupOAuthTokenPersistFailureStaysOnProvider(t *testing.T) { + m := newModel(context.Background(), Options{ + Setup: SetupOptions{Visible: true, Providers: []SetupProviderOption{ + {ID: "xai", Name: "xAI", RequiresAuth: true}, + }}, + }) + m.setup.stage = setupStageProvider + m.setup.oauthPending = true + m.setup.oauthMode = true + m.setup.configPath = t.TempDir() // A directory cannot be read as config.json. + + updated, cmd := m.applySetupOAuth(setupOAuthMsg{tokenLogin: true, providerID: "xai"}) + next := updated.(model) + if cmd != nil { + t.Fatal("failed profile persistence should not start model discovery") + } + if next.setup.stage != setupStageProvider { + t.Fatalf("stage = %v, want provider", next.setup.stage) + } + if next.setup.oauthErr == "" || !strings.Contains(next.setup.oauthErr, "could not be saved") { + t.Fatalf("oauthErr = %q, want profile-save failure", next.setup.oauthErr) + } +} + +func TestCompleteSetupSurfacesMintedOpenRouterKeyWhenSaveFails(t *testing.T) { + const mintedKey = "sk-or-minted-recovery" + m := newModel(context.Background(), Options{ + Setup: SetupOptions{ + Visible: true, + Providers: []SetupProviderOption{ + {ID: "openrouter", Name: "OpenRouter", RequiresAuth: true}, + }, + Save: func(SetupSelection) (SetupResult, error) { + return SetupResult{}, errors.New("config write failed") + }, + }, + }) + m.setup.oauthMode = true + m.setup.stage = setupStageReady + m.setup.apiKey.SetValue(mintedKey) + + updated, _ := m.completeSetup() + next := updated.(model) + if !strings.Contains(next.setup.err, mintedKey) || !strings.Contains(next.setup.err, "was not saved") { + t.Fatalf("setup error = %q, want minted-key recovery", next.setup.err) + } +} + +func TestSetupDevicePreparePreflightsConfigBeforeRequestingCode(t *testing.T) { + msg := setupDevicePrepareCmd("xai", t.TempDir())().(setupOAuthDeviceMsg) + if msg.err == nil { + t.Fatal("device-code preparation should reject an unreadable config before requesting a code") + } +} + func pressSetupContinue(m model) model { m = pressSetupContinueOnce(m) // Transparently skip the connect-method chooser via the API-key/browse path so diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index 6195f1e78..14d1ee429 100644 --- a/internal/tui/provider_wizard.go +++ b/internal/tui/provider_wizard.go @@ -327,8 +327,11 @@ type providerWizardDeviceCodeMsg struct { // providerWizardDevicePrepareCmd runs phase 1 of the device-code login off the UI // goroutine and reports the code to display (or an error). -func providerWizardDevicePrepareCmd(name string, attemptID int) tea.Cmd { +func providerWizardDevicePrepareCmd(name string, attemptID int, configPath ...string) tea.Cmd { return func() tea.Msg { + if err := preflightOAuthUserConfig(firstString(configPath)); err != nil { + return providerWizardDeviceCodeMsg{providerID: name, attemptID: attemptID, err: err} + } auth, cfg, err := oauthDevicePrepare(name) if err != nil { return providerWizardDeviceCodeMsg{providerID: name, attemptID: attemptID, err: err} @@ -364,7 +367,7 @@ func (m model) startProviderDeviceLogin() (model, tea.Cmd) { return m, nil } attemptID := m.providerWizard.beginOAuthAttempt(true) - return m, providerWizardDevicePrepareCmd(provider.ID, attemptID) + return m, providerWizardDevicePrepareCmd(provider.ID, attemptID, m.userConfigPath) } const maxProviderWizardProvidersVisible = 10 @@ -1300,11 +1303,17 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { adopted, adoptErr := config.AdoptPersistedCatalogProviderName(m.userConfigPath, profile) if adoptErr != nil { wizard.err = adoptErr.Error() + if provider.OAuthMintsKey && strings.TrimSpace(wizard.apiKey) != "" { + wizard.err = oauthMintedKeySaveError(wizard.err, wizard.apiKey) + } return m, nil } profile = adopted if err := config.PreflightProviderWrite(m.userConfigPath, profile.Name); err != nil { wizard.err = err.Error() + if provider.OAuthMintsKey && strings.TrimSpace(wizard.apiKey) != "" { + wizard.err = oauthMintedKeySaveError(wizard.err, wizard.apiKey) + } return m, nil } // Capture flip: move the freshly entered key into the encrypted credential @@ -1316,6 +1325,9 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { } if _, err := config.UpsertProvider(m.userConfigPath, profile, true); err != nil { wizard.err = redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{secret, profile.APIKey}}) + if provider.OAuthMintsKey && strings.TrimSpace(secret) != "" { + wizard.err = oauthMintedKeySaveError(wizard.err, secret) + } return m, nil // nothing committed to live state yet } } diff --git a/internal/tui/provider_wizard_oauth_test.go b/internal/tui/provider_wizard_oauth_test.go index bd4ac4d14..df63b1714 100644 --- a/internal/tui/provider_wizard_oauth_test.go +++ b/internal/tui/provider_wizard_oauth_test.go @@ -364,6 +364,29 @@ func TestApplyProviderWizardOAuthIgnoresStaleAttempt(t *testing.T) { } } +func TestProviderWizardSurfacesMintedOpenRouterKeyWhenSaveFails(t *testing.T) { + const mintedKey = "sk-or-wizard-recovery" + m := wizardModelAt(t, "openrouter", providerWizardStepDone) + m.providerWizard.oauthMode = true + m.providerWizard.apiKey = mintedKey + m.userConfigPath = t.TempDir() // A directory cannot be read as config.json. + + next, _ := m.applyProviderWizard() + if !strings.Contains(next.providerWizard.err, mintedKey) || !strings.Contains(next.providerWizard.err, "was not saved") { + t.Fatalf("wizard error = %q, want minted-key recovery", next.providerWizard.err) + } +} + +func TestProviderWizardDevicePreparePreflightsConfigBeforeRequestingCode(t *testing.T) { + msg := providerWizardDevicePrepareCmd("xai", 42, t.TempDir())().(providerWizardDeviceCodeMsg) + if msg.err == nil { + t.Fatal("device-code preparation should reject an unreadable config before requesting a code") + } + if msg.attemptID != 42 { + t.Fatalf("attemptID = %d, want 42", msg.attemptID) + } +} + func TestProviderWizardDeviceCodeIgnoresStaleAttempt(t *testing.T) { m := mouseTestModel() m.providerWizard = m.newProviderWizard() From f1f2b6633a2af036db42df80136a8fcc534b668d Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 1 Aug 2026 16:43:25 +0200 Subject: [PATCH 15/17] fix(providers): scope credential identity to proven ownership Addresses jatmn's #725 review round on 64f186f7. - auth logout resolves its target by exact name before catalog id, and expands to the catalog id only when no other row claims it. Catalog ids are shared by design, so logging out one profile was deleting a sibling's OAuth token and API key. - AdoptPersistedCatalogProviderName follows only a case variant of the catalog's own default NAME. Following an arbitrary row that listed the same catalogId silently retargeted the default profile onto it, overwriting its endpoint, model, and stored key. - ValidatePersistedProviderNames rejects any repeated folded identity, not just case-distinct spellings, so two rows literally named "work" can no longer coalesce at resolve time or overwrite one another's migrated key. - providers use no longer hides a case-distinct ZERO_PROVIDER. The suppression now requires resolution to prove the env value lands on the row just written, so a separate exact-case project profile is reported. - providers remove exits nonzero when the credential-marker handoff to a surviving case variant fails, and says which operation failed. The handoff cannot precede the removal: while both rows exist the config is ambiguous and every write is rejected. - Survivor detection in the CLI and TUI removal paths compares with credstore.NormalizeProvider instead of strings.EqualFold, matching the credential store's own trimmed-lowercase keying. Also merges the current base so the diff no longer reverts unrelated mainline changes. Co-Authored-By: Claude Opus 5 (1M context) --- internal/cli/auth.go | 33 +++- internal/cli/auth_test.go | 102 +++++++++++ internal/cli/provider_onboarding.go | 146 ++++++++++++---- internal/cli/provider_onboarding_test.go | 120 +++++++++++-- internal/config/writer.go | 205 ++++++++++++++++++++--- internal/config/writer_test.go | 158 +++++++++++++++++ internal/credstore/credstore.go | 11 ++ internal/tui/provider_manager.go | 14 +- 8 files changed, 709 insertions(+), 80 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 122caa894..eea07a614 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -509,13 +509,20 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // credential this logout can find for the requested provider. Only the // config WRITE below (clearing the apiKeyStored marker) needs a valid config // and is gated on configErr. + // + // Identity resolution prefers an exact name over a case variant, and a name + // over a catalog id (config.ResolvePersistedProviderIdentity). Taking the + // first row that matched either field let `logout xai` retarget a + // {name:"work-xai", catalogId:"xai"} row — a different profile, with + // different credentials, than the one named. configProvider := provider - if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { + target, identityMatch, identityErr := config.ResolvePersistedProviderIdentity(configPath, provider) + if identityErr != nil { if configErr == nil { configErr = identityErr } - } else if owned { - configProvider = canonical + } else if identityMatch != config.PersistedIdentityNone { + configProvider = strings.TrimSpace(target.Name) } manager, err := newAuthManager(deps, stdout, nil) if err != nil { @@ -528,12 +535,28 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // stores its token under "xai"; `zero auth logout my-xai` must delete that // too, or the login silently survives. The same candidate set is used below // for the API key store, which has the identical asymmetry. + // + // The catalog id joins that set only when the resolved profile is the ONLY + // row claiming it. Catalog ids are shared by design — stored-key "work-xai", + // stored-key "xai", and keyless "personal-xai" can all carry catalogId + // "xai" — so expanding unconditionally made `logout work-xai` delete the + // "xai" token and the "xai" profile's API key while clearing only + // work-xai's marker. When the id is shared, the user has to name the + // profile whose credential they mean. credentialCandidates := []string{provider} if configProvider != provider { credentialCandidates = append(credentialCandidates, configProvider) } - if row, found, rowErr := config.ProviderRow(configPath, configProvider); rowErr == nil && found { - credentialCandidates = appendUniqueOAuthCandidate(credentialCandidates, row.CatalogID) + if identityMatch != config.PersistedIdentityNone { + catalogID := strings.TrimSpace(target.CatalogID) + exclusive, exclusiveErr := config.CatalogIdentityExclusive(configPath, catalogID, configProvider) + if exclusiveErr != nil { + if configErr == nil { + configErr = exclusiveErr + } + } else if exclusive { + credentialCandidates = appendUniqueOAuthCandidate(credentialCandidates, catalogID) + } } removed := false for _, candidate := range credentialCandidates { diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 20e286380..02ffd46e7 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -751,3 +751,105 @@ func readCLIConfigFixture(t *testing.T, path string) config.FileConfig { } return cfg } + +// TestRunAuthLogoutLeavesSharedCatalogCredentialsAlone covers jatmn's #725 +// finding that logout cleanup was scoped by catalog id rather than by proven +// profile ownership. Catalog ids are shared by design: stored-key "work-xai", +// stored-key "xai", and keyless "personal-xai" can all carry catalogId "xai". +// Logging out "work-xai" deleted the shared "xai" OAuth token and the "xai" +// profile's API key — another profile's credentials — while clearing only +// work-xai's own marker. +func TestRunAuthLogoutLeavesSharedCatalogCredentialsAlone(t *testing.T) { + storePath := withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + configData := []byte(`{"providers":[` + + `{"name":"work-xai","catalogId":"xai","apiKeyStored":true},` + + `{"name":"xai","catalogId":"xai","apiKeyStored":true},` + + `{"name":"personal-xai","catalogId":"xai"}]}`) + 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("xai"), oauth.Token{AccessToken: "shared"}); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "sibling-key"); err != nil { + t.Fatal(err) + } + if err := keyStore.Set("work-xai", "own-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "work-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := keyStore.Get("work-xai"); err != nil || ok { + t.Fatalf("the profile's own API key must be deleted: ok=%v err=%v", ok, err) + } + if _, ok, err := store.Load(oauth.ProviderKey("xai")); err != nil || !ok { + t.Fatalf("a catalog token three profiles can use must survive one profile's logout: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("xai"); err != nil || !ok { + t.Fatalf("the sibling xai profile's API key must survive: ok=%v err=%v", ok, err) + } +} + +// TestRunAuthLogoutPrefersTheExactlyNamedProfile is the other half of the same +// finding: identity resolution took the first row matching name OR catalog id, +// so `zero auth logout xai` retargeted an earlier {name:"work-xai", +// catalogId:"xai"} row and cleared that profile's marker instead. +func TestRunAuthLogoutPrefersTheExactlyNamedProfile(t *testing.T) { + withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + configData := []byte(`{"providers":[` + + `{"name":"work-xai","catalogId":"xai","apiKeyStored":true},` + + `{"name":"xai","catalogId":"xai","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, configData, 0o600); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("work-xai", "work-key"); err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "own-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"auth", "logout", "xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }); code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := keyStore.Get("xai"); err != nil || ok { + t.Fatalf("the exactly named profile's key must be deleted: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("work-xai"); err != nil || !ok { + t.Fatalf("an earlier catalog sibling must not be logged out instead: ok=%v err=%v", ok, err) + } + cfg := readFileConfig(t, configPath) + for _, provider := range cfg.Providers { + if provider.Name == "xai" && provider.APIKeyStored { + t.Fatal("the named profile's apiKeyStored marker must be cleared") + } + if provider.Name == "work-xai" && !provider.APIKeyStored { + t.Fatal("the sibling profile's apiKeyStored marker must be left alone") + } + } +} diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 4cc2f9943..7e4c1db38 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -9,6 +9,7 @@ import ( "unicode" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/credstore" "github.com/Gitlawb/zero/internal/providercatalog" "github.com/Gitlawb/zero/internal/provideronboarding" ) @@ -74,6 +75,13 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app return writeAppError(stderr, providerMutationError(configPath, options.name, err), exitCrash) } override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) + // A case-only difference is only harmless when it demonstrably selects the + // row just written; a separate exact-case profile from project config makes + // it a real override of a real, different provider. + if override != "" && strings.EqualFold(override, strings.TrimSpace(cfg.ActiveProvider)) && + activeProviderEnvOverrideSelectsSaved(deps, configPath, cfg.ActiveProvider) { + override = "" + } // An override only becomes the effective provider if Zero can actually // resolve it; a stale value names nothing and fails the next resolution. overrideResolution := activeProviderOverrideAbsent @@ -142,55 +150,87 @@ func (resolution activeProviderOverrideResolution) resolves() any { } // 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 +// names a DIFFERENT spelling than the one just selected, meaning the saved +// `providers use` selection may NOT be the effective active provider until the // env var is unset. applyEnv (resolver.go) makes ZERO_PROVIDER win over // config.json unconditionally, so reporting the write as a plain success reads as // a switch that silently has no effect (issue #721). Empty when nothing overrides // (including when getenv is nil, e.g. a test that did not inject the environment). +// +// A case-only difference is NOT assumed here to name the same provider — see +// activeProviderEnvOverrideSelectsSaved, which resolves that question instead of +// guessing at it. func activeProviderEnvOverride(getenv func(string) string, selected string) string { if getenv == nil { return "" } override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) - // 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)) { + if override == "" || override == strings.TrimSpace(selected) { return "" } return override } -// 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 { +// activeProviderEnvOverrideSelectsSaved reports whether a case-only ZERO_PROVIDER +// difference actually lands on the row `providers use` just wrote. +// +// Folding the comparison outright was wrong: user config resolves the active row +// case-insensitively, but project config and provider commands contribute their +// own rows, and resolution then selects an exact-case match. With a workspace +// profile literally named "WORK", ZERO_PROVIDER=WORK selects THAT row — different +// credentials, different endpoint — while `zero providers use work` reported a +// clean switch with no override at all. So the suppression is granted only when +// resolution proves the env value produces the very spelling just selected. +func activeProviderEnvOverrideSelectsSaved(deps appDeps, configPath string, selected string) bool { + resolved, ok := resolveActiveProviderWithoutProviderCommand(deps, configPath) + if !ok { + return false + } + return resolved == strings.TrimSpace(selected) +} + +// resolveActiveProviderWithoutProviderCommand resolves the effective active +// provider name without running ZERO_PROVIDER_COMMAND. Provider commands are +// arbitrary external programs and `providers use` must remain a config-only +// operation, so a configured one makes the answer unknowable here (false). +func resolveActiveProviderWithoutProviderCommand(deps appDeps, configPath string) (string, bool) { if deps.getenv != nil && strings.TrimSpace(deps.getenv(config.ProviderCommandEnv)) != "" { - return activeProviderOverrideDeferred + return "", false } workspaceRoot, err := resolveWorkspaceRoot("", deps) if err != nil { - return activeProviderOverrideUnresolved + return "", false } options, err := config.DefaultResolveOptions(workspaceRoot) if err != nil { - return activeProviderOverrideUnresolved + return "", false } options.UserConfigPath = configPath options.ProviderCommand = "" resolved, err := config.Resolve(options) + if err != nil { + return "", false + } + return strings.TrimSpace(resolved.ActiveProvider), true +} + +// 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 + } + resolved, ok := resolveActiveProviderWithoutProviderCommand(deps, configPath) // 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) { + if !ok || !strings.EqualFold(resolved, override) { return activeProviderOverrideUnresolved } return activeProviderOverrideResolved @@ -500,26 +540,35 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Decide up front whether a sibling row will survive this removal holding + // the SAME credential-store identity, because that decides what happens to + // the shared secret: hand the marker over, or delete the key outright. + survivor, survivorSurvives, err := survivingCaseVariantProviderName(configPath, name) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } cfg, err := config.RemoveProvider(configPath, name) if err != nil { return writeAppError(stderr, providerMutationError(configPath, name, err), exitCrash) } + // The marker handoff can only run AFTER the removal, not as a transaction + // around it: while both case-variant rows are present the config is + // ambiguous, and writeConfigFile rejects every write against it — removing + // one row is precisely the repair. So the window is real, and the command + // reports it as a partial failure (nonzero below) instead of success. + // // 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. + // non-default config path cannot leave the encrypted key behind. Skipped + // when a case variant survives: that row keeps reading the shared entry. keyRemoved := false var keyErr error markerTransferFailed := false - if survivor, ok := caseVariantProviderName(cfg, name); ok { + if survivorSurvives { if hadBefore && before.APIKeyStored { - if survivorRow, foundSurvivor, err := config.ProviderRow(configPath, survivor); err != nil { + if _, err := config.TransferProviderAPIKeyStoredMarker(configPath, survivor); err != nil { keyErr = err markerTransferFailed = true - } else if foundSurvivor && !survivorRow.APIKeyStored { - if _, err := config.TransferProviderAPIKeyStoredMarker(configPath, survivor); err != nil { - keyErr = err - markerTransferFailed = true - } } } } else { @@ -539,6 +588,11 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps if err := writePrettyJSON(stdout, payload); err != nil { return exitCrash } + // A failed handoff left the survivor unable to reach the shared key. + // Scripts must not read that as a completed removal. + if markerTransferFailed { + return exitCrash + } return exitSuccess } if _, err := fmt.Fprintf(stdout, "Removed provider %s\n", name); err != nil { @@ -547,7 +601,7 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps if keyErr != nil { warning := "its stored API key could not be deleted and remains in the credential store" if markerTransferFailed { - warning = "the stored API key marker could not be transferred to the surviving case-variant provider, so the shared key may be unreachable" + warning = fmt.Sprintf("the stored API key marker could not be handed to %s, which shares its credential entry, so the shared key is now unreachable", survivor) } if _, err := fmt.Fprintf(stderr, "warning: %s: %v\n", warning, keyErr); err != nil { return exitCrash @@ -566,6 +620,9 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exitCrash } } + if markerTransferFailed { + return exitCrash + } return exitSuccess } @@ -580,17 +637,34 @@ func removeStoredProviderKeyAt(configPath string, provider string) (bool, error) return store.Delete(provider) } -// caseVariantProviderName returns the exact name of a row in cfg that folds -// to name (a case-only variant), and whether one was found. -func caseVariantProviderName(cfg config.FileConfig, name string) (string, bool) { - name = strings.TrimSpace(name) - for _, provider := range cfg.Providers { - providerName := strings.TrimSpace(provider.Name) - if strings.EqualFold(providerName, name) { - return providerName, true +// survivingCaseVariantProviderName returns the exact name of another persisted +// row that shares name's CREDENTIAL-STORE identity — a case-only variant that +// will still read the same stored secret once name's row is gone — and whether +// one was found. config.RemoveProvider matches its row exactly, so any row with +// a different exact spelling survives. +// +// The comparison is credstore.NormalizeProvider, not strings.EqualFold, because +// only the former is the store's own equivalence rule. Unicode case folding +// equates "s" and "ſ" while strings.ToLower does not, so EqualFold would let +// `providers remove s` skip deleting the key and hand the marker to "ſ", whose +// lookup uses a different store entry — orphaning the secret and leaving the +// survivor with a marker pointing at nothing. +func survivingCaseVariantProviderName(configPath string, name string) (string, bool, error) { + names, err := config.PersistedProviderNames(configPath) + if err != nil { + return "", false, err + } + removed := strings.TrimSpace(name) + target := credstore.NormalizeProvider(removed) + for _, candidate := range names { + if candidate == removed { + continue + } + if credstore.NormalizeProvider(candidate) == target { + return candidate, true, nil } } - return "", false + return "", false, nil } // runProvidersRename renames a saved provider profile, migrating its stored diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index b6d1a18ab..d50c1e5e4 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -535,14 +535,12 @@ func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { } } -// 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) { +// TestActiveProviderEnvOverrideReportsEveryDistinctSpelling pins the pure +// function's contract after jatmn's #725 round: a case-only difference is no +// longer silently treated as "the same provider". Deciding that needs the +// resolver (see TestRunProvidersUseCaseVariantEnvOverrideFollowsResolution), +// so this layer reports every spelling that is not literally the selection. +func TestActiveProviderEnvOverrideReportsEveryDistinctSpelling(t *testing.T) { getenv := func(value string) func(string) string { return func(key string) string { if key == config.ActiveProviderEnv { @@ -551,14 +549,69 @@ func TestActiveProviderEnvOverrideFoldsCaseAgainstTheSelection(t *testing.T) { return "" } } - 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("work"), "work"); override != "" { + t.Fatalf("activeProviderEnvOverride() = %q, want no override when the env names the selection exactly", override) + } + if override := activeProviderEnvOverride(getenv("WORK"), "work"); override != "WORK" { + t.Fatalf("activeProviderEnvOverride() = %q, want the case-distinct spelling reported to the resolver", override) } if override := activeProviderEnvOverride(getenv("fast"), "work"); override != "fast" { t.Fatalf("activeProviderEnvOverride() = %q, want the genuinely different provider reported", override) } } +// TestRunProvidersUseCaseVariantEnvOverrideFollowsResolution is the regression +// test for jatmn's #725 finding that a case-distinct ZERO_PROVIDER was hidden +// unconditionally. Folding is right only when the env value lands on the row +// `providers use` just wrote; a project config contributing a separate +// exact-case "WORK" profile makes it a real override of a different provider, +// with different credentials, and that must still be reported. +func TestRunProvidersUseCaseVariantEnvOverrideFollowsResolution(t *testing.T) { + run := func(t *testing.T, projectProviders []config.ProviderProfile) string { + t.Helper() + // The resolver reads the real process environment, so the override has + // to be set for real, and the user config has to sit at the default path. + t.Setenv(config.ActiveProviderEnv, "WORK") + configPath := providersUseOverrideConfigAtDefaultUserPath(t) + workspace := t.TempDir() + if len(projectProviders) > 0 { + projectPath := filepath.Join(workspace, ".zero", "config.json") + writeProviderOnboardingConfig(t, projectPath, config.FileConfig{Providers: projectProviders}) + } + deps := providerSetupDeps(configPath) + deps.getwd = func() (string, error) { return workspace, nil } + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "WORK" + } + return "" + } + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "work"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + return stderr.String() + } + + t.Run("case variant of the selection", func(t *testing.T) { + if note := run(t, nil); note != "" { + t.Fatalf("an override that resolves to the selected row must not warn: %q", note) + } + }) + + t.Run("case-distinct project profile", func(t *testing.T) { + note := run(t, []config.ProviderProfile{{ + Name: "WORK", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://api.example.com/v1", + Model: "example-model", + }}) + if !strings.Contains(note, "WORK") || !strings.Contains(note, config.ActiveProviderEnv) { + t.Fatalf("a separate exact-case profile must be reported as an override: %q", note) + } + }) +} + func TestRunProvidersUseSurfacesMalformedConfig(t *testing.T) { var stdout, stderr bytes.Buffer configPath := filepath.Join(t.TempDir(), "config.json") @@ -948,3 +1001,50 @@ func TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant(t *testing.T t.Fatalf("survivor must inherit apiKeyStored marker, got %+v", cfg.Providers) } } + +// TestRunProvidersRemoveUsesCredentialStoreEquivalenceForSurvivors covers +// jatmn's #725 finding that survivor detection compared with strings.EqualFold +// while the credential store keys entries with trimmed strings.ToLower. The two +// relations differ: Unicode case folding equates "s" and "ſ", ToLower does not. +// Both spellings therefore pass persisted-name validation as separate rows with +// separate store entries, but the EqualFold check called "ſ" a survivor of +// removing "s" — skipping the key deletion (orphaning the secret) and handing +// the marker to a row whose lookup uses a different entry entirely. +func TestRunProvidersRemoveUsesCredentialStoreEquivalenceForSurvivors(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + seed := `{"activeProvider":"s","providers":[{"name":"s","apiKeyStored":true},{"name":"\u017f"}]}` + 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("s", "long-s-is-not-s"); 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", "s", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("remove failed: code=%d stderr=%s", code, stderr.String()) + } + if _, ok, err := store.Get("s"); err != nil || ok { + t.Fatalf("key = ok:%v err:%v; the removed row owned its own store entry, which must be deleted", 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; want keyRemoved true", stdout.String(), err) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "\u017f" { + t.Fatalf("providers = %+v, want only the distinct long-s row", cfg.Providers) + } + if cfg.Providers[0].APIKeyStored { + t.Fatal("a row with its own credential-store identity must not inherit another row's marker") + } +} diff --git a/internal/config/writer.go b/internal/config/writer.go index cad48668a..8965d0e6b 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + "github.com/Gitlawb/zero/internal/credstore" "github.com/Gitlawb/zero/internal/providercatalog" ) @@ -16,12 +17,22 @@ import ( // 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. +// +// A repeated folded identity is rejected whether or not the spellings differ. +// Exact duplicates are just as broken as case variants: resolver merging +// silently coalesces the rows, and plaintext-key migration writes both values +// into the same normalized credential-store entry, so the second row's key +// overwrites the first. 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 { + folded := credstore.NormalizeProvider(name) + previous, ok := seen[folded] + if ok && previous == name { + return fmt.Errorf("duplicate persisted provider name %q; remove one of the rows in config.json", name) + } + if ok { 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 @@ -80,27 +91,85 @@ func PreflightCatalogProviderLogin(path, catalogID string) error { // 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. +// name == catalog id) AND a persisted row owns that very NAME under a different +// spelling. 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. +// +// Adoption deliberately does NOT follow the catalog id of an arbitrary row. +// Several profiles may legitimately share one catalog provider — {name: +// "work-xai", catalogId: "xai"} alongside a plain "xai" — so retargeting the +// default "xai" profile at whichever row happened to list catalogId "xai" would +// silently overwrite that row's endpoint, model, and stored key. An exactly +// spelled existing row also short-circuits: there is nothing to adopt, the +// caller's own spelling is already the persisted one. func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (ProviderProfile, error) { catalogID := strings.TrimSpace(profile.CatalogID) - if catalogID == "" || !strings.EqualFold(strings.TrimSpace(profile.Name), catalogID) { + name := strings.TrimSpace(profile.Name) + if catalogID == "" || !strings.EqualFold(name, catalogID) { return profile, nil } - canonical, owned, err := PersistedProviderIdentity(path, catalogID) + providers, err := persistedProviders(path) if err != nil { return profile, err } - if owned { + canonical := "" + variants := 0 + for _, row := range providers { + rowName := strings.TrimSpace(row.Name) + if !strings.EqualFold(rowName, name) { + continue + } + if rowName == name { + return profile, nil + } + canonical = rowName + variants++ + } + if variants == 1 { profile.Name = canonical } return profile, nil } +// PersistedProviderNames returns the exact name of every row in the persisted +// user config, in file order. Callers that must reason about the SET of saved +// rows — e.g. deciding whether removing one row leaves a case variant behind +// that still reads the same credential-store entry — get the raw names here +// rather than re-implementing FileConfig parsing. +func PersistedProviderNames(path string) ([]string, error) { + providers, err := persistedProviders(path) + if err != nil { + return nil, err + } + names := make([]string, 0, len(providers)) + for _, provider := range providers { + names = append(names, strings.TrimSpace(provider.Name)) + } + return names, nil +} + +// persistedProviders reads the provider rows out of the user config at path. +// A missing file is an empty list, not an error: every caller here asks "what +// is already saved?", and "nothing yet" is a legitimate answer. +func persistedProviders(path string) ([]ProviderProfile, error) { + data, err := os.ReadFile(strings.TrimSpace(path)) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + return cfg.Providers, 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. @@ -112,28 +181,114 @@ func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (Pr // 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) { + row, match, err := ResolvePersistedProviderIdentity(path, identity) + if err != nil || match == PersistedIdentityNone { + return "", false, err + } + return strings.TrimSpace(row.Name), true, nil +} + +// PersistedIdentityMatch says HOW a persisted row was addressed, because the +// two ways carry different authority. A name match identifies exactly one +// profile. A catalog-id match identifies "some profile of this kind" — several +// profiles may legitimately share one catalog provider — so callers that go on +// to delete credentials must not treat it as proof of ownership. +type PersistedIdentityMatch uint8 + +const ( + // PersistedIdentityNone means no row owns the identity, or a catalog id was + // given that more than one row claims (an ambiguous request this package + // refuses to guess at). + PersistedIdentityNone PersistedIdentityMatch = iota + // PersistedIdentityName means a row's own name matched, exactly or as a + // case variant. + PersistedIdentityName + // PersistedIdentityCatalog means the identity matched only the catalog id of + // exactly one row. + PersistedIdentityCatalog +) + +// ResolvePersistedProviderIdentity finds the persisted user-config row that +// owns identity and reports how it was addressed. +// +// Names win over catalog ids, and an exact name wins over a case variant. +// Scanning in file order and accepting the first name-OR-catalog hit picked +// whichever row came first, so `zero auth logout xai` against +// [{name:"work-xai", catalogId:"xai"}, {name:"xai"}] resolved to "work-xai" — +// a different profile, with different credentials, than the one named. +// +// A catalog id claimed by more than one row resolves to nothing rather than to +// an arbitrary winner: the caller asked for something the config does not +// uniquely identify, and guessing there is what let one profile's logout delete +// a sibling's shared token. +func ResolvePersistedProviderIdentity(path, identity string) (ProviderProfile, PersistedIdentityMatch, error) { identity = strings.TrimSpace(identity) if identity == "" { - return "", false, nil - } - data, err := os.ReadFile(strings.TrimSpace(path)) - if os.IsNotExist(err) { - return "", false, nil + return ProviderProfile{}, PersistedIdentityNone, nil } + providers, err := persistedProviders(path) if err != nil { - return "", false, fmt.Errorf("read config %s: %w", path, err) + return ProviderProfile{}, PersistedIdentityNone, err + } + var foldedName *ProviderProfile + var catalogRow *ProviderProfile + catalogMatches := 0 + for index := range providers { + row := providers[index] + name := strings.TrimSpace(row.Name) + if name == identity { + return row, PersistedIdentityName, nil + } + if foldedName == nil && strings.EqualFold(name, identity) { + match := row + foldedName = &match + } + if strings.EqualFold(strings.TrimSpace(row.CatalogID), identity) { + catalogMatches++ + if catalogRow == nil { + match := row + catalogRow = &match + } + } } - var cfg FileConfig - if err := json.Unmarshal(data, &cfg); err != nil { - return "", false, fmt.Errorf("invalid config JSON %s: %w", path, err) + if foldedName != nil { + return *foldedName, PersistedIdentityName, nil } - 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 + if catalogMatches == 1 { + return *catalogRow, PersistedIdentityCatalog, nil + } + return ProviderProfile{}, PersistedIdentityNone, nil +} + +// CatalogIdentityExclusive reports whether catalogID is claimed by the row +// named owner and by nothing else in the persisted config — neither as another +// row's name nor as another row's catalog id. +// +// Credential cleanup uses this before treating the catalog id as one of the +// target profile's own credential keys. With stored-key "work-xai", +// stored-key "xai", and keyless "personal-xai" all carrying catalogId "xai", +// the "xai" token and key belong to whoever logged in under that spelling — +// deleting them while logging out of "work-xai" takes down a sibling's login. +func CatalogIdentityExclusive(path, catalogID, owner string) (bool, error) { + catalogID = strings.TrimSpace(catalogID) + owner = strings.TrimSpace(owner) + if catalogID == "" || owner == "" { + return false, nil + } + providers, err := persistedProviders(path) + if err != nil { + return false, err + } + for _, row := range providers { + name := strings.TrimSpace(row.Name) + if name == owner { + continue + } + if strings.EqualFold(name, catalogID) || strings.EqualFold(strings.TrimSpace(row.CatalogID), catalogID) { + return false, nil } } - return "", false, nil + return true, nil } // PreflightProviderWrite also rejects a new spelling that would share a diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 7ad84b4c7..1f336fa1b 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -1233,3 +1233,161 @@ func TestEditProviderRejectsCollisionAndUnknown(t *testing.T) { t.Fatalf("config was rewritten by a rejected edit") } } + +// TestValidatePersistedProviderNamesRejectsExactDuplicates covers jatmn's #725 +// finding: the validator only rejected a repeated folded name when the +// SPELLINGS differed, so two rows literally named "work" passed. That breaks +// the same one-credential-per-folded-name invariant the case check protects — +// resolver merging coalesces the rows, and plaintext-key migration writes both +// values into one normalized credential-store entry, overwriting the first key. +func TestValidatePersistedProviderNamesRejectsExactDuplicates(t *testing.T) { + for name, providers := range map[string][]ProviderProfile{ + "identical spellings": {{Name: "work"}, {Name: "work"}}, + "same after trimming": {{Name: "work"}, {Name: " work "}}, + } { + t.Run(name, func(t *testing.T) { + err := ValidatePersistedProviderNames(FileConfig{Providers: providers}) + if err == nil { + t.Fatal("a repeated folded provider identity must be rejected") + } + if want := `duplicate persisted provider name "work"`; !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want it to contain %q", err, want) + } + }) + } + if err := ValidatePersistedProviderNames(FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "fast"}}}); err != nil { + t.Fatalf("distinct names must validate: %v", err) + } +} + +// TestAdoptPersistedCatalogProviderNameIgnoresCatalogSiblings covers jatmn's +// #725 finding: adoption followed PersistedProviderIdentity, which returned the +// first row sharing the catalog id. Several profiles may legitimately use one +// catalog provider, so adding the default "xai" profile silently retargeted an +// existing {name:"work-xai", catalogId:"xai"} row and would have overwritten its +// endpoint, model, and stored key. +func TestAdoptPersistedCatalogProviderNameIgnoresCatalogSiblings(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v1", Model: "m"}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "xai", CatalogID: "xai"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "xai" { + t.Fatalf("Name = %q, want the default spelling kept — a catalog sibling is not the same profile", adopted.Name) + } +} + +// TestAdoptPersistedCatalogProviderNameFollowsCaseVariantOfTheDefaultName is the +// behaviour adoption exists for: a re-setup of "openrouter" against a row the +// user saved as "OpenRouter" must UPDATE that row instead of colliding with it. +func TestAdoptPersistedCatalogProviderNameFollowsCaseVariantOfTheDefaultName(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "OpenRouter", CatalogID: "openrouter", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://openrouter.ai/api/v1", Model: "m"}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "openrouter", CatalogID: "openrouter"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "OpenRouter" { + t.Fatalf("Name = %q, want the persisted case variant adopted", adopted.Name) + } +} + +// TestResolvePersistedProviderIdentityPrefersNames covers jatmn's #725 finding +// that identity resolution took the first row matching EITHER field, so a +// catalog id on an earlier row outranked a later row with the exact name. +func TestResolvePersistedProviderIdentityPrefersNames(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai"}, + {Name: "xai", CatalogID: "xai"}, + }, + }, 0o600) + + t.Run("exact name beats an earlier catalog id", func(t *testing.T) { + row, match, err := ResolvePersistedProviderIdentity(path, "xai") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if match != PersistedIdentityName || row.Name != "xai" { + t.Fatalf("row = %q match = %v, want the exactly named row", row.Name, match) + } + }) + + t.Run("a shared catalog id resolves to nothing", func(t *testing.T) { + _, match, err := ResolvePersistedProviderIdentity(path, "XAI") + if err != nil { + t.Fatalf("resolve: %v", err) + } + // "XAI" folds to the "xai" row's NAME, so that wins; the point of the + // exclusivity rule shows on a catalog id nothing is named after. + if match != PersistedIdentityName { + t.Fatalf("match = %v, want the case-variant name match", match) + } + }) + + t.Run("unique catalog id still resolves", func(t *testing.T) { + unique := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, unique, FileConfig{ + Providers: []ProviderProfile{{Name: "my-router", CatalogID: "openrouter"}}, + }, 0o600) + row, match, err := ResolvePersistedProviderIdentity(unique, "openrouter") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if match != PersistedIdentityCatalog || row.Name != "my-router" { + t.Fatalf("row = %q match = %v, want the sole catalog owner", row.Name, match) + } + }) + + t.Run("an ambiguous catalog id resolves to nothing", func(t *testing.T) { + shared := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, shared, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai"}, + {Name: "personal-xai", CatalogID: "xai"}, + }, + }, 0o600) + if _, match, err := ResolvePersistedProviderIdentity(shared, "xai"); err != nil || match != PersistedIdentityNone { + t.Fatalf("match = %v err = %v, want no guess at a shared catalog id", match, err) + } + }) +} + +// TestCatalogIdentityExclusive guards the rule credential cleanup depends on: +// a catalog id claimed by any other row is not the target profile's own key. +func TestCatalogIdentityExclusive(t *testing.T) { + shared := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, shared, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai"}, + {Name: "xai", CatalogID: "xai"}, + {Name: "personal-xai", CatalogID: "xai"}, + }, + }, 0o600) + if exclusive, err := CatalogIdentityExclusive(shared, "xai", "work-xai"); err != nil || exclusive { + t.Fatalf("exclusive = %v err = %v, want false for a catalog id three rows claim", exclusive, err) + } + + sole := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, sole, FileConfig{ + Providers: []ProviderProfile{ + {Name: "my-router", CatalogID: "openrouter"}, + {Name: "work", CatalogID: "xai"}, + }, + }, 0o600) + if exclusive, err := CatalogIdentityExclusive(sole, "openrouter", "my-router"); err != nil || !exclusive { + t.Fatalf("exclusive = %v err = %v, want true when only the owner claims the id", exclusive, err) + } +} diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 96530dbc5..428c8605a 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -265,5 +265,16 @@ func (s *Store) write(data map[string]string) error { } func normalizeProvider(provider string) string { + return NormalizeProvider(provider) +} + +// NormalizeProvider is the credential-store's provider-name equivalence rule: +// entries are keyed by the trimmed, lowercased name. Callers that decide +// whether two provider spellings share one stored secret (e.g. removing a +// case-variant row while a sibling survives) must compare with THIS function +// rather than strings.EqualFold — the two relations are not the same. Unicode +// case folding equates "s" and "ſ", strings.ToLower does not, so an EqualFold +// comparison can promise a survivor access to a key it cannot look up. +func NormalizeProvider(provider string) string { return strings.ToLower(strings.TrimSpace(provider)) } diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 3b2393a61..1105a48f8 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -16,6 +16,7 @@ import ( "charm.land/lipgloss/v2" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/credstore" "github.com/Gitlawb/zero/internal/oauth" ) @@ -442,13 +443,18 @@ func removeSavedProvider(saved []config.ProviderProfile, name string) []config.P } // caseVariantProviderNameAfterRemoval returns the exact name of a row in cfg -// that folds to name (a case-only variant surviving the removal), and -// whether one was found. +// that shares name's CREDENTIAL-STORE identity (a case-only variant surviving +// the removal that reads the same stored secret), and whether one was found. +// +// Matching uses credstore.NormalizeProvider, the store's own equivalence rule, +// for the reason spelled out on the CLI twin (caseVariantProviderName in +// internal/cli/provider_onboarding.go): strings.EqualFold is a strictly wider +// relation and would promise a survivor a key it cannot look up. func caseVariantProviderNameAfterRemoval(cfg config.FileConfig, name string) (string, bool) { - name = strings.TrimSpace(name) + target := credstore.NormalizeProvider(name) for _, provider := range cfg.Providers { providerName := strings.TrimSpace(provider.Name) - if strings.EqualFold(providerName, name) { + if credstore.NormalizeProvider(providerName) == target { return providerName, true } } From 6a708cc975abb20978c72ba56d333e10a1839eae Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 1 Aug 2026 19:42:21 +0000 Subject: [PATCH 16/17] fix(providers): preserve distinct credential identities Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-b9dc-73a7-9676-675cf9b4d2c7 Co-authored-by: Pierre Bruno --- internal/config/credentials.go | 14 +++---- internal/config/credentials_test.go | 20 ++++++++++ internal/config/writer.go | 3 +- internal/tui/provider_manager.go | 7 ++-- internal/tui/provider_manager_test.go | 57 +++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 11 deletions(-) diff --git a/internal/config/credentials.go b/internal/config/credentials.go index 47596aa35..d4e5d6816 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -111,12 +111,11 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { } // ClearProviderKeyStoredCaseVariants unsets the APIKeyStored marker on every -// row whose name folds to provider, not just an exact-spelling match. The -// credential store keys its secrets case-folded, so deleting the shared -// secret for one case-variant row (e.g. "WORK") must also clear the marker -// on any sibling row ("work") that pointed at the same now-gone entry — -// leaving it set would claim a key is available when ApplyStoredAPIKey's -// store lookup will always miss. +// row whose name normalizes to the same credential-store identity as provider, +// not just an exact-spelling match. Deleting the shared secret for one +// case-variant row (e.g. "WORK") must also clear the marker on any sibling row +// ("work") that pointed at the same now-gone entry — leaving it set would claim +// a key is available when ApplyStoredAPIKey's store lookup will always miss. func ClearProviderKeyStoredCaseVariants(path, provider string) (bool, error) { path = strings.TrimSpace(path) provider = strings.TrimSpace(provider) @@ -135,8 +134,9 @@ func ClearProviderKeyStoredCaseVariants(path, provider string) (bool, error) { return false, fmt.Errorf("invalid config JSON %s: %w", path, err) } changed := false + providerIdentity := credstore.NormalizeProvider(provider) for index := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), provider) && cfg.Providers[index].APIKeyStored { + if credstore.NormalizeProvider(cfg.Providers[index].Name) == providerIdentity && 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 194564642..6c1fcebf6 100644 --- a/internal/config/credentials_test.go +++ b/internal/config/credentials_test.go @@ -169,6 +169,26 @@ func TestClearProviderKeyStored(t *testing.T) { } } +func TestClearProviderKeyStoredCaseVariantsPreservesDistinctUnicodeIdentity(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"s","apiKeyStored":true},{"name":"ſ","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + + cleared, err := ClearProviderKeyStoredCaseVariants(path, "s") + if err != nil || !cleared { + t.Fatalf("clear = %v,%v; want true,nil", cleared, err) + } + cfg := readConfigFixture(t, path) + if cfg.Providers[0].APIKeyStored { + t.Fatal("s marker should be cleared") + } + if !cfg.Providers[1].APIKeyStored { + t.Fatal("long-s marker belongs to a distinct credential-store identity and must remain set") + } +} + func TestProviderProfileAPIKeyStoredRoundTrips(t *testing.T) { // The apiKeyStored marker survives JSON decode (custom UnmarshalJSON). var p ProviderProfile diff --git a/internal/config/writer.go b/internal/config/writer.go index 8965d0e6b..0ead60926 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -799,13 +799,14 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { } index := -1 + newIdentity := credstore.NormalizeProvider(newName) for i, provider := range cfg.Providers { providerName := strings.TrimSpace(provider.Name) if providerName == oldName { index = i continue } - if strings.EqualFold(providerName, newName) { + if credstore.NormalizeProvider(providerName) == newIdentity { return FileConfig{}, fmt.Errorf("provider %q already exists", newName) } } diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 1105a48f8..78aa868a7 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -698,7 +698,7 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { // Keep the live session's identity in sync with a rename of the provider it // is running on: the exported ZERO_PROVIDER must resolve for spawned children. - if strings.EqualFold(strings.TrimSpace(m.providerName), oldName) { + if strings.TrimSpace(m.providerName) == oldName { m.providerName = newName m.providerProfile.Name = newName config.SetActiveProviderEnv(newName) @@ -716,7 +716,7 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { // liveName is the session's provider AFTER any rename sync, so a single // comparison against the edited profile's final name suffices. func providerEditRestartNote(liveName string, editedName string) string { - if strings.EqualFold(strings.TrimSpace(liveName), strings.TrimSpace(editedName)) { + if strings.TrimSpace(liveName) == strings.TrimSpace(editedName) { return " Press Enter on it to apply the changes to this session." } return "" @@ -725,8 +725,9 @@ func providerEditRestartNote(liveName string, editedName string) string { // applySavedProviderEdit mirrors a persisted config.EditProvider into the // in-memory saved list without wholesale replacement (see saveManagerEdit). func applySavedProviderEdit(saved []config.ProviderProfile, oldName string, edit config.ProviderEdit) []config.ProviderProfile { + oldName = strings.TrimSpace(oldName) for index := range saved { - if !strings.EqualFold(strings.TrimSpace(saved[index].Name), strings.TrimSpace(oldName)) { + if strings.TrimSpace(saved[index].Name) != oldName { continue } profile := &saved[index] diff --git a/internal/tui/provider_manager_test.go b/internal/tui/provider_manager_test.go index 77c12b500..fdc40054f 100644 --- a/internal/tui/provider_manager_test.go +++ b/internal/tui/provider_manager_test.go @@ -284,6 +284,63 @@ func TestProviderManagerRenameFollowsLiveSession(t *testing.T) { } } +func TestProviderManagerEditKeepsDistinctUnicodeIdentityOutOfLiveSession(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(home, "oauth-tokens.json")) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + t.Setenv(config.ActiveProviderEnv, "s") + configPath := filepath.Join(t.TempDir(), "config.json") + seed := config.FileConfig{ + ActiveProvider: "s", + Providers: []config.ProviderProfile{ + {Name: "s", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://s.example.com/v1", Model: "s-model"}, + {Name: "ſ", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://long-s.example.com/v1", Model: "long-s-model"}, + }, + } + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatal(err) + } + + m := newModel(context.Background(), Options{ + ProviderName: "s", + ModelName: "s-model", + Provider: &fakeProvider{}, + ProviderProfile: seed.Providers[0], + SavedProviders: seed.Providers, + UserConfigPath: configPath, + }) + m.providerWizard = &providerWizardState{ + step: providerWizardStepEditMenu, + editOriginal: seed.Providers[1], + editDraft: config.ProviderProfile{ + Name: "ſ", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://long-s.example.com/v1", + Model: "edited-long-s-model", + }, + } + + next, _ := m.saveManagerEdit() + persisted := readManagerConfig(t, configPath) + if persisted.Providers[0].Model != "s-model" || persisted.Providers[1].Model != "edited-long-s-model" { + t.Fatalf("persisted edit targeted wrong identity: %+v", persisted.Providers) + } + if next.savedProviders[0].Model != "s-model" || next.savedProviders[1].Model != "edited-long-s-model" { + t.Fatalf("in-memory edit targeted wrong identity: %+v", next.savedProviders) + } + if next.providerName != "s" || next.providerProfile.Name != "s" || os.Getenv(config.ActiveProviderEnv) != "s" { + t.Fatalf("unrelated live session changed: provider=%q profile=%q env=%q", next.providerName, next.providerProfile.Name, os.Getenv(config.ActiveProviderEnv)) + } + if strings.Contains(next.providerWizard.manageStatus, "Press Enter") { + t.Fatalf("unrelated edit produced live-session restart note: %q", next.providerWizard.manageStatus) + } +} + func TestProviderManagerEscWalksBackThenCloses(t *testing.T) { m := managerTestModel(t) m = managerKey(t, m, testKeyText("e")) From 0279ed7936a8aa13f318ab77591265c5ed67cf94 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 1 Aug 2026 23:17:00 +0200 Subject: [PATCH 17/17] fix(providers): make one identity rule govern every persisted write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR introduced a contract where the credential store's own rule — credstore.NormalizeProvider — decides provider identity, which makes "s" and Unicode long-s "s" separate profiles with separate secrets. Several paths were still comparing with strings.EqualFold, which folds those two together, so the two rules disagreed in both directions: destructive logout expansion adopted an unrelated row's credentials, while ordinary adds rejected the supported pair as a collision. sameProviderIdentity is now the single comparison behind identity resolution, catalog-ownership checks, write preflight, upsert, rename, removal's active-provider handoff, and the stored-key migration's same-entry check. A folded NAME match is also no longer proof of ownership on its own. AdoptPersistedCatalogProviderName required only that a case-variant row carry the requested name, so `zero providers add openrouter` adopted a custom profile saved as {name: "OpenRouter", catalogId: "custom-openai-compatible"} and handed it to UpsertProvider, whose merge overwrites catalog id, endpoint, model, transport and headers while a stored-key marker survives. The row's catalog identity must now agree; a row declaring none is still adopted, since its name is the only identity it has. The TUI's delete also committed its retain-the-shared-key decision before the marker handoff that makes retention safe. The handoff can only run after RemoveProvider (an ambiguous two-row config rejects every write), so its failure is reachable: the key stayed in the store with no row marked to read it while the UI reported a completed "Deleted ." It now reports an incomplete delete naming the survivor and the recovery step, and keeps the secret rather than destroying it over a failed config write — matching `zero providers remove`. Co-Authored-By: Claude Opus 5 (1M context) --- internal/cli/auth_test.go | 53 +++++++++++++++ internal/config/writer.go | 59 ++++++++++++----- internal/config/writer_test.go | 92 +++++++++++++++++++++++++++ internal/tui/provider_manager.go | 26 +++++++- internal/tui/provider_manager_test.go | 79 +++++++++++++++++++++++ 5 files changed, 291 insertions(+), 18 deletions(-) diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 02ffd46e7..108fc8d3a 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -377,6 +377,59 @@ func TestRunAuthLogoutDeletesCatalogIDAPIKey(t *testing.T) { } } +// TestRunAuthLogoutKeepsDistinctUnicodeCredentials pins the end-to-end +// guarantee behind jatmn's #725 finding that destructive candidate expansion +// used strings.EqualFold as authority for credential ownership: a saved "s" +// profile with its own token and key must survive `zero auth logout ſ`, which +// names a provider the config never saved (the credential store defines +// identity with credstore.NormalizeProvider, under which "s" and Unicode +// long-s "ſ" are separate entries). +// +// Two independent layers now enforce that, and this test deliberately asserts +// the outcome rather than either mechanism. The one that fires first is +// oauth.ValidateKey, which rejects the non-ASCII spelling before any deletion +// runs — so the folded-name adoption itself is pinned where it is reachable, in +// TestPersistedProviderIdentityRulesMatchTheCredentialStore (internal/config). +func TestRunAuthLogoutKeepsDistinctUnicodeCredentials(t *testing.T) { + const longS = "ſ" + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"s","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + tokens, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := tokens.Save(oauth.ProviderKey("s"), 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("s", "long-s-is-not-s"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + runWithDeps([]string{"auth", "logout", longS}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + + if _, ok, err := tokens.Load(oauth.ProviderKey("s")); err != nil || !ok { + t.Fatalf("logging out of a distinct identity deleted the saved token: ok=%v err=%v", ok, err) + } + if key, ok, err := keyStore.Get("s"); err != nil || !ok || key != "long-s-is-not-s" { + t.Fatalf("stored API key = %q, %v, %v; want the unrelated profile untouched", key, ok, err) + } + saved := readCLIConfigFixture(t, configPath).Providers + if len(saved) != 1 || saved[0].Name != "s" || !saved[0].APIKeyStored { + t.Fatalf("providers = %+v, want the saved profile keeping its stored-key marker", saved) + } +} + // TestRunAuthLogoutResolvesCandidatesDespiteUnrelatedAmbiguousConfig covers // jatmn's third #725 follow-up finding: identity resolution and OAuth/API-key // candidate expansion were gated on PreflightUserConfig succeeding, even diff --git a/internal/config/writer.go b/internal/config/writer.go index 0ead60926..8b2576f49 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -40,6 +40,17 @@ func ValidatePersistedProviderNames(cfg FileConfig) error { return nil } +// sameProviderIdentity reports whether two persisted spellings name the same +// provider identity. It is credstore.NormalizeProvider — the credential store's +// own rule — rather than strings.EqualFold, because the two disagree and the +// store is the authority: EqualFold folds "s" and Unicode long-s "ſ" together, +// while the store keeps separate entries for them. Treating them as one identity +// let a mutation of one profile reach the other's row and its secret, which is +// precisely what ValidatePersistedProviderNames permits as a distinct pair. +func sameProviderIdentity(a string, b string) bool { + return credstore.NormalizeProvider(a) == credstore.NormalizeProvider(b) +} + // PreflightUserConfig validates existing user config before any command makes // credential-store side effects. func PreflightUserConfig(path string) error { @@ -106,10 +117,21 @@ func PreflightCatalogProviderLogin(path, catalogID string) error { // silently overwrite that row's endpoint, model, and stored key. An exactly // spelled existing row also short-circuits: there is nothing to adopt, the // caller's own spelling is already the persisted one. +// +// Nor is a matching NAME on its own proof of ownership. A custom profile is free +// to be called {name: "OpenRouter", catalogId: "custom-openai-compatible", +// baseURL: "https://corp.example/v1"}; adopting it for `zero providers add +// openrouter` would hand that row to UpsertProvider, whose merge overwrites the +// catalog id, endpoint, model, transport and headers with OpenRouter's defaults +// while a stored-key marker survives the rewrite. So the row's catalog identity +// must agree too, and a row that declares no catalog id at all is accepted only +// because its name is the sole identity it has (the legacy shape this function +// exists for). Anything else keeps the requested name and lets the write report +// the collision instead of mutating a different profile. func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (ProviderProfile, error) { catalogID := strings.TrimSpace(profile.CatalogID) name := strings.TrimSpace(profile.Name) - if catalogID == "" || !strings.EqualFold(name, catalogID) { + if catalogID == "" || !sameProviderIdentity(name, catalogID) { return profile, nil } providers, err := persistedProviders(path) @@ -120,12 +142,17 @@ func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (Pr variants := 0 for _, row := range providers { rowName := strings.TrimSpace(row.Name) - if !strings.EqualFold(rowName, name) { + if !sameProviderIdentity(rowName, name) { continue } if rowName == name { return profile, nil } + if rowCatalogID := strings.TrimSpace(row.CatalogID); rowCatalogID != "" && + !sameProviderIdentity(rowCatalogID, catalogID) { + // A different provider that happens to carry this name. + return profile, nil + } canonical = rowName variants++ } @@ -239,11 +266,11 @@ func ResolvePersistedProviderIdentity(path, identity string) (ProviderProfile, P if name == identity { return row, PersistedIdentityName, nil } - if foldedName == nil && strings.EqualFold(name, identity) { + if foldedName == nil && sameProviderIdentity(name, identity) { match := row foldedName = &match } - if strings.EqualFold(strings.TrimSpace(row.CatalogID), identity) { + if sameProviderIdentity(row.CatalogID, identity) { catalogMatches++ if catalogRow == nil { match := row @@ -284,7 +311,7 @@ func CatalogIdentityExclusive(path, catalogID, owner string) (bool, error) { if name == owner { continue } - if strings.EqualFold(name, catalogID) || strings.EqualFold(strings.TrimSpace(row.CatalogID), catalogID) { + if sameProviderIdentity(name, catalogID) || sameProviderIdentity(row.CatalogID, catalogID) { return false, nil } } @@ -311,7 +338,7 @@ func PreflightProviderWrite(path, name string) error { name = strings.TrimSpace(name) for _, provider := range cfg.Providers { existing := strings.TrimSpace(provider.Name) - if strings.EqualFold(existing, name) && existing != name { + if sameProviderIdentity(existing, name) && existing != name { return fmt.Errorf("provider %q already exists as %q; provider names must be unique case-insensitively", name, existing) } } @@ -340,7 +367,7 @@ func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileC return FileConfig{}, err } for _, existing := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(existing.Name), profile.Name) && strings.TrimSpace(existing.Name) != profile.Name { + if sameProviderIdentity(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) } } @@ -406,8 +433,8 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro return EnsuredProvider{}, fmt.Errorf("read config %s: %w", path, err) } for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.CatalogID), descriptor.ID) || - strings.EqualFold(strings.TrimSpace(provider.Name), descriptor.ID) { + if sameProviderIdentity(provider.CatalogID, descriptor.ID) || + sameProviderIdentity(provider.Name, descriptor.ID) { return EnsuredProvider{Name: provider.Name, Active: cfg.ActiveProvider}, nil } } @@ -600,7 +627,7 @@ func ProviderPersistedCaseInsensitive(path, name string) (bool, error) { 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)) { + if sameProviderIdentity(provider.Name, name) { return true, nil } } @@ -654,7 +681,7 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if providerName == strings.TrimSpace(cfg.ActiveProvider) { activeIndex = i } - if strings.EqualFold(providerName, strings.TrimSpace(cfg.ActiveProvider)) { + if sameProviderIdentity(providerName, cfg.ActiveProvider) { activeFoldedIndex = i activeFoldedMatches++ } @@ -714,14 +741,14 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er index = i continue } - if strings.EqualFold(providerName, newName) { + if sameProviderIdentity(providerName, newName) { return FileConfig{}, fmt.Errorf("provider %q already exists", newName) } } if index < 0 { return FileConfig{}, fmt.Errorf("provider %q not found", oldName) } - if strings.EqualFold(oldName, newName) && cfg.Providers[index].Name == newName { + if sameProviderIdentity(oldName, newName) && cfg.Providers[index].Name == newName { return cfg, nil } @@ -733,7 +760,7 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er } keyMigrated = true } - if strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { + if sameProviderIdentity(cfg.ActiveProvider, previousName) { cfg.ActiveProvider = newName } cfg.Providers[index].Name = newName @@ -829,7 +856,7 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { } keyMigrated = true } - if renamed && strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { + if renamed && sameProviderIdentity(cfg.ActiveProvider, previousName) { cfg.ActiveProvider = newName } @@ -869,7 +896,7 @@ func migrateStoredProviderKey(configPath string, oldName string, newName string) // (groq -> Groq) targets ONE entry: Set(new) rewrites it in place and // Delete(old) would then remove the key that was just "moved". Nothing to // migrate — the existing entry already serves the new name. - if strings.EqualFold(strings.TrimSpace(oldName), strings.TrimSpace(newName)) { + if sameProviderIdentity(oldName, newName) { return nil } store, err := ProviderKeyStoreAt(filepath.Dir(configPath)) diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 1f336fa1b..83ff1b745 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -1303,6 +1303,98 @@ func TestAdoptPersistedCatalogProviderNameFollowsCaseVariantOfTheDefaultName(t * } } +// TestAdoptPersistedCatalogProviderNameIgnoresForeignCatalogRow covers jatmn's +// #725 finding that a case-folded NAME match was taken as proof of ownership. +// A custom profile may legitimately be called "OpenRouter" while pointing at a +// different provider entirely; adopting it for `zero providers add openrouter` +// handed that row to UpsertProvider, whose merge overwrites the catalog id, +// endpoint, model and transport with OpenRouter's defaults while a stored-key +// marker survives the rewrite. +func TestAdoptPersistedCatalogProviderNameIgnoresForeignCatalogRow(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "OpenRouter", CatalogID: "custom-openai-compatible", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://corp.example/v1", Model: "corp-model", APIKeyStored: true}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "openrouter", CatalogID: "openrouter"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "openrouter" { + t.Fatalf("Name = %q, want the requested spelling kept — a differently-catalogued row is another provider", adopted.Name) + } + // And the write that follows must report the collision rather than rewriting + // the custom row in place. + if err := PreflightProviderWrite(path, adopted.Name); err == nil { + t.Fatal("PreflightProviderWrite accepted a name that collides with the custom row; want a reported collision") + } +} + +// TestAdoptPersistedCatalogProviderNameFollowsNameOnlyRow keeps the legacy shape +// adoption exists for: a row whose only identity is its name. +func TestAdoptPersistedCatalogProviderNameFollowsNameOnlyRow(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "OpenRouter", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://openrouter.ai/api/v1", Model: "m"}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "openrouter", CatalogID: "openrouter"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "OpenRouter" { + t.Fatalf("Name = %q, want the persisted case variant adopted", adopted.Name) + } +} + +// TestPersistedProviderIdentityRulesMatchTheCredentialStore pins the identity +// contract this PR introduced across every persisted-config path at once. +// strings.EqualFold folds "s" and Unicode long-s "ſ" together while +// credstore.NormalizeProvider (the store's own rule, and the rule +// ValidatePersistedProviderNames enforces) keeps them apart, so the two +// spellings are separate profiles with separate secrets. Mixing the two +// comparisons made one profile's mutation reach the other's row: destructive +// logout expansion adopted the unrelated row, while ordinary writes rejected +// the pair as a collision. +func TestPersistedProviderIdentityRulesMatchTheCredentialStore(t *testing.T) { + const longS = "ſ" + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "s", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://s.example/v1", Model: "m", APIKeyStored: true}, + }, + }, 0o600) + + t.Run("identity resolution does not adopt the distinct spelling", func(t *testing.T) { + if _, match, err := ResolvePersistedProviderIdentity(path, longS); err != nil || match != PersistedIdentityNone { + t.Fatalf("ResolvePersistedProviderIdentity(%q) = %v, %v; want no match", longS, match, err) + } + }) + + t.Run("a distinct spelling is writable, not a collision", func(t *testing.T) { + if err := PreflightProviderWrite(path, longS); err != nil { + t.Fatalf("PreflightProviderWrite(%q) = %v; want the distinct identity accepted", longS, err) + } + cfg, err := UpsertProvider(path, ProviderProfile{Name: longS, ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://long-s.example/v1", Model: "m"}, false) + if err != nil { + t.Fatalf("UpsertProvider(%q) = %v; want the distinct identity accepted", longS, err) + } + if len(cfg.Providers) != 2 { + t.Fatalf("providers = %+v, want both distinct rows saved", cfg.Providers) + } + }) + + t.Run("a case variant is still a collision", func(t *testing.T) { + if err := PreflightProviderWrite(path, "S"); err == nil { + t.Fatal("PreflightProviderWrite(\"S\") accepted a case variant of a saved row") + } + }) +} + // TestResolvePersistedProviderIdentityPrefersNames covers jatmn's #725 finding // that identity resolution took the first row matching EITHER field, so a // catalog id on an earlier row outranked a later row with the exact name. diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 78aa868a7..bf5459884 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -329,6 +329,13 @@ func (m model) activateManagerSelection() (model, tea.Cmd) { return next, cmd } +// transferProviderAPIKeyStoredMarker is a package var so a test can drive the +// partial-failure path: the handoff runs AFTER RemoveProvider has committed +// (while both case-variant rows are present the config is ambiguous and every +// write against it is rejected), so its failure is a real state the UI must +// report rather than an unreachable branch. +var transferProviderAPIKeyStoredMarker = config.TransferProviderAPIKeyStoredMarker + // deleteManagerSelection removes the confirmed provider: the config write runs // synchronously (the list must reflect the config the instant the confirm // resolves), while the stored-key delete and the OAuth-login lookup — a @@ -368,14 +375,29 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { retainStoredKey := false markerTransferredTo := "" if survivor, ok := caseVariantProviderNameAfterRemoval(cfg, name); ok { + // A surviving case variant reads the same credential-store entry, so + // the shared key must not be deleted — provided that row can still + // REACH it. ApplyStoredAPIKey consults the store only for a row + // carrying apiKeyStored, so retention is only safe once the marker + // is where the survivor can use it. retainStoredKey = true // The removed row owned the shared secret's marker; carry it over to // the surviving case-variant so it stays reachable, mirroring the CLI // (`zero providers remove`) fix for the same case. if row.profile.APIKeyStored { if survivorRow, foundSurvivor, err := config.ProviderRow(m.userConfigPath, survivor); err == nil && foundSurvivor && !survivorRow.APIKeyStored { - if _, err := config.TransferProviderAPIKeyStoredMarker(m.userConfigPath, survivor); err != nil { - notes = append(notes, "Warning: could not keep its stored API key reachable ("+err.Error()+").") + if _, err := transferProviderAPIKeyStoredMarker(m.userConfigPath, survivor); err != nil { + // The row is already gone and the handoff failed, so the + // key is in the store with no profile marked to read it. + // Keep it rather than destroying a secret because a config + // write failed — but say so, and do not report this as a + // completed delete. Re-saving a key for the survivor (or + // restoring its apiKeyStored marker) makes it reachable again. + retainStoredKey = true + notes = []string{ + "Removed " + name + ", but the delete is incomplete: its stored API key marker could not be handed to " + survivor + " (" + err.Error() + ").", + "The shared key is still in the credential store and " + survivor + " cannot reach it until you set a key for it again.", + } } else { markerTransferredTo = survivor } diff --git a/internal/tui/provider_manager_test.go b/internal/tui/provider_manager_test.go index fdc40054f..915fb56fb 100644 --- a/internal/tui/provider_manager_test.go +++ b/internal/tui/provider_manager_test.go @@ -3,6 +3,7 @@ package tui import ( "context" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -219,6 +220,84 @@ func TestProviderManagerDeleteTransfersKeyMarkerToSurvivingCaseVariant(t *testin } } +// TestProviderManagerDeleteReportsFailedKeyMarkerHandoff covers jatmn's #725 +// finding that the manager committed the retain-the-key decision before it knew +// the handoff had worked. The marker transfer can only run AFTER RemoveProvider +// (while both case-variant rows exist the config is ambiguous and every write +// against it is rejected), so its failure is reachable: the shared secret then +// sits in the store with no row marked to read it, while the UI had already +// reported a completed "Deleted ." +func TestProviderManagerDeleteReportsFailedKeyMarkerHandoff(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(home, "oauth-tokens.json")) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + seed := config.FileConfig{ + ActiveProvider: "WORK", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v1", APIKeyStored: true, Model: "work-model"}, + {Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v2", Model: "work-model-2"}, + }, + } + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatalf("encode seed config: %v", err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatalf("write seed config: %v", err) + } + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "shared-key"); err != nil { + t.Fatal(err) + } + original := transferProviderAPIKeyStoredMarker + transferProviderAPIKeyStoredMarker = func(string, string) (bool, error) { + return false, errors.New("injected marker write failure") + } + t.Cleanup(func() { transferProviderAPIKeyStoredMarker = original }) + + m := newModel(context.Background(), Options{ + ProviderName: "WORK", + ModelName: "work-model-2", + Provider: &fakeProvider{}, + ProviderProfile: seed.Providers[1], + SavedProviders: seed.Providers, + UserConfigPath: configPath, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m.width = 120 + m.height = 40 + m, _ = m.openProviderManager() + + m = managerKey(t, m, testKeyText("d")) // select "work" (row 0) + next, cmd := m.handleProviderWizardKey(testKeyText("y")) + next = drainProviderManagerCmds(t, next, cmd) + + status := next.providerWizard.manageStatus + if strings.Contains(status, "Deleted work.") { + t.Fatalf("status = %q, must not report a completed delete while the shared key is unreachable", status) + } + if !strings.Contains(status, "incomplete") || !strings.Contains(status, "WORK") { + t.Fatalf("status = %q, want it to name the incomplete handoff and the survivor", status) + } + // The secret is kept rather than destroyed by a failed config write: it is + // recoverable by setting a key for the survivor again, whereas deleting it + // is not. + if key, ok, err := store.Get("work"); err != nil || !ok || key != "shared-key" { + t.Fatalf("shared provider key = %q, %v, %v; want it preserved for recovery", key, ok, err) + } + persisted := readManagerConfig(t, next.userConfigPath) + if len(persisted.Providers) != 1 || persisted.Providers[0].Name != "WORK" { + t.Fatalf("providers = %+v, want only the survivor", persisted.Providers) + } +} + func TestProviderManagerEditModelPersists(t *testing.T) { m := managerTestModel(t) m = managerKey(t, m, testKeyText("e"))