diff --git a/internal/cli/app.go b/internal/cli/app.go index 4d6b11aab..01f75670f 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..eea07a614 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "strings" "time" @@ -99,6 +100,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 +122,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 +146,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 +194,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 +215,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 +238,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 +372,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 +401,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 +417,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 +432,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) } @@ -425,6 +464,18 @@ func runAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps appDep return exitSuccess } +// appendUniqueOAuthCandidate appends candidate to candidates if it is +// non-blank and not already present. The OAuth token store is a +// case-sensitive map, so comparison here is exact, matching +// ProviderProfile.OAuthLoginCandidates. +func appendUniqueOAuthCandidate(candidates []string, candidate string) []string { + candidate = strings.TrimSpace(candidate) + if candidate == "" || slices.Contains(candidates, candidate) { + return candidates + } + return append(candidates, candidate) +} + func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { parsed, err := parseAuthArgs("logout", args) if err != nil { @@ -438,25 +489,116 @@ 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, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + 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. + // + // Identity resolution and candidate expansion run regardless of configErr: + // PersistedProviderIdentity/ProviderRow only read+parse the raw JSON, they + // never call ValidatePersistedProviderNames, so an unrelated ambiguous + // case-duplicate row elsewhere in the file must not suppress deleting every + // credential this logout can find for the requested provider. Only the + // config WRITE below (clearing the apiKeyStored marker) needs a valid config + // and is gated on configErr. + // + // Identity resolution prefers an exact name over a case variant, and a name + // over a catalog id (config.ResolvePersistedProviderIdentity). Taking the + // first row that matched either field let `logout xai` retarget a + // {name:"work-xai", catalogId:"xai"} row — a different profile, with + // different credentials, than the one named. + configProvider := provider + target, identityMatch, identityErr := config.ResolvePersistedProviderIdentity(configPath, provider) + if identityErr != nil { + if configErr == nil { + configErr = identityErr + } + } else if identityMatch != config.PersistedIdentityNone { + configProvider = strings.TrimSpace(target.Name) } - removed, err := manager.Logout(provider) + manager, err := newAuthManager(deps, stdout, nil) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } + // Delete every candidate the runtime bearer resolver would have tried + // (ProviderProfile.OAuthLoginCandidates: profile name, then catalog id) — + // not just the spelling the user typed. A profile saved as + // {name:"my-xai", catalogId:"xai"} logged in via `zero auth login xai` + // stores its token under "xai"; `zero auth logout my-xai` must delete that + // too, or the login silently survives. The same candidate set is used below + // for the API key store, which has the identical asymmetry. + // + // The catalog id joins that set only when the resolved profile is the ONLY + // row claiming it. Catalog ids are shared by design — stored-key "work-xai", + // stored-key "xai", and keyless "personal-xai" can all carry catalogId + // "xai" — so expanding unconditionally made `logout work-xai` delete the + // "xai" token and the "xai" profile's API key while clearing only + // work-xai's marker. When the id is shared, the user has to name the + // profile whose credential they mean. + credentialCandidates := []string{provider} + if configProvider != provider { + credentialCandidates = append(credentialCandidates, configProvider) + } + if identityMatch != config.PersistedIdentityNone { + catalogID := strings.TrimSpace(target.CatalogID) + exclusive, exclusiveErr := config.CatalogIdentityExclusive(configPath, catalogID, configProvider) + if exclusiveErr != nil { + if configErr == nil { + configErr = exclusiveErr + } + } else if exclusive { + credentialCandidates = appendUniqueOAuthCandidate(credentialCandidates, catalogID) + } + } + removed := false + for _, candidate := range credentialCandidates { + candidateRemoved, err := manager.Logout(candidate) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + removed = removed || candidateRemoved + } // Also drop any stored API key and its marker so `auth logout` clears the whole // credential (OAuth token AND key), not just the OAuth side. Surface deletion // failures rather than reporting success while a credential remains. - keyRemoved, keyErr := config.ForgetProviderKey(provider) + // + // The key store is asked for every OAuth candidate spelling too: a key + // captured by provider setup lives under the persisted row's name, while one + // captured by a catalog login lives under the catalog id. Clearing only the + // spelling the user typed would leave the others behind. + keyStore, keyErr := config.ProviderKeyStoreAt(filepath.Dir(configPath)) if keyErr != nil { return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) } - if configPath, perr := deps.userConfigPath(); perr == nil { - if _, clearErr := config.ClearProviderKeyStored(configPath, provider); clearErr != nil { - return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) + keyRemoved := false + for _, candidate := range credentialCandidates { + candidateRemoved, candidateErr := keyStore.Delete(candidate) + if candidateErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(candidateErr, redaction.Options{}), exitCrash) } + keyRemoved = keyRemoved || candidateRemoved + } + if configErr != nil { + // Credentials are already gone at this point, but an invalid persisted + // config (e.g. ambiguous case-duplicate rows) blocks every config write, + // including clearing a stale apiKeyStored marker — writeConfigFile + // re-validates on every call, so nothing here could succeed either. + // Say so explicitly rather than leaving the marker silently stale. + return writeAppError(stderr, redaction.ErrorMessage(configErr, redaction.Options{})+"; any stale apiKeyStored marker in config.json must be corrected by hand alongside this", exitCrash) + } + // Clear the marker on every case-variant row, not just configProvider's exact + // spelling: the credential store keys secrets case-folded, so a sibling row + // could have pointed at the same now-deleted secret. + if _, clearErr := config.ClearProviderKeyStoredCaseVariants(configPath, configProvider); clearErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) } removed = removed || keyRemoved if parsed.json { @@ -491,7 +633,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 +672,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..108fc8d3a 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,356 @@ 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()) + } +} + +// TestRunAuthLogoutDeletesCatalogIDToken covers jatmn's #725 follow-up +// finding: a profile addressed by its own name but logged in under its +// catalog id (e.g. {name:"my-xai", catalogId:"xai"} via `zero auth login +// xai`) left the "xai" OAuth token behind when logged out as "my-xai", +// because logout only ever deleted the exact spelling the user typed. +func TestRunAuthLogoutDeletesCatalogIDToken(t *testing.T) { + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"my-xai","catalogId":"xai"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("xai"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "my-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("xai")); err != nil || ok { + t.Fatalf("catalog-id OAuth token survived logout: ok=%v err=%v", ok, err) + } + if !strings.Contains(stdout.String(), "Logged out") { + t.Fatalf("stdout = %q, want a logout confirmation", stdout.String()) + } +} + +// TestRunAuthLogoutDeletesCatalogIDAPIKey covers jatmn's second #725 follow-up +// finding: logout's OAuth-token deletion covers the profile name, canonical +// persisted name, and catalog id, but API-key deletion only covered the first +// two — a key stored under the catalog id (e.g. captured via `zero auth +// openrouter`-style catalog flows) survived `zero auth logout my-xai`. +func TestRunAuthLogoutDeletesCatalogIDAPIKey(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"my-xai","catalogId":"xai","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "catalog-id-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "my-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := keyStore.Get("xai"); err != nil || ok { + t.Fatalf("catalog-id API key survived logout: ok=%v err=%v", ok, err) + } + if !strings.Contains(stdout.String(), "Logged out") { + t.Fatalf("stdout = %q, want a logout confirmation", stdout.String()) + } +} + +// TestRunAuthLogoutKeepsDistinctUnicodeCredentials pins the end-to-end +// guarantee behind jatmn's #725 finding that destructive candidate expansion +// used strings.EqualFold as authority for credential ownership: a saved "s" +// profile with its own token and key must survive `zero auth logout ſ`, which +// names a provider the config never saved (the credential store defines +// identity with credstore.NormalizeProvider, under which "s" and Unicode +// long-s "ſ" are separate entries). +// +// Two independent layers now enforce that, and this test deliberately asserts +// the outcome rather than either mechanism. The one that fires first is +// oauth.ValidateKey, which rejects the non-ASCII spelling before any deletion +// runs — so the folded-name adoption itself is pinned where it is reachable, in +// TestPersistedProviderIdentityRulesMatchTheCredentialStore (internal/config). +func TestRunAuthLogoutKeepsDistinctUnicodeCredentials(t *testing.T) { + const longS = "ſ" + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + storePath := withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"s","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + tokens, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := tokens.Save(oauth.ProviderKey("s"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("s", "long-s-is-not-s"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + runWithDeps([]string{"auth", "logout", longS}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + + if _, ok, err := tokens.Load(oauth.ProviderKey("s")); err != nil || !ok { + t.Fatalf("logging out of a distinct identity deleted the saved token: ok=%v err=%v", ok, err) + } + if key, ok, err := keyStore.Get("s"); err != nil || !ok || key != "long-s-is-not-s" { + t.Fatalf("stored API key = %q, %v, %v; want the unrelated profile untouched", key, ok, err) + } + saved := readCLIConfigFixture(t, configPath).Providers + if len(saved) != 1 || saved[0].Name != "s" || !saved[0].APIKeyStored { + t.Fatalf("providers = %+v, want the saved profile keeping its stored-key marker", saved) + } +} + +// TestRunAuthLogoutResolvesCandidatesDespiteUnrelatedAmbiguousConfig covers +// jatmn's third #725 follow-up finding: identity resolution and OAuth/API-key +// candidate expansion were gated on PreflightUserConfig succeeding, even +// though PersistedProviderIdentity/ProviderRow only read+parse raw JSON and +// never validate case-duplicate names. An unrelated ambiguous pair elsewhere +// in the file (demo/DEMO) must not suppress deleting every credential for the +// unambiguous profile actually being logged out — only the final marker-write +// should fail on that unrelated validation error. +func TestRunAuthLogoutResolvesCandidatesDespiteUnrelatedAmbiguousConfig(t *testing.T) { + storePath := withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + configData := []byte(`{"providers":[{"name":"demo"},{"name":"DEMO"},{"name":"my-xai","catalogId":"xai","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, configData, 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("xai"), oauth.Token{AccessToken: "stored"}); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "catalog-id-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "my-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code == exitSuccess || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("exit = %d stderr = %q, want the unrelated ambiguity surfaced as a truthful marker-update failure", code, stderr.String()) + } + if _, ok, err := store.Load(oauth.ProviderKey("xai")); err != nil || ok { + t.Fatalf("catalog-id OAuth token survived logout despite the unrelated ambiguous config: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("xai"); err != nil || ok { + t.Fatalf("catalog-id API key survived logout despite the unrelated ambiguous config: ok=%v err=%v", ok, err) + } +} + +func TestRunAuthLogoutCleansCredentialsWhenConfigIsAmbiguous(t *testing.T) { + storePath := withAuthStore(t) + configHome := t.TempDir() + 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 @@ -321,3 +804,105 @@ func readCLIConfigFixture(t *testing.T, path string) config.FileConfig { } return cfg } + +// TestRunAuthLogoutLeavesSharedCatalogCredentialsAlone covers jatmn's #725 +// finding that logout cleanup was scoped by catalog id rather than by proven +// profile ownership. Catalog ids are shared by design: stored-key "work-xai", +// stored-key "xai", and keyless "personal-xai" can all carry catalogId "xai". +// Logging out "work-xai" deleted the shared "xai" OAuth token and the "xai" +// profile's API key — another profile's credentials — while clearing only +// work-xai's own marker. +func TestRunAuthLogoutLeavesSharedCatalogCredentialsAlone(t *testing.T) { + storePath := withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + configData := []byte(`{"providers":[` + + `{"name":"work-xai","catalogId":"xai","apiKeyStored":true},` + + `{"name":"xai","catalogId":"xai","apiKeyStored":true},` + + `{"name":"personal-xai","catalogId":"xai"}]}`) + if err := os.WriteFile(configPath, configData, 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: storePath}) + if err != nil { + t.Fatal(err) + } + if err := store.Save(oauth.ProviderKey("xai"), oauth.Token{AccessToken: "shared"}); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "sibling-key"); err != nil { + t.Fatal(err) + } + if err := keyStore.Set("work-xai", "own-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + code := runWithDeps([]string{"auth", "logout", "work-xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := keyStore.Get("work-xai"); err != nil || ok { + t.Fatalf("the profile's own API key must be deleted: ok=%v err=%v", ok, err) + } + if _, ok, err := store.Load(oauth.ProviderKey("xai")); err != nil || !ok { + t.Fatalf("a catalog token three profiles can use must survive one profile's logout: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("xai"); err != nil || !ok { + t.Fatalf("the sibling xai profile's API key must survive: ok=%v err=%v", ok, err) + } +} + +// TestRunAuthLogoutPrefersTheExactlyNamedProfile is the other half of the same +// finding: identity resolution took the first row matching name OR catalog id, +// so `zero auth logout xai` retargeted an earlier {name:"work-xai", +// catalogId:"xai"} row and cleared that profile's marker instead. +func TestRunAuthLogoutPrefersTheExactlyNamedProfile(t *testing.T) { + withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + configData := []byte(`{"providers":[` + + `{"name":"work-xai","catalogId":"xai","apiKeyStored":true},` + + `{"name":"xai","catalogId":"xai","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, configData, 0o600); err != nil { + t.Fatal(err) + } + keyStore, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := keyStore.Set("work-xai", "work-key"); err != nil { + t.Fatal(err) + } + if err := keyStore.Set("xai", "own-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"auth", "logout", "xai"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }); code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + if _, ok, err := keyStore.Get("xai"); err != nil || ok { + t.Fatalf("the exactly named profile's key must be deleted: ok=%v err=%v", ok, err) + } + if _, ok, err := keyStore.Get("work-xai"); err != nil || !ok { + t.Fatalf("an earlier catalog sibling must not be logged out instead: ok=%v err=%v", ok, err) + } + cfg := readFileConfig(t, configPath) + for _, provider := range cfg.Providers { + if provider.Name == "xai" && provider.APIKeyStored { + t.Fatal("the named profile's apiKeyStored marker must be cleared") + } + if provider.Name == "work-xai" && !provider.APIKeyStored { + t.Fatal("the sibling profile's apiKeyStored marker must be left alone") + } + } +} diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index a6fab33ec..4003d57b4 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 += fmt.Sprintf(" (not selectable via providers use; source: %s)", provider.Source) + } 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..7e4c1db38 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -9,6 +9,7 @@ import ( "unicode" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/credstore" "github.com/Gitlawb/zero/internal/providercatalog" "github.com/Gitlawb/zero/internal/provideronboarding" ) @@ -71,10 +72,22 @@ 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) + // A case-only difference is only harmless when it demonstrably selects the + // row just written; a separate exact-case profile from project config makes + // it a real override of a real, different provider. + if override != "" && strings.EqualFold(override, strings.TrimSpace(cfg.ActiveProvider)) && + activeProviderEnvOverrideSelectsSaved(deps, configPath, cfg.ActiveProvider) { + override = "" + } + // An override only becomes the effective provider if Zero can actually + // resolve it; a stale value names nothing and fails the next resolution. + overrideResolution := activeProviderOverrideAbsent + if override != "" { + overrideResolution = activeProviderEnvOverrideResolution(deps, configPath, override) + } if options.json { payload := map[string]any{ "activeProvider": cfg.ActiveProvider, @@ -82,8 +95,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,31 +114,128 @@ 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 +// names a DIFFERENT spelling than the one just selected, meaning the saved +// `providers use` selection may NOT be the effective active provider until the // env var is unset. applyEnv (resolver.go) makes ZERO_PROVIDER win over // config.json unconditionally, so reporting the write as a plain success reads as // a switch that silently has no effect (issue #721). Empty when nothing overrides // (including when getenv is nil, e.g. a test that did not inject the environment). +// +// A case-only difference is NOT assumed here to name the same provider — see +// activeProviderEnvOverrideSelectsSaved, which resolves that question instead of +// guessing at it. func activeProviderEnvOverride(getenv func(string) string, selected string) string { if getenv == nil { return "" } override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) - if override == "" || strings.EqualFold(override, strings.TrimSpace(selected)) { + if override == "" || override == strings.TrimSpace(selected) { return "" } return override } +// activeProviderEnvOverrideSelectsSaved reports whether a case-only ZERO_PROVIDER +// difference actually lands on the row `providers use` just wrote. +// +// Folding the comparison outright was wrong: user config resolves the active row +// case-insensitively, but project config and provider commands contribute their +// own rows, and resolution then selects an exact-case match. With a workspace +// profile literally named "WORK", ZERO_PROVIDER=WORK selects THAT row — different +// credentials, different endpoint — while `zero providers use work` reported a +// clean switch with no override at all. So the suppression is granted only when +// resolution proves the env value produces the very spelling just selected. +func activeProviderEnvOverrideSelectsSaved(deps appDeps, configPath string, selected string) bool { + resolved, ok := resolveActiveProviderWithoutProviderCommand(deps, configPath) + if !ok { + return false + } + return resolved == strings.TrimSpace(selected) +} + +// resolveActiveProviderWithoutProviderCommand resolves the effective active +// provider name without running ZERO_PROVIDER_COMMAND. Provider commands are +// arbitrary external programs and `providers use` must remain a config-only +// operation, so a configured one makes the answer unknowable here (false). +func resolveActiveProviderWithoutProviderCommand(deps appDeps, configPath string) (string, bool) { + if deps.getenv != nil && strings.TrimSpace(deps.getenv(config.ProviderCommandEnv)) != "" { + return "", false + } + workspaceRoot, err := resolveWorkspaceRoot("", deps) + if err != nil { + return "", false + } + options, err := config.DefaultResolveOptions(workspaceRoot) + if err != nil { + return "", false + } + options.UserConfigPath = configPath + options.ProviderCommand = "" + resolved, err := config.Resolve(options) + if err != nil { + return "", false + } + return strings.TrimSpace(resolved.ActiveProvider), true +} + +// activeProviderEnvOverrideResolution checks whether ZERO_PROVIDER resolves +// without running ZERO_PROVIDER_COMMAND. Provider commands are arbitrary +// external programs and `providers use` must remain a config-only operation; +// when one is configured, the final provider is therefore explicitly deferred +// until the next normal resolution instead of being guessed here. +func activeProviderEnvOverrideResolution(deps appDeps, configPath string, override string) activeProviderOverrideResolution { + if deps.getenv != nil && strings.TrimSpace(deps.getenv(config.ProviderCommandEnv)) != "" { + return activeProviderOverrideDeferred + } + resolved, ok := resolveActiveProviderWithoutProviderCommand(deps, configPath) + // Fold case: resolution selects the active row case-insensitively and reports + // the row's canonical persisted spelling, so ZERO_PROVIDER=openrouter against + // a saved "OpenRouter" resolves to "OpenRouter". Comparing exactly called that + // a failed override and told the user Zero could not start with it, which was + // the opposite of true. A fold cannot report a false success here: if the + // active row folds to the override, the override is what selected it. + if !ok || !strings.EqualFold(resolved, 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 { @@ -414,14 +531,49 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exit } } - cfg, err := config.RemoveProvider(configPath, name) + // Read the row BEFORE removal so we know whether it owned the stored-key + // marker — RemoveProvider's returned cfg no longer has it, and the + // credential store's secret is keyed case-folded, so removing a keyed row + // while a case-variant sibling survives must carry the marker over + // instead of silently orphaning the still-shared secret. + before, hadBefore, err := config.ProviderRow(configPath, name) if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Decide up front whether a sibling row will survive this removal holding + // the SAME credential-store identity, because that decides what happens to + // the shared secret: hand the marker over, or delete the key outright. + survivor, survivorSurvives, err := survivingCaseVariantProviderName(configPath, name) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + cfg, err := config.RemoveProvider(configPath, name) + if err != nil { + return writeAppError(stderr, providerMutationError(configPath, name, err), exitCrash) + } + // The marker handoff can only run AFTER the removal, not as a transaction + // around it: while both case-variant rows are present the config is + // ambiguous, and writeConfigFile rejects every write against it — removing + // one row is precisely the repair. So the window is real, and the command + // reports it as a partial failure (nonzero below) instead of success. + // // Delete the key from the store BESIDE the config being edited — the same // store setup/rename write to — not the default-path store, so a - // non-default config path cannot leave the encrypted key behind. - keyRemoved, keyErr := removeStoredProviderKeyAt(configPath, name) + // non-default config path cannot leave the encrypted key behind. Skipped + // when a case variant survives: that row keeps reading the shared entry. + keyRemoved := false + var keyErr error + markerTransferFailed := false + if survivorSurvives { + if hadBefore && before.APIKeyStored { + if _, err := config.TransferProviderAPIKeyStoredMarker(configPath, survivor); err != nil { + keyErr = err + markerTransferFailed = true + } + } + } else { + keyRemoved, keyErr = removeStoredProviderKeyAt(configPath, name) + } if options.json { payload := map[string]any{ "removed": name, @@ -436,13 +588,22 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps if err := writePrettyJSON(stdout, payload); err != nil { return exitCrash } + // A failed handoff left the survivor unable to reach the shared key. + // Scripts must not read that as a completed removal. + if markerTransferFailed { + return exitCrash + } return exitSuccess } if _, err := fmt.Fprintf(stdout, "Removed provider %s\n", name); err != nil { return exitCrash } if keyErr != nil { - if _, err := fmt.Fprintf(stderr, "warning: its stored API key could not be deleted and remains in the credential store: %v\n", keyErr); err != nil { + warning := "its stored API key could not be deleted and remains in the credential store" + if markerTransferFailed { + warning = fmt.Sprintf("the stored API key marker could not be handed to %s, which shares its credential entry, so the shared key is now unreachable", survivor) + } + if _, err := fmt.Fprintf(stderr, "warning: %s: %v\n", warning, keyErr); err != nil { return exitCrash } } else if keyRemoved { @@ -459,6 +620,9 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exitCrash } } + if markerTransferFailed { + return exitCrash + } return exitSuccess } @@ -473,6 +637,36 @@ func removeStoredProviderKeyAt(configPath string, provider string) (bool, error) return store.Delete(provider) } +// survivingCaseVariantProviderName returns the exact name of another persisted +// row that shares name's CREDENTIAL-STORE identity — a case-only variant that +// will still read the same stored secret once name's row is gone — and whether +// one was found. config.RemoveProvider matches its row exactly, so any row with +// a different exact spelling survives. +// +// The comparison is credstore.NormalizeProvider, not strings.EqualFold, because +// only the former is the store's own equivalence rule. Unicode case folding +// equates "s" and "ſ" while strings.ToLower does not, so EqualFold would let +// `providers remove s` skip deleting the key and hand the marker to "ſ", whose +// lookup uses a different store entry — orphaning the secret and leaving the +// survivor with a marker pointing at nothing. +func survivingCaseVariantProviderName(configPath string, name string) (string, bool, error) { + names, err := config.PersistedProviderNames(configPath) + if err != nil { + return "", false, err + } + removed := strings.TrimSpace(name) + target := credstore.NormalizeProvider(removed) + for _, candidate := range names { + if candidate == removed { + continue + } + if credstore.NormalizeProvider(candidate) == target { + return candidate, true, nil + } + } + return "", false, nil +} + // 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 +696,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 +720,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 +783,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 +824,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 +860,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..d50c1e5e4 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,83 @@ func TestRunProvidersUseNoWarnWithoutEnvOverride(t *testing.T) { } } +// TestActiveProviderEnvOverrideReportsEveryDistinctSpelling pins the pure +// function's contract after jatmn's #725 round: a case-only difference is no +// longer silently treated as "the same provider". Deciding that needs the +// resolver (see TestRunProvidersUseCaseVariantEnvOverrideFollowsResolution), +// so this layer reports every spelling that is not literally the selection. +func TestActiveProviderEnvOverrideReportsEveryDistinctSpelling(t *testing.T) { + getenv := func(value string) func(string) string { + return func(key string) string { + if key == config.ActiveProviderEnv { + return value + } + return "" + } + } + if override := activeProviderEnvOverride(getenv("work"), "work"); override != "" { + t.Fatalf("activeProviderEnvOverride() = %q, want no override when the env names the selection exactly", override) + } + if override := activeProviderEnvOverride(getenv("WORK"), "work"); override != "WORK" { + t.Fatalf("activeProviderEnvOverride() = %q, want the case-distinct spelling reported to the resolver", override) + } + if override := activeProviderEnvOverride(getenv("fast"), "work"); override != "fast" { + t.Fatalf("activeProviderEnvOverride() = %q, want the genuinely different provider reported", override) + } +} + +// TestRunProvidersUseCaseVariantEnvOverrideFollowsResolution is the regression +// test for jatmn's #725 finding that a case-distinct ZERO_PROVIDER was hidden +// unconditionally. Folding is right only when the env value lands on the row +// `providers use` just wrote; a project config contributing a separate +// exact-case "WORK" profile makes it a real override of a different provider, +// with different credentials, and that must still be reported. +func TestRunProvidersUseCaseVariantEnvOverrideFollowsResolution(t *testing.T) { + run := func(t *testing.T, projectProviders []config.ProviderProfile) string { + t.Helper() + // The resolver reads the real process environment, so the override has + // to be set for real, and the user config has to sit at the default path. + t.Setenv(config.ActiveProviderEnv, "WORK") + configPath := providersUseOverrideConfigAtDefaultUserPath(t) + workspace := t.TempDir() + if len(projectProviders) > 0 { + projectPath := filepath.Join(workspace, ".zero", "config.json") + writeProviderOnboardingConfig(t, projectPath, config.FileConfig{Providers: projectProviders}) + } + deps := providerSetupDeps(configPath) + deps.getwd = func() (string, error) { return workspace, nil } + deps.getenv = func(key string) string { + if key == config.ActiveProviderEnv { + return "WORK" + } + return "" + } + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "work"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + return stderr.String() + } + + t.Run("case variant of the selection", func(t *testing.T) { + if note := run(t, nil); note != "" { + t.Fatalf("an override that resolves to the selected row must not warn: %q", note) + } + }) + + t.Run("case-distinct project profile", func(t *testing.T) { + note := run(t, []config.ProviderProfile{{ + Name: "WORK", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://api.example.com/v1", + Model: "example-model", + }}) + if !strings.Contains(note, "WORK") || !strings.Contains(note, config.ActiveProviderEnv) { + t.Fatalf("a separate exact-case profile must be reported as an override: %q", note) + } + }) +} + func TestRunProvidersUseSurfacesMalformedConfig(t *testing.T) { var stdout, stderr bytes.Buffer configPath := filepath.Join(t.TempDir(), "config.json") @@ -480,3 +921,130 @@ 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) + } +} + +// TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant covers the +// inverse of TestRunProvidersRemoveCaseDuplicateKeepsSurvivorKey: the removed +// row (not the survivor) owns apiKeyStored. Without transferring the marker, +// the still-shared credential-store secret becomes unreachable through the +// surviving row even though it was never deleted. +func TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + seed := `{"activeProvider":"WORK","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK"}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "shared-key"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + deps := appDeps{userConfigPath: func() (string, error) { return configPath, nil }} + if code := runWithDeps([]string{"providers", "remove", "work", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("remove failed: code=%d stderr=%s", code, stderr.String()) + } + if key, ok, err := store.Get("work"); err != nil || !ok || key != "shared-key" { + t.Fatalf("shared provider key = %q, %v, %v; want retained", key, ok, err) + } + var payload struct { + KeyRemoved bool `json:"keyRemoved"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil || payload.KeyRemoved { + t.Fatalf("payload = %s, err = %v; shared key must not be reported removed", stdout.String(), err) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + var cfg config.FileConfig + if err := json.Unmarshal(after, &cfg); err != nil { + t.Fatal(err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "WORK" || !cfg.Providers[0].APIKeyStored { + t.Fatalf("survivor must inherit apiKeyStored marker, got %+v", cfg.Providers) + } +} + +// TestRunProvidersRemoveUsesCredentialStoreEquivalenceForSurvivors covers +// jatmn's #725 finding that survivor detection compared with strings.EqualFold +// while the credential store keys entries with trimmed strings.ToLower. The two +// relations differ: Unicode case folding equates "s" and "ſ", ToLower does not. +// Both spellings therefore pass persisted-name validation as separate rows with +// separate store entries, but the EqualFold check called "ſ" a survivor of +// removing "s" — skipping the key deletion (orphaning the secret) and handing +// the marker to a row whose lookup uses a different entry entirely. +func TestRunProvidersRemoveUsesCredentialStoreEquivalenceForSurvivors(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + seed := `{"activeProvider":"s","providers":[{"name":"s","apiKeyStored":true},{"name":"\u017f"}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("s", "long-s-is-not-s"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + deps := appDeps{userConfigPath: func() (string, error) { return configPath, nil }} + if code := runWithDeps([]string{"providers", "remove", "s", "--json"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("remove failed: code=%d stderr=%s", code, stderr.String()) + } + if _, ok, err := store.Get("s"); err != nil || ok { + t.Fatalf("key = ok:%v err:%v; the removed row owned its own store entry, which must be deleted", ok, err) + } + var payload struct { + KeyRemoved bool `json:"keyRemoved"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil || !payload.KeyRemoved { + t.Fatalf("payload = %s, err = %v; want keyRemoved true", stdout.String(), err) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "\u017f" { + t.Fatalf("providers = %+v, want only the distinct long-s row", cfg.Providers) + } + if cfg.Providers[0].APIKeyStored { + t.Fatal("a row with its own credential-store identity must not inherit another row's marker") + } +} diff --git a/internal/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..d4e5d6816 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -99,7 +99,44 @@ 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 + } + } + if !changed { + return false, nil + } + return true, writeConfigFile(path, cfg) +} + +// ClearProviderKeyStoredCaseVariants unsets the APIKeyStored marker on every +// row whose name normalizes to the same credential-store identity as provider, +// not just an exact-spelling match. Deleting the shared secret for one +// case-variant row (e.g. "WORK") must also clear the marker on any sibling row +// ("work") that pointed at the same now-gone entry — leaving it set would claim +// a key is available when ApplyStoredAPIKey's store lookup will always miss. +func ClearProviderKeyStoredCaseVariants(path, provider string) (bool, error) { + path = strings.TrimSpace(path) + provider = strings.TrimSpace(provider) + if path == "" || provider == "" { + return false, nil + } + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return false, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + changed := false + providerIdentity := credstore.NormalizeProvider(provider) + for index := range cfg.Providers { + if credstore.NormalizeProvider(cfg.Providers[index].Name) == providerIdentity && cfg.Providers[index].APIKeyStored { cfg.Providers[index].APIKeyStored = false changed = true } diff --git a/internal/config/credentials_test.go b/internal/config/credentials_test.go index d627a5cb4..6c1fcebf6 100644 --- a/internal/config/credentials_test.go +++ b/internal/config/credentials_test.go @@ -157,6 +157,36 @@ 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 TestClearProviderKeyStoredCaseVariantsPreservesDistinctUnicodeIdentity(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"s","apiKeyStored":true},{"name":"ſ","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + + cleared, err := ClearProviderKeyStoredCaseVariants(path, "s") + if err != nil || !cleared { + t.Fatalf("clear = %v,%v; want true,nil", cleared, err) + } + cfg := readConfigFixture(t, path) + if cfg.Providers[0].APIKeyStored { + t.Fatal("s marker should be cleared") + } + if !cfg.Providers[1].APIKeyStored { + t.Fatal("long-s marker belongs to a distinct credential-store identity and must remain set") + } } func TestProviderProfileAPIKeyStoredRoundTrips(t *testing.T) { 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 7c54cc63a..8b2576f49 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -8,9 +8,343 @@ import ( "sort" "strings" + "github.com/Gitlawb/zero/internal/credstore" "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. +// +// A repeated folded identity is rejected whether or not the spellings differ. +// Exact duplicates are just as broken as case variants: resolver merging +// silently coalesces the rows, and plaintext-key migration writes both values +// into the same normalized credential-store entry, so the second row's key +// overwrites the first. +func ValidatePersistedProviderNames(cfg FileConfig) error { + seen := make(map[string]string, len(cfg.Providers)) + for _, provider := range cfg.Providers { + name := strings.TrimSpace(provider.Name) + folded := credstore.NormalizeProvider(name) + previous, ok := seen[folded] + if ok && previous == name { + return fmt.Errorf("duplicate persisted provider name %q; remove one of the rows in config.json", name) + } + if ok { + return fmt.Errorf("ambiguous persisted provider names %q and %q differ only by case; rename or remove one row in config.json", previous, name) + } + seen[folded] = name + } + return nil +} + +// sameProviderIdentity reports whether two persisted spellings name the same +// provider identity. It is credstore.NormalizeProvider — the credential store's +// own rule — rather than strings.EqualFold, because the two disagree and the +// store is the authority: EqualFold folds "s" and Unicode long-s "ſ" together, +// while the store keeps separate entries for them. Treating them as one identity +// let a mutation of one profile reach the other's row and its secret, which is +// precisely what ValidatePersistedProviderNames permits as a distinct pair. +func sameProviderIdentity(a string, b string) bool { + return credstore.NormalizeProvider(a) == credstore.NormalizeProvider(b) +} + +// PreflightUserConfig validates existing user config before any command makes +// credential-store side effects. +func PreflightUserConfig(path string) error { + 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) AND a persisted row owns that very NAME under a different +// spelling. That is the case where the two spellings are the same provider by +// construction — a re-setup or re-login of, say, openrouter against a row saved +// as "OpenRouter" — and where EnsureCatalogProvider already reuses the row on +// the login path. A user-chosen name is left alone on purpose: there, a case +// collision with an existing row is a real collision, and silently overwriting +// the other row would be worse than the error. +// +// Adoption deliberately does NOT follow the catalog id of an arbitrary row. +// Several profiles may legitimately share one catalog provider — {name: +// "work-xai", catalogId: "xai"} alongside a plain "xai" — so retargeting the +// default "xai" profile at whichever row happened to list catalogId "xai" would +// silently overwrite that row's endpoint, model, and stored key. An exactly +// spelled existing row also short-circuits: there is nothing to adopt, the +// caller's own spelling is already the persisted one. +// +// Nor is a matching NAME on its own proof of ownership. A custom profile is free +// to be called {name: "OpenRouter", catalogId: "custom-openai-compatible", +// baseURL: "https://corp.example/v1"}; adopting it for `zero providers add +// openrouter` would hand that row to UpsertProvider, whose merge overwrites the +// catalog id, endpoint, model, transport and headers with OpenRouter's defaults +// while a stored-key marker survives the rewrite. So the row's catalog identity +// must agree too, and a row that declares no catalog id at all is accepted only +// because its name is the sole identity it has (the legacy shape this function +// exists for). Anything else keeps the requested name and lets the write report +// the collision instead of mutating a different profile. +func AdoptPersistedCatalogProviderName(path string, profile ProviderProfile) (ProviderProfile, error) { + catalogID := strings.TrimSpace(profile.CatalogID) + name := strings.TrimSpace(profile.Name) + if catalogID == "" || !sameProviderIdentity(name, catalogID) { + return profile, nil + } + providers, err := persistedProviders(path) + if err != nil { + return profile, err + } + canonical := "" + variants := 0 + for _, row := range providers { + rowName := strings.TrimSpace(row.Name) + if !sameProviderIdentity(rowName, name) { + continue + } + if rowName == name { + return profile, nil + } + if rowCatalogID := strings.TrimSpace(row.CatalogID); rowCatalogID != "" && + !sameProviderIdentity(rowCatalogID, catalogID) { + // A different provider that happens to carry this name. + return profile, nil + } + canonical = rowName + variants++ + } + if variants == 1 { + profile.Name = canonical + } + return profile, nil +} + +// PersistedProviderNames returns the exact name of every row in the persisted +// user config, in file order. Callers that must reason about the SET of saved +// rows — e.g. deciding whether removing one row leaves a case variant behind +// that still reads the same credential-store entry — get the raw names here +// rather than re-implementing FileConfig parsing. +func PersistedProviderNames(path string) ([]string, error) { + providers, err := persistedProviders(path) + if err != nil { + return nil, err + } + names := make([]string, 0, len(providers)) + for _, provider := range providers { + names = append(names, strings.TrimSpace(provider.Name)) + } + return names, nil +} + +// persistedProviders reads the provider rows out of the user config at path. +// A missing file is an empty list, not an error: every caller here asks "what +// is already saved?", and "nothing yet" is a legitimate answer. +func persistedProviders(path string) ([]ProviderProfile, error) { + data, err := os.ReadFile(strings.TrimSpace(path)) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("invalid config JSON %s: %w", path, err) + } + return cfg.Providers, nil +} + +// PersistedProviderIdentity reports whether a persisted user-config row already +// owns identity — as its name (case-insensitively) or as its catalog id — and +// returns that row's exact name. +// +// 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) { + row, match, err := ResolvePersistedProviderIdentity(path, identity) + if err != nil || match == PersistedIdentityNone { + return "", false, err + } + return strings.TrimSpace(row.Name), true, nil +} + +// PersistedIdentityMatch says HOW a persisted row was addressed, because the +// two ways carry different authority. A name match identifies exactly one +// profile. A catalog-id match identifies "some profile of this kind" — several +// profiles may legitimately share one catalog provider — so callers that go on +// to delete credentials must not treat it as proof of ownership. +type PersistedIdentityMatch uint8 + +const ( + // PersistedIdentityNone means no row owns the identity, or a catalog id was + // given that more than one row claims (an ambiguous request this package + // refuses to guess at). + PersistedIdentityNone PersistedIdentityMatch = iota + // PersistedIdentityName means a row's own name matched, exactly or as a + // case variant. + PersistedIdentityName + // PersistedIdentityCatalog means the identity matched only the catalog id of + // exactly one row. + PersistedIdentityCatalog +) + +// ResolvePersistedProviderIdentity finds the persisted user-config row that +// owns identity and reports how it was addressed. +// +// Names win over catalog ids, and an exact name wins over a case variant. +// Scanning in file order and accepting the first name-OR-catalog hit picked +// whichever row came first, so `zero auth logout xai` against +// [{name:"work-xai", catalogId:"xai"}, {name:"xai"}] resolved to "work-xai" — +// a different profile, with different credentials, than the one named. +// +// A catalog id claimed by more than one row resolves to nothing rather than to +// an arbitrary winner: the caller asked for something the config does not +// uniquely identify, and guessing there is what let one profile's logout delete +// a sibling's shared token. +func ResolvePersistedProviderIdentity(path, identity string) (ProviderProfile, PersistedIdentityMatch, error) { + identity = strings.TrimSpace(identity) + if identity == "" { + return ProviderProfile{}, PersistedIdentityNone, nil + } + providers, err := persistedProviders(path) + if err != nil { + return ProviderProfile{}, PersistedIdentityNone, err + } + var foldedName *ProviderProfile + var catalogRow *ProviderProfile + catalogMatches := 0 + for index := range providers { + row := providers[index] + name := strings.TrimSpace(row.Name) + if name == identity { + return row, PersistedIdentityName, nil + } + if foldedName == nil && sameProviderIdentity(name, identity) { + match := row + foldedName = &match + } + if sameProviderIdentity(row.CatalogID, identity) { + catalogMatches++ + if catalogRow == nil { + match := row + catalogRow = &match + } + } + } + if foldedName != nil { + return *foldedName, PersistedIdentityName, nil + } + if catalogMatches == 1 { + return *catalogRow, PersistedIdentityCatalog, nil + } + return ProviderProfile{}, PersistedIdentityNone, nil +} + +// CatalogIdentityExclusive reports whether catalogID is claimed by the row +// named owner and by nothing else in the persisted config — neither as another +// row's name nor as another row's catalog id. +// +// Credential cleanup uses this before treating the catalog id as one of the +// target profile's own credential keys. With stored-key "work-xai", +// stored-key "xai", and keyless "personal-xai" all carrying catalogId "xai", +// the "xai" token and key belong to whoever logged in under that spelling — +// deleting them while logging out of "work-xai" takes down a sibling's login. +func CatalogIdentityExclusive(path, catalogID, owner string) (bool, error) { + catalogID = strings.TrimSpace(catalogID) + owner = strings.TrimSpace(owner) + if catalogID == "" || owner == "" { + return false, nil + } + providers, err := persistedProviders(path) + if err != nil { + return false, err + } + for _, row := range providers { + name := strings.TrimSpace(row.Name) + if name == owner { + continue + } + if sameProviderIdentity(name, catalogID) || sameProviderIdentity(row.CatalogID, catalogID) { + return false, nil + } + } + return true, 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 sameProviderIdentity(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 +363,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 sameProviderIdentity(existing.Name, profile.Name) && strings.TrimSpace(existing.Name) != profile.Name { + return FileConfig{}, fmt.Errorf("provider %q already exists as %q; provider names must be unique case-insensitively", profile.Name, existing.Name) + } + } mergeProvider(&cfg, profile) // mergeProfile deliberately ignores APIKeyStored — during resolve-time @@ -91,8 +433,8 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro return EnsuredProvider{}, fmt.Errorf("read config %s: %w", path, err) } for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.CatalogID), descriptor.ID) || - strings.EqualFold(strings.TrimSpace(provider.Name), descriptor.ID) { + if sameProviderIdentity(provider.CatalogID, descriptor.ID) || + sameProviderIdentity(provider.Name, descriptor.ID) { return EnsuredProvider{Name: provider.Name, Active: cfg.ActiveProvider}, nil } } @@ -133,8 +475,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 +510,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 +537,97 @@ 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.TrimSpace(provider.Name) == name { + return true, nil + } + } + return false, nil +} + +// ProviderRow returns the persisted user-config row whose Name matches name +// exactly, and whether one was found. Unlike ProviderPersisted's yes/no, this +// hands back the row itself (CatalogID, APIKeyStored, ...) so callers that +// need more than presence — e.g. deciding whether a stored key marker must be +// carried over to a surviving case-variant row before it is removed — don't +// each re-implement FileConfig parsing. +func ProviderRow(path string, name string) (ProviderProfile, bool, error) { + path = strings.TrimSpace(path) + name = strings.TrimSpace(name) + if path == "" || name == "" { + return ProviderProfile{}, false, nil + } + if _, err := os.Stat(path); os.IsNotExist(err) { + return ProviderProfile{}, false, nil + } else if err != nil { + return ProviderProfile{}, false, fmt.Errorf("stat config %s: %w", path, err) + } + cfg, err := loadConfigFile(path) + if err != nil { + return ProviderProfile{}, false, err + } + for _, provider := range cfg.Providers { + if strings.TrimSpace(provider.Name) == name { + return provider, true, nil + } + } + return ProviderProfile{}, false, nil +} + +// TransferProviderAPIKeyStoredMarker sets the APIKeyStored marker on the row +// named to, without touching any other field on it. The credential store keys +// its secrets case-folded, so removing a case-variant row that owned the +// marker leaves the shared secret orphaned unless a surviving case-variant +// row is marked to take over reading it. No-op (false, nil) when to isn't +// found or already carries the marker. +func TransferProviderAPIKeyStoredMarker(path string, to string) (bool, error) { + path = strings.TrimSpace(path) + to = strings.TrimSpace(to) + if path == "" || to == "" { + return false, nil + } cfg, err := loadConfigFile(path) if err != nil { return false, err } + for index := range cfg.Providers { + if strings.TrimSpace(cfg.Providers[index].Name) == to { + if cfg.Providers[index].APIKeyStored { + return false, nil + } + cfg.Providers[index].APIKeyStored = true + return true, writeConfigFile(path, cfg) + } + } + return false, fmt.Errorf("provider %q not found", to) +} + +// ProviderPersistedCaseInsensitive reports whether any persisted user-config +// row has the same folded name. CLI runtime-only guidance uses this to avoid +// describing a case-variant typo of a saved profile as environment-derived. +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), name) { + if sameProviderIdentity(provider.Name, name) { return true, nil } } @@ -229,9 +659,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 +673,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 sameProviderIdentity(providerName, 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,22 +727,28 @@ 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 } - if strings.EqualFold(providerName, newName) { + if sameProviderIdentity(providerName, newName) { return FileConfig{}, fmt.Errorf("provider %q already exists", newName) } } if index < 0 { return FileConfig{}, fmt.Errorf("provider %q not found", oldName) } - if strings.EqualFold(oldName, newName) && cfg.Providers[index].Name == newName { + if sameProviderIdentity(oldName, newName) && cfg.Providers[index].Name == newName { return cfg, nil } @@ -307,7 +760,7 @@ func RenameProvider(path string, oldName string, newName string) (FileConfig, er } keyMigrated = true } - if strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { + if sameProviderIdentity(cfg.ActiveProvider, previousName) { cfg.ActiveProvider = newName } cfg.Providers[index].Name = newName @@ -325,7 +778,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 +797,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,15 +821,19 @@ 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 + newIdentity := credstore.NormalizeProvider(newName) for i, provider := range cfg.Providers { providerName := strings.TrimSpace(provider.Name) - if strings.EqualFold(providerName, oldName) { + if providerName == oldName { index = i continue } - if strings.EqualFold(providerName, newName) { + if credstore.NormalizeProvider(providerName) == newIdentity { return FileConfig{}, fmt.Errorf("provider %q already exists", newName) } } @@ -400,7 +856,7 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { } keyMigrated = true } - if renamed && strings.EqualFold(strings.TrimSpace(cfg.ActiveProvider), strings.TrimSpace(previousName)) { + if renamed && sameProviderIdentity(cfg.ActiveProvider, previousName) { cfg.ActiveProvider = newName } @@ -440,7 +896,7 @@ func migrateStoredProviderKey(configPath string, oldName string, newName string) // (groq -> Groq) targets ONE entry: Set(new) rewrites it in place and // Delete(old) would then remove the key that was just "moved". Nothing to // migrate — the existing entry already serves the new name. - if strings.EqualFold(strings.TrimSpace(oldName), strings.TrimSpace(newName)) { + if sameProviderIdentity(oldName, newName) { return nil } store, err := ProviderKeyStoreAt(filepath.Dir(configPath)) @@ -485,8 +941,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 +1194,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..83ff1b745 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") @@ -1021,3 +1233,253 @@ func TestEditProviderRejectsCollisionAndUnknown(t *testing.T) { t.Fatalf("config was rewritten by a rejected edit") } } + +// TestValidatePersistedProviderNamesRejectsExactDuplicates covers jatmn's #725 +// finding: the validator only rejected a repeated folded name when the +// SPELLINGS differed, so two rows literally named "work" passed. That breaks +// the same one-credential-per-folded-name invariant the case check protects — +// resolver merging coalesces the rows, and plaintext-key migration writes both +// values into one normalized credential-store entry, overwriting the first key. +func TestValidatePersistedProviderNamesRejectsExactDuplicates(t *testing.T) { + for name, providers := range map[string][]ProviderProfile{ + "identical spellings": {{Name: "work"}, {Name: "work"}}, + "same after trimming": {{Name: "work"}, {Name: " work "}}, + } { + t.Run(name, func(t *testing.T) { + err := ValidatePersistedProviderNames(FileConfig{Providers: providers}) + if err == nil { + t.Fatal("a repeated folded provider identity must be rejected") + } + if want := `duplicate persisted provider name "work"`; !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want it to contain %q", err, want) + } + }) + } + if err := ValidatePersistedProviderNames(FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "fast"}}}); err != nil { + t.Fatalf("distinct names must validate: %v", err) + } +} + +// TestAdoptPersistedCatalogProviderNameIgnoresCatalogSiblings covers jatmn's +// #725 finding: adoption followed PersistedProviderIdentity, which returned the +// first row sharing the catalog id. Several profiles may legitimately use one +// catalog provider, so adding the default "xai" profile silently retargeted an +// existing {name:"work-xai", catalogId:"xai"} row and would have overwritten its +// endpoint, model, and stored key. +func TestAdoptPersistedCatalogProviderNameIgnoresCatalogSiblings(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v1", Model: "m"}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "xai", CatalogID: "xai"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "xai" { + t.Fatalf("Name = %q, want the default spelling kept — a catalog sibling is not the same profile", adopted.Name) + } +} + +// TestAdoptPersistedCatalogProviderNameFollowsCaseVariantOfTheDefaultName is the +// behaviour adoption exists for: a re-setup of "openrouter" against a row the +// user saved as "OpenRouter" must UPDATE that row instead of colliding with it. +func TestAdoptPersistedCatalogProviderNameFollowsCaseVariantOfTheDefaultName(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "OpenRouter", CatalogID: "openrouter", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://openrouter.ai/api/v1", Model: "m"}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "openrouter", CatalogID: "openrouter"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "OpenRouter" { + t.Fatalf("Name = %q, want the persisted case variant adopted", adopted.Name) + } +} + +// TestAdoptPersistedCatalogProviderNameIgnoresForeignCatalogRow covers jatmn's +// #725 finding that a case-folded NAME match was taken as proof of ownership. +// A custom profile may legitimately be called "OpenRouter" while pointing at a +// different provider entirely; adopting it for `zero providers add openrouter` +// handed that row to UpsertProvider, whose merge overwrites the catalog id, +// endpoint, model and transport with OpenRouter's defaults while a stored-key +// marker survives the rewrite. +func TestAdoptPersistedCatalogProviderNameIgnoresForeignCatalogRow(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "OpenRouter", CatalogID: "custom-openai-compatible", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://corp.example/v1", Model: "corp-model", APIKeyStored: true}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "openrouter", CatalogID: "openrouter"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "openrouter" { + t.Fatalf("Name = %q, want the requested spelling kept — a differently-catalogued row is another provider", adopted.Name) + } + // And the write that follows must report the collision rather than rewriting + // the custom row in place. + if err := PreflightProviderWrite(path, adopted.Name); err == nil { + t.Fatal("PreflightProviderWrite accepted a name that collides with the custom row; want a reported collision") + } +} + +// TestAdoptPersistedCatalogProviderNameFollowsNameOnlyRow keeps the legacy shape +// adoption exists for: a row whose only identity is its name. +func TestAdoptPersistedCatalogProviderNameFollowsNameOnlyRow(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "OpenRouter", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://openrouter.ai/api/v1", Model: "m"}, + }, + }, 0o600) + + adopted, err := AdoptPersistedCatalogProviderName(path, ProviderProfile{Name: "openrouter", CatalogID: "openrouter"}) + if err != nil { + t.Fatalf("adopt: %v", err) + } + if adopted.Name != "OpenRouter" { + t.Fatalf("Name = %q, want the persisted case variant adopted", adopted.Name) + } +} + +// TestPersistedProviderIdentityRulesMatchTheCredentialStore pins the identity +// contract this PR introduced across every persisted-config path at once. +// strings.EqualFold folds "s" and Unicode long-s "ſ" together while +// credstore.NormalizeProvider (the store's own rule, and the rule +// ValidatePersistedProviderNames enforces) keeps them apart, so the two +// spellings are separate profiles with separate secrets. Mixing the two +// comparisons made one profile's mutation reach the other's row: destructive +// logout expansion adopted the unrelated row, while ordinary writes rejected +// the pair as a collision. +func TestPersistedProviderIdentityRulesMatchTheCredentialStore(t *testing.T) { + const longS = "ſ" + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "s", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://s.example/v1", Model: "m", APIKeyStored: true}, + }, + }, 0o600) + + t.Run("identity resolution does not adopt the distinct spelling", func(t *testing.T) { + if _, match, err := ResolvePersistedProviderIdentity(path, longS); err != nil || match != PersistedIdentityNone { + t.Fatalf("ResolvePersistedProviderIdentity(%q) = %v, %v; want no match", longS, match, err) + } + }) + + t.Run("a distinct spelling is writable, not a collision", func(t *testing.T) { + if err := PreflightProviderWrite(path, longS); err != nil { + t.Fatalf("PreflightProviderWrite(%q) = %v; want the distinct identity accepted", longS, err) + } + cfg, err := UpsertProvider(path, ProviderProfile{Name: longS, ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://long-s.example/v1", Model: "m"}, false) + if err != nil { + t.Fatalf("UpsertProvider(%q) = %v; want the distinct identity accepted", longS, err) + } + if len(cfg.Providers) != 2 { + t.Fatalf("providers = %+v, want both distinct rows saved", cfg.Providers) + } + }) + + t.Run("a case variant is still a collision", func(t *testing.T) { + if err := PreflightProviderWrite(path, "S"); err == nil { + t.Fatal("PreflightProviderWrite(\"S\") accepted a case variant of a saved row") + } + }) +} + +// TestResolvePersistedProviderIdentityPrefersNames covers jatmn's #725 finding +// that identity resolution took the first row matching EITHER field, so a +// catalog id on an earlier row outranked a later row with the exact name. +func TestResolvePersistedProviderIdentityPrefersNames(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai"}, + {Name: "xai", CatalogID: "xai"}, + }, + }, 0o600) + + t.Run("exact name beats an earlier catalog id", func(t *testing.T) { + row, match, err := ResolvePersistedProviderIdentity(path, "xai") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if match != PersistedIdentityName || row.Name != "xai" { + t.Fatalf("row = %q match = %v, want the exactly named row", row.Name, match) + } + }) + + t.Run("a shared catalog id resolves to nothing", func(t *testing.T) { + _, match, err := ResolvePersistedProviderIdentity(path, "XAI") + if err != nil { + t.Fatalf("resolve: %v", err) + } + // "XAI" folds to the "xai" row's NAME, so that wins; the point of the + // exclusivity rule shows on a catalog id nothing is named after. + if match != PersistedIdentityName { + t.Fatalf("match = %v, want the case-variant name match", match) + } + }) + + t.Run("unique catalog id still resolves", func(t *testing.T) { + unique := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, unique, FileConfig{ + Providers: []ProviderProfile{{Name: "my-router", CatalogID: "openrouter"}}, + }, 0o600) + row, match, err := ResolvePersistedProviderIdentity(unique, "openrouter") + if err != nil { + t.Fatalf("resolve: %v", err) + } + if match != PersistedIdentityCatalog || row.Name != "my-router" { + t.Fatalf("row = %q match = %v, want the sole catalog owner", row.Name, match) + } + }) + + t.Run("an ambiguous catalog id resolves to nothing", func(t *testing.T) { + shared := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, shared, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai"}, + {Name: "personal-xai", CatalogID: "xai"}, + }, + }, 0o600) + if _, match, err := ResolvePersistedProviderIdentity(shared, "xai"); err != nil || match != PersistedIdentityNone { + t.Fatalf("match = %v err = %v, want no guess at a shared catalog id", match, err) + } + }) +} + +// TestCatalogIdentityExclusive guards the rule credential cleanup depends on: +// a catalog id claimed by any other row is not the target profile's own key. +func TestCatalogIdentityExclusive(t *testing.T) { + shared := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, shared, FileConfig{ + Providers: []ProviderProfile{ + {Name: "work-xai", CatalogID: "xai"}, + {Name: "xai", CatalogID: "xai"}, + {Name: "personal-xai", CatalogID: "xai"}, + }, + }, 0o600) + if exclusive, err := CatalogIdentityExclusive(shared, "xai", "work-xai"); err != nil || exclusive { + t.Fatalf("exclusive = %v err = %v, want false for a catalog id three rows claim", exclusive, err) + } + + sole := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, sole, FileConfig{ + Providers: []ProviderProfile{ + {Name: "my-router", CatalogID: "openrouter"}, + {Name: "work", CatalogID: "xai"}, + }, + }, 0o600) + if exclusive, err := CatalogIdentityExclusive(sole, "openrouter", "my-router"); err != nil || !exclusive { + t.Fatalf("exclusive = %v err = %v, want true when only the owner claims the id", exclusive, err) + } +} diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index 96530dbc5..428c8605a 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -265,5 +265,16 @@ func (s *Store) write(data map[string]string) error { } func normalizeProvider(provider string) string { + return NormalizeProvider(provider) +} + +// NormalizeProvider is the credential-store's provider-name equivalence rule: +// entries are keyed by the trimmed, lowercased name. Callers that decide +// whether two provider spellings share one stored secret (e.g. removing a +// case-variant row while a sibling survives) must compare with THIS function +// rather than strings.EqualFold — the two relations are not the same. Unicode +// case folding equates "s" and "ſ", strings.ToLower does not, so an EqualFold +// comparison can promise a survivor access to a key it cannot look up. +func NormalizeProvider(provider string) string { return strings.ToLower(strings.TrimSpace(provider)) } diff --git a/internal/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..0537779f9 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)} } } } @@ -571,8 +575,11 @@ type setupOAuthDeviceMsg struct { err error } -func setupDevicePrepareCmd(name string) tea.Cmd { +func setupDevicePrepareCmd(name string, configPath ...string) tea.Cmd { return func() tea.Msg { + if err := preflightOAuthUserConfig(firstString(configPath)); err != nil { + return setupOAuthDeviceMsg{providerID: name, err: err} + } auth, cfg, err := oauthDevicePrepare(name) if err != nil { return setupOAuthDeviceMsg{providerID: name, err: err} @@ -587,9 +594,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)} } } @@ -604,7 +615,7 @@ func (m model) startSetupDeviceLogin(descriptor providercatalog.Descriptor) (tea m.setup.oauthErr = "" m.setup.deviceUserCode = "" m.setup.deviceVerificationURI = "" - return m, setupDevicePrepareCmd(descriptor.ID) + return m, setupDevicePrepareCmd(descriptor.ID, m.setup.configPath) } // applySetupOAuthDeviceCode handles phase 1 of device-code login: show the code, @@ -626,7 +637,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 @@ -652,9 +663,12 @@ func (m model) applySetupOAuth(msg setupOAuthMsg) (tea.Model, tea.Cmd) { if msg.tokenLogin { // Early persist on the Update goroutine (see persistOAuthLoginProvider's // threading contract) so quitting setup after the login doesn't lose it; - // completeSetup persists the full profile with the chosen model anyway, - // so a failure here is recoverable and not fatal to setup. - _ = persistOAuthLoginProvider(m.setup.configPath, msg.providerID) + // do not advance when that write fails or the stored token would have no + // reachable provider profile after setup exits. + if err := persistOAuthLoginProvider(m.setup.configPath, msg.providerID); err != nil { + m.setup.oauthErr = "Signed in, but the provider profile could not be saved: " + redaction.ErrorMessage(err, redaction.Options{}) + return m, nil + } } m.setup.oauthErr = "" m.setup.err = "" @@ -790,7 +804,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 { @@ -874,6 +888,9 @@ func (m model) completeSetup() (tea.Model, tea.Cmd) { }) if err != nil { m.setup.err = err.Error() + if m.setup.oauthMode && m.setupProviderDescriptor().OAuthMintsKey && strings.TrimSpace(apiKey) != "" { + m.setup.err = oauthMintedKeySaveError(m.setup.err, apiKey) + } return m, nil } @@ -903,6 +920,10 @@ func (m model) completeSetup() (tea.Model, tea.Cmd) { return m.exitSetupToChat() } +func oauthMintedKeySaveError(message, apiKey string) string { + return fmt.Sprintf("%s\nThe minted API key was not saved. Copy it now before leaving Zero:\n%s", message, apiKey) +} + func (m *model) resetSetupModels() { option := m.setupProvider() provider := setupProviderDescriptor(option) diff --git a/internal/tui/onboarding_test.go b/internal/tui/onboarding_test.go index 1f70616fb..7d247f45d 100644 --- a/internal/tui/onboarding_test.go +++ b/internal/tui/onboarding_test.go @@ -2066,6 +2066,61 @@ func TestApplySetupOAuthSuccessAdvancesToModel(t *testing.T) { } } +func TestApplySetupOAuthTokenPersistFailureStaysOnProvider(t *testing.T) { + m := newModel(context.Background(), Options{ + Setup: SetupOptions{Visible: true, Providers: []SetupProviderOption{ + {ID: "xai", Name: "xAI", RequiresAuth: true}, + }}, + }) + m.setup.stage = setupStageProvider + m.setup.oauthPending = true + m.setup.oauthMode = true + m.setup.configPath = t.TempDir() // A directory cannot be read as config.json. + + updated, cmd := m.applySetupOAuth(setupOAuthMsg{tokenLogin: true, providerID: "xai"}) + next := updated.(model) + if cmd != nil { + t.Fatal("failed profile persistence should not start model discovery") + } + if next.setup.stage != setupStageProvider { + t.Fatalf("stage = %v, want provider", next.setup.stage) + } + if next.setup.oauthErr == "" || !strings.Contains(next.setup.oauthErr, "could not be saved") { + t.Fatalf("oauthErr = %q, want profile-save failure", next.setup.oauthErr) + } +} + +func TestCompleteSetupSurfacesMintedOpenRouterKeyWhenSaveFails(t *testing.T) { + const mintedKey = "sk-or-minted-recovery" + m := newModel(context.Background(), Options{ + Setup: SetupOptions{ + Visible: true, + Providers: []SetupProviderOption{ + {ID: "openrouter", Name: "OpenRouter", RequiresAuth: true}, + }, + Save: func(SetupSelection) (SetupResult, error) { + return SetupResult{}, errors.New("config write failed") + }, + }, + }) + m.setup.oauthMode = true + m.setup.stage = setupStageReady + m.setup.apiKey.SetValue(mintedKey) + + updated, _ := m.completeSetup() + next := updated.(model) + if !strings.Contains(next.setup.err, mintedKey) || !strings.Contains(next.setup.err, "was not saved") { + t.Fatalf("setup error = %q, want minted-key recovery", next.setup.err) + } +} + +func TestSetupDevicePreparePreflightsConfigBeforeRequestingCode(t *testing.T) { + msg := setupDevicePrepareCmd("xai", t.TempDir())().(setupOAuthDeviceMsg) + if msg.err == nil { + t.Fatal("device-code preparation should reject an unreadable config before requesting a code") + } +} + func pressSetupContinue(m model) model { m = pressSetupContinueOnce(m) // Transparently skip the connect-method chooser via the API-key/browse path so diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 6816074d7..bf5459884 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -16,6 +16,7 @@ import ( "charm.land/lipgloss/v2" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/credstore" "github.com/Gitlawb/zero/internal/oauth" ) @@ -328,6 +329,13 @@ func (m model) activateManagerSelection() (model, tea.Cmd) { return next, cmd } +// transferProviderAPIKeyStoredMarker is a package var so a test can drive the +// partial-failure path: the handoff runs AFTER RemoveProvider has committed +// (while both case-variant rows are present the config is ambiguous and every +// write against it is rejected), so its failure is a real state the UI must +// report rather than an unreachable branch. +var transferProviderAPIKeyStoredMarker = config.TransferProviderAPIKeyStoredMarker + // deleteManagerSelection removes the confirmed provider: the config write runs // synchronously (the list must reflect the config the instant the confirm // resolves), while the stored-key delete and the OAuth-login lookup — a @@ -364,7 +372,53 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { } activeAfter = cfg.ActiveProvider notes = []string{"Deleted " + name + "."} - cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile) + retainStoredKey := false + markerTransferredTo := "" + if survivor, ok := caseVariantProviderNameAfterRemoval(cfg, name); ok { + // A surviving case variant reads the same credential-store entry, so + // the shared key must not be deleted — provided that row can still + // REACH it. ApplyStoredAPIKey consults the store only for a row + // carrying apiKeyStored, so retention is only safe once the marker + // is where the survivor can use it. + retainStoredKey = true + // The removed row owned the shared secret's marker; carry it over to + // the surviving case-variant so it stays reachable, mirroring the CLI + // (`zero providers remove`) fix for the same case. + if row.profile.APIKeyStored { + if survivorRow, foundSurvivor, err := config.ProviderRow(m.userConfigPath, survivor); err == nil && foundSurvivor && !survivorRow.APIKeyStored { + if _, err := transferProviderAPIKeyStoredMarker(m.userConfigPath, survivor); err != nil { + // The row is already gone and the handoff failed, so the + // key is in the store with no profile marked to read it. + // Keep it rather than destroying a secret because a config + // write failed — but say so, and do not report this as a + // completed delete. Re-saving a key for the survivor (or + // restoring its apiKeyStored marker) makes it reachable again. + retainStoredKey = true + notes = []string{ + "Removed " + name + ", but the delete is incomplete: its stored API key marker could not be handed to " + survivor + " (" + err.Error() + ").", + "The shared key is still in the credential store and " + survivor + " cannot reach it until you set a key for it again.", + } + } else { + markerTransferredTo = survivor + } + } + } + } + cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, retainStoredKey) + // reloadProviderManagerRows (below, via the caller) rebuilds the manager + // list from savedProviders, so the on-disk marker transfer above must be + // mirrored here too — otherwise the survivor reads APIKeyStored: false in + // memory until the process restarts, and providerManagerCredState (which + // gates store lookups on that field) reports "no credential" for a + // profile whose key is actually reachable. + if markerTransferredTo != "" { + for index := range m.savedProviders { + if strings.TrimSpace(m.savedProviders[index].Name) == markerTransferredTo { + m.savedProviders[index].APIKeyStored = true + break + } + } + } } else { // Env-derived providers have no persisted profile or credential to // delete. Keep this path session-only. @@ -394,11 +448,15 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { return next, tea.Batch(cmd, cleanup) } -// removeSavedProvider drops one profile from the in-memory saved list. +// removeSavedProvider drops one profile from the in-memory saved list. Uses +// exact-name matching to agree with config.RemoveProvider, which the caller +// just invoked: with case-variant rows now a supported layout (e.g. "work" +// and "WORK"), a fold-based match here could evict the surviving row from the +// UI list while it remains in config.json. func removeSavedProvider(saved []config.ProviderProfile, name string) []config.ProviderProfile { kept := saved[:0] for _, profile := range saved { - if strings.EqualFold(strings.TrimSpace(profile.Name), strings.TrimSpace(name)) { + if strings.TrimSpace(profile.Name) == strings.TrimSpace(name) { continue } kept = append(kept, profile) @@ -406,6 +464,25 @@ func removeSavedProvider(saved []config.ProviderProfile, name string) []config.P return kept } +// caseVariantProviderNameAfterRemoval returns the exact name of a row in cfg +// that shares name's CREDENTIAL-STORE identity (a case-only variant surviving +// the removal that reads the same stored secret), and whether one was found. +// +// Matching uses credstore.NormalizeProvider, the store's own equivalence rule, +// for the reason spelled out on the CLI twin (caseVariantProviderName in +// internal/cli/provider_onboarding.go): strings.EqualFold is a strictly wider +// relation and would promise a survivor a key it cannot look up. +func caseVariantProviderNameAfterRemoval(cfg config.FileConfig, name string) (string, bool) { + target := credstore.NormalizeProvider(name) + for _, provider := range cfg.Providers { + providerName := strings.TrimSpace(provider.Name) + if credstore.NormalizeProvider(providerName) == target { + return providerName, 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 +494,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 +683,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, @@ -631,7 +720,7 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { // Keep the live session's identity in sync with a rename of the provider it // is running on: the exported ZERO_PROVIDER must resolve for spawned children. - if strings.EqualFold(strings.TrimSpace(m.providerName), oldName) { + if strings.TrimSpace(m.providerName) == oldName { m.providerName = newName m.providerProfile.Name = newName config.SetActiveProviderEnv(newName) @@ -649,7 +738,7 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { // liveName is the session's provider AFTER any rename sync, so a single // comparison against the edited profile's final name suffices. func providerEditRestartNote(liveName string, editedName string) string { - if strings.EqualFold(strings.TrimSpace(liveName), strings.TrimSpace(editedName)) { + if strings.TrimSpace(liveName) == strings.TrimSpace(editedName) { return " Press Enter on it to apply the changes to this session." } return "" @@ -658,8 +747,9 @@ func providerEditRestartNote(liveName string, editedName string) string { // applySavedProviderEdit mirrors a persisted config.EditProvider into the // in-memory saved list without wholesale replacement (see saveManagerEdit). func applySavedProviderEdit(saved []config.ProviderProfile, oldName string, edit config.ProviderEdit) []config.ProviderProfile { + oldName = strings.TrimSpace(oldName) for index := range saved { - if !strings.EqualFold(strings.TrimSpace(saved[index].Name), strings.TrimSpace(oldName)) { + if strings.TrimSpace(saved[index].Name) != oldName { continue } profile := &saved[index] diff --git a/internal/tui/provider_manager_test.go b/internal/tui/provider_manager_test.go index ddc2429fd..915fb56fb 100644 --- a/internal/tui/provider_manager_test.go +++ b/internal/tui/provider_manager_test.go @@ -3,6 +3,7 @@ package tui import ( "context" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -152,6 +153,151 @@ func TestProviderManagerDeleteConfirmsAndRemoves(t *testing.T) { } } +// TestProviderManagerDeleteTransfersKeyMarkerToSurvivingCaseVariant mirrors +// the CLI's TestRunProvidersRemoveTransfersKeyMarkerToSurvivingCaseVariant: +// deleting the case-variant row that owns apiKeyStored must carry the marker +// to the surviving sibling, since the credential store's secret is keyed +// case-folded and stays put either way. +func TestProviderManagerDeleteTransfersKeyMarkerToSurvivingCaseVariant(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(home, "oauth-tokens.json")) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + seed := config.FileConfig{ + ActiveProvider: "WORK", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v1", APIKeyStored: true, Model: "work-model"}, + {Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v2", Model: "work-model-2"}, + }, + } + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatalf("encode seed config: %v", err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatalf("write seed config: %v", err) + } + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "shared-key"); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "WORK", + ModelName: "work-model-2", + Provider: &fakeProvider{}, + ProviderProfile: seed.Providers[1], + SavedProviders: seed.Providers, + UserConfigPath: configPath, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m.width = 120 + m.height = 40 + m, _ = m.openProviderManager() + + m = managerKey(t, m, testKeyText("d")) // select "work" (row 0) + next, _ := m.handleProviderWizardKey(testKeyText("y")) + + persisted := readManagerConfig(t, next.userConfigPath) + if len(persisted.Providers) != 1 || persisted.Providers[0].Name != "WORK" || !persisted.Providers[0].APIKeyStored { + t.Fatalf("survivor must inherit apiKeyStored marker, got %+v", persisted.Providers) + } + if key, ok, err := store.Get("work"); err != nil || !ok || key != "shared-key" { + t.Fatalf("shared provider key = %q, %v, %v; want retained", key, ok, err) + } + // The marker transfer must be mirrored into the in-memory savedProviders + // too — reloadProviderManagerRows rebuilds the manager list from it, and + // providerManagerCredState gates store lookups on APIKeyStored, so a stale + // false here would show "no credential" for the survivor until restart + // even though config.json and the credstore both already agree. + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "WORK" || !next.savedProviders[0].APIKeyStored { + t.Fatalf("in-memory savedProviders must inherit apiKeyStored too, got %+v", next.savedProviders) + } +} + +// TestProviderManagerDeleteReportsFailedKeyMarkerHandoff covers jatmn's #725 +// finding that the manager committed the retain-the-key decision before it knew +// the handoff had worked. The marker transfer can only run AFTER RemoveProvider +// (while both case-variant rows exist the config is ambiguous and every write +// against it is rejected), so its failure is reachable: the shared secret then +// sits in the store with no row marked to read it, while the UI had already +// reported a completed "Deleted ." +func TestProviderManagerDeleteReportsFailedKeyMarkerHandoff(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(home, "oauth-tokens.json")) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + configPath := filepath.Join(t.TempDir(), "config.json") + seed := config.FileConfig{ + ActiveProvider: "WORK", + Providers: []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v1", APIKeyStored: true, Model: "work-model"}, + {Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example.com/v2", Model: "work-model-2"}, + }, + } + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatalf("encode seed config: %v", err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatalf("write seed config: %v", err) + } + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "shared-key"); err != nil { + t.Fatal(err) + } + original := transferProviderAPIKeyStoredMarker + transferProviderAPIKeyStoredMarker = func(string, string) (bool, error) { + return false, errors.New("injected marker write failure") + } + t.Cleanup(func() { transferProviderAPIKeyStoredMarker = original }) + + m := newModel(context.Background(), Options{ + ProviderName: "WORK", + ModelName: "work-model-2", + Provider: &fakeProvider{}, + ProviderProfile: seed.Providers[1], + SavedProviders: seed.Providers, + UserConfigPath: configPath, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m.width = 120 + m.height = 40 + m, _ = m.openProviderManager() + + m = managerKey(t, m, testKeyText("d")) // select "work" (row 0) + next, cmd := m.handleProviderWizardKey(testKeyText("y")) + next = drainProviderManagerCmds(t, next, cmd) + + status := next.providerWizard.manageStatus + if strings.Contains(status, "Deleted work.") { + t.Fatalf("status = %q, must not report a completed delete while the shared key is unreachable", status) + } + if !strings.Contains(status, "incomplete") || !strings.Contains(status, "WORK") { + t.Fatalf("status = %q, want it to name the incomplete handoff and the survivor", status) + } + // The secret is kept rather than destroyed by a failed config write: it is + // recoverable by setting a key for the survivor again, whereas deleting it + // is not. + if key, ok, err := store.Get("work"); err != nil || !ok || key != "shared-key" { + t.Fatalf("shared provider key = %q, %v, %v; want it preserved for recovery", key, ok, err) + } + persisted := readManagerConfig(t, next.userConfigPath) + if len(persisted.Providers) != 1 || persisted.Providers[0].Name != "WORK" { + t.Fatalf("providers = %+v, want only the survivor", persisted.Providers) + } +} + func TestProviderManagerEditModelPersists(t *testing.T) { m := managerTestModel(t) m = managerKey(t, m, testKeyText("e")) @@ -217,6 +363,63 @@ func TestProviderManagerRenameFollowsLiveSession(t *testing.T) { } } +func TestProviderManagerEditKeepsDistinctUnicodeIdentityOutOfLiveSession(t *testing.T) { + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(home, "oauth-tokens.json")) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + t.Setenv(config.ActiveProviderEnv, "s") + configPath := filepath.Join(t.TempDir(), "config.json") + seed := config.FileConfig{ + ActiveProvider: "s", + Providers: []config.ProviderProfile{ + {Name: "s", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://s.example.com/v1", Model: "s-model"}, + {Name: "ſ", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://long-s.example.com/v1", Model: "long-s-model"}, + }, + } + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatal(err) + } + + m := newModel(context.Background(), Options{ + ProviderName: "s", + ModelName: "s-model", + Provider: &fakeProvider{}, + ProviderProfile: seed.Providers[0], + SavedProviders: seed.Providers, + UserConfigPath: configPath, + }) + m.providerWizard = &providerWizardState{ + step: providerWizardStepEditMenu, + editOriginal: seed.Providers[1], + editDraft: config.ProviderProfile{ + Name: "ſ", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://long-s.example.com/v1", + Model: "edited-long-s-model", + }, + } + + next, _ := m.saveManagerEdit() + persisted := readManagerConfig(t, configPath) + if persisted.Providers[0].Model != "s-model" || persisted.Providers[1].Model != "edited-long-s-model" { + t.Fatalf("persisted edit targeted wrong identity: %+v", persisted.Providers) + } + if next.savedProviders[0].Model != "s-model" || next.savedProviders[1].Model != "edited-long-s-model" { + t.Fatalf("in-memory edit targeted wrong identity: %+v", next.savedProviders) + } + if next.providerName != "s" || next.providerProfile.Name != "s" || os.Getenv(config.ActiveProviderEnv) != "s" { + t.Fatalf("unrelated live session changed: provider=%q profile=%q env=%q", next.providerName, next.providerProfile.Name, os.Getenv(config.ActiveProviderEnv)) + } + if strings.Contains(next.providerWizard.manageStatus, "Press Enter") { + t.Fatalf("unrelated edit produced live-session restart note: %q", next.providerWizard.manageStatus) + } +} + func TestProviderManagerEscWalksBackThenCloses(t *testing.T) { m := managerTestModel(t) m = managerKey(t, m, testKeyText("e")) diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index 8f4ee8bb3..14d1ee429 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 @@ -297,8 +327,11 @@ type providerWizardDeviceCodeMsg struct { // providerWizardDevicePrepareCmd runs phase 1 of the device-code login off the UI // goroutine and reports the code to display (or an error). -func providerWizardDevicePrepareCmd(name string, attemptID int) tea.Cmd { +func providerWizardDevicePrepareCmd(name string, attemptID int, configPath ...string) tea.Cmd { return func() tea.Msg { + if err := preflightOAuthUserConfig(firstString(configPath)); err != nil { + return providerWizardDeviceCodeMsg{providerID: name, attemptID: attemptID, err: err} + } auth, cfg, err := oauthDevicePrepare(name) if err != nil { return providerWizardDeviceCodeMsg{providerID: name, attemptID: attemptID, err: err} @@ -316,9 +349,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)} } } @@ -330,7 +367,7 @@ func (m model) startProviderDeviceLogin() (model, tea.Cmd) { return m, nil } attemptID := m.providerWizard.beginOAuthAttempt(true) - return m, providerWizardDevicePrepareCmd(provider.ID, attemptID) + return m, providerWizardDevicePrepareCmd(provider.ID, attemptID, m.userConfigPath) } const maxProviderWizardProvidersVisible = 10 @@ -968,7 +1005,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 +1298,24 @@ 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() + if provider.OAuthMintsKey && strings.TrimSpace(wizard.apiKey) != "" { + wizard.err = oauthMintedKeySaveError(wizard.err, wizard.apiKey) + } + return m, nil + } + profile = adopted + if err := config.PreflightProviderWrite(m.userConfigPath, profile.Name); err != nil { + wizard.err = err.Error() + if provider.OAuthMintsKey && strings.TrimSpace(wizard.apiKey) != "" { + wizard.err = oauthMintedKeySaveError(wizard.err, wizard.apiKey) + } + return m, nil + } // Capture flip: move the freshly entered key into the encrypted credential // store before persisting, so config.json never holds the cleartext. The // provider was already built above from runtimeProfile, which has the key. @@ -1270,6 +1325,9 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { } if _, err := config.UpsertProvider(m.userConfigPath, profile, true); err != nil { wizard.err = redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{secret, profile.APIKey}}) + if provider.OAuthMintsKey && strings.TrimSpace(secret) != "" { + wizard.err = oauthMintedKeySaveError(wizard.err, secret) + } return m, nil // nothing committed to live state yet } } @@ -1329,10 +1387,18 @@ 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) } - _, _ = config.ClearProviderKeyStored(m.userConfigPath, name) + // The credential store keys secrets case-folded, so a case-variant + // sibling row (e.g. "work" beside the deleted key's "WORK") pointed at + // the same now-gone entry — clear the marker on every folded match, + // not just the exact row the user picked. + _, _ = config.ClearProviderKeyStoredCaseVariants(m.userConfigPath, name) } else { _, _ = config.ForgetProviderKey(name) } 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. diff --git a/internal/tui/provider_wizard_oauth_test.go b/internal/tui/provider_wizard_oauth_test.go index bd4ac4d14..df63b1714 100644 --- a/internal/tui/provider_wizard_oauth_test.go +++ b/internal/tui/provider_wizard_oauth_test.go @@ -364,6 +364,29 @@ func TestApplyProviderWizardOAuthIgnoresStaleAttempt(t *testing.T) { } } +func TestProviderWizardSurfacesMintedOpenRouterKeyWhenSaveFails(t *testing.T) { + const mintedKey = "sk-or-wizard-recovery" + m := wizardModelAt(t, "openrouter", providerWizardStepDone) + m.providerWizard.oauthMode = true + m.providerWizard.apiKey = mintedKey + m.userConfigPath = t.TempDir() // A directory cannot be read as config.json. + + next, _ := m.applyProviderWizard() + if !strings.Contains(next.providerWizard.err, mintedKey) || !strings.Contains(next.providerWizard.err, "was not saved") { + t.Fatalf("wizard error = %q, want minted-key recovery", next.providerWizard.err) + } +} + +func TestProviderWizardDevicePreparePreflightsConfigBeforeRequestingCode(t *testing.T) { + msg := providerWizardDevicePrepareCmd("xai", 42, t.TempDir())().(providerWizardDeviceCodeMsg) + if msg.err == nil { + t.Fatal("device-code preparation should reject an unreadable config before requesting a code") + } + if msg.attemptID != 42 { + t.Fatalf("attemptID = %d, want 42", msg.attemptID) + } +} + func TestProviderWizardDeviceCodeIgnoresStaleAttempt(t *testing.T) { m := mouseTestModel() m.providerWizard = m.newProviderWizard()