Clarify saved and effective provider selection

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
 	}

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 <preset>` (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 <provider> [--device] [--scope <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>")
 	}
 	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 <catalog id>` — 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>")
 	}
 	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)
 	}