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/command_center.go b/internal/cli/command_center.go index a6fab33ec..a8f893101 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,24 @@ type providerSummary = zerocommands.ProviderSnapshot type modelSummary = zerocommands.ModelSnapshot type providerCatalogSummary = zerocommands.ProviderCatalogSnapshot +// providerSourceUserConfig marks a profile saved in the user config file — the +// only place `zero providers use` can switch, since it writes that file. +// providerSourceResolved covers every other way a profile reaches the resolved +// config: project config, a provider command, or a profile synthesized from an +// ambient env var. Those are deliberately NOT called "runtime": some of them are +// persisted, just not where `providers use` writes, so the only guarantee the +// label carries is that the entry cannot be selected with that command. +const ( + providerSourceUserConfig = "user-config" + providerSourceResolved = "resolved" +) + +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 +145,34 @@ 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 { + // 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 + } + 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 +190,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.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:] @@ -325,7 +387,18 @@ func formatConfigSummary(summary configSummary) string { return strings.Join(lines, "\n") } +// formatProviderSummaries renders a list whose selectability was not computed +// (the `zero config` summary and tests), so every entry reads as an ordinary +// saved profile and no misleading marker appears. 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: providerSourceUserConfig}) + } + return formatProviderCLISummaries(command, cliProviders) +} + +func formatProviderCLISummaries(command string, providers []providerCLISummary) string { title := "Providers" if command == "current" { title = "Provider" @@ -343,24 +416,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: providerSourceUserConfig}) +} + +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 += " (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 430468c9f..e07318178 100644 --- a/internal/cli/command_center_test.go +++ b/internal/cli/command_center_test.go @@ -191,6 +191,96 @@ 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 != "resolved" || !payload.Providers[1].Selectable || payload.Providers[1].Source != "user-config" { + t.Fatalf("unexpected providers: %#v", payload.Providers) + } + + 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(), "not selectable via providers use") { + t.Fatalf("non-selectable marker missing: %s", stdout.String()) + } +} + +// 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 != "user-config" { + t.Fatalf("exact-case persisted entry should be selectable: %#v", provider) + } + case "WORK": + if provider.Selectable || provider.Source != "resolved" { + 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 08650c4a8..2524e921d 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -73,8 +73,13 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - 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. + overrideResolution := activeProviderOverrideAbsent + if override != "" { + overrideResolution = activeProviderEnvOverrideResolution(deps, configPath, override) + } if options.json { payload := map[string]any{ "activeProvider": cfg.ActiveProvider, @@ -82,8 +87,15 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app } if override != "" { // A JSON consumer must not read this as an effective switch either. - payload["effectiveProvider"] = override payload["overriddenByEnv"] = config.ActiveProviderEnv + payload["envProvider"] = override + payload["envProviderResolves"] = overrideResolution.resolves() + if overrideResolution == activeProviderOverrideDeferred { + payload["envProviderResolution"] = "deferred" + } + if overrideResolution == activeProviderOverrideResolved { + payload["effectiveProvider"] = override + } } if err := writePrettyJSON(stdout, payload); err != nil { return exitCrash @@ -94,13 +106,41 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app return exitCrash } if override != "" { - if _, err := fmt.Fprintf(stderr, "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); err != nil { + 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 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) + } + if _, err := fmt.Fprint(stderr, note); err != nil { return exitCrash } } 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 @@ -113,12 +153,38 @@ 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 } +// 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 activeProviderOverrideResolved +} + func runProvidersSetup(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseProviderSetupArgs(args) if err != nil { @@ -526,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.EqualFold(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 @@ -542,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 6118939d8..c83cee00c 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" + "strconv" "strings" "testing" @@ -12,6 +14,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") @@ -75,6 +78,44 @@ func TestRunProvidersUseJSONIncludesActiveProviderAndConfigPath(t *testing.T) { } } +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) + } + if !strings.Contains(stderr.String(), `provider "runtime" not found`) { + t.Fatalf("error missing plain not-found: %s", stderr.String()) + } +} + +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(), `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) + } +} + func providersUseOverrideConfig(t *testing.T) string { t.Helper() configPath := filepath.Join(t.TempDir(), "config.json") @@ -88,6 +129,28 @@ 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. +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). @@ -121,7 +184,14 @@ func TestRunProvidersUseWarnsWhenEnvOverrides(t *testing.T) { func TestRunProvidersUseJSONFlagsEnvOverride(t *testing.T) { var stdout, stderr bytes.Buffer - deps := providerSetupDeps(providersUseOverrideConfig(t)) + // 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 + // 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" @@ -145,6 +215,170 @@ func TestRunProvidersUseJSONFlagsEnvOverride(t *testing.T) { } } +// A ZERO_PROVIDER value that names nothing resolvable must not be reported as +// the effective provider: the next resolution fails on it, so the note has to say +// the override is broken rather than send the user to check a provider that does +// not exist. +func TestRunProvidersUseFlagsUnresolvableEnvOverride(t *testing.T) { + configPath := providersUseOverrideConfig(t) + getenv := func(key string) string { + if key == config.ActiveProviderEnv { + return "removed-profile" + } + return "" + } + + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(configPath) + deps.getenv = getenv + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + note := stderr.String() + for _, want := range []string{config.ActiveProviderEnv, "removed-profile", "can be resolved"} { + if !strings.Contains(note, want) { + t.Fatalf("unresolvable-override note missing %q, got %q", want, note) + } + } + if strings.Contains(note, "stays the active provider") { + t.Fatalf("an unresolvable override must not be called the active provider: %q", note) + } + + stdout.Reset() + stderr.Reset() + jsonDeps := providerSetupDeps(configPath) + jsonDeps.getenv = getenv + if code := runWithDeps([]string{"providers", "use", "fast", "--json"}, &stdout, &stderr, jsonDeps); 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("an unresolvable override must not be reported as effective: %#v", payload) + } + if payload["envProvider"] != "removed-profile" || payload["overriddenByEnv"] != config.ActiveProviderEnv { + t.Fatalf("JSON must still name the override, got %#v", payload) + } + if resolves, ok := payload["envProviderResolves"].(bool); !ok || resolves { + t.Fatalf("envProviderResolves = %#v, want false", payload["envProviderResolves"]) + } +} + +// 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()) + } +} + +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{ @@ -171,6 +405,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/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/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/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 861eb3146..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 @@ -165,7 +242,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 @@ -192,12 +269,40 @@ 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 } for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if strings.TrimSpace(provider.Name) == name { + return true, nil + } + } + 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 } } @@ -228,10 +333,15 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if err := json.Unmarshal(data, &cfg); err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return FileConfig{}, err + } + // Persisted provider identity is exact. Resolution may fold names from + // runtime sources, but config mutations must target the requested row. 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 +351,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 @@ -280,11 +390,17 @@ 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. + // 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) - if strings.EqualFold(providerName, oldName) { + if providerName == oldName { index = i continue } @@ -307,7 +423,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 @@ -325,7 +441,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 { @@ -344,8 +460,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) @@ -369,11 +484,14 @@ 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 { providerName := strings.TrimSpace(provider.Name) - if strings.EqualFold(providerName, oldName) { + if providerName == oldName { index = i continue } @@ -400,7 +518,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 } @@ -485,8 +603,10 @@ func SetProviderModel(path string, name string, model string) (FileConfig, error return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + // 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.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 @@ -736,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 c66fc26ba..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" @@ -31,7 +33,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 +48,56 @@ 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 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) + + 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{ @@ -170,7 +222,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) } @@ -217,6 +269,22 @@ 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") + before := writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{ + {Name: "work", ProviderKind: ProviderKindOpenAICompatible, Model: "m1"}, + {Name: "WORK", ProviderKind: ProviderKindOpenAICompatible, Model: "m2"}, + }, + }, 0o600) + + _, err := SetProviderModel(path, "WORK", "m2-updated") + assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") +} + func TestUpsertProviderTightensExistingConfigFilePermissions(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows does not expose POSIX mode bits reliably") @@ -594,7 +662,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) } @@ -639,6 +707,24 @@ 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") + before := 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) + + _, err := RemoveProvider(path, "WORK") + assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") +} + func TestRemoveProviderRejectsUnknownWithoutRewriting(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") before := writeConfigFixture(t, path, FileConfig{ @@ -727,6 +813,22 @@ 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") + before := 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) + + _, err := RenameProvider(path, "WORK", "renamed") + assertAmbiguousConfigUnchanged(t, path, before, err, "work", "WORK") +} + 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 @@ -915,11 +1017,40 @@ func TestEditProviderAppliesRenameFieldsAndDescriptionAtomically(t *testing.T) { } } +func TestEditProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + before := 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) + + _, 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) + } + after, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatal(readErr) + } + if !bytes.Equal(after, before) { + t.Fatalf("ambiguous mutation rewrote config\nbefore: %s\nafter: %s", before, after) + } +} + // 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") 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.