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..7c0df38f7 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -99,6 +99,16 @@ func runAuthOpenRouter(args []string, stdout io.Writer, stderr io.Writer, deps a if len(args) > 0 { return writeExecUsageError(stderr, fmt.Sprintf("zero auth openrouter takes no arguments (got %q)", args[0])) } + // Check the config BEFORE the browser flow, as every other auth entry point + // does. Finding out the file is unusable only after the user has completed a + // PKCE round trip wastes their work and mints a key Zero then cannot store. + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if err := config.PreflightCatalogProviderLogin(configPath, "openrouter"); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } key, err := deps.openRouterLogin(context.Background(), provideroauth.OpenRouterOptions{ Out: stdout, HTTPClient: &http.Client{Timeout: 30 * time.Second}, @@ -111,10 +121,14 @@ func runAuthOpenRouter(args []string, stdout io.Writer, stderr io.Writer, deps a key = strings.TrimSpace(key) line, err := saveOpenRouterProviderKey(deps, key) if err != nil { - if _, writeErr := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key minted, but Zero could not save it: %s\nUse it manually, e.g.:\n export OPENROUTER_API_KEY=%s\n", err, key); writeErr != nil { + // The key is real and the user paid for it with a browser round trip, so + // print it rather than losing it. But nothing was persisted, so this is a + // failure: exiting 0 here reported success for a command that left the + // provider unusable, and a script would have carried on. + if _, writeErr := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key minted, but Zero could not save it.\nUse it manually, e.g.:\n export OPENROUTER_API_KEY=%s\n", key); writeErr != nil { return exitCrash } - return exitSuccess + return writeAppError(stderr, "could not save the OpenRouter API key: "+err.Error(), exitCrash) } if _, err := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key saved.\n%s\n", line); err != nil { return exitCrash @@ -131,6 +145,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 +193,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.PreflightCatalogProviderLogin(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 +214,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 +237,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.PreflightCatalogProviderLogin(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 +371,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 +400,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 +416,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 +431,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.PreflightCatalogProviderLogin(configPath, provider); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + manager, err := newAuthManager(deps, stdout, func() error { + return config.PreflightCatalogProviderLogin(configPath, provider) + }) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -438,7 +476,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) + } + configErr := config.PreflightUserConfig(configPath) + // Resolve the target the way login does. Login accepts a catalog id or a case + // variant and stores its token under the catalog id, and the TUI tells users + // to run `zero auth logout ` — so refusing that spelling here left + // the OAuth token and any stored API key in place for exactly the command the + // UI documents. The credential store keys stay on what the user typed (that is + // where login put them); only the config mutation below needs the persisted + // spelling, because those mutators match a row exactly. + configProvider := provider + if configErr == nil { + if canonical, owned, identityErr := config.PersistedProviderIdentity(configPath, provider); identityErr != nil { + configErr = identityErr + } else if owned { + configProvider = canonical + } + } + manager, err := newAuthManager(deps, stdout, nil) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -449,14 +507,31 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // Also drop any stored API key and its marker so `auth logout` clears the whole // credential (OAuth token AND key), not just the OAuth side. Surface deletion // failures rather than reporting success while a credential remains. - keyRemoved, keyErr := config.ForgetProviderKey(provider) + // + // The key store is asked for both spellings when they differ: a key captured + // by provider setup lives under the persisted row's name, while one captured + // by a catalog login lives under the catalog id. Clearing only the spelling + // the user typed would leave the other behind. + keyStore, keyErr := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if keyErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) + } + keyRemoved, keyErr := keyStore.Delete(provider) if keyErr != nil { return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) } - if configPath, perr := deps.userConfigPath(); perr == nil { - if _, clearErr := config.ClearProviderKeyStored(configPath, provider); clearErr != nil { - return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) + if configProvider != provider { + canonicalRemoved, canonicalErr := keyStore.Delete(configProvider) + if canonicalErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(canonicalErr, redaction.Options{}), exitCrash) } + keyRemoved = keyRemoved || canonicalRemoved + } + if configErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(configErr, redaction.Options{}), exitCrash) + } + if _, clearErr := config.ClearProviderKeyStored(configPath, configProvider); clearErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) } removed = removed || keyRemoved if parsed.json { @@ -491,7 +566,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 +605,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..4b922f56c 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -4,6 +4,10 @@ import ( "bytes" "context" "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -111,6 +115,135 @@ 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) + } +} + +// TestRunAuthChatGPTAllowsCaseVariantPersistedProfile is the regression test for +// jatmn's #725 finding: preflighting a login as if it were a new provider write +// rejected the very row it was logging into. A config whose sole ChatGPT profile +// is spelled "ChatGPT" made `zero auth chatgpt` fail before the browser flow with +// `provider "chatgpt" already exists as "ChatGPT"` — while the TUI, which only +// validates the file, completed the same login. A login mints no new spelling: +// EnsureCatalogProvider reuses whatever row owns the identity. +func TestRunAuthChatGPTAllowsCaseVariantPersistedProfile(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"ChatGPT"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + chatGPTLogin: func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) { + return oauth.Token{AccessToken: "fresh"}, nil + }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, want success; stderr = %q", code, stderr.String()) + } + if strings.Contains(stderr.String(), "already exists as") { + t.Fatalf("a case-variant re-login must not be treated as a colliding new provider: %q", stderr.String()) + } + token, ok, err := store.Load(oauth.ProviderKey("chatgpt")) + if err != nil || !ok || token.AccessToken != "fresh" { + t.Fatalf("stored token = %+v, %v, %v; want the fresh login saved", token, ok, err) + } + // The ambiguous-config guard is unchanged: a login still refuses to run + // against a file with two case-duplicate rows. + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"chatgpt"},{"name":"ChatGPT"}]}`), 0o600); err != nil { + t.Fatal(err) + } + stdout.Reset() + stderr.Reset() + code = runWithDeps([]string{"auth", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + chatGPTLogin: func(context.Context, provideroauth.ChatGPTOptions) (oauth.Token, error) { + return oauth.Token{AccessToken: "should-not-save"}, nil + }, + }) + if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("exit = %d, stderr = %q; want the ambiguous-config failure preserved", code, stderr.String()) + } +} + func TestRunAuthRefreshNoToken(t *testing.T) { withAuthStore(t) t.Setenv("ZERO_OAUTH_DEMO_CLIENT_ID", "client") // so config resolves; refresh still fails (no token) @@ -138,6 +271,188 @@ func TestRunAuthRejectsWrongFlags(t *testing.T) { } } +// TestRunAuthLogoutResolvesCatalogIdentity covers jatmn's #725 finding: login +// accepts a catalog id and stores its token under that key, and the TUI tells +// users to run `zero auth logout chatgpt` — but logout hard-stopped whenever a +// persisted row matched case-insensitively without matching exactly, so the +// documented command left the token and any stored key in place. +func TestRunAuthLogoutResolvesCatalogIdentity(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"ChatGPT"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("chatgpt"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "chatgpt"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if strings.Contains(stderr.String(), "capitalization") { + t.Fatalf("logout refused the spelling the UI documents: %q", stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("chatgpt")); err != nil || ok { + t.Fatalf("stored token survived logout: ok=%v err=%v", ok, err) + } + if !strings.Contains(stdout.String(), "Logged out") { + t.Fatalf("stdout = %q, want a logout confirmation", stdout.String()) + } +} + +func TestRunAuthLogoutCleansCredentialsWhenConfigIsAmbiguous(t *testing.T) { + storePath := withAuthStore(t) + configHome := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configHome) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(configHome, "zero", "config.json") + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + configData := []byte(`{"providers":[{"name":"demo"},{"name":"DEMO","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, configData, 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("demo"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("demo", "stored-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "demo"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("exit = %d stderr = %q, want truthful marker-update failure", code, stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("demo")); err != nil || ok { + t.Fatalf("OAuth credential survived recovery logout: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("demo"); err != nil || ok { + t.Fatalf("API key survived recovery logout: ok=%v err=%v", ok, err) + } + after, err := os.ReadFile(configPath) + if err != nil || !bytes.Equal(after, configData) { + t.Fatalf("invalid config changed during recovery logout: err=%v content=%s", err, after) + } +} + +// TestRunAuthOpenRouterPreflightsBeforeTheBrowserFlow covers the second half of +// the same finding: every other auth entry point validates the config before +// opening a browser, and this one minted a key first and only discovered the +// config was unusable when trying to save it. +func TestRunAuthOpenRouterPreflightsBeforeTheBrowserFlow(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"work"},{"name":"WORK"}]}`), 0o600); err != nil { + t.Fatal(err) + } + loginCalled := false + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "openrouter"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + openRouterLogin: func(context.Context, provideroauth.OpenRouterOptions) (string, error) { + loginCalled = true + return "sk-should-not-be-minted", nil + }, + }) + if code == exitSuccess { + t.Fatalf("an unusable config must fail before login; stdout = %q", stdout.String()) + } + if loginCalled { + t.Fatal("the browser flow ran before the config was validated") + } + if !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("stderr = %q, want the config error", stderr.String()) + } +} + +// TestRunAuthOpenRouterFailsWhenTheKeyCannotBeSaved pins the exit code: the +// minted key is still printed so the user does not lose it, but nothing was +// persisted, and reporting success left a script believing the provider was +// configured. +func TestRunAuthOpenRouterFailsWhenTheKeyCannotBeSaved(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[]}`), 0o600); err != nil { + t.Fatal(err) + } + calls := 0 + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "openrouter"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { + calls++ + if calls > 1 { + // The preflight passed; break the path only for the save that follows. + return "", errors.New("config path unavailable") + } + return configPath, nil + }, + openRouterLogin: func(context.Context, provideroauth.OpenRouterOptions) (string, error) { + return "sk-openrouter-test", nil + }, + }) + if code == exitSuccess { + t.Fatal("a failed save must not report success") + } + if !strings.Contains(stdout.String(), "sk-openrouter-test") { + t.Fatalf("stdout = %q, want the minted key printed so it is not lost", stdout.String()) + } + if !strings.Contains(stderr.String(), "could not save") { + t.Fatalf("stderr = %q, want the save failure reported", stderr.String()) + } +} + +// TestProviderSetupAdoptsPersistedCatalogRowCasing covers the third: catalog +// OAuth reused an existing row through EnsureCatalogProvider while setup and the +// wizard still collided with it on write, so re-running setup for a provider +// saved as "OpenRouter" failed after a successful capture. +func TestProviderSetupAdoptsPersistedCatalogRowCasing(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"OpenRouter","catalogId":"openrouter","model":"x"}]}`), 0o600); err != nil { + t.Fatal(err) + } + profile := config.ProviderProfile{Name: "openrouter", CatalogID: "openrouter", Model: "y"} + + adopted, err := config.AdoptPersistedCatalogProviderName(configPath, profile) + if err != nil { + t.Fatalf("AdoptPersistedCatalogProviderName: %v", err) + } + if adopted.Name != "OpenRouter" { + t.Fatalf("adopted name = %q, want the persisted row's spelling", adopted.Name) + } + if err := config.PreflightProviderWrite(configPath, adopted.Name); err != nil { + t.Fatalf("write preflight rejected the adopted name: %v", err) + } + + // A user-chosen name is left alone: colliding with an existing row there is a + // real collision, and silently overwriting it would be worse than the error. + custom := config.ProviderProfile{Name: "openrouter", CatalogID: "anthropic"} + kept, err := config.AdoptPersistedCatalogProviderName(configPath, custom) + if err != nil { + t.Fatalf("AdoptPersistedCatalogProviderName: %v", err) + } + if kept.Name != "openrouter" { + t.Fatalf("name = %q, want a non-catalog-default name left untouched", kept.Name) + } +} + func TestRunAuthOpenRouterRejectsArgs(t *testing.T) { withAuthStore(t) var stdout, stderr bytes.Buffer diff --git a/internal/cli/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..9ddd3d935 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -71,10 +71,15 @@ func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps app } cfg, err := config.SetActiveProvider(configPath, options.name) if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) + return writeAppError(stderr, providerMutationError(configPath, options.name, err), exitCrash) } - override := activeProviderEnvOverride(deps.getenv, cfg.ActiveProvider) + // An override only becomes the effective provider if Zero can actually + // 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,49 @@ func activeProviderEnvOverride(getenv func(string) string, selected string) stri return "" } override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) + // Fold case: resolution selects the active row case-insensitively, so + // ZERO_PROVIDER=WORK against a saved "work" names the same provider the write + // just selected. Warning that the switch "has no effect" there described a + // conflict that does not exist — the runtime lands on exactly the row the user + // asked for. Only a genuinely different provider is an override. if override == "" || strings.EqualFold(override, strings.TrimSpace(selected)) { 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) + // Fold case: resolution selects the active row case-insensitively and reports + // the row's canonical persisted spelling, so ZERO_PROVIDER=openrouter against + // a saved "OpenRouter" resolves to "OpenRouter". Comparing exactly called that + // a failed override and told the user Zero could not start with it, which was + // the opposite of true. A fold cannot report a false success here: if the + // active row folds to the override, the override is what selected it. + if err != nil || !strings.EqualFold(strings.TrimSpace(resolved.ActiveProvider), override) { + return activeProviderOverrideUnresolved + } + return activeProviderOverrideResolved +} + func runProvidersSetup(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseProviderSetupArgs(args) if err != nil { @@ -416,12 +493,16 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps } cfg, err := config.RemoveProvider(configPath, name) if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) + return writeAppError(stderr, providerMutationError(configPath, name, err), exitCrash) } // Delete the key from the store BESIDE the config being edited — the same // store setup/rename write to — not the default-path store, so a // non-default config path cannot leave the encrypted key behind. - keyRemoved, keyErr := removeStoredProviderKeyAt(configPath, name) + keyRemoved := false + var keyErr error + if !configContainsCaseVariantProvider(cfg, name) { + keyRemoved, keyErr = removeStoredProviderKeyAt(configPath, name) + } if options.json { payload := map[string]any{ "removed": name, @@ -473,6 +554,16 @@ func removeStoredProviderKeyAt(configPath string, provider string) (bool, error) return store.Delete(provider) } +func configContainsCaseVariantProvider(cfg config.FileConfig, name string) bool { + name = strings.TrimSpace(name) + for _, provider := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + return true + } + } + return false +} + // runProvidersRename renames a saved provider profile, migrating its stored // API key and the activeProvider pointer along with it (config.RenameProvider). func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { @@ -502,7 +593,7 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps } cfg, err := config.RenameProvider(configPath, options.names[0], options.names[1]) if err != nil { - return writeAppError(stderr, err.Error(), exitCrash) + return writeAppError(stderr, providerMutationError(configPath, options.names[0], err), exitCrash) } if options.json { if err := writePrettyJSON(stdout, map[string]any{ @@ -526,11 +617,58 @@ 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 +} + +// providerMutationError renders a provider mutator's failure, naming the saved +// profile when the config owns what the user asked for under another spelling. +// The mutators match exactly on purpose — one credential identity, one row — so +// `zero providers use SAVED` against a saved "saved", or a catalog id against a +// row saved under a different name, correctly fails not-found. Saying only that +// leaves the user to guess what they got wrong, and `zero auth logout` already +// explains the same situation; this keeps the provider commands consistent with +// it, and goes one better by naming the spelling that works. +func providerMutationError(configPath, name string, err error) string { + message := err.Error() + if !strings.Contains(message, "not found") { + return message + } + canonical, owned, lookupErr := config.PersistedProviderIdentity(configPath, name) + if lookupErr != nil || !owned || canonical == strings.TrimSpace(name) { + return message + } + return fmt.Sprintf("%s; the saved profile is named %q and provider names are matched exactly", message, canonical) +} + +// configOwnsProviderIdentity reports whether a persisted row already owns name — +// under any capitalization, or as the catalog id of a row saved under a +// different name. Every runtime-only report below must consult it first: its +// message claims the provider exists only because an environment variable is +// set, and that is simply false for a provider the config owns. The caller falls +// through to the command's own not-found handling instead, which is the right +// answer for a mis-addressed saved profile — `zero providers use openrouter` +// against a saved {name: "my-router", catalogId: "openrouter"} is the wrong name +// for a real profile, not an env-derived provider. +// +// failed reports that reading the config failed; the error is already on stderr +// and exit carries the code. owned is meaningful only when failed is false. +func configOwnsProviderIdentity(stderr io.Writer, configPath, name string) (owned bool, exit int, failed bool) { + if _, ok, err := config.PersistedProviderIdentity(configPath, name); err != nil { + return false, writeAppError(stderr, err.Error(), exitCrash), true + } else if ok { + return true, exitSuccess, false + } + return false, exitSuccess, false } // reportUnpersistedProviderUse handles `zero providers use ` for a @@ -542,6 +680,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 owned, exit, failed := configOwnsProviderIdentity(stderr, configPath, options.name); failed { + return exit, true + } else if owned { + return exitSuccess, false + } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { // resolveCommandCenterConfig already wrote its own error to stderr; @@ -578,6 +721,11 @@ func reportUnpersistedProviderUse(stdout, stderr io.Writer, deps appDeps, option // name isn't resolvable at all, it returns handled=false so the caller falls // through to RemoveProvider's real "not found" error. func reportUnpersistedProviderRemove(stdout, stderr io.Writer, deps appDeps, name string, jsonOutput bool, configPath string) (int, bool) { + if owned, exit, failed := configOwnsProviderIdentity(stderr, configPath, name); failed { + return exit, true + } else if owned { + return exitSuccess, false + } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { return exitCode, true @@ -609,6 +757,11 @@ func reportUnpersistedProviderRemove(stdout, stderr io.Writer, deps appDeps, nam } func reportUnpersistedProviderRename(stdout, stderr io.Writer, deps appDeps, name string, jsonOutput bool, configPath string) (int, bool) { + if owned, exit, failed := configOwnsProviderIdentity(stderr, configPath, name); failed { + return exit, true + } else if owned { + return exitSuccess, false + } resolved, exitCode := resolveCommandCenterConfig(stderr, deps) if exitCode != exitSuccess { return exitCode, true diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 6118939d8..612edafc7 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,158 @@ 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 +} + +// TestRunProvidersUseResolvesCaseVariantEnvOverride is the regression test for +// jatmn's #725 review: the override check ran the real resolver but compared its +// result to the raw env string exactly. Resolution matches the active row +// case-insensitively and reports the row's persisted spelling, so +// ZERO_PROVIDER=WORK against a saved "work" resolves fine at runtime yet was +// reported as unresolvable — telling the user Zero could not start on an +// override that works. +func TestRunProvidersUseResolvesCaseVariantEnvOverride(t *testing.T) { + var stdout, stderr bytes.Buffer + // As in TestRunProvidersUseJSONFlagsEnvOverride: the resolver reads the real + // process environment, so the override has to be set for real. + t.Setenv(config.ActiveProviderEnv, "WORK") + deps := providerSetupDeps(providersUseOverrideConfigAtDefaultUserPath(t)) + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "WORK" + } + return "" + } + + if code := runWithDeps([]string{"providers", "use", "fast", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + var payload map[string]any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("JSON did not decode: %v\n%s", err, stdout.String()) + } + if resolves, ok := payload["envProviderResolves"].(bool); !ok || !resolves { + t.Fatalf("envProviderResolves = %#v, want true for a case-variant override of a saved profile", payload["envProviderResolves"]) + } + if payload["effectiveProvider"] != "WORK" { + t.Fatalf("effectiveProvider = %#v, want the override reported as effective", payload["effectiveProvider"]) + } + + stdout.Reset() + stderr.Reset() + textDeps := providerSetupDeps(providersUseOverrideConfigAtDefaultUserPath(t)) + textDeps.getenv = deps.getenv + if code := runWithDeps([]string{"providers", "use", "fast"}, &stdout, &stderr, textDeps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + if note := stderr.String(); strings.Contains(note, "can be resolved") { + t.Fatalf("a resolvable case-variant override must not be reported as broken: %q", note) + } +} + +// TestRunProvidersUseRejectsCatalogIDOfSavedProfile covers jatmn's #725 finding +// that catalog-id addressing of a SAVED row took the runtime-only path: the row +// is not persisted under that name, but the config plainly owns the identity, so +// the env-derived explanation is false and exiting 0 hides a failed switch. +func TestRunProvidersUseRejectsCatalogIDOfSavedProfile(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "work", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAI, BaseURL: config.OpenAIBaseURL, Model: "gpt-4.1"}, + {Name: "my-router", ProviderKind: config.ProviderKindOpenAICompatible, CatalogID: "openrouter", BaseURL: "https://openrouter.ai/api/v1", Model: "x"}, + }, + }) + + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(configPath) + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + saved := config.ProviderProfile{Name: "my-router", ProviderKind: config.ProviderKindOpenAICompatible, CatalogID: "openrouter", Model: "x"} + return config.ResolvedConfig{ActiveProvider: "work", Provider: saved, Providers: []config.ProviderProfile{saved}}, nil + } + if code := runWithDeps([]string{"providers", "use", "openrouter"}, &stdout, &stderr, deps); code != exitCrash { + t.Fatalf("exit = %d, want %d (stdout %q, stderr %q)", code, exitCrash, stdout.String(), stderr.String()) + } + if strings.Contains(stdout.String(), "environment variable") { + t.Fatalf("a saved profile addressed by catalog id must not be described as environment-derived: %q", stdout.String()) + } + if !strings.Contains(stderr.String(), `provider "openrouter" not found`) { + t.Fatalf("stderr = %q, want the real not-found error", stderr.String()) + } + if !strings.Contains(stderr.String(), `"my-router"`) { + t.Fatalf("stderr = %q, want the saved profile's name named in the hint", stderr.String()) + } + if cfg := readFileConfig(t, configPath); cfg.ActiveProvider != "work" { + t.Fatalf("ActiveProvider = %q, want work (nothing should have switched)", cfg.ActiveProvider) + } +} + +// TestRunProvidersRemoveRenameRejectCaseVariantOfPersistedProvider extends the +// guard `providers use` already had to remove and rename, which jatmn found had +// been left behind: `zero providers remove SAVED` against a saved "saved" exited +// 0 with the environment-variable explanation instead of failing not-found. +func TestRunProvidersRemoveRenameRejectCaseVariantOfPersistedProvider(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {name: "remove", args: []string{"providers", "remove", "SAVED"}}, + {name: "rename", args: []string{"providers", "rename", "SAVED", "renamed"}}, + } { + t.Run(tc.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "saved", + Providers: []config.ProviderProfile{ + {Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }) + + var stdout, stderr bytes.Buffer + deps := providerSetupDeps(configPath) + deps.resolveConfig = func(string, config.Overrides) (config.ResolvedConfig, error) { + profile := config.ProviderProfile{Name: "saved", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"} + return config.ResolvedConfig{ActiveProvider: "saved", Provider: profile, Providers: []config.ProviderProfile{profile}}, nil + } + if code := runWithDeps(tc.args, &stdout, &stderr, deps); code != exitCrash { + t.Fatalf("exit = %d, want %d (stdout %q, stderr %q)", code, exitCrash, stdout.String(), stderr.String()) + } + if strings.Contains(stdout.String(), "environment variable") { + t.Fatalf("a case variant of a saved profile must not be described as environment-derived: %q", stdout.String()) + } + if !strings.Contains(stderr.String(), `provider "SAVED" not found`) { + t.Fatalf("stderr = %q, want the real not-found error", stderr.String()) + } + if !strings.Contains(stderr.String(), `"saved"`) { + t.Fatalf("stderr = %q, want the persisted spelling named in the hint", stderr.String()) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "saved" { + t.Fatalf("config was mutated by a rejected command: %#v", cfg.Providers) + } + }) + } +} + // The write to config.json still succeeds, but when ZERO_PROVIDER names a // different provider the saved selection is NOT effective, so the command must // warn instead of reporting a silent success (issue #721). @@ -121,7 +314,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 +345,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 +535,30 @@ func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { } } +// TestActiveProviderEnvOverrideFoldsCaseAgainstTheSelection replaces the earlier +// TestActiveProviderEnvOverrideTreatsCaseVariantAsDistinct, which locked in a +// warning jatmn showed to be misleading: resolution selects the active row +// case-insensitively, so ZERO_PROVIDER=WORK against a saved "work" lands on +// exactly the row `providers use work` just selected. Telling the user their +// switch stays overridden described a conflict that does not exist. A genuinely +// different provider must still warn. +func TestActiveProviderEnvOverrideFoldsCaseAgainstTheSelection(t *testing.T) { + getenv := func(value string) func(string) string { + return func(key string) string { + if key == config.ActiveProviderEnv { + return value + } + return "" + } + } + if override := activeProviderEnvOverride(getenv("WORK"), "work"); override != "" { + t.Fatalf("activeProviderEnvOverride() = %q, want no override for a case variant of the selection", override) + } + if override := activeProviderEnvOverride(getenv("fast"), "work"); override != "fast" { + t.Fatalf("activeProviderEnvOverride() = %q, want the genuinely different provider reported", override) + } +} + func TestRunProvidersUseSurfacesMalformedConfig(t *testing.T) { var stdout, stderr bytes.Buffer configPath := filepath.Join(t.TempDir(), "config.json") @@ -480,3 +868,35 @@ func TestRunProvidersRemoveDeletesKeyBesideConfig(t *testing.T) { t.Fatalf("stored key must be deleted from the store beside the config") } } + +func TestRunProvidersRemoveCaseDuplicateKeepsSurvivorKey(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + seed := `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK"}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "survivor-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + deps := appDeps{userConfigPath: func() (string, error) { return configPath, nil }} + if code := runWithDeps([]string{"providers", "remove", "WORK", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("remove failed: code=%d stderr=%s", code, stderr.String()) + } + if key, ok, err := store.Get("work"); err != nil || !ok || key != "survivor-key" { + t.Fatalf("surviving provider key = %q, %v, %v; want retained", key, ok, err) + } + var payload struct { + KeyRemoved bool `json:"keyRemoved"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil || payload.KeyRemoved { + t.Fatalf("payload = %s, err = %v; shared key must not be reported removed", stdout.String(), err) + } +} diff --git a/internal/cli/provider_setup.go b/internal/cli/provider_setup.go index de13f26ce..68686727f 100644 --- a/internal/cli/provider_setup.go +++ b/internal/cli/provider_setup.go @@ -53,6 +53,16 @@ func runProvidersAdd(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Retarget a catalog-default name at the row that already owns that catalog + // identity, so re-running setup for a provider saved under different casing + // updates it instead of failing the collision check. + profile, err = config.AdoptPersistedCatalogProviderName(configPath, profile) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } // 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..66f75ade9 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -264,6 +264,15 @@ func saveSetupProvider(deps appDeps, selection tui.SetupSelection, options setup if err != nil { return tui.SetupResult{}, err } + // Same identity resolution the login path uses: a catalog-default name yields + // to the row already saved for that catalog provider. + profile, err = config.AdoptPersistedCatalogProviderName(configPath, profile) + if err != nil { + return tui.SetupResult{}, err + } + if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { + return tui.SetupResult{}, err + } // 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..38fba63fd 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -11,6 +11,158 @@ 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) +} + +// PreflightCatalogProviderLogin is the preflight for a credential login against +// a catalog provider. It is deliberately weaker than PreflightProviderWrite: +// a login does not mint a new spelling. EnsureCatalogProvider reuses whatever +// row already owns the identity — matching on catalog id OR folded name — so a +// persisted "OpenRouter" is the same credential as a `zero auth login +// openrouter`, not a colliding second row. Preflighting that as a collision +// blocked OAuth outright for anyone whose config used a capitalized spelling, +// while the TUI (which only validates the file) completed the same login. +// +// The collision check still runs when nothing owns the identity, because that is +// the case where a row WILL be created. It is unreachable today given +// EnsureCatalogProvider's own matching, and kept so that if that matching ever +// narrows, this fails fast before the browser flow instead of after it. +func PreflightCatalogProviderLogin(path, catalogID string) error { + if err := PreflightUserConfig(path); err != nil { + return err + } + if _, owned, err := PersistedProviderIdentity(path, catalogID); err != nil { + return err + } else if owned { + return nil + } + return PreflightProviderWrite(path, catalogID) +} + +// AdoptPersistedCatalogProviderName retargets a catalog-named profile at the row +// that already owns its catalog identity, so persisting it UPDATES that row +// instead of colliding with it. +// +// It applies only when the caller took the catalog's own default name (profile +// name == catalog id). That is the case where the two spellings are the same +// provider by construction — a re-setup or re-login of, say, openrouter against +// a row saved as "OpenRouter" — and where EnsureCatalogProvider already reuses +// the row on the login path. A user-chosen name is left alone on purpose: there, +// a case collision with an existing row is a real collision, and silently +// overwriting the other row would be worse than the error. +func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (ProviderProfile, error) { + catalogID := strings.TrimSpace(profile.CatalogID) + if catalogID == "" || !strings.EqualFold(strings.TrimSpace(profile.Name), catalogID) { + return profile, nil + } + canonical, owned, err := PersistedProviderIdentity(path, catalogID) + if err != nil { + return profile, err + } + if owned { + profile.Name = canonical + } + return profile, nil +} + +// PersistedProviderIdentity reports whether a persisted user-config row already +// owns identity — as its name (case-insensitively) or as its catalog id — and +// returns that row's exact name. +// +// Callers use it to answer "is this the config's provider, however it was +// spelled or addressed?", which is a different question from ProviderPersisted's +// "is this the exact key a mutator will match". A row saved as +// {name: "my-router", catalogId: "openrouter"} is owned by both spellings here, +// so `zero providers use openrouter` is a wrong way to address a saved profile +// rather than an environment-derived provider. +func PersistedProviderIdentity(path, identity string) (string, bool, error) { + identity = strings.TrimSpace(identity) + if identity == "" { + return "", false, nil + } + data, err := os.ReadFile(strings.TrimSpace(path)) + if os.IsNotExist(err) { + return "", false, nil + } + if err != nil { + return "", false, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return "", false, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + for _, provider := range cfg.Providers { + name := strings.TrimSpace(provider.Name) + if strings.EqualFold(name, identity) || strings.EqualFold(strings.TrimSpace(provider.CatalogID), identity) { + return name, true, nil + } + } + return "", false, nil +} + +// PreflightProviderWrite also rejects a new spelling that would share a +// case-insensitive credential key with an existing persisted row. +func PreflightProviderWrite(path, name string) error { + 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 +181,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 +293,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 +328,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 +355,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 } } @@ -229,9 +420,13 @@ func RemoveProvider(path string, name 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. This + // lookup intentionally precedes validation so an exact removal can repair a + // case-duplicate config; writeConfigFile validates the resulting config. index := -1 for i, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if strings.TrimSpace(provider.Name) == name { index = i break } @@ -239,9 +434,22 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if index < 0 { return FileConfig{}, fmt.Errorf("provider %q not found", name) } - removed := cfg.Providers[index] + activeIndex := -1 + activeFoldedIndex := -1 + activeFoldedMatches := 0 + for i, provider := range cfg.Providers { + providerName := strings.TrimSpace(provider.Name) + if providerName == strings.TrimSpace(cfg.ActiveProvider) { + activeIndex = i + } + if strings.EqualFold(providerName, strings.TrimSpace(cfg.ActiveProvider)) { + activeFoldedIndex = i + activeFoldedMatches++ + } + } + removedWasActive := activeIndex == index || (activeIndex < 0 && activeFoldedMatches == 1 && activeFoldedIndex == index) cfg.Providers = append(cfg.Providers[:index], cfg.Providers[index+1:]...) - if strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(removed.Name)) { + if removedWasActive { cfg.ActiveProvider = "" if len(cfg.Providers) > 0 { cfg.ActiveProvider = cfg.Providers[0].Name @@ -280,11 +488,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 } @@ -325,7 +539,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 +558,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 +582,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 } @@ -485,8 +701,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 +954,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..7ad84b4c7 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,105 @@ func TestRemoveProviderKeepsActiveWhenOtherRemoved(t *testing.T) { } } +func TestProviderMutatorsHandOffCaseVariantActiveProvider(t *testing.T) { + tests := []struct { + name string + mutate func(string) (FileConfig, error) + wantActive string + wantName string + }{ + {name: "remove", mutate: func(path string) (FileConfig, error) { return RemoveProvider(path, "work") }}, + {name: "rename", mutate: func(path string) (FileConfig, error) { return RenameProvider(path, "work", "office") }, wantActive: "office", wantName: "office"}, + {name: "edit", mutate: func(path string) (FileConfig, error) { + return EditProvider(path, ProviderEdit{Name: "work", NewName: "office", Model: "updated"}) + }, wantActive: "office", wantName: "office"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ActiveProvider: "WORK", Providers: []ProviderProfile{{Name: "work", Model: "old"}}}, 0o600) + cfg, err := test.mutate(path) + if err != nil { + t.Fatal(err) + } + if cfg.ActiveProvider != test.wantActive { + t.Fatalf("activeProvider = %q, want %q", cfg.ActiveProvider, test.wantActive) + } + if test.wantName == "" && len(cfg.Providers) != 0 { + t.Fatalf("providers = %+v, want none", cfg.Providers) + } + if test.wantName != "" && (len(cfg.Providers) != 1 || cfg.Providers[0].Name != test.wantName) { + t.Fatalf("providers = %+v, want canonical name %q", cfg.Providers, test.wantName) + } + }) + } +} + +// UpsertProvider merges by exact name, so a config file can end up with two +// rows that differ only by case (e.g. one saved as "work", another later +// saved as "WORK"). RemoveProvider must delete the exact row the caller +// named, not whichever case-variant sorts first. +func TestRemoveProviderRequiresExactProviderIdentityAmongCaseVariants(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + 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("exact removal should repair case duplicates: %v", err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" || cfg.ActiveProvider != "work" { + t.Fatalf("repaired config = %+v", cfg) + } +} + +func TestRemoveProviderRejectsNonExactCaseDuplicateTarget(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "WORK"}}}, 0o600) + _, err := RemoveProvider(path, "WoRk") + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("error = %v, want exact-target not-found error", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil || !bytes.Equal(after, before) { + t.Fatalf("rejected removal rewrote config: readErr=%v", readErr) + } +} + +func TestRemoveProviderRejectsRepairThatRemainsAmbiguous(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "WORK"}, {Name: "Work"}}}, 0o600) + _, err := RemoveProvider(path, "Work") + if err == nil || !strings.Contains(err.Error(), "ambiguous persisted provider names") { + t.Fatalf("error = %v, want resulting-config validation error", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil || !bytes.Equal(after, before) { + t.Fatalf("invalid repair rewrote config: readErr=%v", readErr) + } +} + +func TestRemoveProviderKeepsExactActiveCaseVariant(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{{Name: "alpha"}, {Name: "work"}, {Name: "WORK"}}, + }, 0o600) + + cfg, err := RemoveProvider(path, "WORK") + if err != nil { + t.Fatal(err) + } + if cfg.ActiveProvider != "work" { + t.Fatalf("activeProvider = %q, want exact surviving row work", cfg.ActiveProvider) + } +} + func TestRemoveProviderRejectsUnknownWithoutRewriting(t *testing.T) { path := filepath.Join(t.TempDir(), "zero.json") before := writeConfigFixture(t, path, FileConfig{ @@ -727,6 +894,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 +1098,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..dba19d640 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -364,7 +364,7 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { } activeAfter = cfg.ActiveProvider notes = []string{"Deleted " + name + "."} - cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile) + cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, configContainsCaseVariantProviderAfterRemoval(cfg, name)) } else { // Env-derived providers have no persisted profile or credential to // delete. Keep this path session-only. @@ -406,6 +406,16 @@ func removeSavedProvider(saved []config.ProviderProfile, name string) []config.P return kept } +func configContainsCaseVariantProviderAfterRemoval(cfg config.FileConfig, name string) bool { + name = strings.TrimSpace(name) + for _, provider := range cfg.Providers { + if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + return true + } + } + return false +} + // providerManagerCleanupMsg reports the off-thread half of a delete: the // stored-key removal outcome and the OAuth-login hint. type providerManagerCleanupMsg struct { @@ -417,17 +427,19 @@ type providerManagerCleanupMsg struct { // reads the token store — blocking work the confirm keypress must not wait on. // A failed key delete is surfaced rather than letting a lingering secret read // as a clean removal. -func providerManagerCleanupCmd(configPath string, profile config.ProviderProfile) tea.Cmd { +func providerManagerCleanupCmd(configPath string, profile config.ProviderProfile, retainStoredKey bool) tea.Cmd { name := profile.Name catalogID := profile.CatalogID return func() tea.Msg { notes := []string{} - keyStore, storeErr := providerKeyStoreForPath(configPath) - if storeErr == nil { - _, storeErr = keyStore.Delete(name) - } - if storeErr != nil { - notes = append(notes, "Warning: its stored API key could not be deleted ("+storeErr.Error()+").") + if !retainStoredKey { + keyStore, storeErr := providerKeyStoreForPath(configPath) + if storeErr == nil { + _, storeErr = keyStore.Delete(name) + } + if storeErr != nil { + notes = append(notes, "Warning: its stored API key could not be deleted ("+storeErr.Error()+").") + } } if login, ok := oauthLoginName(config.ProviderProfile{Name: name, CatalogID: catalogID}); ok { notes = append(notes, "OAuth login kept — remove with `zero auth logout "+login+"`.") @@ -604,6 +616,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..7324c780d 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,18 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { nextProvider = built } if strings.TrimSpace(m.userConfigPath) != "" { + // A catalog-default name yields to the row that already owns that catalog + // identity, matching the CLI login path; a user-chosen name still collides. + adopted, adoptErr := config.AdoptPersistedCatalogProviderName(m.userConfigPath, profile) + if adoptErr != nil { + wizard.err = adoptErr.Error() + return m, nil + } + profile = adopted + if err := config.PreflightProviderWrite(m.userConfigPath, profile.Name); err != nil { + wizard.err = err.Error() + return m, nil + } // 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 +1375,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.