From 4b260698dda43a9c714d970ab802c4236d20220a Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 17 Jul 2026 22:30:04 +0000 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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.