diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dfe4fea8..aed6f41dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), aims to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html) once the first release is tagged. Until then, source builds report the version `dev`. +## Unreleased + +### Features + +* **providers:** add `zero providers repair-config [--name ]` to recover a single legacy unnamed provider profile while preserving the active-provider name or falling back to `openai` + +### Bug Fixes + +* **oauth:** validate provider configuration before authorization and immediately before token replacement across CLI, TUI, setup, and device flows, preserving existing credentials when validation fails + ## [0.8.0](https://github.com/Gitlawb/zero/compare/v0.7.0...v0.8.0) (2026-08-21) @@ -26,7 +36,6 @@ tagged. Until then, source builds report the version `dev`. * **modelregistry:** expose reasoning effort for DeepSeek V4 models ([#931](https://github.com/Gitlawb/zero/issues/931)) ([90dcfd1](https://github.com/Gitlawb/zero/commit/90dcfd127e8a6d9902ec9f4e72f6d03fef1a0fc6)) * **providers:** discover ChatGPT capabilities ([#890](https://github.com/Gitlawb/zero/issues/890)) ([2d2450e](https://github.com/Gitlawb/zero/commit/2d2450e9a744349f0d01b1d4e9ba29c24ba5650d)) * **sandbox:** normalize launcher names before the command-prefix denylist ([#934](https://github.com/Gitlawb/zero/issues/934)) ([6edf9a8](https://github.com/Gitlawb/zero/commit/6edf9a8b78dc030dc44598919db1c7fa9d4f809a)) - ## [0.7.0](https://github.com/Gitlawb/zero/compare/v0.6.0...v0.7.0) (2026-08-10) diff --git a/README.md b/README.md index 3d3590814..91c6749c1 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,12 @@ zero models list zero doctor ``` +If an upgraded `config.json` contains one legacy provider profile without a +name, repair it with `zero providers repair-config`. The command preserves the +saved `activeProvider` name (falling back to `openai`), or accepts an explicit +replacement with `--name `. Multiple unnamed rows are not guessed; repair +those directly in `config.json`. + For API providers, set the matching environment variable before setup or enter the key in the wizard: @@ -290,7 +296,7 @@ zero exec one-shot or scripted agent run zero setup first-run provider setup zero auth OAuth/login helpers for supported providers zero models model registry and capabilities -zero providers provider profiles and detection +zero providers provider profiles, recovery, and detection zero doctor setup, key, and connectivity checks zero context context-budget report zero repo-map deterministic repository map diff --git a/README_ZH.md b/README_ZH.md index ff5186a6d..5611e4ab1 100644 --- a/README_ZH.md +++ b/README_ZH.md @@ -99,6 +99,12 @@ zero models list zero doctor ``` +如果升级后的 `config.json` 中有一个旧版未命名的提供商配置，请运行 +`zero providers repair-config` 进行修复。该命令会保留已保存的 +`activeProvider` 名称（未设置时回退到 `openai`），也可以通过 +`--name <名称>` 显式指定新名称。对于多个未命名的配置行，Zero 不会猜测， +请直接在 `config.json` 中修复。 + 对于 API 提供商，在设置之前设置匹配的环境变量或在向导中输入密钥： ```bash @@ -208,7 +214,7 @@ zero exec 一次性或脚本化智能体运行 zero setup 首次运行提供商设置 zero auth 支持提供商的 OAuth/登录辅助 zero models 模型注册表和能力 -zero providers 提供商配置和检测 +zero providers 提供商配置、修复和检测 zero doctor 设置、密钥和连接检查 zero context 上下文预算报告 zero repo-map 确定性仓库映射 diff --git a/docs/oauth-subscriptions.md b/docs/oauth-subscriptions.md index 6bb70c98d..6c89897b8 100644 --- a/docs/oauth-subscriptions.md +++ b/docs/oauth-subscriptions.md @@ -32,6 +32,13 @@ When a login exists for a provider, the **OpenAI and Anthropic** providers send before. Tokens are stored 0600 (or the OS keyring with `ZERO_OAUTH_STORAGE=keyring`) and never logged. See `zero auth --help`. +Provider OAuth login validates the persisted user configuration before opening +authorization and revalidates it immediately before replacing a stored token. +This applies to CLI login, the TUI/setup wizard, and device-code completion. If +the configuration is invalid at either check, Zero aborts without overwriting +the previous OAuth credential. If the error identifies one legacy unnamed +provider profile, repair it with `zero providers repair-config`. + ### In the setup wizard (`/provider`) Running `/provider` opens a **"How do you want to connect?"** chooser: diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index aa9faaf14..fc7eb9494 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -403,6 +403,27 @@ func TestRunNoArgsFailsWhenResolveErrorIsNotProviderRelated(t *testing.T) { } } +func TestRunNoArgsOffersRepairCommandForPersistedNameFailure(t *testing.T) { + var stdout, stderr bytes.Buffer + cwd := t.TempDir() + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{{Name: ""}, {Name: "work"}, {Name: "WORK"}}}) + exitCode := runWithDeps(nil, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return cwd, nil }, + userConfigPath: func() (string, error) { return configPath, nil }, + resolveConfig: func(string, config.Overrides) (config.ResolvedConfig, error) { + return config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}) + }, + runTUI: func(context.Context, tui.Options) int { + t.Fatal("TUI must not launch with ambiguous persisted identities") + return 0 + }, + }) + if exitCode == exitSuccess || !strings.Contains(stderr.String(), "zero providers repair-config") { + t.Fatalf("exit=%d stderr=%q, want actionable repair path", exitCode, stderr.String()) + } +} + func TestRunNoArgsLaunchesTUIWithMCPState(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer diff --git a/internal/cli/auth.go b/internal/cli/auth.go index f3ecdcc42..62b9bcc06 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -6,7 +6,6 @@ import ( "io" "net/http" "os" - "path/filepath" "strings" "time" @@ -39,7 +38,9 @@ func ensureLoginProviderProfile(deps appDeps, provider string) string { if err != nil { return "warning: login saved, but no provider profile was written: " + err.Error() } - active := strings.EqualFold(strings.TrimSpace(ensured.Active), strings.TrimSpace(ensured.Name)) + // Both sides are persisted provider names, so this is a provider-identity + // question and uses the credential store's rule rather than EqualFold. + active := config.SameProviderIdentity(ensured.Active, ensured.Name) switch { case ensured.Created && active: return fmt.Sprintf("Added provider %q to your config and set it active.", ensured.Name) @@ -52,6 +53,14 @@ func ensureLoginProviderProfile(deps appDeps, provider string) string { } } +func preflightAuthLogin(deps appDeps) error { + configPath, err := deps.userConfigPath() + if err != nil { + return err + } + return config.PreflightUserConfig(configPath) +} + // runAuth dispatches `zero auth ` for provider OAuth login. It is // additive and independent of `zero mcp oauth` (MCP server auth), which is // unchanged. @@ -99,6 +108,18 @@ 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])) } + // Two checks, one lifecycle. This one refuses config we can already tell is + // unpublishable BEFORE the browser flow mints a live remote credential: + // validation lived only inside saveOpenRouterProviderKey, past the + // irreversible boundary, so a legacy unnamed or duplicate-name config sent + // the user through OpenRouter's authorization and then handed back an + // orphaned key with a nonzero exit. The sibling chatgpt and login flows check + // here too. The second check stays where it is, immediately before + // EnsureCatalogProvider, because the config can change while the browser flow + // is open. + if err := preflightAuthLogin(deps); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } key, err := deps.openRouterLogin(context.Background(), provideroauth.OpenRouterOptions{ Out: stdout, HTTPClient: &http.Client{Timeout: 30 * time.Second}, @@ -111,10 +132,13 @@ func runAuthOpenRouter(args []string, stdout io.Writer, stderr io.Writer, deps a key = strings.TrimSpace(key) line, err := saveOpenRouterProviderKey(deps, key) if err != nil { + // The key was minted, so still hand it over for manual use — but a login + // that could not be persisted is a failure, not a success: exiting 0 here + // told scripts (and users) the provider was configured when it was not. 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 { return exitCrash } - return exitSuccess + return exitCrash } if _, err := fmt.Fprintf(stdout, "\nOpenRouter login complete — new API key saved.\n%s\n", line); err != nil { return exitCrash @@ -131,24 +155,24 @@ func saveOpenRouterProviderKey(deps appDeps, key string) (string, error) { if err != nil { return "", err } - ensured, err := config.EnsureCatalogProvider(configPath, "openrouter") - if err != nil { + // Validate the persisted config BEFORE EnsureCatalogProvider's lookup, which + // matches case-insensitively and would happily return one of a pair of legacy + // duplicate rows — whose shared credential the capture below then overwrites + // for a config that can never be published. + if err := config.PreflightUserConfig(configPath); err != nil { return "", err } - store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + ensured, err := config.EnsureCatalogProvider(configPath, "openrouter") if err != nil { return "", err } - if err := store.Set(ensured.Name, key); err != nil { + // One operation owns validate → capture → publish, and restores the previous + // stored key if publication is rejected, so a failed login never costs the + // user the key they were already working with. + if err := config.PublishProviderCredential(configPath, ensured.Name, key); err != nil { return "", err } - if err := config.MarkProviderAPIKeyStored(configPath, ensured.Name); err != nil { - // Best-effort rollback: don't leave the key orphaned in the credential - // store while config.json still says it isn't there. - _, _ = store.Delete(ensured.Name) - return "", err - } - active := strings.EqualFold(strings.TrimSpace(ensured.Active), strings.TrimSpace(ensured.Name)) + active := config.SameProviderIdentity(ensured.Active, ensured.Name) switch { case ensured.Created && active: return fmt.Sprintf("Added provider %q to your config and set it active.", ensured.Name), nil @@ -176,6 +200,9 @@ 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])) } + if err := preflightAuthLogin(deps); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), 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 @@ -212,6 +239,9 @@ 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 := preflightAuthLogin(deps); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } if err := store.Save(oauth.ProviderKey("chatgpt"), token); err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } @@ -372,6 +402,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: func() error { return preflightAuthLogin(deps) }, }) } @@ -402,6 +433,9 @@ func runAuthLogin(args []string, stdout io.Writer, stderr io.Writer, deps appDep } return runAuthChatGPT(nil, stdout, stderr, deps) } + if err := preflightAuthLogin(deps); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } manager, err := newAuthManager(deps, stdout) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -438,6 +472,13 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe return writeExecUsageError(stderr, "usage: zero auth logout ") } provider := parsed.positional[0] + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if err := config.PreflightUserConfig(configPath); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } manager, err := newAuthManager(deps, stdout) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) @@ -449,15 +490,16 @@ func runAuthLogout(args []string, stdout io.Writer, stderr io.Writer, deps appDe // Also drop any stored API key and its marker so `auth logout` clears the whole // credential (OAuth token AND key), not just the OAuth side. Surface deletion // failures rather than reporting success while a credential remains. + // Marker first, then the secret: the reverse order leaves apiKeyStored:true + // with nothing behind it if the config write fails. Provider credentials are + // user-scoped, so deletion uses the same default user store as runtime lookup. + if _, clearErr := config.ClearProviderKeyStoredCaseVariants(configPath, provider); clearErr != nil { + return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) + } keyRemoved, keyErr := config.ForgetProviderKey(provider) if keyErr != nil { return writeAppError(stderr, redaction.ErrorMessage(keyErr, redaction.Options{}), exitCrash) } - if configPath, perr := deps.userConfigPath(); perr == nil { - if _, clearErr := config.ClearProviderKeyStored(configPath, provider); clearErr != nil { - return writeAppError(stderr, redaction.ErrorMessage(clearErr, redaction.Options{}), exitCrash) - } - } removed = removed || keyRemoved if parsed.json { payload := struct { diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 9b1ba0fb5..0a6fb26fd 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -154,6 +155,8 @@ func TestRunAuthOpenRouterRejectsArgs(t *testing.T) { } func TestRunAuthOpenRouterSavesMintedKey(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) configPath := filepath.Join(t.TempDir(), "config.json") var stdout, stderr bytes.Buffer @@ -178,7 +181,7 @@ func TestRunAuthOpenRouterSavesMintedKey(t *testing.T) { if profile.Name != "openrouter" || profile.CatalogID != "openrouter" || !profile.APIKeyStored || profile.APIKey != "" || profile.APIKeyEnv != "" { t.Fatalf("provider not stored-key sanitized: %#v", profile) } - store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + store, err := config.ProviderKeyStore() if err != nil { t.Fatal(err) } @@ -321,3 +324,321 @@ func readCLIConfigFixture(t *testing.T, path string) config.FileConfig { } return cfg } + +func TestRunAuthLogoutClearsCaseVariantStoredMarker(t *testing.T) { + withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + configPath, err := config.DefaultUserConfigPath() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-old"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + deps := appDeps{userConfigPath: func() (string, error) { return configPath, nil }} + if code := runWithDeps([]string{"auth", "logout", "WORK"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("logout failed: code=%d stderr=%s", code, stderr.String()) + } + if _, ok, getErr := store.Get("work"); getErr != nil || ok { + t.Fatalf("stored key still present: ok=%v err=%v", ok, getErr) + } + cfg := readCLIConfigFixture(t, configPath) + if cfg.Providers[0].APIKeyStored { + t.Fatal("case-variant logout left apiKeyStored set") + } +} + +func TestRunAuthLogoutRejectsAmbiguousConfigBeforeCredentialDeletion(t *testing.T) { + withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + configPath, err := config.DefaultUserConfigPath() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + seed := []byte(`{"providers":[{"name":"work","apiKeyStored":true},{"name":"WORK","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, seed, 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-shared"); err != nil { + t.Fatal(err) + } + oauthStore, err := oauth.NewStore(oauth.StoreOptions{}) + if err != nil { + t.Fatal(err) + } + oauthToken := oauth.Token{AccessToken: "oauth-access", RefreshToken: "oauth-refresh", Account: "work@example.com"} + if err := oauthStore.Save(oauth.ProviderKey("work"), oauthToken); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + deps := appDeps{userConfigPath: func() (string, error) { return configPath, nil }} + if code := runWithDeps([]string{"auth", "logout", "WORK"}, &stdout, &stderr, deps); code != exitCrash { + t.Fatalf("logout exit = %d, want validation failure", code) + } + if key, ok, getErr := store.Get("work"); getErr != nil || !ok || key != "sk-shared" { + t.Fatalf("shared API credential changed before rejection: present=%v err=%v", ok, getErr) + } + storedOAuth, ok, loadErr := oauthStore.Load(oauth.ProviderKey("work")) + if loadErr != nil || !ok { + t.Fatalf("OAuth credential missing after rejection: ok=%v err=%v", ok, loadErr) + } + if storedOAuth.AccessToken != oauthToken.AccessToken || + storedOAuth.RefreshToken != oauthToken.RefreshToken || + storedOAuth.Account != oauthToken.Account { + t.Fatal("OAuth credential changed before ambiguous-config rejection") + } + after, readErr := os.ReadFile(configPath) + if readErr != nil { + t.Fatal(readErr) + } + if string(after) != string(seed) { + t.Fatal("rejected logout rewrote ambiguous config") + } +} + +func TestRunAuthLoginRejectsAmbiguousConfigBeforeTokenReplacement(t *testing.T) { + for _, test := range []struct { + name string + provider string + args []string + }{ + {name: "generic", provider: "xai", args: []string{"auth", "login", "xai"}}, + {name: "chatgpt", provider: "chatgpt", args: []string{"auth", "chatgpt"}}, + } { + t.Run(test.name, func(t *testing.T) { + withAuthStore(t) + configPath := filepath.Join(t.TempDir(), "config.json") + seed := []byte(`{"providers":[{"name":"xai"},{"name":"XAI"}]}`) + if err := os.WriteFile(configPath, seed, 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{}) + if err != nil { + t.Fatal(err) + } + previous := oauth.Token{AccessToken: "previous-access", RefreshToken: "previous-refresh", Account: "previous-account"} + if err := store.Save(oauth.ProviderKey(test.provider), previous); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + code := runWithDeps(test.args, &stdout, &stderr, appDeps{userConfigPath: func() (string, error) { return configPath, nil }}) + if code != exitCrash || !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("login exit=%d stdout=%q stderr=%q", code, stdout.String(), stderr.String()) + } + stored, ok, err := store.Load(oauth.ProviderKey(test.provider)) + if err != nil || !ok || stored.AccessToken != previous.AccessToken || stored.RefreshToken != previous.RefreshToken || stored.Account != previous.Account { + t.Fatalf("rejected login changed previous token: ok=%v err=%v", ok, err) + } + after, err := os.ReadFile(configPath) + if err != nil || !bytes.Equal(after, seed) { + t.Fatalf("rejected login changed config: readErr=%v", err) + } + }) + } +} + +func TestRunAuthLogoutRejectsConfigPathFailureBeforeCredentialDeletion(t *testing.T) { + withAuthStore(t) + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-shared"); err != nil { + t.Fatal(err) + } + oauthStore, err := oauth.NewStore(oauth.StoreOptions{}) + if err != nil { + t.Fatal(err) + } + oauthToken := oauth.Token{AccessToken: "oauth-access", RefreshToken: "oauth-refresh", Account: "work@example.com"} + if err := oauthStore.Save(oauth.ProviderKey("work"), oauthToken); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + pathErr := errors.New("config path unavailable") + code := runWithDeps([]string{"auth", "logout", "work"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return "", pathErr }, + }) + if code != exitCrash { + t.Fatalf("logout exit = %d, want path failure", code) + } + if !strings.Contains(stderr.String(), pathErr.Error()) { + t.Fatalf("stderr = %q, want config path failure", stderr.String()) + } + if key, ok, getErr := store.Get("work"); getErr != nil || !ok || key != "sk-shared" { + t.Fatalf("API credential changed before path rejection: present=%v len=%d err=%v", ok, len(key), getErr) + } + storedOAuth, ok, loadErr := oauthStore.Load(oauth.ProviderKey("work")) + if loadErr != nil || !ok { + t.Fatalf("OAuth credential missing after path rejection: ok=%v err=%v", ok, loadErr) + } + if storedOAuth.AccessToken != oauthToken.AccessToken || storedOAuth.RefreshToken != oauthToken.RefreshToken || storedOAuth.Account != oauthToken.Account { + t.Fatal("OAuth credential changed before config path rejection") + } +} + +// Local state that already makes publication impossible must be rejected BEFORE +// the browser flow mints a live remote credential. Validation used to live only +// inside saveOpenRouterProviderKey, past that irreversible boundary, so a legacy +// config sent the user through OpenRouter's authorization and then handed back +// an orphaned key with a nonzero exit. +func TestRunAuthOpenRouterRejectsInvalidConfigBeforeAuthorizing(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + configPath := filepath.Join(t.TempDir(), "config.json") + seed := `{"activeProvider":"openrouter","providers":[{"name":"openrouter","apiKeyStored":true},{"name":"OPENROUTER","apiKeyStored":true}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + + 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) { + t.Error("browser authorization started despite config that cannot be published") + return "sk-minted", nil + }, + }) + if code == exitSuccess { + t.Fatalf("exit = %d, want non-zero for a config that cannot be published", code) + } + if !strings.Contains(stderr.String(), "ambiguous persisted provider names") { + t.Fatalf("stderr = %q, want the local rejection", stderr.String()) + } + // No key was minted, so nothing is handed back for manual use either. + if strings.Contains(stdout.String(), "sk-minted") { + t.Fatalf("stdout = %q, want no minted key", stdout.String()) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(after) != seed { + t.Fatalf("rejected login rewrote config:\n%s", after) + } +} + +// The second check is not redundant with the first: the config can change while +// the browser flow is open. A legacy duplicate-row config that appears during +// authorization must still be rejected, and the rejection must not cost the user +// the OpenRouter key they were already working with — the capture is validated +// first, and a rejected publication restores the previous secret rather than +// deleting the shared entry. +func TestRunAuthOpenRouterPreservesExistingKeyWhenConfigRejected(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + // Valid when the command starts, so the preflight before authorization passes. + if err := os.WriteFile(configPath, []byte(`{"activeProvider":"openrouter","providers":[{"name":"openrouter","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + seed := `{"activeProvider":"openrouter","providers":[{"name":"openrouter","apiKeyStored":true},{"name":"OPENROUTER","apiKeyStored":true}]}` + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("openrouter", "sk-working"); err != nil { + t.Fatal(err) + } + + 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) { + // Another process edits config.json while the browser flow is open. + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed the mid-authorization config: %v", err) + } + return "sk-minted", nil + }, + }) + + // A login that could not be persisted is a failure, not a success. + if code == exitSuccess { + t.Fatalf("exit = %d, want non-zero for an unsaved login: %s", code, stdout.String()) + } + if !strings.Contains(stdout.String(), "ambiguous persisted provider names") { + t.Fatalf("stdout = %q, want ambiguous persisted-name rejection", stdout.String()) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(after) != seed { + t.Fatalf("rejected login rewrote config:\n%s", after) + } + key, ok, err := store.Get("openrouter") + if err != nil { + t.Fatal(err) + } + if !ok || key != "sk-working" { + t.Fatalf("stored key does not match (present=%v, len=%d), want the previous sk-working preserved", ok, len(key)) + } + // The minted key is still handed over for manual use. + if !strings.Contains(stdout.String(), "sk-minted") { + t.Fatalf("stdout = %q, want the manual-export hint with the minted key", stdout.String()) + } +} + +// auth logout deletes the secret by normalized identity, so the marker cleanup +// must use the same relation: a mixed-case argument against a lowercase row +// previously left apiKeyStored:true with no secret behind it. +func TestRunAuthLogoutClearsMarkerForCaseVariantSpelling(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + withAuthStore(t) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-work"); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"auth", "logout", "WORK"}, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }); code != exitSuccess { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + cfg := readCLIConfigFixture(t, configPath) + if cfg.Providers[0].APIKeyStored { + t.Fatal("logout left a marker claiming a credential it deleted") + } + if _, ok, err := store.Get("work"); err != nil { + t.Fatal(err) + } else if ok { + t.Fatal("logout left the stored key behind") + } +} diff --git a/internal/cli/command_center.go b/internal/cli/command_center.go index a6fab33ec..f850acac8 100644 --- a/internal/cli/command_center.go +++ b/internal/cli/command_center.go @@ -54,6 +54,31 @@ func runConfig(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) return exitSuccess } +// providersSubcommands is the ONE inventory of `zero providers` subcommands. +// Dispatch, the help text, and the shell completion tree previously each kept +// their own list, and `repair-config` shipped in the first two while every +// generated completion script offered the old set — so the recovery command the +// new validation errors name could not be tab-completed into existence. +// +// The completion tree is built from this slice (see completionRoot) and +// TestProvidersSubcommandInventoryMatchesDispatchAndHelp holds the other two +// surfaces to it, so a new subcommand cannot reach users through one door only. +// Each entry is the canonical name first, then its aliases. +var providersSubcommands = [][]string{ + {"current"}, + {"list"}, + {"catalog"}, + {"add"}, + {"check"}, + {"use"}, + {"remove", "rm"}, + {"rename"}, + {"repair-config"}, + {"setup"}, + {"detect"}, + {"models"}, +} + func runProviders(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { command := "list" if len(args) > 0 && !strings.HasPrefix(args[0], "-") { @@ -81,6 +106,9 @@ func runProviders(args []string, stdout io.Writer, stderr io.Writer, deps appDep if command == "rename" { return runProvidersRename(args, stdout, stderr, deps) } + if command == "repair-config" { + return runProvidersRepairConfig(args, stdout, stderr, deps) + } if command == "setup" { return runProvidersSetup(args, stdout, stderr, deps) } @@ -497,6 +525,7 @@ func writeProvidersHelp(w io.Writer) error { zero providers use [flags] zero providers remove [flags] zero providers rename [flags] + zero providers repair-config [flags] zero providers setup [flags] zero providers detect [flags] zero providers models [name] [flags] @@ -527,6 +556,9 @@ Setup flags: --base-url Planned base URL override --api-key-env Planned API key environment variable --set-active Include --set-active in the add command + +Repair-config flags: + --name Explicit name for the legacy unnamed provider -h, --help Show this help `) return err diff --git a/internal/cli/completions.go b/internal/cli/completions.go index 5f9058946..63fdb9be6 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -42,12 +42,10 @@ var completionRoot = completionNode{ {names: []string{"setup"}}, {names: []string{"config"}}, {names: []string{"models"}, children: []completionNode{{names: []string{"list", "ls"}}}}, - {names: []string{"providers"}, children: []completionNode{ - {names: []string{"current"}}, {names: []string{"list"}}, {names: []string{"catalog"}}, - {names: []string{"add"}}, {names: []string{"check"}}, {names: []string{"use"}}, - {names: []string{"remove", "rm"}}, {names: []string{"rename"}}, {names: []string{"setup"}}, - {names: []string{"detect"}}, {names: []string{"models"}}, - }}, + // Built from providersSubcommands rather than restated here: the two + // inventories drifted, and `repair-config` was dispatched and documented + // while no generated script could complete it. + {names: []string{"providers"}, children: aliasNodes(providersSubcommands)}, {names: []string{"doctor"}}, {names: []string{"context"}}, {names: []string{"repo-map", "repomap"}}, @@ -103,6 +101,16 @@ var completionRoot = completionNode{ }, } +// aliasNodes builds one leaf per command, keeping each command's aliases on the +// same node so a generated script offers every accepted spelling. +func aliasNodes(commands [][]string) []completionNode { + nodes := make([]completionNode, 0, len(commands)) + for _, names := range commands { + nodes = append(nodes, completionNode{names: append([]string{}, names...)}) + } + return nodes +} + func leafNodes(names ...string) []completionNode { nodes := make([]completionNode, 0, len(names)) for _, name := range names { diff --git a/internal/cli/completions_test.go b/internal/cli/completions_test.go index b7eb059b7..1a55e63bb 100644 --- a/internal/cli/completions_test.go +++ b/internal/cli/completions_test.go @@ -2,6 +2,7 @@ package cli import ( "bytes" + "os" "os/exec" "strings" "testing" @@ -177,3 +178,92 @@ func assertCandidates(t *testing.T, got []string, wants ...string) { } } } + +// Dispatch, help, and completions each carried their own list of provider +// subcommands, and they drifted: `repair-config` was dispatched and documented +// while no generated script could complete it — so the recovery command the new +// validation errors point users at was undiscoverable by tab. +// +// completionRoot now builds the providers node from providersSubcommands. This +// holds the other two surfaces to the same inventory, so the next provider +// command cannot ship through one door only. +func TestProvidersSubcommandInventoryMatchesDispatchAndHelp(t *testing.T) { + var help bytes.Buffer + if err := writeProvidersHelp(&help); err != nil { + t.Fatalf("writeProvidersHelp: %v", err) + } + helpText := help.String() + + dispatch, err := os.ReadFile("command_center.go") + if err != nil { + t.Fatalf("read dispatch source: %v", err) + } + dispatchSource := runProvidersDispatchSource(t, string(dispatch)) + + contexts := completionContexts(completionRoot) + var completionCandidates []string + for _, context := range contexts { + if context.path == "providers" { + completionCandidates = context.candidates + } + } + if completionCandidates == nil { + t.Fatal("completion contexts have no providers path") + } + completed := make(map[string]bool, len(completionCandidates)) + for _, candidate := range completionCandidates { + completed[candidate] = true + } + + for _, names := range providersSubcommands { + canonical := names[0] + // Help documents the canonical spelling; aliases are not separate lines. + if !strings.Contains(helpText, "zero providers "+canonical) { + t.Errorf("providers help does not document %q", canonical) + } + for _, name := range names { + if !completed[name] { + t.Errorf("providers completion context does not offer %q (candidates: %v)", name, completionCandidates) + } + // `list`, `current`, and `catalog` fall through to the shared + // options parser rather than an `if command ==` branch, so they are + // matched by the final guard instead. + if !strings.Contains(dispatchSource, `"`+name+`"`) { + t.Errorf("runProviders does not dispatch %q", name) + } + } + } + + // The reverse direction: a command the completion tree offers but nothing + // dispatches would be just as broken. + known := make(map[string]bool) + for _, names := range providersSubcommands { + for _, name := range names { + known[name] = true + } + } + for _, candidate := range completionCandidates { + if strings.HasPrefix(candidate, "-") { + continue + } + if !known[candidate] { + t.Errorf("providers completion offers %q, which is not in providersSubcommands", candidate) + } + } +} + +// runProvidersDispatchSource returns just the body of runProviders, so a name +// mentioned elsewhere in the file cannot satisfy the dispatch assertion. +func runProvidersDispatchSource(t *testing.T, source string) string { + t.Helper() + start := strings.Index(source, "func runProviders(args []string") + if start < 0 { + t.Fatal("runProviders not found in command_center.go") + } + rest := source[start:] + end := strings.Index(rest, "\nfunc ") + if end < 0 { + return rest + } + return rest[:end] +} diff --git a/internal/cli/observability.go b/internal/cli/observability.go index c361d24e0..197142ba4 100644 --- a/internal/cli/observability.go +++ b/internal/cli/observability.go @@ -44,12 +44,18 @@ func runDoctor(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) userConfig = resolveOptions.UserConfigPath projectConfig = resolveOptions.ProjectConfigPath } + if path, pathErr := deps.userConfigPath(); pathErr == nil { + userConfig = path + } var provider config.ProviderProfile var sandboxConfig config.SandboxConfig + var configResolveErr error if resolved, resolveErr := deps.resolveConfig(workspaceRoot, config.Overrides{}); resolveErr == nil { provider = resolved.Provider sandboxConfig = resolved.Sandbox + } else { + configResolveErr = resolveErr } var health *providerhealth.Result if options.connectivity && config.HasProviderProfile(provider) { @@ -69,6 +75,7 @@ func runDoctor(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) UserConfig: userConfig, ProjectConfig: projectConfig, Provider: provider, + ResolveError: configResolveErr, WorkspaceRoot: workspaceRoot, Sandbox: sandboxConfig, Connectivity: options.connectivity, diff --git a/internal/cli/provider_identity_matrix_test.go b/internal/cli/provider_identity_matrix_test.go new file mode 100644 index 000000000..fd3f3d633 --- /dev/null +++ b/internal/cli/provider_identity_matrix_test.go @@ -0,0 +1,235 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +// providerIdentityFixture seeds a config file plus the user-scoped credential +// store and hands back the config path. Every row of the matrix below starts +// from one of these so CLI mutations and runtime credential lookup see the same +// store even when the injected config path is non-default. +type providerIdentityFixture struct { + // configJSON is written verbatim: these scenarios need spellings and + // duplicate rows that FileConfig round-tripping would not preserve. + configJSON string + // storedKeys are seeded into the user-scoped credential store. + storedKeys map[string]string +} + +func seedProviderIdentityFixture(t *testing.T, fixture providerIdentityFixture) string { + t.Helper() + + setCLIUserConfigRoot(t) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(fixture.configJSON), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + if len(fixture.storedKeys) > 0 { + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatalf("open credential store: %v", err) + } + for provider, key := range fixture.storedKeys { + if err := store.Set(provider, key); err != nil { + t.Fatalf("seed key for %q: %v", provider, err) + } + } + } + return configPath +} + +func storedProviderKey(t *testing.T, provider string) (string, bool) { + t.Helper() + + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatalf("open credential store: %v", err) + } + key, ok, err := store.Get(provider) + if err != nil { + t.Fatalf("read key for %q: %v", provider, err) + } + return key, ok +} + +// TestProviderIdentityMatrix is the invariant test for this slice's contract: +// user input is matched by CREDENTIAL IDENTITY, persisted rows are mutated by +// EXACT SPELLING, a stored key survives only while a remaining row still claims +// it, and nothing is captured before the config it belongs to validates. Each +// row is one boundary where those rules previously disagreed; adding a row is +// how a future boundary gets covered, rather than another per-finding test. +func TestProviderIdentityMatrix(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + + t.Run("case-variant remove targets the sole persisted row", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: `{"activeProvider":"WORK","providers":[{"name":"WORK","apiKeyStored":true}]}`, + storedKeys: map[string]string{"WORK": "sk-work"}, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "remove", "work"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + if cfg := readFileConfig(t, configPath); len(cfg.Providers) != 0 { + t.Fatalf("providers = %#v, want the row removed", cfg.Providers) + } + if _, ok := storedProviderKey(t, "work"); ok { + t.Fatal("stored key survived removal of its only owner") + } + }) + + t.Run("case-variant rename targets the sole persisted row", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: `{"activeProvider":"WORK","providers":[{"name":"WORK"}]}`, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "rename", "work", "acme"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "acme" || cfg.ActiveProvider != "acme" { + t.Fatalf("config = %#v, want the row and active pointer renamed to acme", cfg) + } + }) + + t.Run("ambiguous duplicate rows are rejected before any mutation", func(t *testing.T) { + seed := `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK"}]}` + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: seed, + storedKeys: map[string]string{"work": "sk-work"}, + }) + var stdout, stderr bytes.Buffer + + // "Work" matches neither row exactly and both by identity. + if code := runWithDeps([]string{"providers", "remove", "Work"}, &stdout, &stderr, providerSetupDeps(configPath)); code == exitSuccess { + t.Fatalf("ambiguous removal reported success: %s", stdout.String()) + } + if !strings.Contains(stderr.String(), "ambiguous provider") { + t.Fatalf("stderr = %q, want an ambiguity error", stderr.String()) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(after) != seed { + t.Fatalf("rejected removal rewrote config:\n%s", after) + } + if key, ok := storedProviderKey(t, "work"); !ok || key != "sk-work" { + t.Fatalf("stored key does not match (present=%v, len=%d), want sk-work untouched", ok, len(key)) + } + }) + + t.Run("removing a row whose case variant still claims the key keeps it", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK","apiKeyStored":true}]}`, + storedKeys: map[string]string{"work": "sk-shared"}, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "remove", "work"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "WORK" || !cfg.Providers[0].APIKeyStored { + t.Fatalf("config = %#v, want WORK surviving with its marker", cfg) + } + key, ok := storedProviderKey(t, "WORK") + if !ok || key != "sk-shared" { + t.Fatalf("stored key does not match (present=%v, len=%d), want the survivor's sk-shared kept", ok, len(key)) + } + // The survivor must actually be able to load it. + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if loaded := config.ApplyStoredAPIKey(cfg.Providers[0], store); strings.TrimSpace(loaded.APIKey) != "sk-shared" { + t.Fatalf("survivor did not load the retained key (len=%d), want sk-shared", len(strings.TrimSpace(loaded.APIKey))) + } + }) + + t.Run("removing the only row that claims the key deletes it", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK"}]}`, + storedKeys: map[string]string{"work": "sk-shared"}, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "remove", "work"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + // The surviving WORK row never claimed the credential, so keeping the + // secret would only orphan it behind a marker ApplyStoredAPIKey skips. + if _, ok := storedProviderKey(t, "WORK"); ok { + t.Fatal("stored key was orphaned behind a markerless survivor") + } + if !strings.Contains(stdout.String(), "Deleted its stored API key.") { + t.Fatalf("stdout = %q, want the key-deletion note", stdout.String()) + } + }) + + t.Run("repair removal re-points a stale activeProvider spelling", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: `{"activeProvider":"WoRk","providers":[{"name":"work"},{"name":"WORK"}]}`, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "remove", "WORK"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + cfg := readFileConfig(t, configPath) + if cfg.ActiveProvider != "work" { + t.Fatalf("activeProvider = %q, want the surviving row's spelling work", cfg.ActiveProvider) + } + // The exact mutators must be able to find it again. + if _, err := config.SetProviderModel(configPath, cfg.ActiveProvider, "gpt-4"); err != nil { + t.Fatalf("exact mutator cannot address the repaired active row: %v", err) + } + }) + + t.Run("case-variant use activates the persisted row", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: `{"activeProvider":"other","providers":[{"name":"other"},{"name":"OpenAI"}]}`, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "use", "openai"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + if cfg := readFileConfig(t, configPath); cfg.ActiveProvider != "OpenAI" { + t.Fatalf("activeProvider = %q, want the row's own spelling OpenAI", cfg.ActiveProvider) + } + }) + + t.Run("unicode long-s stays a distinct identity end to end", func(t *testing.T) { + configPath := seedProviderIdentityFixture(t, providerIdentityFixture{ + configJSON: "{\"activeProvider\":\"s\",\"providers\":[{\"name\":\"s\",\"apiKeyStored\":true},{\"name\":\"ſ\",\"apiKeyStored\":true}]}", + storedKeys: map[string]string{"s": "sk-latin", "ſ": "sk-long"}, + }) + var stdout, stderr bytes.Buffer + + if code := runWithDeps([]string{"providers", "remove", "s"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("exit = %d, want %d: %s", code, exitSuccess, stderr.String()) + } + // strings.EqualFold folds these two together; the credential store does + // not, so removing "s" must not reach the long-s profile or its secret. + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "ſ" { + t.Fatalf("config = %#v, want only the long-s row remaining", cfg) + } + if key, ok := storedProviderKey(t, "ſ"); !ok || key != "sk-long" { + t.Fatalf("long-s key does not match (present=%v, len=%d), want sk-long untouched", ok, len(key)) + } + if _, ok := storedProviderKey(t, "s"); ok { + t.Fatal("latin-s key survived removal of its only owner") + } + }) +} diff --git a/internal/cli/provider_onboarding.go b/internal/cli/provider_onboarding.go index 08650c4a8..054be8437 100644 --- a/internal/cli/provider_onboarding.go +++ b/internal/cli/provider_onboarding.go @@ -3,7 +3,6 @@ package cli import ( "fmt" "io" - "path/filepath" "strconv" "strings" "unicode" @@ -11,6 +10,7 @@ import ( "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/providercatalog" "github.com/Gitlawb/zero/internal/provideronboarding" + "github.com/Gitlawb/zero/internal/redaction" ) type providerUseOptions struct { @@ -37,6 +37,11 @@ type providerSetupPlan struct { EnvVar string `json:"envVar"` } +type providerRepairOptions struct { + name string + json bool +} + func runProvidersUse(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseProviderUseArgs(args) if err != nil { @@ -113,7 +118,7 @@ func activeProviderEnvOverride(getenv func(string) string, selected string) stri return "" } override := strings.TrimSpace(getenv(config.ActiveProviderEnv)) - if override == "" || strings.EqualFold(override, strings.TrimSpace(selected)) { + if override == "" || config.SameProviderIdentity(override, selected) { return "" } return override @@ -379,6 +384,73 @@ func parseProviderNamesArgs(args []string, want int, usage string) (providerName return options, false, nil } +func runProvidersRepairConfig(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + options, help, err := parseProviderRepairArgs(args) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + if help { + if err := writeProvidersHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + } + configPath, err := deps.userConfigPath() + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + // repaired comes from the repair itself. Re-deriving the defaulting rules + // here reported activeProvider as the repaired name whenever that value + // already belonged to a different row and the fallback had actually named + // this one — the command said "Named legacy provider Groq" about a row it + // had named "openai". + _, repaired, err := config.RepairUnnamedProvider(configPath, options.name) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if options.json { + if err := writePrettyJSON(stdout, map[string]any{"repairedProvider": repaired, "configPath": configPath}); err != nil { + return exitCrash + } + return exitSuccess + } + if _, err := fmt.Fprintf(stdout, "Named legacy provider %s in %s\n", repaired, configPath); err != nil { + return exitCrash + } + return exitSuccess +} + +func parseProviderRepairArgs(args []string) (providerRepairOptions, bool, error) { + options := providerRepairOptions{} + for index := 0; index < len(args); index++ { + arg := args[index] + switch { + case arg == "-h" || arg == "--help" || arg == "help": + return options, true, nil + case arg == "--json": + options.json = true + case arg == "--name": + value, next, err := nextFlagValue(args, index, arg) + if err != nil { + return options, false, err + } + options.name = value + index = next + case strings.HasPrefix(arg, "--name="): + value, err := requiredInlineFlagValue(arg, "--name") + if err != nil { + return options, false, err + } + options.name = value + case strings.HasPrefix(arg, "-"): + return options, false, execUsageError{fmt.Sprintf("unknown flag %q", arg)} + default: + return options, false, execUsageError{fmt.Sprintf("unexpected argument %q", arg)} + } + } + return options, false, nil +} + // runProvidersRemove deletes a saved provider profile and its stored API key. // The OAuth token (if any) is kept — logins outlive profiles so re-adding the // provider needs no new browser round-trip; `zero auth logout ` removes it. @@ -414,14 +486,26 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exit } } + // ProviderPersisted above answers a credential-identity question, but + // RemoveProvider targets a row by its exact spelling. Bridge the two, so + // `zero providers remove work` against a sole saved "WORK" row removes it + // instead of failing "not found" right after the persisted check passed. + name, err = config.ResolvePersistedProviderName(configPath, name) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } cfg, err := config.RemoveProvider(configPath, name) if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } - // Delete the key from the store BESIDE the config being edited — the same - // store setup/rename write to — not the default-path store, so a - // non-default config path cannot leave the encrypted key behind. - keyRemoved, keyErr := removeStoredProviderKeyAt(configPath, name) + // Provider credentials are user-scoped, so delete from the same default user + // store runtime lookup uses. A surviving case variant that still claims the + // credential keeps it (see config.CredentialKeyRetained); a survivor that + // never claimed it does not. + keyRemoved, keyErr := false, error(nil) + if !config.CredentialKeyRetained(cfg.Providers, name) { + keyRemoved, keyErr = config.ForgetProviderKey(name) + } if options.json { payload := map[string]any{ "removed": name, @@ -431,20 +515,24 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps } if keyErr != nil { // A lingering secret must not read as a clean removal. - payload["keyError"] = keyErr.Error() + payload["keyError"] = redaction.ErrorMessage(keyErr, redaction.Options{}) } if err := writePrettyJSON(stdout, payload); err != nil { return exitCrash } + if keyErr != nil { + 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 { + if _, err := fmt.Fprintf(stderr, "warning: its stored API key could not be deleted and remains in the credential store: %s\n", redaction.ErrorMessage(keyErr, redaction.Options{})); err != nil { return exitCrash } + return exitCrash } else if keyRemoved { if _, err := fmt.Fprintln(stdout, "Deleted its stored API key."); err != nil { return exitCrash @@ -462,17 +550,6 @@ func runProvidersRemove(args []string, stdout io.Writer, stderr io.Writer, deps return exitSuccess } -// removeStoredProviderKeyAt deletes a provider's API key from the credential -// store co-located with configPath (the store SecureProviderProfile captured -// it into and RenameProvider migrates within). -func removeStoredProviderKeyAt(configPath string, provider string) (bool, error) { - store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) - if err != nil { - return false, err - } - return store.Delete(provider) -} - // 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 { @@ -500,13 +577,19 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps return exit } } - cfg, err := config.RenameProvider(configPath, options.names[0], options.names[1]) + // Same bridge as remove: the persisted check matches credential identity + // while RenameProvider matches the row's exact spelling. + oldName, err = config.ResolvePersistedProviderName(configPath, oldName) + if err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } + cfg, err := config.RenameProvider(configPath, oldName, options.names[1]) if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } if options.json { if err := writePrettyJSON(stdout, map[string]any{ - "renamed": map[string]string{"from": options.names[0], "to": options.names[1]}, + "renamed": map[string]string{"from": oldName, "to": options.names[1]}, "activeProvider": cfg.ActiveProvider, "configPath": configPath, }); err != nil { @@ -514,7 +597,7 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps } return exitSuccess } - if _, err := fmt.Fprintf(stdout, "Renamed provider %s to %s\n", options.names[0], options.names[1]); err != nil { + if _, err := fmt.Fprintf(stdout, "Renamed provider %s to %s\n", oldName, options.names[1]); err != nil { return exitCrash } return exitSuccess @@ -526,7 +609,7 @@ func runProvidersRename(args []string, stdout io.Writer, stderr io.Writer, deps func providerResolvedByName(providers []config.ProviderProfile, name string) bool { name = strings.TrimSpace(name) for _, provider := range providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if config.SameProviderIdentity(provider.Name, name) { return true } } diff --git a/internal/cli/provider_onboarding_test.go b/internal/cli/provider_onboarding_test.go index 6118939d8..81ce6cdbc 100644 --- a/internal/cli/provider_onboarding_test.go +++ b/internal/cli/provider_onboarding_test.go @@ -43,6 +43,104 @@ func TestRunProvidersUseSetsActiveProvider(t *testing.T) { } } +func TestRunProvidersRepairConfigRecoversLegacyUnnamedProvider(t *testing.T) { + for _, jsonOutput := range []bool{false, true} { + name := "text" + if jsonOutput { + name = "json" + } + t.Run(name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + seed := []byte(`{"activeProvider":"legacy","providers":[{"name":"","provider_kind":"openai","model":"gpt-4o"}],"maxTurns":17}`) + if err := os.WriteFile(configPath, seed, 0o600); err != nil { + t.Fatal(err) + } + if _, err := config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}); err == nil { + t.Fatal("legacy unnamed config unexpectedly resolved before repair") + } + args := []string{"providers", "repair-config"} + if jsonOutput { + args = append(args, "--json") + } + code := runWithDeps(args, &stdout, &stderr, providerSetupDeps(configPath)) + if code != exitSuccess { + t.Fatalf("repair exit = %d, stderr=%q", code, stderr.String()) + } + resolved, err := config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}) + if err != nil { + t.Fatalf("repaired config does not resolve: %v", err) + } + if resolved.ActiveProvider != "legacy" || resolved.Provider.Name != "legacy" || resolved.Provider.Model != "gpt-4o" || resolved.MaxTurns != 17 { + t.Fatalf("resolved repaired config = %+v", resolved) + } + if jsonOutput { + var payload map[string]any + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil || payload["repairedProvider"] != "legacy" { + t.Fatalf("repair JSON = %q, err=%v", stdout.String(), err) + } + } else if !strings.Contains(stdout.String(), "Named legacy provider legacy") { + t.Fatalf("repair output = %q", stdout.String()) + } + }) + } +} + +func TestRunProvidersRepairConfigMigratesLegacyActiveReference(t *testing.T) { + var stdout, stderr bytes.Buffer + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + seed := []byte(`{"activeProvider":"legacy","providers":[{"name":"","provider_kind":"openai","model":"gpt-4o"},{"name":"other","provider_kind":"openai","model":"gpt-4.1"}]}`) + if err := os.WriteFile(configPath, seed, 0o600); err != nil { + t.Fatal(err) + } + + code := runWithDeps( + []string{"providers", "repair-config", "--name", "work"}, + &stdout, + &stderr, + providerSetupDeps(configPath), + ) + if code != exitSuccess { + t.Fatalf("repair exit = %d, stderr=%q", code, stderr.String()) + } + resolved, err := config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}) + if err != nil { + t.Fatalf("fresh Resolve after repair: %v", err) + } + if resolved.ActiveProvider != "work" || resolved.Provider.Name != "work" { + t.Fatalf("resolved active provider = %q profile = %q, want work", resolved.ActiveProvider, resolved.Provider.Name) + } +} + +func TestProviderRepairCommandsCanResolveIndependentLegacyNameProblems(t *testing.T) { + var stdout, stderr bytes.Buffer + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{Providers: []config.ProviderProfile{ + {Name: ""}, {Name: "work"}, {Name: "WORK"}, + }}) + deps := providerSetupDeps(configPath) + if code := runWithDeps([]string{"providers", "repair-config", "--name", "legacy"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("repair-config exit=%d stderr=%q", code, stderr.String()) + } + if err := config.ValidatePersistedProviderNames(readFileConfig(t, configPath)); err == nil || !strings.Contains(err.Error(), "ambiguous") { + t.Fatalf("first repair should leave only the independent duplicate issue, got %v", err) + } + stdout.Reset() + stderr.Reset() + if code := runWithDeps([]string{"providers", "remove", "WORK"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("remove exit=%d stderr=%q", code, stderr.String()) + } + if err := config.ValidatePersistedProviderNames(readFileConfig(t, configPath)); err != nil { + t.Fatalf("final config remains invalid: %v", err) + } +} + func TestRunProvidersUseJSONIncludesActiveProviderAndConfigPath(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer @@ -436,18 +534,18 @@ func writeProviderOnboardingConfig(t *testing.T, path string, cfg config.FileCon } } -// TestRunProvidersRemoveDeletesKeyBesideConfig: the stored key must be deleted -// from the credential store CO-LOCATED with the config being edited (where -// SecureProviderProfile captured it), not the default-path store. -func TestRunProvidersRemoveDeletesKeyBesideConfig(t *testing.T) { +// Runtime provider credentials are user-scoped even when tests inject a +// non-default config path. +func TestRunProvidersRemoveDeletesKeyFromUserStore(t *testing.T) { t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") seed := `{"activeProvider":"gw","providers":[{"name":"gw","provider_kind":"openai-compatible","baseURL":"https://gw.example.com/v1","apiKeyStored":true,"model":"m1"},{"name":"other","provider_kind":"openai-compatible","baseURL":"https://o.example.com/v1","model":"m2"}]}` if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { t.Fatalf("seed config: %v", err) } - store, err := config.ProviderKeyStoreAt(dir) + store, err := config.ProviderKeyStore() if err != nil { t.Fatalf("open store: %v", err) } @@ -477,6 +575,268 @@ func TestRunProvidersRemoveDeletesKeyBesideConfig(t *testing.T) { t.Fatalf("active must hand off, got %q", payload.ActiveProvider) } if _, ok, _ := store.Get("gw"); ok { - t.Fatalf("stored key must be deleted from the store beside the config") + t.Fatalf("stored key must be deleted from the user-scoped store") + } +} + +func TestRunProvidersRemoveFailsWhenStoredKeyCleanupFails(t *testing.T) { + for _, jsonOutput := range []bool{false, true} { + name := "text" + if jsonOutput { + name = "json" + } + t.Run(name, func(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "file") + setCLIUserConfigRoot(t) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"gw","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("gw", "sk-secret"); err != nil { + t.Fatal(err) + } + // A directory at the lock-file path is a hermetic, cross-platform + // failure: Delete cannot acquire its write lock. + userConfigPath, err := config.DefaultUserConfigPath() + if err != nil { + t.Fatal(err) + } + lockPath := filepath.Join(filepath.Dir(userConfigPath), "credentials.json.lock") + if err := os.Remove(lockPath); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(lockPath, 0o700); err != nil { + t.Fatal(err) + } + + args := []string{"providers", "remove", "gw"} + if jsonOutput { + args = append(args, "--json") + } + var stdout, stderr bytes.Buffer + code := runWithDeps(args, &stdout, &stderr, appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }) + if code != exitCrash { + t.Fatalf("exit = %d, want cleanup failure; stdout=%q stderr=%q", code, stdout.String(), stderr.String()) + } + if jsonOutput { + var payload struct { + KeyError string `json:"keyError"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("decode JSON: %v\n%s", err, stdout.String()) + } + if payload.KeyError == "" { + t.Fatal("JSON cleanup failure omitted keyError") + } + } else if !strings.Contains(stderr.String(), "could not be deleted") { + t.Fatalf("stderr = %q, want cleanup warning", stderr.String()) + } + + if err := os.Remove(lockPath); err != nil { + t.Fatal(err) + } + if key, ok, getErr := store.Get("gw"); getErr != nil || !ok || key != "sk-secret" { + t.Fatalf("failed cleanup changed key: present=%v len=%d err=%v", ok, len(key), getErr) + } + }) + } +} + +func TestRunProvidersRemoveKeepsSharedCredentialForCaseVariantSurvivor(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCLIUserConfigRoot(t) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + seed := []byte(`{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK","apiKeyStored":true}]}`) + if err := os.WriteFile(configPath, seed, 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-shared"); 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()) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" || !cfg.Providers[0].APIKeyStored { + t.Fatalf("survivor = %+v, want credentialed work row", cfg.Providers) + } + if key, ok, getErr := store.Get("work"); getErr != nil || !ok || key != "sk-shared" { + t.Fatalf("shared key = %q,%v,%v; want sk-shared,true,nil", key, ok, getErr) + } + var payload struct { + KeyRemoved bool `json:"keyRemoved"` + } + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatal(err) + } + if payload.KeyRemoved { + t.Fatal("remove reported deleting a credential still owned by the survivor") + } +} + +func TestRunProvidersUseMatchesCredentialIdentityButNotUnicodeCaseFold(t *testing.T) { + t.Run("case variant selects persisted spelling", func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "fast", + Providers: []config.ProviderProfile{ + {Name: "OpenAI", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + {Name: "fast", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }) + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "openai"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("use failed: code=%d stderr=%s", code, stderr.String()) + } + if active := readFileConfig(t, configPath).ActiveProvider; active != "OpenAI" { + t.Fatalf("active provider = %q, want persisted spelling OpenAI", active) + } + }) + + t.Run("environment provider accepts case variant", func(t *testing.T) { + t.Setenv("OPENAI_API_KEY", "sk-env") + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{}) + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "OpenAI"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("environment use failed: code=%d stderr=%s", code, stderr.String()) + } + }) + + t.Run("long s is not plain s", func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "s", + Providers: []config.ProviderProfile{{Name: "s", ProviderKind: config.ProviderKindOpenAI, Model: "gpt-4.1"}}, + }) + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"providers", "use", "ſ"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitCrash { + t.Fatalf("use exit = %d, want crash for distinct identity; stdout=%s stderr=%s", code, stdout.String(), stderr.String()) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatal("distinct Unicode identity request rewrote config") + } + }) +} + +// The bare repair used activeProvider as the unnamed row's default name even +// when that value already selected a DIFFERENT named row. It then proposed a +// duplicate, rejected its own candidate, left the file unchanged, and reported +// that the file contained duplicate rows — about a file whose second row has no +// name at all — without mentioning the --name escape that works. +// +// activeProvider is now only a default while it selects no named row, so this +// case falls through to the "openai" fallback and succeeds. The command also +// reports the name the row actually got: it used to re-derive the defaulting +// rules and say "Named legacy provider Groq" about a row named "openai". +func TestRunProvidersRepairConfigDoesNotProposeAnOwnedActiveName(t *testing.T) { + var stdout, stderr bytes.Buffer + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + seed := `{"activeProvider":"Groq","providers":[{"name":"","provider_kind":"openai","model":"gpt-4o"},{"name":"Groq","provider_kind":"openai","model":"llama"}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + if code := runWithDeps([]string{"providers", "repair-config"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess { + t.Fatalf("bare repair exit=%d stderr=%q", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "Named legacy provider openai") { + t.Fatalf("repair reported a name the row did not get: %q", stdout.String()) + } + cfg := readFileConfig(t, configPath) + if len(cfg.Providers) != 2 || cfg.Providers[0].Name != "openai" || cfg.Providers[1].Name != "Groq" { + t.Fatalf("repaired rows = %+v", cfg.Providers) + } + // activeProvider already selected the named row; the repair must not move it. + if cfg.ActiveProvider != "Groq" { + t.Fatalf("ActiveProvider = %q, want the untouched Groq pointer", cfg.ActiveProvider) + } + resolved, err := config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}) + if err != nil { + t.Fatalf("fresh Resolve after repair: %v", err) + } + if resolved.Provider.Name != "Groq" { + t.Fatalf("resolved provider = %q, want Groq", resolved.Provider.Name) + } +} + +// When the fallback name is ALSO owned there is no free default left, so the +// repair must stop before mutating and say which name it wanted, who owns it, +// and the command that works. Reporting a duplicate that exists only in the +// rejected candidate state left the user with no next step. +func TestRunProvidersRepairConfigExplainsCollidingDefaultName(t *testing.T) { + var stdout, stderr bytes.Buffer + configPath := filepath.Join(t.TempDir(), "zero", "config.json") + if err := os.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { + t.Fatal(err) + } + seed := `{"activeProvider":"openai","providers":[{"name":"","provider_kind":"openai","model":"gpt-4o"},{"name":"openai","provider_kind":"openai","model":"gpt-4.1"}]}` + if err := os.WriteFile(configPath, []byte(seed), 0o600); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + deps := providerSetupDeps(configPath) + + if code := runWithDeps([]string{"providers", "repair-config"}, &stdout, &stderr, deps); code == exitSuccess { + t.Fatalf("bare repair succeeded with no free default name; stdout=%q", stdout.String()) + } + message := stderr.String() + if !strings.Contains(message, `"openai"`) { + t.Fatalf("failure does not name the proposed default: %q", message) + } + if !strings.Contains(message, "zero providers repair-config --name") { + t.Fatalf("failure does not show the escape command: %q", message) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Fatalf("refused repair rewrote config:\nbefore=%s\nafter=%s", before, after) + } + + // The guidance must actually work, and the result must resolve. + stdout.Reset() + stderr.Reset() + if code := runWithDeps([]string{"providers", "repair-config", "--name", "legacy"}, &stdout, &stderr, deps); code != exitSuccess { + t.Fatalf("guided repair exit=%d stderr=%q", code, stderr.String()) + } + resolved, err := config.Resolve(config.ResolveOptions{UserConfigPath: configPath, Env: map[string]string{}}) + if err != nil { + t.Fatalf("fresh Resolve after guided repair: %v", err) + } + if resolved.Provider.Name != "openai" { + t.Fatalf("resolved provider = %q, want the untouched active openai row", resolved.Provider.Name) + } + if readFileConfig(t, configPath).Providers[0].Name != "legacy" { + t.Fatalf("guided repair did not name the legacy row: %+v", readFileConfig(t, configPath).Providers) } } diff --git a/internal/cli/provider_setup.go b/internal/cli/provider_setup.go index de13f26ce..71dbce40c 100644 --- a/internal/cli/provider_setup.go +++ b/internal/cli/provider_setup.go @@ -53,8 +53,17 @@ func runProvidersAdd(args []string, stdout io.Writer, stderr io.Writer, deps app if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } // Persist with the key moved into the encrypted credential store (capture flip); // the local profile keeps the key for the verification build below. + // + // Fail-soft capture: the preflight above rules out a validation failure + // AFTER the store write, but a config write that fails for another reason + // (permissions, disk full) still leaves a store entry with no apiKeyStored + // marker. Atomic capture+publish for this path is #894's transaction, not + // the OpenRouter-style PublishProviderCredential rollback this PR wires. cfg, err := config.UpsertProvider(configPath, config.SecureProviderProfile(profile, configPath), options.setActive) if err != nil { return writeAppError(stderr, err.Error(), exitCrash) diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 766cea69b..77025f9a5 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -264,8 +264,13 @@ func saveSetupProvider(deps appDeps, selection tui.SetupSelection, options setup if err != nil { return tui.SetupResult{}, err } + if err := config.PreflightProviderWrite(configPath, profile.Name); err != nil { + return tui.SetupResult{}, err + } // Persist with the key moved into the encrypted credential store (capture flip); // the returned profile keeps the key for this run's immediate use. + // Fail-soft capture with no rollback on a failed config write — see the + // matching note in provider_setup.go; atomicity is #894. if _, err := config.UpsertProvider(configPath, config.SecureProviderProfile(profile, configPath), true); err != nil { return tui.SetupResult{}, err } diff --git a/internal/cli/setup_test.go b/internal/cli/setup_test.go index 905ba7f23..e0fbb85d0 100644 --- a/internal/cli/setup_test.go +++ b/internal/cli/setup_test.go @@ -2,6 +2,7 @@ package cli import ( "context" + "os" "path/filepath" "reflect" "strings" @@ -367,3 +368,46 @@ func TestVerifySetupProviderDistinguishesMissingFromRejectedKey(t *testing.T) { t.Fatal("a keyless local provider should still be probed") } } + +func TestSaveSetupProviderRejectsCaseVariantBeforeCredentialCapture(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + writeProviderOnboardingConfig(t, configPath, config.FileConfig{ + ActiveProvider: "work", + Providers: []config.ProviderProfile{{Name: "work", APIKeyStored: true}}, + }) + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "OLD"); err != nil { + t.Fatal(err) + } + + _, err = saveSetupProvider(appDeps{ + userConfigPath: func() (string, error) { return configPath, nil }, + }, tui.SetupSelection{ + CatalogID: "ollama-cloud", + Name: "WORK", + Model: "qwen3-coder:480b", + APIKey: "NEW", + }, setupSaveOptions{}) + if err == nil || !strings.Contains(err.Error(), `provider "WORK" already exists as "work"`) { + t.Fatalf("saveSetupProvider() error = %v, want case-variant collision", err) + } + after, readErr := os.ReadFile(configPath) + if readErr != nil { + t.Fatal(readErr) + } + if string(after) != string(before) { + t.Fatalf("rejected setup rewrote config\nbefore: %s\nafter: %s", before, after) + } + if key, ok, getErr := store.Get("work"); getErr != nil || !ok || key != "OLD" { + t.Fatalf("existing credential = %q,%v,%v; want OLD,true,nil", key, ok, getErr) + } +} diff --git a/internal/config/credentials.go b/internal/config/credentials.go index f9432cfd2..e90097642 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -66,6 +66,66 @@ func SecureProviderProfile(profile ProviderProfile, configPath string) ProviderP return secured } +// PublishProviderCredential captures key into the user-scoped credential store +// and publishes the matching APIKeyStored marker for exactName as ONE operation, +// so a rejected publication cannot leave the user worse off than before the call. +// +// Hand-rolled Set-then-Mark sequences got this wrong in both directions: they +// wrote the secret before any validation could reject the config, and their +// rollback deleted the entry outright — destroying a working key that some +// other row (the store folds "openrouter" and "OPENROUTER" onto one entry) was +// still using. This validates first, snapshots whatever the store held, and on +// a marker failure restores that snapshot rather than deleting. +// +// exactName must be a persisted row's own spelling; callers holding user or +// session input resolve it with ResolvePersistedProviderName first. +func PublishProviderCredential(path string, exactName string, key string) error { + path = strings.TrimSpace(path) + if path == "" { + return fmt.Errorf("config path is required") + } + exactName = strings.TrimSpace(exactName) + if exactName == "" { + return fmt.Errorf("provider name is required") + } + if strings.TrimSpace(key) == "" { + return fmt.Errorf("api key is required") + } + if err := PreflightUserConfig(path); err != nil { + return err + } + store, err := ProviderKeyStore() + if err != nil { + return err + } + previous, hadPrevious, err := store.Get(exactName) + if err != nil { + return err + } + if err := store.Set(exactName, key); err != nil { + return err + } + if err := MarkProviderAPIKeyStored(path, exactName); err != nil { + // Put the store back exactly as it was: restore a prior key rather than + // deleting it, and only delete when this call created the entry. + var rollbackErr error + if hadPrevious { + rollbackErr = store.Set(exactName, previous) + } else { + _, rollbackErr = store.Delete(exactName) + } + if rollbackErr != nil { + // A failed rollback is the state the caller most needs to hear + // about: the store now holds a key the config does not describe. + // Never let it be reported as a plain publication failure. The key + // value itself stays out of the message. + return fmt.Errorf("%w (credential store rollback also failed: %v)", err, rollbackErr) + } + return err + } + return nil +} + // ForgetProviderKey removes a provider's stored API key from the credential store, // reporting whether one existed. Used by the lifecycle "remove key" / auth logout. func ForgetProviderKey(provider string) (bool, error) { @@ -86,6 +146,12 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { if path == "" || provider == "" { return false, nil } + return clearProviderKeyStoredWhere(path, func(name string) bool { + return strings.TrimSpace(name) == provider + }) +} + +func clearProviderKeyStoredWhere(path string, matches func(string) bool) (bool, error) { data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { @@ -99,7 +165,7 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { } changed := false for index := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(cfg.Providers[index].Name), provider) && cfg.Providers[index].APIKeyStored { + if matches(cfg.Providers[index].Name) && cfg.Providers[index].APIKeyStored { cfg.Providers[index].APIKeyStored = false changed = true } @@ -110,6 +176,24 @@ func ClearProviderKeyStored(path, provider string) (bool, error) { 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 + } + providerIdentity := credstore.NormalizeProvider(provider) + return clearProviderKeyStoredWhere(path, func(name string) bool { + return credstore.NormalizeProvider(name) == providerIdentity + }) +} + // MigratePlaintextProviderKeys moves any inline plaintext API key in the config at // path into the credential store, marking the profile APIKeyStored and stripping // the inline secret — but ONLY after the store write succeeds, so a failed Set @@ -132,6 +216,9 @@ func MigratePlaintextProviderKeys(path string, store APIKeySetter) (int, error) if err := json.Unmarshal(data, &cfg); err != nil { return 0, fmt.Errorf("invalid config JSON %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return 0, err + } migrated := 0 for index := range cfg.Providers { profile := &cfg.Providers[index] diff --git a/internal/config/credentials_test.go b/internal/config/credentials_test.go index d627a5cb4..b7e37ca78 100644 --- a/internal/config/credentials_test.go +++ b/internal/config/credentials_test.go @@ -5,10 +5,25 @@ import ( "errors" "os" "path/filepath" + "runtime" "strings" "testing" ) +func setCredentialTestUserConfigRoot(t *testing.T) { + t.Helper() + + root := t.TempDir() + switch runtime.GOOS { + case "windows": + t.Setenv("APPDATA", root) + case "darwin": + t.Setenv("XDG_CONFIG_HOME", root) + default: + t.Setenv("XDG_CONFIG_HOME", root) + } +} + type fakeKeyGetter struct { keys map[string]string err error @@ -125,6 +140,30 @@ func TestMigrateLeavesKeyWhenStoreSetFails(t *testing.T) { } } +func TestMigratePlaintextProviderKeysValidatesBeforeStoreWrites(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + before := []byte(`{"providers":[{"name":"","apiKey":"sk-implicit"},{"name":"openai","apiKey":"sk-openai"}]}`) + if err := os.WriteFile(path, before, 0o600); err != nil { + t.Fatal(err) + } + store := &fakeKeySetter{keys: map[string]string{}} + + n, err := MigratePlaintextProviderKeys(path, store) + if err == nil || !strings.Contains(err.Error(), "persisted provider name cannot be empty") { + t.Fatalf("migrate = %d,%v; want validation error", n, err) + } + if n != 0 || len(store.keys) != 0 { + t.Fatalf("invalid config mutated credential store: migrated=%d keyCount=%d", n, len(store.keys)) + } + after, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatal(readErr) + } + if string(after) != string(before) { + t.Fatalf("invalid config was rewritten: beforeBytes=%d afterBytes=%d", len(before), len(after)) + } +} + func TestClearProviderKeyStored(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "config.json") @@ -157,6 +196,16 @@ func TestClearProviderKeyStored(t *testing.T) { if cleared, _ := ClearProviderKeyStored(path, "nope"); cleared { t.Fatal("unknown provider should report no change") } + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + if cleared, err := ClearProviderKeyStored(path, "WORK"); err != nil || cleared { + t.Fatalf("case-variant clear = %v,%v; want false,nil", cleared, err) + } + cfg = readConfigFixture(t, path) + if !cfg.Providers[0].APIKeyStored { + t.Fatalf("clear must require exact provider identity: %+v", cfg.Providers) + } } func TestProviderProfileAPIKeyStoredRoundTrips(t *testing.T) { @@ -317,3 +366,121 @@ func TestProviderProfileMissingCredentialEnv(t *testing.T) { }) } } + +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") + } +} + +// A rejected publication must leave the user exactly where they started: the +// previous working key intact, not deleted by a rollback that assumed this +// call had created the entry. +func TestPublishProviderCredentialRestoresPreviousKeyWhenMarkerRejected(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCredentialTestUserConfigRoot(t) + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + // Legacy duplicate rows: the write-time validator rejects this config, so + // the marker publication fails after the credential has been captured. + original := []byte(`{"providers":[{"name":"openrouter","apiKeyStored":true},{"name":"OPENROUTER","apiKeyStored":true}]}`) + if err := os.WriteFile(path, original, 0o600); err != nil { + t.Fatal(err) + } + store, err := ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("openrouter", "sk-working"); err != nil { + t.Fatal(err) + } + + if err := PublishProviderCredential(path, "openrouter", "sk-new"); err == nil { + t.Fatal("publication must be rejected for an ambiguous persisted config") + } + + key, ok, err := store.Get("openrouter") + if err != nil { + t.Fatal(err) + } + if !ok || key != "sk-working" { + t.Fatalf("stored key does not match the previous value (present=%v, len=%d), want sk-working restored", ok, len(key)) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(after) != string(original) { + t.Fatalf("config was rewritten by a rejected publication:\n%s", after) + } +} + +// When the call created the entry there is nothing to restore, so a rejected +// publication must not leave an orphaned secret behind either. +func TestPublishProviderCredentialDeletesEntryItCreatedWhenMarkerRejected(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCredentialTestUserConfigRoot(t) + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"work"},{"name":"WORK"}]}`), 0o600); err != nil { + t.Fatal(err) + } + if err := PublishProviderCredential(path, "work", "sk-new"); err == nil { + t.Fatal("publication must be rejected for an ambiguous persisted config") + } + store, err := ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if _, ok, err := store.Get("work"); err != nil { + t.Fatal(err) + } else if ok { + t.Fatal("rejected publication left an orphaned secret in the store") + } +} + +func TestPublishProviderCredentialStoresAndMarks(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + setCredentialTestUserConfigRoot(t) + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"openrouter","apiKeyEnv":"OPENROUTER_API_KEY"}]}`), 0o600); err != nil { + t.Fatal(err) + } + if err := PublishProviderCredential(path, "openrouter", "sk-new"); err != nil { + t.Fatal(err) + } + store, err := ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + key, ok, err := store.Get("openrouter") + if err != nil || !ok || key != "sk-new" { + t.Fatalf("stored key does not match (present=%v, len=%d, err=%v), want sk-new", ok, len(key), err) + } + var cfg FileConfig + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(data, &cfg); err != nil { + t.Fatal(err) + } + if !cfg.Providers[0].APIKeyStored || strings.TrimSpace(cfg.Providers[0].APIKeyEnv) != "" { + t.Fatalf("marker not published: apiKeyStored=%v apiKeyEnv=%q", cfg.Providers[0].APIKeyStored, cfg.Providers[0].APIKeyEnv) + } +} diff --git a/internal/config/provider_ownership.go b/internal/config/provider_ownership.go new file mode 100644 index 000000000..92ae08aa1 --- /dev/null +++ b/internal/config/provider_ownership.go @@ -0,0 +1,187 @@ +package config + +import ( + "fmt" + "strings" +) + +// Provider identity has three distinct questions, and conflating them is what +// let a mutation aimed at one row land on another: +// +// 1. "Which stored secret is this?" — SameProviderIdentity, the credential +// store's normalization. Two spellings can share one secret. +// 2. "Which row does this mutator target?" — exact trimmed equality. Persisted +// writers address rows exactly, because that is what they rewrite. +// 3. "Which persisted row, if any, does this RESOLVED row own?" — this file. +// +// A resolved provider list is a merge of user config, project config, and +// environment discovery, and the merge is exact: user "work" and project "WORK" +// are two rows, not one. Once such a row is flattened into a ProviderProfile, +// its display Name no longer says which layer produced it, so asking question 1 +// and acting as if it answered question 3 made "shares a credential identity +// with a user row" mean "IS that user row" — and a delete or edit of the project +// row rewrote the user's. + +// ProviderNameLookup is the outcome of resolving a provider spelling against a +// set of candidate row names. +type ProviderNameLookup uint8 + +const ( + // ProviderNameNotFound means no candidate carries the spelling or its + // credential identity. + ProviderNameNotFound ProviderNameLookup = iota + // ProviderNameExact means a candidate carries the spelling byte for byte. + ProviderNameExact + // ProviderNameNormalized means exactly one candidate carries the credential + // identity under a different spelling. + ProviderNameNormalized + // ProviderNameAmbiguous means several candidates carry the identity. It is + // deliberately distinct from NotFound: callers that used first-match picked + // one of them silently. + ProviderNameAmbiguous +) + +// Resolved reports whether the lookup produced a usable name. +func (lookup ProviderNameLookup) Resolved() bool { + return lookup == ProviderNameExact || lookup == ProviderNameNormalized +} + +// LookupProviderName is the ONE rule for resolving a provider spelling when only +// a name is available: an exact match wins outright, a credential-identity match +// is accepted only when exactly ONE candidate carries that identity, and several +// candidates return Ambiguous rather than the first one encountered. +// +// The exact-first half is what keeps case siblings distinct — a caller holding +// "work" must never be handed "WORK" while both exist — and the single-candidate +// half is what still lines a session launched with ZERO_PROVIDER=openai up with +// a sole saved "OpenAI" row. +func LookupProviderName(candidates []string, want string) (string, ProviderNameLookup) { + want = strings.TrimSpace(want) + if want == "" { + return "", ProviderNameNotFound + } + match := "" + matches := 0 + for _, candidate := range candidates { + name := strings.TrimSpace(candidate) + if name == "" { + continue + } + if name == want { + return name, ProviderNameExact + } + if sameProviderIdentity(name, want) { + match = name + matches++ + } + } + switch { + case matches == 1: + return match, ProviderNameNormalized + case matches > 1: + return "", ProviderNameAmbiguous + default: + return "", ProviderNameNotFound + } +} + +// ProviderProfileNames extracts row spellings for the lookup helpers. +func ProviderProfileNames(profiles []ProviderProfile) []string { + names := make([]string, 0, len(profiles)) + for _, profile := range profiles { + names = append(names, strings.TrimSpace(profile.Name)) + } + return names +} + +// ProviderRowOwnership is the answer to question 3 above: may this resolved row +// mutate a user-config row, and which exact row? +type ProviderRowOwnership struct { + // UserBacked is true only when a user-config mutator may run for this row. + // A project- or environment-derived row is session-only and never sets it. + UserBacked bool + // PersistedName is the EXACT user-config row spelling to hand to mutators — + // RemoveProvider, EditProvider, SetProviderModel, key deletion. Empty unless + // UserBacked. + PersistedName string + // Reason explains a non-user-backed answer in the user's terms, so a UI can + // say why an edit or delete is session-only instead of silently doing + // something else. + Reason string + // Lookup is the underlying name-resolution outcome: ProviderNameNotFound for + // the ordinary case of a row with no persisted counterpart at all (e.g. an + // environment-derived provider), or ProviderNameAmbiguous when several + // persisted rows share the identity. A caller that wants to stay quiet for + // the ordinary case and surface Reason only for the surprising ones branches + // on this instead of parsing Reason's text. + Lookup ProviderNameLookup + // Shadowed is true when a credential-identity match was found but rejected + // because a DIFFERENT resolved row already carries that persisted row's + // exact spelling — the case-sibling defect this type exists to prevent. + Shadowed bool +} + +// ResolveProviderRowOwnership decides which persisted user-config row a resolved +// provider row owns. +// +// persisted is the user config's rows. resolvedNames is every row spelling in +// the resolved list the caller is displaying — the siblings matter, and are what +// a plain name lookup cannot see. +// +// The rule: +// +// - An exact persisted row is owned outright. +// - Otherwise, a credential-identity match is a candidate only when exactly +// one persisted row carries that identity. Several is ambiguous, and a +// mutation under ambiguity would pick a row at random. +// - The candidate is REJECTED when another resolved row already carries that +// persisted row's exact spelling. That row is the user row's own entry in +// the list; this one is a project or environment row that merely shares a +// credential identity with it, and it must not write through it. +// +// The last clause is the whole defect: with user "work" and project "WORK" both +// resolved, "WORK" found the sole identity match "work" and edited or deleted it +// — while the in-memory operation targeted exact "WORK", so the row changed on +// disk was not the row changed in the session. +func ResolveProviderRowOwnership(persisted []ProviderProfile, resolvedNames []string, name string) ProviderRowOwnership { + name = strings.TrimSpace(name) + if name == "" { + return ProviderRowOwnership{Reason: "this provider has no name"} + } + persistedName, lookup := LookupProviderName(ProviderProfileNames(persisted), name) + switch lookup { + case ProviderNameExact: + return ProviderRowOwnership{UserBacked: true, PersistedName: persistedName, Lookup: lookup} + case ProviderNameAmbiguous: + return ProviderRowOwnership{Lookup: lookup, Reason: fmt.Sprintf( + "several rows in config.json differ from %q only by case; rename or remove one before changing it", name)} + case ProviderNameNotFound: + return ProviderRowOwnership{Lookup: lookup, Reason: fmt.Sprintf("%q isn't saved in config.json", name)} + } + for _, resolved := range resolvedNames { + resolved = strings.TrimSpace(resolved) + if resolved == name || resolved == "" { + continue + } + if resolved == persistedName { + return ProviderRowOwnership{Lookup: lookup, Shadowed: true, Reason: fmt.Sprintf( + "%q is not the saved provider %q — that row is listed separately, so this entry comes from project config or the environment", + name, persistedName)} + } + } + return ProviderRowOwnership{UserBacked: true, PersistedName: persistedName, Lookup: lookup} +} + +// ProviderRowOwnershipAt is ResolveProviderRowOwnership reading the persisted +// rows from a config path. A path that cannot be read is not user-backed: +// nothing may be mutated through a file this process cannot see. +func ProviderRowOwnershipAt(path string, resolvedNames []string, name string) (ProviderRowOwnership, error) { + if strings.TrimSpace(path) == "" { + return ProviderRowOwnership{Reason: "no user config path"}, nil + } + providers, err := persistedProviders(path) + if err != nil { + return ProviderRowOwnership{}, err + } + return ResolveProviderRowOwnership(providers, resolvedNames, name), nil +} diff --git a/internal/config/provider_ownership_test.go b/internal/config/provider_ownership_test.go new file mode 100644 index 000000000..90e9f7871 --- /dev/null +++ b/internal/config/provider_ownership_test.go @@ -0,0 +1,151 @@ +package config + +import "testing" + +// The ownership matrix. Every row is a shape the resolver can validly produce, +// and the answer decides whether a user-config or credential-store mutator may +// run at all — so a wrong answer here is a write to a row the user never chose. +func TestResolveProviderRowOwnership(t *testing.T) { + for _, testCase := range []struct { + name string + // persisted are the user config's rows. + persisted []string + // resolved is every row spelling the session is displaying. + resolved []string + // row is the one being acted on. + row string + wantBacked bool + wantPersisted string + }{ + { + name: "exact user row", + persisted: []string{"work"}, + resolved: []string{"work"}, + row: "work", + wantBacked: true, + wantPersisted: "work", + }, + { + // The defect. Cross-layer merging is exact, so both rows exist; the + // project row must not write through the user row it merely shares a + // credential identity with. + name: "project row beside its user case sibling", + persisted: []string{"work"}, + resolved: []string{"work", "WORK"}, + row: "WORK", + wantBacked: false, + }, + { + // The same pair from the other side: the user row is still its own. + name: "user row beside its project case sibling", + persisted: []string{"work"}, + resolved: []string{"work", "WORK"}, + row: "work", + wantBacked: true, + wantPersisted: "work", + }, + { + // A sole case variant is the case the normalized fallback exists for: + // a session launched with ZERO_PROVIDER=openai against a saved + // "OpenAI" row, with no sibling to confuse it. + name: "sole case variant", + persisted: []string{"OpenAI"}, + resolved: []string{"openai"}, + row: "openai", + wantBacked: true, + wantPersisted: "OpenAI", + }, + { + name: "env-only row with no persisted counterpart", + persisted: []string{"work"}, + resolved: []string{"work", "groq"}, + row: "groq", + wantBacked: false, + }, + { + // Two persisted rows carry the identity, so no single row can be the + // target. First-match would have picked one. + name: "ambiguous persisted rows", + persisted: []string{"work", "WORK"}, + resolved: []string{"work", "WORK", "Work"}, + row: "Work", + wantBacked: false, + }, + { + // "s" and long-s "ſ" are DIFFERENT credential identities — the store + // keeps separate entries — so neither may claim the other's row. + name: "long-s is a distinct identity", + persisted: []string{"s"}, + resolved: []string{"s", "ſ"}, + row: "ſ", + wantBacked: false, + wantPersisted: "", + }, + { + name: "long-s row of its own", + persisted: []string{"s", "ſ"}, + resolved: []string{"s", "ſ"}, + row: "ſ", + wantBacked: true, + wantPersisted: "ſ", + }, + { + name: "unnamed row", + persisted: []string{"work"}, + resolved: []string{"work"}, + row: "", + wantBacked: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + persisted := make([]ProviderProfile, 0, len(testCase.persisted)) + for _, name := range testCase.persisted { + persisted = append(persisted, ProviderProfile{Name: name}) + } + owner := ResolveProviderRowOwnership(persisted, testCase.resolved, testCase.row) + if owner.UserBacked != testCase.wantBacked { + t.Fatalf("UserBacked = %t, want %t (reason %q)", owner.UserBacked, testCase.wantBacked, owner.Reason) + } + if owner.PersistedName != testCase.wantPersisted { + t.Fatalf("PersistedName = %q, want %q", owner.PersistedName, testCase.wantPersisted) + } + if !owner.UserBacked && owner.Reason == "" { + t.Fatal("a refusal must carry a reason the UI can show") + } + if owner.UserBacked && owner.Reason != "" { + t.Fatalf("a backed row must carry no refusal reason, got %q", owner.Reason) + } + }) + } +} + +// The shared lookup rule. Ambiguous is a distinct outcome from NotFound because +// the callers this replaces returned the first match instead. +func TestLookupProviderName(t *testing.T) { + for _, testCase := range []struct { + name string + candidates []string + want string + wantName string + wantResult ProviderNameLookup + }{ + {name: "exact wins over a sibling", candidates: []string{"WORK", "work"}, want: "work", wantName: "work", wantResult: ProviderNameExact}, + {name: "exact wins in either order", candidates: []string{"work", "WORK"}, want: "WORK", wantName: "WORK", wantResult: ProviderNameExact}, + {name: "sole identity match", candidates: []string{"OpenAI"}, want: "openai", wantName: "OpenAI", wantResult: ProviderNameNormalized}, + {name: "several identity matches", candidates: []string{"work", "WORK"}, want: "Work", wantResult: ProviderNameAmbiguous}, + {name: "no match", candidates: []string{"work"}, want: "groq", wantResult: ProviderNameNotFound}, + {name: "long-s is not s", candidates: []string{"s"}, want: "ſ", wantResult: ProviderNameNotFound}, + {name: "blank", candidates: []string{"work"}, want: " ", wantResult: ProviderNameNotFound}, + } { + t.Run(testCase.name, func(t *testing.T) { + name, result := LookupProviderName(testCase.candidates, testCase.want) + if result != testCase.wantResult || name != testCase.wantName { + t.Fatalf("LookupProviderName(%q, %q) = %q/%v, want %q/%v", + testCase.candidates, testCase.want, name, result, testCase.wantName, testCase.wantResult) + } + if result.Resolved() != (name != "") { + t.Fatalf("Resolved() = %t for name %q", result.Resolved(), name) + } + }) + } +} diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 16936874d..55436d44e 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 != "" { @@ -957,26 +960,51 @@ 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; credential-store identity is only a fallback when it identifies + // one row. This prevents an invalid case-variant sibling from making an exact + // target fail while keeping distinct identities such as "s" and "ſ" separate. + 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 !sameProviderIdentity(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 13038664e..e2bc9f20a 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" @@ -2252,3 +2253,117 @@ func TestResolveRejectsInvalidCrossSessionInbound(t *testing.T) { t.Fatalf("error = %v", err) } } + +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 TestNormalizeProvidersActiveFallbackUsesCredentialIdentity(t *testing.T) { + providers, active, err := normalizeProviders([]ProviderProfile{ + {Name: "s", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://s.example/v1", Model: "s-model"}, + {Name: "ſ", ProviderKind: ProviderKindOpenAICompatible, BaseURL: "https://long-s.example/v1", Model: "long-s-model"}, + }, "S") + if err != nil { + t.Fatalf("normalizeProviders() error = %v", err) + } + if len(providers) != 2 || active.Name != "s" || active.Model != "s-model" { + t.Fatalf("providers=%#v active=%#v, want credential identity s selected", providers, active) + } +} + +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 TestResolveRejectsBlankPersistedNameBeforeImplicitOpenAIIdentity(t *testing.T) { + path := writeConfig(t, `{ + "providers": [ + {"name":"","providerKind":"openai","model":"gpt-4.1"}, + {"name":"openai","providerKind":"openai","model":"gpt-4.1"} + ] + }`) + _, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) + if err == nil || !strings.Contains(err.Error(), "persisted provider name cannot be empty") { + t.Fatalf("Resolve() error = %v, want blank persisted-name rejection", err) + } +} diff --git a/internal/config/writer.go b/internal/config/writer.go index e3b6846f2..c9f930c8c 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -8,9 +8,396 @@ import ( "sort" "strings" + "github.com/Gitlawb/zero/internal/credstore" "github.com/Gitlawb/zero/internal/providercatalog" ) +// ValidatePersistedProviderNames rejects empty names and user-config rows that +// share the credential store's normalized identity. Allowing either would make +// resolver defaults or credential writes and deletes affect another row. +// This validator intentionally applies only to raw persisted user config, not +// to profiles merged from project, environment, or provider-command layers. +// +// A repeated normalized identity is rejected whether or not the spellings +// differ. Exact duplicates are just as broken as case variants: resolver +// merging coalesces the rows, and plaintext-key migration writes both values +// into the same 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) + if name == "" { + return fmt.Errorf("persisted provider name cannot be empty; run `zero providers repair-config` to name the legacy provider") + } + 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 { + // Name the repair command, not just the problem: this rejection is + // reached at config READ time, so a legacy duplicate blocks the + // interactive shell and the TUI outright. `zero providers remove` + // reads config.json directly instead of going through Resolve, so + // it still works while everything else refuses to start. + return fmt.Errorf("ambiguous persisted provider names %q and %q differ only by case; run `zero providers remove %s` (exact spelling) or rename one row in config.json", previous, name, name) + } + seen[folded] = name + } + return nil +} + +// persistedProviderNameProblems returns independently repairable persisted-name +// problems. Keys are stable identities so a repair can prove it reduced an +// existing problem without introducing or worsening another one. +func persistedProviderNameProblems(cfg FileConfig) map[string]int { + problems := map[string]int{} + seen := map[string]int{} + for _, provider := range cfg.Providers { + name := strings.TrimSpace(provider.Name) + if name == "" { + problems["unnamed"]++ + continue + } + seen[credstore.NormalizeProvider(name)]++ + } + for identity, count := range seen { + if count > 1 { + problems["duplicate:"+identity] = count - 1 + } + } + return problems +} + +func writeProviderNameRepair(path string, before FileConfig, after FileConfig) error { + oldProblems := persistedProviderNameProblems(before) + if len(oldProblems) == 0 { + return writeConfigFile(path, after) + } + newProblems := persistedProviderNameProblems(after) + oldTotal, newTotal := 0, 0 + for _, count := range oldProblems { + oldTotal += count + } + for problem, count := range newProblems { + newTotal += count + if count > oldProblems[problem] { + return ValidatePersistedProviderNames(after) + } + } + if newTotal >= oldTotal { + return ValidatePersistedProviderNames(after) + } + data, err := json.MarshalIndent(after, "", " ") + if err != nil { + return fmt.Errorf("encode config JSON: %w", err) + } + return writeConfigData(path, data) +} + +// RepairUnnamedProvider gives legacy provider rows that predate required names +// an explicit persisted identity. Older releases resolved one unnamed row as +// activeProvider, falling back to "openai"; preserve that choice unless the +// user supplies a replacement. Multiple unnamed rows are left untouched because +// selecting one would silently merge or discard profiles. +// +// The chosen name is returned rather than left for the caller to re-derive: the +// defaulting rules below are the only thing that knows which name the row got, +// and the CLI re-deriving them reported activeProvider as the repaired name +// while the row had actually been named by the fallback. +func RepairUnnamedProvider(path string, replacement string) (FileConfig, string, error) { + path = strings.TrimSpace(path) + if path == "" { + return FileConfig{}, "", fmt.Errorf("config path is required") + } + data, err := os.ReadFile(path) + if err != nil { + return FileConfig{}, "", fmt.Errorf("read config %s: %w", path, err) + } + var cfg FileConfig + if err := json.Unmarshal(data, &cfg); err != nil { + return FileConfig{}, "", fmt.Errorf("invalid config JSON %s: %w", path, err) + } + unnamed := -1 + for index := range cfg.Providers { + if strings.TrimSpace(cfg.Providers[index].Name) != "" { + continue + } + if unnamed >= 0 { + return FileConfig{}, "", fmt.Errorf("multiple unnamed persisted providers require manual repair in config.json") + } + unnamed = index + } + if unnamed < 0 { + return FileConfig{}, "", fmt.Errorf("no unnamed persisted provider found") + } + activeName := strings.TrimSpace(cfg.ActiveProvider) + activeMatchesNamedRow := false + if activeName != "" { + for index := range cfg.Providers { + rowName := strings.TrimSpace(cfg.Providers[index].Name) + if rowName == "" { + continue + } + if rowName == activeName || sameProviderIdentity(rowName, activeName) { + activeMatchesNamedRow = true + break + } + } + } + name := strings.TrimSpace(replacement) + explicit := name != "" + if !explicit { + // activeProvider is a safe default for the unnamed row ONLY while it + // selects no named row. Once activeMatchesNamedRow is true, that value is + // evidence the active pointer belongs to the OTHER row — reusing it as + // this row's name proposes a duplicate, and the validation below then + // rejects a state the file never had, reporting "duplicate rows" + // about a file whose second row has no name at all. + if !activeMatchesNamedRow { + name = activeName + } + if name == "" { + name = "openai" + } + } + if conflict, collides := conflictingProviderRowName(cfg, unnamed, name); collides { + if explicit { + return FileConfig{}, "", fmt.Errorf( + "cannot name the unnamed provider %q: persisted provider %q already uses that identity; choose a different --name", + name, conflict) + } + // Say what the proposed name was, that it is already taken, and the exact + // command that gets out of it. The bare form has no other escape. + return FileConfig{}, "", fmt.Errorf( + "the unnamed provider would default to %q, which persisted provider %q already uses; rerun with `zero providers repair-config --name `", + name, conflict) + } + cfg.Providers[unnamed].Name = name + // A nonempty active name that matched no named row was the legacy selector + // for this sole unnamed row. Repair the reference in the same atomic write; + // otherwise an explicit --name can report success but leave Resolve unable to + // find the active provider. + if activeName != "" && !activeMatchesNamedRow { + cfg.ActiveProvider = name + } + var before FileConfig + if err := json.Unmarshal(data, &before); err != nil { + return FileConfig{}, "", fmt.Errorf("invalid config JSON %s: %w", path, err) + } + if err := writeProviderNameRepair(path, before, cfg); err != nil { + return FileConfig{}, "", err + } + return cfg, name, nil +} + +// conflictingProviderRowName reports the persisted row, other than the one being +// repaired, that already owns the proposed name. Identity is the credential +// store's rule, matching ValidatePersistedProviderNames, so the check refuses +// exactly what the write would refuse — before the write, and with a message +// that describes the file rather than the rejected candidate state. +func conflictingProviderRowName(cfg FileConfig, repairing int, name string) (string, bool) { + name = strings.TrimSpace(name) + if name == "" { + return "", false + } + for index := range cfg.Providers { + if index == repairing { + continue + } + rowName := strings.TrimSpace(cfg.Providers[index].Name) + if rowName == "" { + continue + } + if rowName == name || sameProviderIdentity(rowName, name) { + return rowName, true + } + } + return "", false +} + +// 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) +} + +// SameProviderIdentity exposes the credential store's provider-name identity +// rule to UI and CLI list operations. It deliberately differs from +// strings.EqualFold for Unicode spellings such as "s" and long-s. +func SameProviderIdentity(a string, b string) bool { + return sameProviderIdentity(strings.TrimSpace(a), strings.TrimSpace(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) +} + +// 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 +} + +// ResolvePersistedProviderName maps a user- or session-supplied provider +// spelling to the EXACT name of the persisted row it addresses, so a caller +// that gated on credential identity (ProviderPersisted, a resolved provider +// list, a live session's provider name) can hand a row-targeting mutator +// (RemoveProvider, RenameProvider, EditProvider, SetProviderModel, +// MarkProviderAPIKeyStored) a spelling those mutators can actually find. +// +// It is the one bridge between the two identity rules this package defines: +// an exact spelling always wins, credential identity is a fallback, and an +// identity that matches more than one row is an error rather than an +// arbitrary pick — the same ambiguity ValidatePersistedProviderNames rejects +// at write time, reported here for configs that predate that validation. +func ResolvePersistedProviderName(path string, input string) (string, error) { + providers, err := persistedProviders(path) + if err != nil { + return "", err + } + return resolvePersistedProviderName(providers, input) +} + +// resolvePersistedProviderName is LookupProviderName with errors — the shared +// exact-first / unique-normalized / ambiguous rule, not a second copy of it. +func resolvePersistedProviderName(providers []ProviderProfile, input string) (string, error) { + input = strings.TrimSpace(input) + if input == "" { + return "", fmt.Errorf("provider name is required") + } + name, lookup := LookupProviderName(ProviderProfileNames(providers), input) + switch lookup { + case ProviderNameExact, ProviderNameNormalized: + return name, nil + case ProviderNameAmbiguous: + matches := 0 + for _, provider := range providers { + if sameProviderIdentity(strings.TrimSpace(provider.Name), input) { + matches++ + } + } + return "", fmt.Errorf("ambiguous provider %q: %d rows in config.json differ only by case; rename or remove one row", input, matches) + default: + return "", fmt.Errorf("provider %q not found", input) + } +} + +// CredentialKeyRetained reports whether, after removedName's row is gone, some +// REMAINING row still owns the credential-store entry that row pointed at — so +// deleting the shared secret would break a profile the user did not remove. +// +// Ownership is the marker, not the name: the store normalizes "work" and +// "WORK" to one entry, but a surviving row with apiKeyStored:false never reads +// it (ApplyStoredAPIKey gates on the marker), so keeping the secret for that +// row would only orphan it. Retain the key when a survivor actually claims it; +// otherwise the removal took the last owner with it and the key must go. +func CredentialKeyRetained(providers []ProviderProfile, removedName string) bool { + removedName = strings.TrimSpace(removedName) + if removedName == "" { + return false + } + for _, provider := range providers { + if provider.APIKeyStored && sameProviderIdentity(strings.TrimSpace(provider.Name), removedName) { + return true + } + } + return false +} + +// ProviderKeyRetainedAfterRemoval answers CredentialKeyRetained's question +// against the config on disk, simulating the removal of name's row. Callers +// that must know the outcome BEFORE mutating anything use this — a delete +// confirmation has to promise exactly what the delete will do, and computing +// it from a different rule is how the prompt came to claim "this also removes +// its stored API key" for a delete that keeps the key. +func ProviderKeyRetainedAfterRemoval(path string, name string) (bool, error) { + providers, err := persistedProviders(path) + if err != nil { + return false, err + } + // Resolve first, for the same reason the delete does: callers hand this a + // spelling from a resolved list, which may not be the row's own. Previewing + // against an unresolved name removes nothing, so a "key is kept" preview + // could precede a delete that resolves the row and takes the key with it. + name, err = resolvePersistedProviderName(providers, name) + if err != nil { + return false, err + } + remaining := make([]ProviderProfile, 0, len(providers)) + removed := false + for _, provider := range providers { + if !removed && strings.TrimSpace(provider.Name) == name { + removed = true + continue + } + remaining = append(remaining, provider) + } + return CredentialKeyRetained(remaining, name), 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 +} + func UpsertProvider(path string, profile ProviderProfile, setActive bool) (FileConfig, error) { path = strings.TrimSpace(path) if path == "" { @@ -29,6 +416,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 @@ -90,9 +485,14 @@ func EnsureCatalogProvider(path string, catalogID string) (EnsuredProvider, erro } else if !os.IsNotExist(err) { return EnsuredProvider{}, fmt.Errorf("read config %s: %w", path, err) } + if err := ValidatePersistedProviderNames(cfg); err != nil { + return EnsuredProvider{}, err + } for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.CatalogID), descriptor.ID) || - strings.EqualFold(strings.TrimSpace(provider.Name), descriptor.ID) { + // Which persisted row already serves this catalog entry is a provider + // identity question, so it uses the credential store's rule. + if sameProviderIdentity(strings.TrimSpace(provider.CatalogID), descriptor.ID) || + sameProviderIdentity(strings.TrimSpace(provider.Name), descriptor.ID) { return EnsuredProvider{Name: provider.Name, Active: cfg.ActiveProvider}, nil } } @@ -133,8 +533,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 @@ -164,17 +567,19 @@ func SetActiveProvider(path string, name string) (FileConfig, error) { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } - for _, provider := range cfg.Providers { - if strings.EqualFold(provider.Name, name) { - cfg.ActiveProvider = provider.Name - if err := writeConfigFile(path, cfg); err != nil { - return FileConfig{}, err - } - return cfg, nil - } + // Activation accepts any spelling that names this credential identity, then + // records the row's own spelling so every later row-targeting mutator can + // find it. ResolvePersistedProviderName is that bridge — see its doc for + // why an ambiguous identity is an error rather than an arbitrary pick. + resolved, err := resolvePersistedProviderName(cfg.Providers, name) + if err != nil { + return FileConfig{}, err } - - return FileConfig{}, fmt.Errorf("provider %q not found", name) + cfg.ActiveProvider = resolved + if err := writeConfigFile(path, cfg); err != nil { + return FileConfig{}, err + } + return cfg, nil } // ProviderPersisted reports whether a provider profile named name actually has @@ -197,7 +602,7 @@ func ProviderPersisted(path string, name string) (bool, error) { return false, err } for _, provider := range cfg.Providers { - if strings.EqualFold(strings.TrimSpace(provider.Name), name) { + if sameProviderIdentity(provider.Name, name) { return true, nil } } @@ -228,10 +633,16 @@ func RemoveProvider(path string, name string) (FileConfig, error) { if err := json.Unmarshal(data, &cfg); err != nil { return FileConfig{}, fmt.Errorf("invalid config JSON %s: %w", path, err) } + before := cfg + before.Providers = append([]ProviderProfile(nil), cfg.Providers...) + // 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,15 +650,38 @@ 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 } + } else if active := strings.TrimSpace(cfg.ActiveProvider); active != "" { + // Repairing a case-duplicate config can strand activeProvider on a third + // spelling ("WoRk" with rows "work"/"WORK"): the pointer survived the + // removal but now matches no remaining row exactly, so every exact + // mutator fails until the user hand-edits config.json. Re-point it at the + // survivor's own spelling when exactly one row still carries the + // identity; leave it alone when it is still ambiguous or already exact. + if resolved, resolveErr := resolvePersistedProviderName(cfg.Providers, active); resolveErr == nil { + cfg.ActiveProvider = resolved + } } - if err := writeConfigFile(path, cfg); err != nil { + if err := writeProviderNameRepair(path, before, cfg); err != nil { return FileConfig{}, err } return cfg, nil @@ -280,22 +714,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 +747,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 +765,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 +784,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) @@ -370,14 +809,19 @@ func EditProvider(path string, edit ProviderEdit) (FileConfig, error) { 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 +844,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 +884,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 +929,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 @@ -765,6 +1211,9 @@ func NormalizeRecentModels(entries []RecentModelEntry) []RecentModelEntry { } func writeConfigFile(path string, cfg FileConfig) error { + if err := ValidatePersistedProviderNames(cfg); err != nil { + return err + } data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return fmt.Errorf("encode config JSON: %w", err) diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index c66fc26ba..03663950b 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) } @@ -170,7 +172,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) } @@ -594,7 +596,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) } @@ -917,9 +919,9 @@ func TestEditProviderAppliesRenameFieldsAndDescriptionAtomically(t *testing.T) { // 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 +1023,573 @@ func TestEditProviderRejectsCollisionAndUnknown(t *testing.T) { t.Fatalf("config was rewritten by a rejected edit") } } + +func TestUpsertProviderRejectsCaseVariantWithoutRewritingConfig(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://work.example/v1", Model: "m1"}, + }, + }, 0o600) + + _, err := UpsertProvider(path, ProviderProfile{Name: "WORK", Model: "m2"}, false) + if err == nil || !strings.Contains(err.Error(), `provider "WORK" already exists as "work"`) { + t.Fatalf("UpsertProvider() error = %v, want case-variant collision", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatalf("read config: %v", readErr) + } + if !bytes.Equal(after, before) { + t.Fatalf("rejected upsert rewrote config\nbefore: %s\nafter: %s", before, after) + } +} + +func TestSetActiveProviderUsesCredentialIdentityWithoutUnicodeFolding(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "ſ", + Providers: []ProviderProfile{ + {Name: "s", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"}, + {Name: "ſ", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }, 0o600) + + cfg, err := SetActiveProvider(path, "S") + if err != nil { + t.Fatalf("SetActiveProvider() error = %v", err) + } + if cfg.ActiveProvider != "s" { + t.Fatalf("ActiveProvider = %q, want exact persisted spelling s", cfg.ActiveProvider) + } +} + +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 TestProviderPersistedUsesCredentialIdentityWithoutUnicodeFolding(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "s"}}}, 0o600) + + persisted, err := ProviderPersisted(path, "S") + if err != nil { + t.Fatalf("ProviderPersisted() error = %v", err) + } + if !persisted { + t.Fatal("ProviderPersisted() = false for case-variant credential identity") + } + persisted, err = ProviderPersisted(path, "ſ") + if err != nil { + t.Fatalf("ProviderPersisted(long-s) error = %v", err) + } + if persisted { + t.Fatal("ProviderPersisted() conflated s with Unicode long-s") + } +} + +// 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 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 TestRemoveProviderPublishesRepairThatReducesRemainingAmbiguity(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "work"}, {Name: "WORK"}, {Name: "Work"}}}, 0o600) + cfg, err := RemoveProvider(path, "Work") + if err != nil { + t.Fatalf("strictly reducing repair failed: %v", err) + } + if len(cfg.Providers) != 2 || cfg.Providers[0].Name != "work" || cfg.Providers[1].Name != "WORK" { + t.Fatalf("repaired config = %+v", cfg) + } +} + +func TestRemoveProviderPublishesRepairWhileUnnamedProblemRemains(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: ""}, {Name: "work"}, {Name: "WORK"}}}, 0o600) + cfg, err := RemoveProvider(path, "WORK") + if err != nil { + t.Fatalf("exact duplicate repair failed while an unnamed row remained: %v", err) + } + if len(cfg.Providers) != 2 || cfg.Providers[0].Name != "" || cfg.Providers[1].Name != "work" { + t.Fatalf("repaired config = %+v", cfg) + } + if err := ValidatePersistedProviderNames(cfg); err == nil || !strings.Contains(err.Error(), "cannot be empty") { + t.Fatalf("repair should leave only the independent unnamed-row problem, got %v", err) + } +} + +func TestRepairUnnamedProviderRejectsRepairThatIntroducesDuplicate(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: ""}, {Name: "work"}}}, 0o600) + _, _, err := RepairUnnamedProvider(path, "WORK") + // The collision is now caught BEFORE the candidate config is built, so the + // message names the row that owns the identity instead of reporting an + // "ambiguous persisted provider names" state the file never had. The + // rejection and the untouched file are unchanged. + if err == nil || !strings.Contains(err.Error(), `persisted provider "work" already uses that identity`) { + t.Fatalf("error = %v, want a collision rejection naming the owning row", err) + } + if !strings.Contains(err.Error(), "--name") { + t.Fatalf("error = %v, want the escape flag named", err) + } + after, readErr := os.ReadFile(path) + if readErr != nil || !bytes.Equal(after, before) { + t.Fatalf("rejected 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) + } +} + +// 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 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() + // The message must name the repair command: this rejection reaches the user + // at config read time, where it blocks interactive startup entirely. + want := fmt.Sprintf("ambiguous persisted provider names %q and %q differ only by case; run `zero providers remove %s` (exact spelling) or rename one row in config.json", first, second, 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) + } +} + +// 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) + } +} + +func TestValidatePersistedProviderNamesRejectsImplicitOpenAICollision(t *testing.T) { + err := ValidatePersistedProviderNames(FileConfig{Providers: []ProviderProfile{ + {Name: ""}, + {Name: "openai"}, + }}) + if err == nil || !strings.Contains(err.Error(), "persisted provider name cannot be empty") { + t.Fatalf("error = %v, want empty persisted-provider name rejection", err) + } +} + +func TestRepairUnnamedProviderPreservesLegacyNameResolution(t *testing.T) { + t.Run("active provider", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "work", + Providers: []ProviderProfile{{Name: " ", Model: "legacy-model"}}, + MaxTurns: 17, + }, 0o600) + cfg, _, err := RepairUnnamedProvider(path, "") + if err != nil { + t.Fatal(err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" || cfg.Providers[0].Model != "legacy-model" || cfg.MaxTurns != 17 { + t.Fatalf("repaired config = %+v", cfg) + } + if err := ValidatePersistedProviderNames(cfg); err != nil { + t.Fatalf("repaired config remains invalid: %v", err) + } + }) + + t.Run("explicit name migrates legacy active reference", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "legacy", + Providers: []ProviderProfile{ + {Name: "", ProviderKind: ProviderKindOpenAI, Model: "gpt-4o"}, + {Name: "other", ProviderKind: ProviderKindOpenAI, Model: "gpt-4.1"}, + }, + }, 0o600) + cfg, _, err := RepairUnnamedProvider(path, "work") + if err != nil { + t.Fatal(err) + } + if cfg.ActiveProvider != "work" { + t.Fatalf("active provider = %q, want repaired name work", cfg.ActiveProvider) + } + resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}}) + if err != nil { + t.Fatalf("Resolve after repair: %v", err) + } + if resolved.ActiveProvider != "work" || resolved.Provider.Name != "work" { + t.Fatalf("resolved active provider = %q profile = %q, want work", resolved.ActiveProvider, resolved.Provider.Name) + } + }) + + t.Run("openai fallback", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Model: "gpt-4o"}}}, 0o600) + cfg, _, err := RepairUnnamedProvider(path, "") + if err != nil { + t.Fatal(err) + } + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "openai" { + t.Fatalf("repaired config = %+v, want openai", cfg) + } + }) +} + +func TestRepairUnnamedProviderRejectsAmbiguousRepairWithoutWriting(t *testing.T) { + for name, cfg := range map[string]FileConfig{ + "name collision": {Providers: []ProviderProfile{{Name: ""}, {Name: "OPENAI"}}}, + "multiple unnamed": {Providers: []ProviderProfile{{Name: ""}, {Name: " "}}}, + } { + t.Run(name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + before := writeConfigFixture(t, path, cfg, 0o600) + if _, _, err := RepairUnnamedProvider(path, ""); err == nil { + t.Fatal("ambiguous repair succeeded") + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, before) { + t.Fatalf("rejected repair changed config\nbefore: %s\nafter: %s", before, after) + } + }) + } +} + +func TestRepairUnnamedProviderAllowsExplicitUniqueName(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: ""}, {Name: "OPENAI"}}}, 0o600) + cfg, _, err := RepairUnnamedProvider(path, "legacy") + if err != nil { + t.Fatal(err) + } + if cfg.Providers[0].Name != "legacy" { + t.Fatalf("repaired name = %q, want legacy", cfg.Providers[0].Name) + } +} + +func TestEnsureCatalogProviderValidatesBeforeExistingProfileShortcut(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + before := writeConfigFixture(t, path, FileConfig{Providers: []ProviderProfile{{Name: "xai"}, {Name: "XAI"}}}, 0o600) + if _, err := EnsureCatalogProvider(path, "xai"); err == nil || !strings.Contains(err.Error(), "ambiguous persisted provider names") { + t.Fatalf("EnsureCatalogProvider error = %v, want ambiguous config rejection", err) + } + after, err := os.ReadFile(path) + if err != nil || !bytes.Equal(after, before) { + t.Fatalf("rejected ensure changed config: readErr=%v", err) + } +} + +func TestResolvePersistedProviderNameBridgesIdentityToExactSpelling(t *testing.T) { + cases := []struct { + name string + providers []ProviderProfile + input string + want string + wantErr string + }{ + { + name: "exact spelling", + providers: []ProviderProfile{{Name: "OpenAI"}}, + input: "OpenAI", + want: "OpenAI", + }, + { + name: "case variant resolves to the row's own spelling", + providers: []ProviderProfile{{Name: "WORK"}}, + input: "work", + want: "WORK", + }, + { + name: "exact spelling wins over an earlier case variant", + providers: []ProviderProfile{{Name: "WORK"}, {Name: "work"}}, + input: "work", + want: "work", + }, + { + name: "ambiguous identity is an error, not an arbitrary pick", + providers: []ProviderProfile{{Name: "WORK"}, {Name: "Work"}}, + input: "work", + wantErr: "ambiguous provider", + }, + { + // The credential store keeps "s" and Unicode long-s apart, so these + // are two identities and neither resolves the other. + name: "unicode long-s is a distinct identity", + providers: []ProviderProfile{{Name: "ſ"}}, + input: "s", + wantErr: "not found", + }, + { + name: "unknown name", + providers: []ProviderProfile{{Name: "openai"}}, + input: "anthropic", + wantErr: "not found", + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{Providers: testCase.providers}, 0o600) + got, err := ResolvePersistedProviderName(path, testCase.input) + if testCase.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), testCase.wantErr) { + t.Fatalf("error = %v, want containing %q", err, testCase.wantErr) + } + return + } + if err != nil { + t.Fatal(err) + } + if got != testCase.want { + t.Fatalf("resolved = %q, want %q", got, testCase.want) + } + }) + } +} + +// Credential ownership is the marker, not the name: a surviving case variant +// that never claimed the shared key cannot keep it alive, or the secret is +// orphaned behind a profile ApplyStoredAPIKey will never read. +func TestCredentialKeyRetainedRequiresASurvivingOwner(t *testing.T) { + cases := []struct { + name string + providers []ProviderProfile + removed string + want bool + }{ + { + name: "survivor claims the credential", + providers: []ProviderProfile{{Name: "WORK", APIKeyStored: true}}, + removed: "work", + want: true, + }, + { + name: "survivor exists but never claimed the credential", + providers: []ProviderProfile{{Name: "WORK"}}, + removed: "work", + want: false, + }, + { + name: "no survivor shares the identity", + providers: []ProviderProfile{{Name: "other", APIKeyStored: true}}, + removed: "work", + want: false, + }, + { + name: "unicode long-s does not share the identity", + providers: []ProviderProfile{{Name: "ſ", APIKeyStored: true}}, + removed: "s", + want: false, + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + if got := CredentialKeyRetained(testCase.providers, testCase.removed); got != testCase.want { + t.Fatalf("CredentialKeyRetained = %v, want %v", got, testCase.want) + } + }) + } +} + +// The delete confirmation must be able to promise exactly what the delete does, +// so the pre-mutation answer has to match the post-mutation one. +func TestProviderKeyRetainedAfterRemovalMatchesPostRemovalAnswer(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + Providers: []ProviderProfile{{Name: "work", APIKeyStored: true}, {Name: "WORK", APIKeyStored: true}}, + }, 0o600) + + before, err := ProviderKeyRetainedAfterRemoval(path, "work") + if err != nil { + t.Fatal(err) + } + if !before { + t.Fatal("pre-removal answer = false, want true (WORK still claims the credential)") + } + cfg, err := RemoveProvider(path, "work") + if err != nil { + t.Fatal(err) + } + if after := CredentialKeyRetained(cfg.Providers, "work"); after != before { + t.Fatalf("post-removal answer = %v, want %v", after, before) + } +} + +// Repairing a case-duplicate config can leave activeProvider on a third +// spelling that matches no remaining row exactly, which every exact mutator +// then fails against. Removal re-points it at the survivor's own spelling. +func TestRemoveProviderNormalizesStaleActiveProviderSpelling(t *testing.T) { + path := filepath.Join(t.TempDir(), "zero.json") + writeConfigFixture(t, path, FileConfig{ + ActiveProvider: "WoRk", + Providers: []ProviderProfile{{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 the surviving row's spelling work", cfg.ActiveProvider) + } + if _, err := SetProviderModel(path, cfg.ActiveProvider, "gpt-4"); err != nil { + t.Fatalf("exact mutator still cannot find the active row: %v", err) + } +} diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index c95036383..25103936d 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -309,5 +309,16 @@ func (s *Store) lockPath() string { return s.file + ".lock" } func filepathDir(path string) string { return filepath.Dir(path) } 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/doctor/doctor.go b/internal/doctor/doctor.go index b6f222573..d40cb30a2 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -47,6 +47,7 @@ type Options struct { UserConfig string ProjectConfig string Provider config.ProviderProfile + ResolveError error WorkspaceRoot string Sandbox config.SandboxConfig Connectivity bool @@ -70,7 +71,7 @@ func Run(options Options) Report { configFilesCheck(options.UserConfig, options.ProjectConfig), configValidationCheck(options.UserConfig, options.ProjectConfig), } - providerCheck := providerConfigCheck(options.Provider) + providerCheck := providerConfigCheck(options.Provider, options.ResolveError) checks = append(checks, providerCheck) modelCheck := providerModelCheck(options.Provider) checks = append(checks, modelCheck) @@ -137,7 +138,10 @@ func configFilesCheck(userPath string, projectPath string) Check { return check("config.files", "Config files", StatusPass, "Zero config file inputs are available for inspection.", details) } -func providerConfigCheck(profile config.ProviderProfile) Check { +func providerConfigCheck(profile config.ProviderProfile, resolveErrors ...error) Check { + if len(resolveErrors) > 0 && resolveErrors[0] != nil { + return check("provider.config", "Provider config", StatusFail, "Provider config could not be resolved: "+resolveErrors[0].Error(), map[string]any{"help": "Follow the repair command in the error, then run `zero doctor` again."}) + } if emptyProviderProfile(profile) { return check("provider.config", "Provider config", StatusFail, "No LLM provider is configured.", map[string]any{"help": "Set a provider in config or environment."}) } @@ -410,16 +414,24 @@ func configValidationCheck(userPath string, projectPath string) Check { continue } _, issues := config.ValidateBytes(data) - if len(issues) == 0 { - continue - } - messages := make([]string, 0, len(issues)) + messages := make([]string, 0, len(issues)+1) for _, issue := range issues { messages = append(messages, issue.Message) } + if path == userPath { + var persisted config.FileConfig + if err := json.Unmarshal(data, &persisted); err == nil { + if err := config.ValidatePersistedProviderNames(persisted); err != nil { + messages = append(messages, err.Error()) + } + } + } + if len(messages) == 0 { + continue + } details[path] = map[string]any{"issues": messages} status = StatusFail - issueCount += len(issues) + issueCount += len(messages) } if status == StatusPass { diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 4fecf761b..00f2937b5 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -1,6 +1,7 @@ package doctor import ( + "fmt" "os" "path/filepath" "strings" @@ -201,6 +202,19 @@ func TestConfigValidationCheckPassesForValidConfig(t *testing.T) { } } +func TestConfigValidationCheckReportsPersistedProviderNameRepair(t *testing.T) { + path := writeDoctorConfig(t, `{"providers":[{"name":""},{"name":"work"},{"name":"WORK"}]}`) + report := Run(Options{Runtime: "go", UserConfig: path, ResolveError: config.ValidatePersistedProviderNames(config.FileConfig{Providers: []config.ProviderProfile{{Name: ""}}})}) + check := report.Check("config.validation") + if check == nil || check.Status != StatusFail || !strings.Contains(fmt.Sprint(check.Details), "providers repair-config") { + t.Fatalf("persisted-name validation = %#v", check) + } + provider := report.Check("provider.config") + if provider == nil || !strings.Contains(provider.Message, "could not be resolved") || strings.Contains(provider.Message, "No LLM provider") { + t.Fatalf("provider resolution diagnostic = %#v", provider) + } +} + func TestConfigValidationCheckFailsMalformedJSONWithLineCol(t *testing.T) { // Unterminated object: the trailing comma + EOF yields a *json.SyntaxError // whose offset is the end of the 32-byte document (line 3, col 1). diff --git a/internal/oauth/manager.go b/internal/oauth/manager.go index dbdf7817e..d6d61ae57 100644 --- a/internal/oauth/manager.go +++ b/internal/oauth/manager.go @@ -32,6 +32,10 @@ type Manager struct { now func() time.Time buffer time.Duration out io.Writer + // beforeSave revalidates caller-owned state immediately before a completed + // login replaces a token. Interactive OAuth can take minutes, so validating + // only before it starts leaves a race where config becomes invalid mid-flow. + beforeSave func() error // openBrowser is invoked with the authorization URL for loopback logins. // Tests inject a function that drives the loopback redirect. openBrowser func(authURL string) error @@ -59,6 +63,10 @@ type ManagerOptions struct { RefreshBuffer time.Duration Out io.Writer OpenBrowser func(authURL string) error + // BeforeSave runs after authorization succeeds but before the token store is + // mutated. Login-only callers use it to fail closed when related config state + // changed during an interactive browser or device flow. + BeforeSave func() error } // NewManager builds a Manager, filling defaults. @@ -96,7 +104,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, + env: env, now: now, buffer: buffer, out: out, openBrowser: open, beforeSave: opts.BeforeSave, }, nil } @@ -143,6 +151,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 +212,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/oauth/manager_test.go b/internal/oauth/manager_test.go index 219389940..6ddafaa65 100644 --- a/internal/oauth/manager_test.go +++ b/internal/oauth/manager_test.go @@ -293,3 +293,34 @@ func TestManagerLogout(t *testing.T) { t.Fatal("second logout should report nothing removed") } } + +func TestCompleteDeviceLoginBeforeSaveFailurePreservesPreviousToken(t *testing.T) { + fp := newFakeProvider(t, `{"access_token":"replacement","refresh_token":"replacement-refresh","expires_in":3600}`) + store, err := NewStore(StoreOptions{FilePath: filepath.Join(t.TempDir(), "oauth.json")}) + if err != nil { + t.Fatal(err) + } + key := ProviderKey("demo") + previous := Token{AccessToken: "previous", RefreshToken: "previous-refresh"} + if err := store.Save(key, previous); err != nil { + t.Fatal(err) + } + wantErr := errors.New("config changed during login") + manager, err := NewManager(ManagerOptions{ + Store: store, + HTTPClient: fp.server.Client(), + BeforeSave: func() error { return wantErr }, + }) + if err != nil { + t.Fatal(err) + } + auth := DeviceAuth{DeviceCode: "device", ExpiresAt: time.Now().Add(time.Minute), Interval: time.Millisecond} + _, err = manager.CompleteDeviceLogin(context.Background(), "demo", Config{TokenEndpoint: fp.server.URL + "/token", ClientID: "client"}, auth) + if !errors.Is(err, wantErr) { + t.Fatalf("CompleteDeviceLogin error = %v, want pre-save rejection", err) + } + stored, ok, err := store.Load(key) + if err != nil || !ok || stored.AccessToken != previous.AccessToken || stored.RefreshToken != previous.RefreshToken { + t.Fatalf("pre-save rejection changed token: ok=%v err=%v token=%+v", ok, err, stored) + } +} diff --git a/internal/tui/command_center.go b/internal/tui/command_center.go index f922ee5bd..f38390817 100644 --- a/internal/tui/command_center.go +++ b/internal/tui/command_center.go @@ -449,7 +449,12 @@ func (m model) handleModelCommand(args string) (model, string) { if err != nil { return m, "Model\n" + err.Error() } - persisted, persistErr := m.persistSelectedModel(nextProfile) + persisted, persistedName, persistErr := m.persistSelectedModel(nextProfile) + if persisted { + // Same reconciliation switchProviderModel does: the manager and picker + // read models from savedProviders, not from the live profile. + m.savedProviders = syncSavedProviderModel(m.savedProviders, persistedName, nextProfile.Model) + } m.providerProfile = nextProfile m.provider = nextProvider @@ -580,9 +585,47 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string, ) // Keep sub-agent child processes on the same provider we just switched to. config.SetActiveProviderEnv(target.Name) + persistNote := "" if strings.TrimSpace(m.userConfigPath) != "" { - _, _ = config.SetActiveProvider(m.userConfigPath, target.Name) - _, _ = config.SetProviderModel(m.userConfigPath, target.Name, target.Model) + // SetActiveProvider accepts any spelling of the credential identity and + // returns the config with the persisted row's OWN spelling in + // ActiveProvider. SetProviderModel matches rows exactly, so persist with + // that resolved name — passing the session's spelling silently wrote + // nothing whenever the two differed (session "openai", row "OpenAI"). + // + // Env-derived providers have no row to update, so they are skipped + // silently; a failure to write a row that DOES exist is surfaced rather + // than swallowed, since the session and config.json then disagree. + // Ownership, not a credential-identity probe: "config.json carries this + // identity" was true for a project row whose identity a DIFFERENT user + // row owns, and the switch then pointed activeProvider at that user row + // and wrote this row's model onto it — a profile with another endpoint + // that the user never selected. + owner, err := config.ProviderRowOwnershipAt(m.userConfigPath, config.ProviderProfileNames(m.savedProviders), target.Name) + switch { + case err != nil: + persistNote = "\nNote: the switch applies to this session, but config.json could not be read: " + redaction.RedactString(err.Error(), redaction.Options{}) + case owner.UserBacked: + if cfg, err := config.SetActiveProvider(m.userConfigPath, owner.PersistedName); err != nil { + persistNote = "\nNote: the switch applies to this session, but config.json was not updated: " + redaction.RedactString(err.Error(), redaction.Options{}) + } else if _, err := config.SetProviderModel(m.userConfigPath, cfg.ActiveProvider, target.Model); err != nil { + persistNote = "\nNote: the active provider was saved, but its model was not: " + redaction.RedactString(err.Error(), redaction.Options{}) + } else { + // Reconcile the in-memory list the manager and picker read from, + // or those surfaces keep showing the previous model until restart. + m.savedProviders = syncSavedProviderModel(m.savedProviders, cfg.ActiveProvider, target.Model) + } + case owner.Lookup == config.ProviderNameNotFound: + // The ordinary case: an environment-derived provider has no + // config.json row to update at all. Stay silent, same as before — + // only the surprising outcomes below (shadowed, ambiguous) are worth + // a note. + default: + // Shadowed by a listed sibling, or ambiguous: say so rather than + // silently writing through a row that only shares the credential + // identity, or picking one of several at random. + persistNote = "\nNote: the switch applies to this session only — " + owner.Reason + "." + } } // Warm discovery for the provider we just switched to, same as Init() does // for the provider active at launch — otherwise the context-usage gauge has @@ -597,6 +640,7 @@ func (m model) switchProviderModel(providerName, modelID string) (model, string, } } status := fmt.Sprintf("Model\nSwitched to %s · %s", target.Name, target.Model) + status += persistNote if warn := m.visionDropWarning(); warn != "" { status += "\n" + warn } @@ -674,44 +718,85 @@ func oauthLoginName(profile config.ProviderProfile) (string, bool) { return strings.TrimPrefix(key, oauth.KeyPrefixProvider), true } +// activeProviderRowName is the saved-row spelling this session actually runs on. +// It is sessionRowName's answer — exact first, sole identity match otherwise — +// so every "is this the provider I am on?" comparison uses one value instead of +// each caller re-deciding what "active" means from a credential identity. +func (m model) activeProviderRowName() string { + return sessionRowName(m.providerName, m.savedProviders) +} + +// savedProviderByName resolves a provider spelling to its saved profile through +// the ONE shared rule (config.LookupProviderName): an exact spelling wins +// outright, a credential-identity match is accepted only when exactly one saved +// row carries that identity, and several rows are AMBIGUOUS rather than +// first-match. +// +// First-match was the defect: with saved "Target" and "target" resolved side by +// side, a lookup for either spelling returned whichever row came first, so a +// model chosen under one endpoint could be applied to the other. The rule also +// stays off strings.EqualFold on purpose — EqualFold folds "s" and Unicode +// long-s "ſ" together while the credential store keeps them separate, so folding +// here could hand back a different provider's profile and reach its secret. func (m model) savedProviderByName(name string) (config.ProviderProfile, bool) { - name = strings.TrimSpace(name) - for _, profile := range m.savedProviders { - if strings.EqualFold(strings.TrimSpace(profile.Name), name) { - return profile, true + resolved, lookup := config.LookupProviderName(config.ProviderProfileNames(m.savedProviders), name) + if lookup.Resolved() { + for _, profile := range m.savedProviders { + if strings.TrimSpace(profile.Name) == resolved { + return profile, true + } } } - if strings.EqualFold(strings.TrimSpace(m.providerProfile.Name), name) { + if lookup == config.ProviderNameAmbiguous { + // Say nothing rather than guess: the caller falls back to the active + // provider, which is a visible outcome, instead of silently writing + // through one of several rows. + return config.ProviderProfile{}, false + } + if _, live := config.LookupProviderName([]string{m.providerProfile.Name}, name); live.Resolved() { return m.providerProfile, true } return config.ProviderProfile{}, false } -func (m model) persistSelectedModel(profile config.ProviderProfile) (bool, error) { +// persistSelectedModel writes profile's model to its config.json row and +// returns the EXACT row spelling it wrote to, so the caller can mirror the same +// change into savedProviders with syncSavedProviderModel rather than re-deriving +// the row from the session's spelling. +func (m model) persistSelectedModel(profile config.ProviderProfile) (bool, string, error) { path := strings.TrimSpace(m.userConfigPath) if path == "" { - return false, nil + return false, "", nil } name := strings.TrimSpace(profile.Name) if name == "" { - return false, nil + return false, "", nil } model := strings.TrimSpace(profile.Model) if model == "" { - return false, nil - } - persisted, err := config.ProviderPersisted(path, name) + return false, "", nil + } + // Provenance, not a credential-identity probe. "config.json carries this + // identity" was true for a project row whose identity a DIFFERENT user row + // owns, and the model was then persisted onto that user row — a profile the + // user never selected, with its own endpoint. Ownership consults the siblings + // the session resolved, which is what tells those two cases apart, and + // PersistedName is the exact spelling SetProviderModel needs (a case + // difference would otherwise make the write a silent no-op). + owner, err := config.ProviderRowOwnershipAt(path, config.ProviderProfileNames(m.savedProviders), name) if err != nil { - return false, err + return false, "", err } - if !persisted { - // Env-derived providers have no config.json row to update. - return false, nil + if !owner.UserBacked { + // Project- and environment-derived rows have no config.json row of their + // own; the model change stays in this session. + return false, "", nil } - if _, err := config.SetProviderModel(path, name, model); err != nil { - return false, err + exactName := owner.PersistedName + if _, err := config.SetProviderModel(path, exactName, model); err != nil { + return false, "", err } - return true, nil + return true, exactName, nil } type modelSwitchTarget struct { diff --git a/internal/tui/command_center_test.go b/internal/tui/command_center_test.go new file mode 100644 index 000000000..cf322041f --- /dev/null +++ b/internal/tui/command_center_test.go @@ -0,0 +1,215 @@ +package tui + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// The session's provider spelling can differ from the persisted row's +// ("openai" vs a saved "OpenAI"). Activation matches credential identity while +// the model write matches the row exactly, so the model write has to use the +// resolved spelling or it silently persists nothing. +func TestModelPersistenceUsesResolvedPersistedSpelling(t *testing.T) { + newConfig := func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"activeProvider":"OpenAI","providers":[{"name":"OpenAI","catalogID":"openai","model":"gpt-5.1"},{"name":"ollama","catalogID":"ollama","provider_kind":"openai-compatible","baseURL":"http://localhost:11434/v1","model":"m1"}]}`), 0o600); err != nil { + t.Fatal(err) + } + return path + } + + t.Run("persistSelectedModel", func(t *testing.T) { + configPath := newConfig(t) + m := newModel(context.Background(), Options{UserConfigPath: configPath}) + // The session spelling "openai" addresses the persisted "OpenAI" row. + persisted, persistedName, err := m.persistSelectedModel(config.ProviderProfile{Name: "openai", Model: "gpt-5.5"}) + if err != nil { + t.Fatal(err) + } + if !persisted { + t.Fatal("persistSelectedModel reported no write for a persisted case variant") + } + // The returned spelling is what the caller mirrors into savedProviders, + // so it has to be the row's own — not the session's. + if persistedName != "OpenAI" { + t.Fatalf("persisted row = %q, want the row's spelling OpenAI", persistedName) + } + cfg := readTUIConfigFixture(t, configPath) + if cfg.Providers[0].Model != "gpt-5.5" { + t.Fatalf("model = %q, want gpt-5.5 written to the OpenAI row", cfg.Providers[0].Model) + } + }) + + t.Run("switchProviderModel", func(t *testing.T) { + configPath := newConfig(t) + saved := []config.ProviderProfile{ + {Name: "OpenAI", CatalogID: "openai", Model: "gpt-5.1", APIKey: "sk-test"}, + {Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "http://localhost:11434/v1", Model: "m1"}, + } + m := newModel(context.Background(), Options{ + UserConfigPath: configPath, + ProviderName: "ollama", + ModelName: "m1", + Provider: &fakeProvider{}, + ProviderProfile: saved[1], + SavedProviders: saved, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + + // "openai" is the picker row's owner spelling, not the persisted one. + if _, status, ok, _ := m.switchProviderModel("openai", "gpt-5.5"); !ok { + t.Fatalf("switch to a case-variant provider spelling failed: %s", status) + } + cfg := readTUIConfigFixture(t, configPath) + if cfg.ActiveProvider != "OpenAI" { + t.Fatalf("activeProvider = %q, want the row's spelling OpenAI", cfg.ActiveProvider) + } + if cfg.Providers[0].Model != "gpt-5.5" { + t.Fatalf("model = %q, want gpt-5.5 persisted onto the OpenAI row", cfg.Providers[0].Model) + } + }) +} + +// The provider manager's rows and the picker's model sections are built from +// savedProviders, not from the live profile: a switch that updates the client +// and config.json without mirroring the list leaves those surfaces showing the +// previous model until the TUI restarts and re-resolves providers from config. +func TestModelSwitchSyncsSavedProviders(t *testing.T) { + newSession := func(t *testing.T) model { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"activeProvider":"OpenAI","providers":[{"name":"OpenAI","catalogID":"openai","model":"gpt-5.1"},{"name":"ollama","catalogID":"ollama","provider_kind":"openai-compatible","baseURL":"http://localhost:11434/v1","model":"m1"}]}`), 0o600); err != nil { + t.Fatal(err) + } + saved := []config.ProviderProfile{ + {Name: "OpenAI", CatalogID: "openai", Model: "gpt-5.1", APIKey: "sk-test"}, + {Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "http://localhost:11434/v1", Model: "m1"}, + } + return newModel(context.Background(), Options{ + UserConfigPath: path, + ProviderName: "ollama", + ModelName: "m1", + Provider: &fakeProvider{}, + ProviderProfile: saved[1], + SavedProviders: saved, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + } + + t.Run("switchProviderModel", func(t *testing.T) { + m := newSession(t) + // "openai" is the picker row's owner spelling, not the persisted one: + // the mirror must land on the row SetProviderModel actually wrote. + next, status, ok, _ := m.switchProviderModel("openai", "gpt-5.5") + if !ok { + t.Fatalf("switch failed: %s", status) + } + if next.savedProviders[0].Model != "gpt-5.5" { + t.Fatalf("savedProviders model = %q, want the switched gpt-5.5 without a restart", next.savedProviders[0].Model) + } + if next.savedProviders[1].Model != "m1" { + t.Fatalf("switch touched an unrelated row: %+v", next.savedProviders[1]) + } + // The manager renders each row's model straight off this list. + if meta := providerManagerRowMeta(next.savedProviders[0]); !strings.Contains(meta, "gpt-5.5") { + t.Fatalf("manager row meta = %q, want the switched model", meta) + } + }) + + t.Run("handleModelCommand", func(t *testing.T) { + m := newSession(t) + // Exercise the production caller that owns both persistence and the + // savedProviders mirror; calling the two helpers separately would stay + // green if their production pairing were removed. + m.providerName = "openai" + m.providerProfile = m.savedProviders[0] + m.modelName = m.providerProfile.Model + next, status := m.handleModelCommand("gpt-4.1-mini") + if next.savedProviders[0].Model != "gpt-4.1-mini" { + t.Fatalf("savedProviders model = %q, want gpt-4.1-mini; status=%q", next.savedProviders[0].Model, status) + } + if next.savedProviders[1].Model != "m1" { + t.Fatalf("mirror touched an unrelated row: %+v", next.savedProviders[1]) + } + }) +} + +// The switch deliberately continues in-session when config.json cannot be +// updated, so the note is the only thing telling the user the two now disagree. +// Silence here would read as a saved switch that was never persisted. +func TestSwitchProviderModelReportsPersistenceFailures(t *testing.T) { + saved := []config.ProviderProfile{ + {Name: "OpenAI", CatalogID: "openai", Model: "gpt-5.1", APIKey: "sk-test"}, + {Name: "ollama", CatalogID: "ollama", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "http://localhost:11434/v1", Model: "m1"}, + } + newSwitchModel := func(t *testing.T, configJSON string) model { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(configJSON), 0o600); err != nil { + t.Fatal(err) + } + return newModel(context.Background(), Options{ + UserConfigPath: path, + ProviderName: "ollama", + ModelName: "m1", + Provider: &fakeProvider{}, + ProviderProfile: saved[1], + SavedProviders: saved, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + } + + t.Run("unreadable config", func(t *testing.T) { + m := newSwitchModel(t, `{"providers":[`) // invalid JSON + next, status, ok, _ := m.switchProviderModel("OpenAI", "gpt-5.5") + if !ok { + t.Fatalf("the in-session switch must still succeed: %s", status) + } + if !strings.Contains(status, "config.json could not be read") { + t.Fatalf("status = %q, want a persistence note", status) + } + // The session did switch, which is exactly why the note has to be there. + if next.providerName != "OpenAI" { + t.Fatalf("providerName = %q, want OpenAI", next.providerName) + } + }) + + t.Run("ambiguous rows block the write", func(t *testing.T) { + // Duplicate case variants pass the persisted gate but make the write + // itself unresolvable. + m := newSwitchModel(t, `{"providers":[{"name":"OpenAI"},{"name":"openai"}]}`) + _, status, ok, _ := m.switchProviderModel("OpenAI", "gpt-5.5") + if !ok { + t.Fatalf("the in-session switch must still succeed: %s", status) + } + if !strings.Contains(status, "config.json was not updated") { + t.Fatalf("status = %q, want the active-provider persistence note", status) + } + }) + + t.Run("env-derived provider stays silent", func(t *testing.T) { + // No row to update is not a failure, so it must not produce a note. + m := newSwitchModel(t, `{"providers":[{"name":"ollama","model":"m1"}]}`) + _, status, ok, _ := m.switchProviderModel("OpenAI", "gpt-5.5") + if !ok { + t.Fatalf("switch failed: %s", status) + } + if strings.Contains(status, "Note:") { + t.Fatalf("status = %q, want no note for a provider with no persisted row", status) + } + }) +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 72b081258..9e56ecde5 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -88,6 +88,8 @@ type model struct { probeProviderHealth func(context.Context, providerhealth.Options) providerhealth.Result discoverProviderModels func(context.Context, config.ProviderProfile) ([]providermodeldiscovery.Model, error) discoverOllamaContextWindow func(ctx context.Context, baseURL string, model string) (int, error) + deleteProviderKey func(configPath, provider string) (bool, error) + clearProviderKeyStored func(configPath, provider string) (bool, error) registry *tools.Registry // lspManager is created once per session and reused across prompts so gopls (and // other language servers) stay warm — a fresh manager per run would cold-start @@ -969,6 +971,8 @@ func newModel(ctx context.Context, options Options) model { probeProviderHealth: options.ProbeProviderHealth, discoverProviderModels: options.DiscoverProviderModels, discoverOllamaContextWindow: options.DiscoverOllamaContextWindow, + deleteProviderKey: deleteProviderKey, + clearProviderKeyStored: config.ClearProviderKeyStoredCaseVariants, registry: registry, sessionStore: sessionStore, peerService: options.PeerService, @@ -4409,14 +4413,25 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { previousProvider, previousModel := m.providerName, m.modelName text := "" owner := strings.TrimSpace(item.OwnerProvider) - _, ownerIsSavedProvider := m.savedProviderByName(owner) - if owner != "" && !strings.EqualFold(owner, strings.TrimSpace(m.providerName)) && ownerIsSavedProvider { + ownerProfile, ownerIsSavedProvider := m.savedProviderByName(owner) + // Compare the resolved owner ROW to the resolved active ROW, not the two + // credential identities. Identity comparison made an item rendered under + // project "target" equal to active user "Target", so the branch below was + // skipped and the model was applied to — and persisted on — the OTHER + // endpoint's profile with nothing shown to say so. savedProviderByName + // now also refuses an ambiguous spelling rather than returning the first + // row, so an unresolvable owner lands on the active provider instead of a + // coin flip. + sameRow := ownerIsSavedProvider && + strings.TrimSpace(ownerProfile.Name) == strings.TrimSpace(m.activeProviderRowName()) + if owner != "" && ownerIsSavedProvider && !sameRow { // A model from another saved provider: switch provider + model together. - m, text, _, cmd = m.switchProviderModel(owner, item.Value) + m, text, _, cmd = m.switchProviderModel(ownerProfile.Name, item.Value) } else { - // OwnerProvider is blank, matches the active provider, or (registry-fallback - // / stale-history rows) doesn't resolve to any saved provider: apply against - // the active provider instead of attempting an unresolvable provider switch. + // OwnerProvider is blank, resolves to the row this session already runs + // on, or (registry-fallback / stale-history / ambiguous rows) resolves + // to no single saved provider: apply against the active provider + // instead of attempting an unresolvable or self-directed switch. m, text = m.handleModelCommand(item.Value) } if m.providerName != previousProvider || m.modelName != previousModel { diff --git a/internal/tui/oauth_device.go b/internal/tui/oauth_device.go index 00bdb557c..457e2ec0b 100644 --- a/internal/tui/oauth_device.go +++ b/internal/tui/oauth_device.go @@ -9,9 +9,17 @@ import ( "time" "github.com/Gitlawb/zero/internal/browser" + "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/oauth" ) +func preflightOAuthLogin(configPath string) error { + if strings.TrimSpace(configPath) == "" { + return nil + } + return config.PreflightUserConfig(configPath) +} + // oauthPreferDeviceFlow reports whether the device-code flow should be the // default for a device-capable provider because no usable browser is likely // present (SSH session or a headless Linux box). On a desktop the browser flow @@ -59,7 +67,10 @@ 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(configPath string, name string, cfg oauth.Config, auth oauth.DeviceAuth) error { + if err := preflightOAuthLogin(configPath); err != nil { + return err + } store, err := oauth.NewStore(oauth.StoreOptions{}) if err != nil { return err @@ -68,6 +79,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 preflightOAuthLogin(configPath) }, }) if err != nil { return err diff --git a/internal/tui/onboarding.go b/internal/tui/onboarding.go index ed09f3a87..b1f14b41b 100644 --- a/internal/tui/onboarding.go +++ b/internal/tui/onboarding.go @@ -537,10 +537,13 @@ 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 { switch { case provider.OAuthMintsKey: return func() tea.Msg { + if err := preflightOAuthLogin(configPath); 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 +552,13 @@ func setupOAuthCmd(provider providercatalog.Descriptor) tea.Cmd { } case provider.ID == "chatgpt": return func() tea.Msg { - err := runProviderChatGPTLogin() + err := runProviderChatGPTLogin(configPath) 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(configPath, name)} } } } @@ -571,8 +574,11 @@ type setupOAuthDeviceMsg struct { err error } -func setupDevicePrepareCmd(name string) tea.Cmd { +func setupDevicePrepareCmd(configPath string, name string) tea.Cmd { return func() tea.Msg { + if err := preflightOAuthLogin(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 +593,9 @@ func setupDevicePrepareCmd(name string) tea.Cmd { } } -func setupDevicePollCmd(name string, cfg oauth.Config, auth oauth.DeviceAuth) tea.Cmd { +func setupDevicePollCmd(configPath string, name string, cfg oauth.Config, auth oauth.DeviceAuth) tea.Cmd { return func() tea.Msg { - return setupOAuthMsg{tokenLogin: true, providerID: name, err: oauthDeviceComplete(name, cfg, auth)} + return setupOAuthMsg{tokenLogin: true, providerID: name, err: oauthDeviceComplete(configPath, name, cfg, auth)} } } @@ -604,7 +610,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(m.setup.configPath, descriptor.ID) } // applySetupOAuthDeviceCode handles phase 1 of device-code login: show the code, @@ -626,7 +632,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(m.setup.configPath, msg.providerID, msg.cfg, msg.auth) } // applySetupOAuth folds an OAuth login result into the first-run setup: on success @@ -790,7 +796,7 @@ func (m model) advanceSetup() (tea.Model, tea.Cmd) { m.setup.oauthPending = true m.setup.oauthDevice = false m.setup.oauthErr = "" - return m, setupOAuthCmd(descriptor) + return m, setupOAuthCmd(descriptor, m.setup.configPath) } } if m.setup.stage == setupStageProvider { diff --git a/internal/tui/picker.go b/internal/tui/picker.go index 563dffe62..a84ab8e3b 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -196,7 +196,11 @@ func (m model) newModelPicker() *commandPicker { return nil } activeModel := strings.TrimSpace(m.modelName) - activeProvider := strings.TrimSpace(m.providerName) + // The saved ROW this session runs on, not the raw live spelling: a + // credential-identity comparison marked BOTH of a pair of case-sibling rows + // active, so the picker showed two "active" endpoints and gave no way to tell + // which one a selection would land on. + activeProvider := strings.TrimSpace(m.activeProviderRowName()) recent := []pickerItem{} for _, pair := range m.recentModelPairsForPicker() { recent = append(recent, m.modelPickerRecentItem(registry, pair.Provider, pair.Model)) @@ -282,7 +286,11 @@ func (m model) modelPickerProviders() []config.ProviderProfile { // active provider prefers its live-discovered models when available. func (m model) savedProviderModelPickerItems(profile config.ProviderProfile, activeProvider, activeModel string) []pickerItem { providerName := strings.TrimSpace(profile.Name) - isActive := providerName != "" && strings.EqualFold(providerName, activeProvider) + // Exact row equality. activeProvider is already the resolved active ROW (see + // newModelPicker), and SameProviderIdentity here marked user "Target" and + // project "target" active at the same time — two rows with different + // endpoints, one badge each, and no way to tell them apart. + isActive := providerName != "" && providerName == strings.TrimSpace(activeProvider) descriptor, hasDescriptor := m.descriptorForProfile(profile) group := modelPickerProviderGroup(profile, descriptor, hasDescriptor) diff --git a/internal/tui/provider_identity_test.go b/internal/tui/provider_identity_test.go new file mode 100644 index 000000000..70e434c29 --- /dev/null +++ b/internal/tui/provider_identity_test.go @@ -0,0 +1,25 @@ +package tui + +import ( + "testing" + + "github.com/Gitlawb/zero/internal/config" +) + +func TestSavedProviderByNameDistinguishesUnicodeCredentialIdentities(t *testing.T) { + m := model{ + providerProfile: config.ProviderProfile{Name: "s", Model: "ascii-model"}, + savedProviders: []config.ProviderProfile{ + {Name: "s", Model: "ascii-model"}, + {Name: "ſ", Model: "long-s-model"}, + }, + } + + profile, ok := m.savedProviderByName("ſ") + if !ok { + t.Fatal("long-s provider not found") + } + if profile.Name != "ſ" || profile.Model != "long-s-model" { + t.Fatalf("selected provider = %q/%q, want long-s provider", profile.Name, profile.Model) + } +} diff --git a/internal/tui/provider_manager.go b/internal/tui/provider_manager.go index 6816074d7..f63d156e2 100644 --- a/internal/tui/provider_manager.go +++ b/internal/tui/provider_manager.go @@ -17,6 +17,7 @@ import ( "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/oauth" + "github.com/Gitlawb/zero/internal/redaction" ) const providerManagerMaxVisible = 10 @@ -24,10 +25,19 @@ const providerManagerMaxVisible = 10 // providerManagerRow is one saved provider in the list. cred is resolved // asynchronously (keychain reads shell out to `security` on macOS and must // never block the render loop); empty means "still checking". +// +// owner is the row's PROVENANCE, resolved once when the list is built. A row +// carries a resolved profile whose Name says nothing about which layer produced +// it, and the mutation paths used to reconstruct ownership from that string — +// so a project row that merely shared a credential identity with a user row +// edited and deleted through it. Only owner.UserBacked may reach a user-config +// or credential-store mutator, and owner.PersistedName is the exact row those +// mutators address. type providerManagerRow struct { profile config.ProviderProfile local bool cred string + owner config.ProviderRowOwnership } type providerEditField int @@ -95,9 +105,20 @@ func (m model) reloadProviderManagerRows() (model, tea.Cmd) { for _, row := range m.providerWizard.manageRows { previous[row.profile.Name] = row.cred } + // Provenance is resolved ONCE per row, here, against the whole resolved list + // — the siblings are what distinguish "this row is the user's row under a + // different spelling" from "the user's row is listed separately and this one + // is a project/env row that only shares its credential identity". + resolvedNames := config.ProviderProfileNames(m.savedProviders) rows := make([]providerManagerRow, 0, len(m.savedProviders)) for _, profile := range m.savedProviders { row := providerManagerRow{profile: profile, cred: previous[profile.Name]} + owner, err := config.ProviderRowOwnershipAt(m.userConfigPath, resolvedNames, profile.Name) + if err != nil { + // An unreadable config is not a licence to write to it. + owner = config.ProviderRowOwnership{Reason: "config.json could not be read: " + err.Error()} + } + row.owner = owner if descriptor, ok := m.descriptorForProfile(profile); ok { row.local = descriptor.Local } @@ -106,8 +127,10 @@ func (m model) reloadProviderManagerRows() (model, tea.Cmd) { m.providerWizard.manageRows = rows m.providerWizard.manageCursor = clampInt(m.providerWizard.manageCursor, 0, maxInt(0, len(rows)-1)) // The session's live provider is the truth the user cares about (config's - // activeProvider follows it on every switch). - m.providerWizard.manageActiveName = m.providerName + // activeProvider follows it on every switch). Resolve it to the row it + // refers to once, here, so the render's exact comparison and the sync paths + // below share one value instead of each re-deciding what "active" means. + m.providerWizard.manageActiveName = sessionRowName(m.providerName, m.savedProviders) m.providerWizard.manageCredGen++ return m, providerManagerCredsCmd(m.providerWizard.manageCredGen, rows, m.userConfigPath) } @@ -277,13 +300,25 @@ func (m model) handleProviderManageListKey(msg tea.KeyMsg) (model, tea.Cmd) { return m, nil case strings.EqualFold(keyText(msg), "e"): if row, ok := wizard.currentManagerRow(); ok { - wizard.beginProviderEdit(row.profile) + // An edit writes to config.json and can replace a stored key, so a + // row with no user-config row of its own has nothing to edit. Saying + // so beats applying this row's draft to whichever user row happens to + // share its credential identity. + if !row.owner.UserBacked { + wizard.manageStatus = "Can't edit " + row.profile.Name + ": " + row.owner.Reason + "." + return m, nil + } + wizard.beginProviderEdit(row.profile, row.owner) } return m, nil case strings.EqualFold(keyText(msg), "d"): - if _, ok := wizard.currentManagerRow(); ok { + if row, ok := wizard.currentManagerRow(); ok { wizard.manageDeleting = true wizard.manageStatus = "" + // Resolve the retention outcome now, from the same ownership the + // delete uses, so the confirmation cannot promise a key removal the + // delete will not perform. + wizard.manageDeleteKeyNote = providerDeleteKeyNote(m.userConfigPath, row.owner) } return m, nil } @@ -348,39 +383,54 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { return m, nil } - persisted, err := config.ProviderPersisted(m.userConfigPath, name) - if err != nil { - wizard.manageStatus = "Delete failed: " + err.Error() - return m, nil - } var notes []string var activeAfter string var cleanup tea.Cmd - if persisted { - cfg, err := config.RemoveProvider(m.userConfigPath, name) + // The row's provenance decides this, not a fresh name lookup. Asking + // "does config.json hold this credential identity?" answered yes for a + // project row whose identity a DIFFERENT user row owns, and the delete then + // removed that user row while the in-memory removal took the project one. + if row.owner.UserBacked { + exactName := row.owner.PersistedName + cfg, err := config.RemoveProvider(m.userConfigPath, exactName) if err != nil { wizard.manageStatus = "Delete failed: " + err.Error() return m, nil } activeAfter = cfg.ActiveProvider - notes = []string{"Deleted " + name + "."} - cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile) + deleteStoredKey := !config.CredentialKeyRetained(cfg.Providers, exactName) + if deleteStoredKey { + notes = []string{"Deleted " + name + ". Its stored API key will also be deleted."} + } else { + notes = []string{"Deleted " + name + ". Kept its stored API key because another saved provider still uses that credential."} + } + cleanup = providerManagerCleanupCmd(m.userConfigPath, row.profile, deleteStoredKey) } else { - // Env-derived providers have no persisted profile or credential to - // delete. Keep this path session-only. - notes = []string{ - "Removed " + name + " from this session.", - "It wasn't saved in config.json (likely set via an environment variable) — unset it to stop Zero from detecting it automatically.", + // Project- and environment-derived rows have no user-config row and no + // credential of their own to delete. Removing them from the session is + // the whole operation; nothing on disk is touched. The reason names the + // row that DOES own the identity when one exists, so a user who expected + // a saved provider to disappear can see why it did not. + notes = []string{"Removed " + name + " from this session."} + if reason := strings.TrimSpace(row.owner.Reason); reason != "" { + notes = append(notes, reason+" — nothing in config.json changed.") + } else { + notes = append(notes, "It wasn't saved in config.json (likely set via an environment variable) — unset it to stop Zero from detecting it automatically.") } } + // Decide whether the deleted row is the one this session runs on BEFORE the + // list shrinks: sessionRowName counts identity-carrying rows, and removing + // one of them changes that count. + deletedLiveRow := sessionRefersToPersistedRow(m.providerName, name, m.savedProviders) + // Surgical removal — see saveManagerEdit for why the raw cfg.Providers list // must not replace the resolved/filtered savedProviders wholesale. m.savedProviders = removeSavedProvider(m.savedProviders, name) - if strings.EqualFold(strings.TrimSpace(m.providerName), strings.TrimSpace(name)) { + if deletedLiveRow { notes = append(notes, "This session keeps running on it until you switch.") - } else if activeAfter != "" && !strings.EqualFold(activeAfter, name) { + } else if activeAfter != "" && !samePersistedProviderName(activeAfter, name) { notes = append(notes, "Active provider: "+activeAfter+".") } @@ -398,7 +448,7 @@ func (m model) deleteManagerSelection() (model, tea.Cmd) { 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 +456,78 @@ func removeSavedProvider(saved []config.ProviderProfile, name string) []config.P return kept } +// providerDeleteKeyNote is the delete confirmation's sentence about the stored +// key, computed from the same OWNERSHIP the delete itself uses so the prompt can +// never promise an outcome the delete will not produce. It returns "" — no claim +// at all — for a row with nothing to say: a project/env row with no user-config +// row of its own, no user config path, or a config whose ambiguity keeps the +// delete off disk entirely. +func providerDeleteKeyNote(configPath string, owner config.ProviderRowOwnership) string { + if strings.TrimSpace(configPath) == "" || !owner.UserBacked { + return "" + } + retained, err := config.ProviderKeyRetainedAfterRemoval(configPath, owner.PersistedName) + if err != nil { + return "" + } + if retained { + return "Its stored API key is kept — another saved provider still uses that credential." + } + return "This also removes its stored API key." +} + +func samePersistedProviderName(left, right string) bool { + return strings.TrimSpace(left) == strings.TrimSpace(right) +} + +// sessionRowName resolves the LIVE session's provider spelling to the persisted +// row it actually refers to. This answers a third question, distinct from the +// two identity rules config defines: not "which stored secret is this?" +// (config.SameProviderIdentity) and not "which row does this mutator target?" +// (exact trimmed equality), but "is this the provider I am running on?". +// +// An exact spelling always wins, so sibling rows that differ only by case +// ("work" and "WORK") stay distinct — a session on "work" must never follow an +// edit or delete aimed at "WORK", and "s"/"ſ" must not re-merge. Only when the +// credential identity is carried by exactly ONE row is the session's spelling +// resolved to that row's own, which is what lines a session launched with +// ZERO_PROVIDER=openai (or resumed session metadata, or a `zero providers use +// openai` run in another terminal) up with the sole saved "OpenAI" row. +// +// When nothing resolves — env-derived providers, ambiguous duplicate identities +// — the live spelling comes back unchanged, so every comparison built on this +// degrades to exact equality rather than guessing. +func sessionRowName(live string, providers []config.ProviderProfile) string { + live = strings.TrimSpace(live) + if live == "" { + return "" + } + match := "" + matches := 0 + for _, provider := range providers { + name := strings.TrimSpace(provider.Name) + if name == live { + return name + } + if config.SameProviderIdentity(name, live) { + match = name + matches++ + } + } + if matches == 1 { + return match + } + return live +} + +// sessionRefersToPersistedRow reports whether the live session runs on row. +// See sessionRowName for why this is neither blind SameProviderIdentity nor +// plain exact equality. +func sessionRefersToPersistedRow(live string, row string, providers []config.ProviderProfile) bool { + resolved := sessionRowName(live, providers) + return resolved != "" && resolved == strings.TrimSpace(row) +} + // providerManagerCleanupMsg reports the off-thread half of a delete: the // stored-key removal outcome and the OAuth-login hint. type providerManagerCleanupMsg struct { @@ -417,17 +539,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, deleteStoredKey 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 deleteStoredKey { + 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 ("+redaction.ErrorMessage(storeErr, redaction.Options{})+").") + } } if login, ok := oauthLoginName(config.ProviderProfile{Name: name, CatalogID: catalogID}); ok { notes = append(notes, "OAuth login kept — remove with `zero auth logout "+login+"`.") @@ -455,8 +579,9 @@ func (m model) applyProviderManagerCleanup(msg providerManagerCleanupMsg) (model // --- edit ------------------------------------------------------------------- -func (wizard *providerWizardState) beginProviderEdit(profile config.ProviderProfile) { +func (wizard *providerWizardState) beginProviderEdit(profile config.ProviderProfile, owner config.ProviderRowOwnership) { wizard.editOriginal = profile + wizard.editOwner = owner wizard.editDraft = profile wizard.editDraft.APIKey = "" // key field is enter-to-replace, never prefilled wizard.editCursor = 0 @@ -590,32 +715,41 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { return m, nil } oldName := strings.TrimSpace(wizard.editOriginal.Name) - persisted, err := config.ProviderPersisted(m.userConfigPath, oldName) - if err != nil { - wizard.err = err.Error() - return m, nil - } - if !persisted { - wizard.err = "provider " + oldName + " is not saved in config.json, so there is no saved profile to edit" + // The row's provenance, captured when the edit began — not a fresh lookup + // from oldName. That lookup answered "does config.json carry this credential + // identity?", which is true for a project row whose identity a DIFFERENT user + // row owns, and the draft (replacement key included) was then applied to that + // user row while the session updated the project one. + if !wizard.editOwner.UserBacked { + wizard.err = "cannot edit " + oldName + ": " + wizard.editOwner.Reason return m, nil } + // EditProvider matches rows exactly, and PersistedName is that exact + // spelling, so the credential capture and the write target the same row. + exactName := wizard.editOwner.PersistedName newName := strings.TrimSpace(wizard.editDraft.Name) if newName == "" { wizard.err = "name cannot be empty" return m, nil } edit := config.ProviderEdit{ - Name: oldName, + Name: exactName, NewName: newName, BaseURL: strings.TrimSpace(wizard.editDraft.BaseURL), Model: strings.TrimSpace(wizard.editDraft.Model), Description: wizard.editDraft.Description, } if key := strings.TrimSpace(wizard.editDraft.APIKey); key != "" { - captured := config.SecureProviderProfile(config.ProviderProfile{Name: oldName, APIKey: key}, m.userConfigPath) + if err := config.PreflightUserConfig(m.userConfigPath); err != nil { + wizard.err = err.Error() + return m, nil + } + captured := config.SecureProviderProfile(config.ProviderProfile{Name: exactName, APIKey: key}, m.userConfigPath) // On a store failure SecureProviderProfile keeps the inline key, which // EditProvider then persists (the startup migration re-captures later) — - // the same fail-soft posture as every other capture path. + // the same fail-soft posture as every other capture path. A failed + // EditProvider below does not roll the capture back either; atomic + // capture+publish for this path is #894, not this PR. edit.APIKey = captured.APIKey edit.APIKeyStored = captured.APIKeyStored } @@ -623,6 +757,11 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { wizard.err = err.Error() return m, nil } + // Decide whether the edited row is the live one BEFORE the list is rewritten: + // a rename changes which rows carry the session's credential identity, and + // sessionRowName's sole-row resolution depends on that count. + editedLiveRow := sessionRefersToPersistedRow(m.providerName, oldName, m.savedProviders) + // Mirror the edit into the in-memory list surgically. savedProviders was // seeded from the RESOLVED (project-config layered) and usability-FILTERED // provider set — substituting the raw user-file list here would drop @@ -631,7 +770,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 editedLiveRow { m.providerName = newName m.providerProfile.Name = newName config.SetActiveProviderEnv(newName) @@ -639,7 +778,7 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { wizard.step = providerWizardStepManage next, cmd := m.reloadProviderManagerRows() - next.providerWizard.manageStatus = "Updated " + newName + "." + providerEditRestartNote(next.providerName, newName) + next.providerWizard.manageStatus = "Updated " + newName + "." + providerEditRestartNote(next.providerName, newName, next.savedProviders) return next, cmd } @@ -647,19 +786,64 @@ func (m model) saveManagerEdit() (model, tea.Cmd) { // this session is running on — endpoint/model/key changes only apply to the // built client after a switch (Enter on the row re-activates and rebuilds). // 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)) { +// comparison against the edited profile's final name suffices — routed through +// sessionRefersToPersistedRow so a sole row the session spells differently +// (live "openai", row "OpenAI") still gets the note, while case-variant +// siblings do not. +func providerEditRestartNote(liveName string, editedName string, providers []config.ProviderProfile) string { + if sessionRefersToPersistedRow(liveName, editedName, providers) { return " Press Enter on it to apply the changes to this session." } return "" } +// syncSavedProviderModel mirrors a model that was just written to config.json +// into the in-memory saved list — the single reconciliation point every path +// that persists a model must call. +// +// The provider manager builds its rows from savedProviders (see +// reloadProviderManagerRows) and renders each row's model from that list, as do +// the picker's saved-provider model sections. A switch that updates the live +// client and config.json but not this list leaves those surfaces showing the +// previous model until the TUI restarts and re-resolves providers from config +// — the same "disk says X, session says Y" drift the wizard's key removal +// fixed with applyProviderKeyRemovalToSession. +// +// exactName must be the PERSISTED row's spelling — the one SetProviderModel was +// handed, not the session's — because savedProviders carries row spellings. +// +// This is a PARTIAL update and must not route through applySavedProviderEdit. +// config.ProviderEdit is a value struct with no field-presence semantics, so an +// omitted field is indistinguishable from an intentional clear: that mirror +// assigns Description unconditionally, and a model-only edit therefore wiped a +// nonempty description out of savedProviders while config.json kept it. The +// manager, the picker, and any copies sharing the slice then disagreed with disk +// until the next full resolution. +// +// The slice is copied rather than mutated in place for the same reason: other +// holders of the backing array — a picker snapshot taken before the switch — +// must not observe a model change through a slice they were handed earlier. +func syncSavedProviderModel(saved []config.ProviderProfile, exactName string, model string) []config.ProviderProfile { + if strings.TrimSpace(exactName) == "" || strings.TrimSpace(model) == "" { + return saved + } + for index := range saved { + if strings.TrimSpace(saved[index].Name) != strings.TrimSpace(exactName) { + continue + } + updated := make([]config.ProviderProfile, len(saved)) + copy(updated, saved) + updated[index].Model = model + return updated + } + return saved +} + // 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 { for index := range saved { - if !strings.EqualFold(strings.TrimSpace(saved[index].Name), strings.TrimSpace(oldName)) { + if strings.TrimSpace(saved[index].Name) != strings.TrimSpace(oldName) { continue } profile := &saved[index] @@ -691,7 +875,7 @@ func applySavedProviderEdit(saved []config.ProviderProfile, oldName string, edit // in-memory saved list (replace by name, else append). func upsertSavedProviderProfile(saved []config.ProviderProfile, profile config.ProviderProfile) []config.ProviderProfile { for index := range saved { - if strings.EqualFold(strings.TrimSpace(saved[index].Name), strings.TrimSpace(profile.Name)) { + if strings.TrimSpace(saved[index].Name) == strings.TrimSpace(profile.Name) { saved[index] = profile return saved } @@ -727,7 +911,7 @@ func (wizard *providerWizardState) renderManageStep(width int) []string { marker = surface(zeroTheme.accent).Render("❯ ") } active := "" - if strings.EqualFold(strings.TrimSpace(row.profile.Name), strings.TrimSpace(wizard.manageActiveName)) { + if strings.TrimSpace(row.profile.Name) == strings.TrimSpace(wizard.manageActiveName) { active = surface(zeroTheme.accent).Render(" ● active") } name := padProviderManagerCell(row.profile.Name, nameWidth) @@ -753,7 +937,11 @@ func (wizard *providerWizardState) renderManageStep(width int) []string { } lines = append(lines, fitStyledLine(zeroTheme.faint.Render(detail), width)) if wizard.manageDeleting { - lines = append(lines, fitStyledLine(zeroTheme.red.Render("Delete "+row.profile.Name+"? This also removes its stored API key. Enter/y confirm · Esc/n cancel"), width)) + prompt := "Delete " + row.profile.Name + "?" + if note := strings.TrimSpace(wizard.manageDeleteKeyNote); note != "" { + prompt += " " + note + } + lines = append(lines, fitStyledLine(zeroTheme.red.Render(prompt+" Enter/y confirm · Esc/n cancel"), width)) } } return lines diff --git a/internal/tui/provider_manager_test.go b/internal/tui/provider_manager_test.go index ddc2429fd..e52c2165f 100644 --- a/internal/tui/provider_manager_test.go +++ b/internal/tui/provider_manager_test.go @@ -645,3 +645,434 @@ func TestProviderManagerCredStateFallsThroughStaleMarker(t *testing.T) { t.Fatalf("expected stored key missing with no fallback, got %q", state) } } + +func TestProviderManagerRemoveKeepsSharedCredentialForCaseVariantSurvivor(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + profiles := []config.ProviderProfile{ + {Name: "work", APIKeyStored: true}, + {Name: "WORK", APIKeyStored: true}, + } + if err := os.WriteFile(configPath, []byte(`{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-shared"); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "work", + ProviderProfile: profiles[0], + SavedProviders: profiles, + UserConfigPath: configPath, + }) + m, _ = m.openProviderManager() + m.providerWizard.manageCursor = 1 + next, cmd := m.deleteManagerSelection() + next = drainProviderManagerCmds(t, next, cmd) + + cfg := readManagerConfig(t, configPath) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" || !cfg.Providers[0].APIKeyStored { + t.Fatalf("survivor = %+v, want credentialed work row", cfg.Providers) + } + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "work" { + t.Fatalf("in-memory survivor = %+v, want work", next.savedProviders) + } + if key, ok, getErr := store.Get("work"); getErr != nil || !ok || key != "sk-shared" { + t.Fatalf("shared key changed: present=%v err=%v", ok, getErr) + } + if next.providerName != "work" || next.providerProfile.Name != "work" { + t.Fatalf("removing WORK changed live work identity: name=%q profile=%q", next.providerName, next.providerProfile.Name) + } + if status := next.providerWizard.manageStatus; !strings.Contains(status, "Kept its stored API key") || !strings.Contains(status, "Active provider: work") { + t.Fatalf("delete status did not describe retained key and surviving active row: %q", status) + } +} + +func TestProviderManagerKeepsDistinctUnicodeLiveProviderOnOtherRowMutation(t *testing.T) { + newModelWithRows := func(t *testing.T) model { + t.Helper() + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + profiles := []config.ProviderProfile{ + {Name: "s", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://s.example/v1", Model: "s-model"}, + {Name: "ſ", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://long-s.example/v1", Model: "long-s-model"}, + } + path := filepath.Join(t.TempDir(), "config.json") + data, err := json.Marshal(config.FileConfig{ActiveProvider: "ſ", Providers: profiles}) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "ſ", + ProviderProfile: profiles[1], + SavedProviders: profiles, + UserConfigPath: path, + }) + m, _ = m.openProviderManager() + return m + } + + t.Run("edit s", func(t *testing.T) { + t.Setenv(config.ActiveProviderEnv, "ſ") + m := newModelWithRows(t) + m.providerWizard.beginProviderEdit(m.savedProviders[0], managerRowOwnership(t, m, m.savedProviders[0].Name)) + m.providerWizard.editDraft.Model = "s-updated" + next, _ := m.saveManagerEdit() + if next.providerName != "ſ" || next.providerProfile.Name != "ſ" { + t.Fatalf("editing s rewrote live long-s identity: name=%q profile=%q", next.providerName, next.providerProfile.Name) + } + if got := os.Getenv(config.ActiveProviderEnv); got != "ſ" { + t.Fatalf("%s = %q, want long-s unchanged", config.ActiveProviderEnv, got) + } + if next.savedProviders[0].Model != "s-updated" || next.savedProviders[1].Name != "ſ" { + t.Fatalf("wrong in-memory edit target: %+v", next.savedProviders) + } + }) + + t.Run("remove s", func(t *testing.T) { + m := newModelWithRows(t) + m.providerWizard.manageCursor = 0 + next, _ := m.deleteManagerSelection() + if next.providerName != "ſ" { + t.Fatalf("removing s changed live long-s provider to %q", next.providerName) + } + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "ſ" { + t.Fatalf("wrong in-memory removal target: %+v", next.savedProviders) + } + }) +} + +// A session can spell its provider differently from the row it runs on — +// ZERO_PROVIDER=work against a saved "WORK", resumed session metadata, or a +// `zero providers use work` from another terminal. When that row is the SOLE +// carrier of the credential identity there is no other row the session could +// mean, so the manager must mark it active and carry a rename onto the live +// session; otherwise ZERO_PROVIDER keeps exporting a name no row answers to. +func TestProviderManagerSoleRowCaseVariantTracksLiveSession(t *testing.T) { + t.Setenv(config.ActiveProviderEnv, "work") + profile := config.ProviderProfile{ + Name: "WORK", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://other.example/v1", + Model: "other-model", + } + path := filepath.Join(t.TempDir(), "config.json") + data, err := json.Marshal(config.FileConfig{ActiveProvider: "WORK", Providers: []config.ProviderProfile{profile}}) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "work", + ProviderProfile: config.ProviderProfile{Name: "work"}, + SavedProviders: []config.ProviderProfile{profile}, + UserConfigPath: path, + }) + m, _ = m.openProviderManager() + + // The row the session actually runs on must render as active even though + // the session spells it differently. + if got := m.providerWizard.manageActiveName; got != "WORK" { + t.Fatalf("manageActiveName = %q, want the sole row's spelling WORK", got) + } + + m.providerWizard.beginProviderEdit(profile, managerRowOwnership(t, m, profile.Name)) + m.providerWizard.editDraft.Name = "OFFICE" + next, _ := m.saveManagerEdit() + + if next.providerWizard == nil || next.providerWizard.err != "" { + t.Fatalf("sole-row case-variant edit failed: %+v", next.providerWizard) + } + if next.providerName != "OFFICE" || next.providerProfile.Name != "OFFICE" { + t.Fatalf("rename did not follow the live session: name=%q profile=%q", next.providerName, next.providerProfile.Name) + } + if got := os.Getenv(config.ActiveProviderEnv); got != "OFFICE" { + t.Fatalf("%s = %q, want the renamed row so spawned children resolve it", config.ActiveProviderEnv, got) + } + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "OFFICE" { + t.Fatalf("wrong in-memory edit target: %+v", next.savedProviders) + } + cfg := readManagerConfig(t, path) + if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "OFFICE" { + t.Fatalf("wrong persisted edit target: %+v", cfg.Providers) + } +} + +// The sole-row resolution above must NOT reach case-variant siblings: with both +// "work" and "WORK" persisted, a session on "work" is one specific row, and a +// delete aimed at the other must leave it alone. (Edit cannot be exercised here +// — EditProvider validates the duplicate-identity config before mutating.) +func TestProviderManagerCaseVariantDeleteDoesNotChangeLiveSibling(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + t.Setenv(config.ActiveProviderEnv, "work") + profiles := []config.ProviderProfile{ + {Name: "work", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://work.example/v1", Model: "work-model"}, + {Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://other.example/v1", Model: "other-model"}, + } + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"activeProvider":"work","providers":[{"name":"work"},{"name":"WORK"}]}`), 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "work", + ProviderProfile: profiles[0], + SavedProviders: profiles, + UserConfigPath: path, + }) + m, _ = m.openProviderManager() + // Exact spelling wins, so the live row is "work" and not its sibling. + if got := m.providerWizard.manageActiveName; got != "work" { + t.Fatalf("manageActiveName = %q, want the exact live row work", got) + } + + m.providerWizard.manageCursor = 1 + next, _ := m.deleteManagerSelection() + + if next.providerName != "work" || next.providerProfile.Name != "work" { + t.Fatalf("deleting WORK rewrote live work identity: name=%q profile=%q", next.providerName, next.providerProfile.Name) + } + if got := os.Getenv(config.ActiveProviderEnv); got != "work" { + t.Fatalf("%s = %q, want live work unchanged", config.ActiveProviderEnv, got) + } + if status := next.providerWizard.manageStatus; strings.Contains(status, "keeps running on it until you switch") { + t.Fatalf("delete of the sibling row claimed the live session runs on it: %q", status) + } + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "work" { + t.Fatalf("wrong in-memory removal target: %+v", next.savedProviders) + } +} + +func TestProviderManagerAmbiguousCaseVariantSessionDoesNotGuessLiveRow(t *testing.T) { + providers := []config.ProviderProfile{{Name: "work"}, {Name: "WORK"}} + if got := sessionRowName("Work", providers); got != "Work" { + t.Fatalf("sessionRowName = %q, want unresolved live spelling Work", got) + } + for _, row := range providers { + if sessionRefersToPersistedRow("Work", row.Name, providers) { + t.Fatalf("ambiguous live spelling must not select row %q", row.Name) + } + } +} + +func TestProviderManagerCleanupRedactsCredentialStoreError(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "file") + secret := "sk-proj-12345678901234567890" + dir := filepath.Join(t.TempDir(), secret) + if err := os.MkdirAll(filepath.Join(dir, "credentials.json.lock"), 0o700); err != nil { + t.Fatal(err) + } + msg, ok := providerManagerCleanupCmd(filepath.Join(dir, "config.json"), config.ProviderProfile{Name: "work"}, true)().(providerManagerCleanupMsg) + if !ok { + t.Fatal("cleanup command returned the wrong message type") + } + text := strings.Join(msg.notes, " ") + if strings.Contains(text, secret) { + t.Fatalf("cleanup warning leaked credential-like text: %q", text) + } + if !strings.Contains(text, "could not be deleted") { + t.Fatalf("cleanup warning missing failure context: %q", text) + } +} + +// The confirmation prompt must promise what the delete actually does: with a +// case variant that still claims the shared credential, the key is kept, so +// the prompt must not say it is about to be removed. +func TestProviderManagerDeleteConfirmMatchesKeyRetentionPolicy(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + + newManagerAtRow := func(t *testing.T, configJSON string, profiles []config.ProviderProfile, cursor int) model { + t.Helper() + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(configJSON), 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: profiles[0].Name, + ProviderProfile: profiles[0], + SavedProviders: profiles, + UserConfigPath: configPath, + }) + m, _ = m.openProviderManager() + m.providerWizard.manageCursor = cursor + next, _ := m.handleProviderWizardKey(testKeyText("d")) + if !next.providerWizard.manageDeleting { + t.Fatal("d must arm the delete confirm") + } + return next + } + + t.Run("shared credential is kept", func(t *testing.T) { + m := newManagerAtRow(t, + `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK","apiKeyStored":true}]}`, + []config.ProviderProfile{{Name: "work", APIKeyStored: true}, {Name: "WORK", APIKeyStored: true}}, + 1, + ) + if m.providerWizard.manageDeleteKeyNote == "" { + t.Fatal("retention not resolved for a survivor that claims the credential") + } + view := strings.Join(m.providerWizard.renderManageStep(80), "\n") + if !strings.Contains(view, "stored API key is kept") { + t.Fatalf("confirm text = %q, want the key-kept wording", view) + } + }) + + t.Run("last owner removal deletes the key", func(t *testing.T) { + m := newManagerAtRow(t, + `{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"other"}]}`, + []config.ProviderProfile{{Name: "work", APIKeyStored: true}, {Name: "other"}}, + 0, + ) + if m.providerWizard.manageDeleteKeyNote == "" { + t.Fatal("delete confirmation made no claim about a persisted row's key") + } + view := strings.Join(m.providerWizard.renderManageStep(80), "\n") + if !strings.Contains(view, "also removes its stored API key") { + t.Fatalf("confirm text = %q, want the key-removal wording", view) + } + }) +} + +// A markerless case variant does not own the shared credential, so removing +// the only row that claimed it must delete the secret rather than orphan it +// behind a profile ApplyStoredAPIKey will never read. +func TestProviderManagerRemoveDeletesKeyWhenSurvivorNeverClaimedIt(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + profiles := []config.ProviderProfile{ + {Name: "work", APIKeyStored: true}, + {Name: "WORK"}, + } + if err := os.WriteFile(configPath, []byte(`{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true},{"name":"WORK"}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-shared"); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{ + ProviderName: "work", + ProviderProfile: profiles[0], + SavedProviders: profiles, + UserConfigPath: configPath, + }) + m, _ = m.openProviderManager() + m.providerWizard.manageCursor = 0 + next, cmd := m.deleteManagerSelection() + next = drainProviderManagerCmds(t, next, cmd) + + if _, ok, getErr := store.Get("WORK"); getErr != nil { + t.Fatal(getErr) + } else if ok { + t.Fatal("shared key was orphaned behind a markerless survivor") + } + if status := next.providerWizard.manageStatus; !strings.Contains(status, "stored API key will also be deleted") { + t.Fatalf("delete status = %q, want the key-deletion note", status) + } +} + +// A row visible only because Resolve() synthesized it from an env var has no +// persisted profile and no stored key, so the confirmation must make no claim +// about a key rather than promising a removal that cannot happen. The same +// holds when the config is too ambiguous for the delete to proceed at all. +func TestProviderDeleteKeyNoteMakesNoClaimWithoutAResolvableRow(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + + cases := []struct { + name string + configJSON string + row string + // resolved is every row spelling the manager is displaying. + resolved []string + }{ + { + name: "env-derived row with no persisted profile", + configJSON: `{"providers":[{"name":"other"}]}`, + row: "openai", + resolved: []string{"other", "openai"}, + }, + { + name: "ambiguous duplicate rows the delete cannot resolve", + configJSON: `{"providers":[{"name":"work","apiKeyStored":true},{"name":"WORK","apiKeyStored":true}]}`, + row: "Work", + resolved: []string{"work", "WORK", "Work"}, + }, + { + // The project row's identity belongs to the user row listed beside + // it, so this row owns nothing on disk and promises nothing. + name: "project row whose identity a listed user row owns", + configJSON: `{"providers":[{"name":"work","apiKeyStored":true}]}`, + row: "WORK", + resolved: []string{"work", "WORK"}, + }, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(testCase.configJSON), 0o600); err != nil { + t.Fatal(err) + } + owner, err := config.ProviderRowOwnershipAt(path, testCase.resolved, testCase.row) + if err != nil { + t.Fatal(err) + } + if note := providerDeleteKeyNote(path, owner); note != "" { + t.Fatalf("note = %q, want no claim about the stored key", note) + } + }) + } + // No user config path at all: nothing can be promised either. + if note := providerDeleteKeyNote("", config.ProviderRowOwnership{UserBacked: true, PersistedName: "work"}); note != "" { + t.Fatalf("note = %q, want no claim without a config path", note) + } +} + +// The preview must resolve the row the same way the delete does: a case-variant +// spelling that removes nothing would preview "key kept" for a delete that +// resolves the row and takes the key with it. +func TestProviderDeleteKeyNoteResolvesCaseVariantSpelling(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"WORK","apiKeyStored":true},{"name":"other"}]}`), 0o600); err != nil { + t.Fatal(err) + } + // "work" addresses the sole WORK row, whose removal takes the key with it. + // No other displayed row carries "WORK", so the bridge is safe here — that + // sibling check is the whole difference from the project-row case above. + owner, err := config.ProviderRowOwnershipAt(path, []string{"work", "other"}, "work") + if err != nil { + t.Fatal(err) + } + if !owner.UserBacked || owner.PersistedName != "WORK" { + t.Fatalf("ownership = %+v, want the sole WORK row", owner) + } + note := providerDeleteKeyNote(path, owner) + if !strings.Contains(note, "also removes its stored API key") { + t.Fatalf("note = %q, want the key-removal wording for the resolved row", note) + } +} + +// managerRowOwnership resolves a row's provenance exactly as +// reloadProviderManagerRows does, so a test that drives beginProviderEdit +// directly cannot hand the edit a stronger ownership than the manager would. +func managerRowOwnership(t *testing.T, m model, name string) config.ProviderRowOwnership { + t.Helper() + owner, err := config.ProviderRowOwnershipAt(m.userConfigPath, config.ProviderProfileNames(m.savedProviders), name) + if err != nil { + t.Fatalf("resolve ownership for %q: %v", name, err) + } + return owner +} diff --git a/internal/tui/provider_ownership_test.go b/internal/tui/provider_ownership_test.go new file mode 100644 index 000000000..885a56348 --- /dev/null +++ b/internal/tui/provider_ownership_test.go @@ -0,0 +1,343 @@ +package tui + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/credstore" + "github.com/Gitlawb/zero/internal/zeroruntime" +) + +// caseSiblingModel builds the shape the resolver validly produces and the +// identity comparisons could not tell apart: user config holds "work", and the +// session ALSO resolved a project-config "WORK" with its own endpoint and model. +// +// activeName puts either row in the active seat, because the defect behaved +// differently depending on which one the session ran on. +// +// builtProfiles records every profile handed to newProvider, so a test can +// assert which endpoint a selection actually built — the outcome neither a +// status line nor a config row can show. +func caseSiblingModel(t *testing.T, activeName string, builtProfiles *[]config.ProviderProfile) model { + t.Helper() + 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") + userRow := config.ProviderProfile{ + Name: "work", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://user.example.com/v1", + Model: "user-model", + Description: "User row", + APIKeyStored: true, + } + seed := config.FileConfig{ActiveProvider: activeName, Providers: []config.ProviderProfile{userRow}} + data, err := json.MarshalIndent(seed, "", " ") + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(configPath, data, 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-user"); err != nil { + t.Fatal(err) + } + + // The project row exists only in the resolved/session list, exactly as + // cross-layer merging produces it: same credential identity, different + // endpoint and model, and NO row of its own in config.json. + projectRow := config.ProviderProfile{ + Name: "WORK", + ProviderKind: config.ProviderKindOpenAICompatible, + BaseURL: "https://project.example.com/v1", + Model: "project-model", + Description: "Project row", + APIKey: "sk-project", + } + resolved := []config.ProviderProfile{userRow, projectRow} + active := userRow + if activeName == "WORK" { + active = projectRow + } + m := newModel(context.Background(), Options{ + ProviderName: activeName, + ModelName: active.Model, + Provider: &fakeProvider{}, + ProviderProfile: active, + SavedProviders: resolved, + UserConfigPath: configPath, + NewProvider: func(profile config.ProviderProfile) (zeroruntime.Provider, error) { + if builtProfiles != nil { + *builtProfiles = append(*builtProfiles, profile) + } + return &fakeProvider{}, nil + }, + }) + m.width = 120 + m.height = 40 + next, _ := m.openProviderManager() + return next +} + +// selectManagerRow moves the manager cursor onto the named row. +func selectManagerRow(t *testing.T, m model, name string) model { + t.Helper() + for index, row := range m.providerWizard.manageRows { + if strings.TrimSpace(row.profile.Name) == name { + m.providerWizard.manageCursor = index + return m + } + } + t.Fatalf("manager has no row %q", name) + return m +} + +// assertUserRowUntouched pins both durable surfaces at once: the config bytes +// and the credential store. +func assertUserRowUntouched(t *testing.T, m model, before []byte) { + t.Helper() + after, err := os.ReadFile(m.userConfigPath) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatalf("user config changed:\nbefore=%s\nafter=%s", before, after) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + key, ok, err := store.Get("work") + if err != nil { + t.Fatal(err) + } + if !ok || key != "sk-user" { + t.Fatalf("user credential changed: present=%t", ok) + } +} + +// Deleting the project row must remove it from the SESSION and leave the user's +// row and its credential exactly as they were. The delete used to resolve the +// project row's spelling onto the user row and remove that instead, so the row +// gone from disk was not the row gone from the list. +func TestProviderManagerDeleteProjectRowLeavesUserRowIntact(t *testing.T) { + for _, activeName := range []string{"work", "WORK"} { + t.Run("active_"+activeName, func(t *testing.T) { + m := caseSiblingModel(t, activeName, nil) + before, err := os.ReadFile(m.userConfigPath) + if err != nil { + t.Fatal(err) + } + m = selectManagerRow(t, m, "WORK") + m = managerKey(t, m, testKeyText("d")) + // The confirmation must promise nothing about a key it cannot delete. + if note := m.providerWizard.manageDeleteKeyNote; note != "" { + t.Fatalf("delete note = %q, want no claim for a row with no config row", note) + } + next, cmd := m.handleProviderWizardKey(testKeyText("y")) + next = drainProviderManagerCmds(t, next, cmd) + + assertUserRowUntouched(t, next, before) + if len(next.savedProviders) != 1 || next.savedProviders[0].Name != "work" { + t.Fatalf("savedProviders = %+v, want only the user row left in session", next.savedProviders) + } + if next.providerWizard == nil { + t.Fatal("manager closed while a provider remains") + } + if !strings.Contains(next.providerWizard.manageStatus, "nothing in config.json changed") { + t.Fatalf("status = %q, want it to say the config was untouched", next.providerWizard.manageStatus) + } + }) + } +} + +// Editing the project row must not apply this row's draft — a replacement key +// included — to the user row that merely shares its credential identity. +func TestProviderManagerEditProjectRowIsRefused(t *testing.T) { + for _, activeName := range []string{"work", "WORK"} { + t.Run("active_"+activeName, func(t *testing.T) { + m := caseSiblingModel(t, activeName, nil) + before, err := os.ReadFile(m.userConfigPath) + if err != nil { + t.Fatal(err) + } + m = selectManagerRow(t, m, "WORK") + m = managerKey(t, m, testKeyText("e")) + + if m.providerWizard.step == providerWizardStepEditMenu { + t.Fatal("edit opened for a row with no config.json row of its own") + } + if !strings.Contains(m.providerWizard.manageStatus, "Can't edit WORK") { + t.Fatalf("status = %q, want a refusal naming the row", m.providerWizard.manageStatus) + } + assertUserRowUntouched(t, m, before) + + // The user row beside it stays editable — the refusal is about + // provenance, not about the name colliding. + m = selectManagerRow(t, m, "work") + m = managerKey(t, m, testKeyText("e")) + if m.providerWizard.step != providerWizardStepEditMenu { + t.Fatalf("user row must stay editable, step = %v status = %q", m.providerWizard.step, m.providerWizard.manageStatus) + } + }) + } +} + +// assertUserProviderRowUnchanged is assertUserRowUntouched for the paths that +// legitimately write elsewhere in config.json — model selection records recent +// models under preferences — so the assertion is on the provider row and the +// active pointer rather than the whole file. +func assertUserProviderRowUnchanged(t *testing.T, m model, want config.ProviderProfile, wantActive string) { + t.Helper() + cfg := readManagerConfig(t, m.userConfigPath) + if cfg.ActiveProvider != wantActive { + t.Fatalf("activeProvider = %q, want %q", cfg.ActiveProvider, wantActive) + } + found := false + for _, provider := range cfg.Providers { + if provider.Name != want.Name { + continue + } + found = true + if provider.Model != want.Model || provider.BaseURL != want.BaseURL || + provider.Description != want.Description || provider.APIKeyStored != want.APIKeyStored { + t.Fatalf("user row changed:\n got %+v\nwant %+v", provider, want) + } + } + if !found { + t.Fatalf("user row %q is gone from %+v", want.Name, cfg.Providers) + } + store, err := config.ProviderKeyStore() + if err != nil { + t.Fatal(err) + } + key, ok, err := store.Get("work") + if err != nil { + t.Fatal(err) + } + if !ok || key != "sk-user" { + t.Fatalf("user credential changed: present=%t", ok) + } +} + +// Choosing a model listed under the project row must build THAT endpoint and +// must not write the model onto the user row. The picker used to treat the two +// as one provider through credential normalization. +func TestModelPickerSelectionStaysOnTheOwningRow(t *testing.T) { + for _, activeName := range []string{"work", "WORK"} { + t.Run("active_"+activeName, func(t *testing.T) { + var built []config.ProviderProfile + m := caseSiblingModel(t, activeName, &built) + m.providerWizard = nil + userRow := config.ProviderProfile{ + Name: "work", + BaseURL: "https://user.example.com/v1", + Model: "user-model", + Description: "User row", + APIKeyStored: true, + } + + m.picker = &commandPicker{ + kind: pickerModel, + items: []pickerItem{{Label: "project-next", Value: "project-next", OwnerProvider: "WORK"}}, + } + updated, _ := m.choosePicker() + next, ok := updated.(model) + if !ok { + t.Fatalf("choosePicker returned %T", updated) + } + + // The user row's model must not move, and its key must not be touched. + assertUserProviderRowUnchanged(t, next, userRow, activeName) + for _, profile := range built { + if strings.TrimSpace(profile.Name) == "work" { + t.Fatalf("selection under WORK built the user row's endpoint: %+v", profile) + } + } + if activeName == "work" { + // A real switch: the project endpoint must be what got built. + if len(built) == 0 { + t.Fatal("no provider was built for a cross-row selection") + } + last := built[len(built)-1] + if last.BaseURL != "https://project.example.com/v1" { + t.Fatalf("built endpoint = %q, want the project row's", last.BaseURL) + } + } + // Whatever happened, the session must not claim to run on the user row + // under the project row's model. + if next.providerName == "work" && next.modelName == "project-next" { + t.Fatalf("project row's model landed on the user row: provider=%q model=%q", next.providerName, next.modelName) + } + }) + } +} + +// Both rows must not read as active at once: they are different endpoints, and +// the picker is where the user decides between them. +func TestActiveProviderRowNameIsTheExactRow(t *testing.T) { + for _, activeName := range []string{"work", "WORK"} { + t.Run("active_"+activeName, func(t *testing.T) { + m := caseSiblingModel(t, activeName, nil) + other := "WORK" + if activeName == "WORK" { + other = "work" + } + if credstore.NormalizeProvider(activeName) != credstore.NormalizeProvider(other) { + t.Fatalf("fixture no longer exercises a shared credential identity: %q vs %q", activeName, other) + } + if got := m.activeProviderRowName(); got != activeName { + t.Fatalf("activeProviderRowName = %q, want the exact active row %q", got, activeName) + } + }) + } +} + +// syncSavedProviderModel is a PARTIAL update: persisting a model must not clear +// the description config.json keeps, and must not reach other holders of the +// slice through the shared backing array. +func TestSyncSavedProviderModelPreservesTheRestOfTheProfile(t *testing.T) { + saved := []config.ProviderProfile{ + {Name: "work", Model: "old", Description: "User row", BaseURL: "https://user.example.com/v1", APIKeyStored: true}, + {Name: "other", Model: "other-model", Description: "Other"}, + } + snapshot := append([]config.ProviderProfile{}, saved...) + + updated := syncSavedProviderModel(saved, "work", "new") + + if updated[0].Model != "new" { + t.Fatalf("model not updated: %+v", updated[0]) + } + for _, field := range []struct{ name, got, want string }{ + {"Description", updated[0].Description, "User row"}, + {"BaseURL", updated[0].BaseURL, "https://user.example.com/v1"}, + } { + if field.got != field.want { + t.Fatalf("%s = %q, want %q", field.name, field.got, field.want) + } + } + if !updated[0].APIKeyStored { + t.Fatal("stored-key marker cleared by a model-only sync") + } + if updated[1].Name != snapshot[1].Name || updated[1].Model != snapshot[1].Model || + updated[1].Description != snapshot[1].Description { + t.Fatalf("unrelated row changed: %+v", updated[1]) + } + // The slice handed in must be unchanged: a picker snapshot taken before the + // switch must not observe the new model through the same backing array. + if saved[0].Model != "old" { + t.Fatalf("input slice mutated in place: %+v", saved[0]) + } +} diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index 8f4ee8bb3..a19f51230 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(m.userConfigPath, msg.providerID, msg.attemptID, msg.cfg, msg.auth) } // providerWizardSupportsOAuth reports whether the credential step should offer a @@ -155,11 +155,14 @@ 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 switch { case provider.OAuthMintsKey: return func() tea.Msg { + if err := preflightOAuthLogin(configPath); 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 +171,12 @@ func providerWizardOAuthCmdFor(provider providercatalog.Descriptor, attemptID in } case providerID == "chatgpt": return func() tea.Msg { - err := runProviderChatGPTLogin() + err := runProviderChatGPTLogin(configPath) 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(configPath, providerID)} } } } @@ -183,7 +186,10 @@ 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 { + if err := preflightOAuthLogin(configPath); err != nil { + return err + } env := buildOAuthPresetEnv() token, err := provideroauth.ChatGPTLogin(context.Background(), provideroauth.ChatGPTOptions{ Env: env, @@ -194,6 +200,9 @@ func runProviderChatGPTLogin() error { if err != nil { return err } + if err := preflightOAuthLogin(configPath); err != nil { + return err + } store, err := oauth.NewStore(oauth.StoreOptions{}) if err != nil { return err @@ -210,8 +219,10 @@ func appendOAuthLoginProfile(saved []config.ProviderProfile, providerID string) return saved } for _, profile := range saved { - if strings.EqualFold(strings.TrimSpace(profile.CatalogID), descriptor.ID) || - strings.EqualFold(strings.TrimSpace(profile.Name), descriptor.ID) { + // "Does this profile already serve the catalog entry?" is a provider + // identity question — same rule as everywhere else, not EqualFold. + if config.SameProviderIdentity(profile.CatalogID, descriptor.ID) || + config.SameProviderIdentity(profile.Name, descriptor.ID) { return saved } } @@ -260,7 +271,10 @@ 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(configPath string, name string) error { + if err := preflightOAuthLogin(configPath); err != nil { + return err + } store, err := oauth.NewStore(oauth.StoreOptions{}) if err != nil { return err @@ -273,6 +287,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 preflightOAuthLogin(configPath) }, }) if err != nil { return err @@ -297,8 +312,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(configPath string, name string, attemptID int) tea.Cmd { return func() tea.Msg { + if err := preflightOAuthLogin(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 +334,9 @@ 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(configPath string, name string, attemptID int, cfg oauth.Config, auth oauth.DeviceAuth) tea.Cmd { return func() tea.Msg { - return providerWizardOAuthMsg{providerID: name, attemptID: attemptID, tokenLogin: true, err: oauthDeviceComplete(name, cfg, auth)} + return providerWizardOAuthMsg{providerID: name, attemptID: attemptID, tokenLogin: true, err: oauthDeviceComplete(configPath, name, cfg, auth)} } } @@ -330,7 +348,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(m.userConfigPath, provider.ID, attemptID) } const maxProviderWizardProvidersVisible = 10 @@ -443,19 +461,28 @@ type providerWizardState struct { // the wizard is on providerWizardStepAimlapi. aimlapi *aimlapiOnboardState // Manager state (provider_manager.go): the list-first /provider surface. - manage bool - manageRows []providerManagerRow - manageCursor int - manageDeleting bool - manageStatus string - manageCredGen int - manageActiveName string + manage bool + manageRows []providerManagerRow + manageCursor int + manageDeleting bool + // manageDeleteKeyNote is resolved when the delete confirmation opens, from + // config.ProviderKeyRetainedAfterRemoval, so the prompt and the delete agree + // about what happens to the stored key. "" means make no claim. + manageDeleteKeyNote string + manageStatus string + manageCredGen int + manageActiveName string // Edit state: field-level editor for one saved profile. editOriginal config.ProviderProfile editDraft config.ProviderProfile - editCursor int - editField providerEditField - editBuffer string + // editOwner is the provenance of the row being edited, captured when the + // edit began. saveManagerEdit consumes it instead of re-deriving ownership + // from editOriginal.Name, which is a resolved spelling that says nothing + // about which layer produced the row. + editOwner config.ProviderRowOwnership + editCursor int + editField providerEditField + editBuffer string } func (m model) newProviderWizard() *providerWizardState { @@ -968,7 +995,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,9 +1288,15 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { nextProvider = built } if strings.TrimSpace(m.userConfigPath) != "" { + if err := config.PreflightProviderWrite(m.userConfigPath, profile.Name); err != nil { + wizard.err = redaction.RedactString(err.Error(), redaction.Options{ExtraSecretValues: []string{profile.APIKey, runtimeProfile.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. + // Fail-soft capture with no rollback if the config write below fails — + // see the note in cli/provider_setup.go; atomicity is #894. secret := profile.APIKey if !preserveExistingCredentialReference { profile = config.SecureProviderProfile(profile, m.userConfigPath) @@ -1298,20 +1331,46 @@ func (m model) applyProviderWizard() (model, tea.Cmd) { // wizardProviderStoredKey reports the saved provider name that has a key in the // credential store matching the wizard-selected descriptor, so the wizard can offer // keep/replace/remove instead of forcing a new key entry. +// +// The name comparisons ask a credential question — "does this profile's store +// entry serve the descriptor?" — so they use the store's own normalization +// rather than strings.EqualFold, which folds "s" and Unicode long-s "ſ" into +// one identity the store keeps apart. func (m model) wizardProviderStoredKey(provider providercatalog.Descriptor) (string, bool) { for _, profile := range m.savedProviders { if !profile.APIKeyStored { continue } - if strings.EqualFold(strings.TrimSpace(profile.Name), strings.TrimSpace(provider.Name)) || - strings.EqualFold(strings.TrimSpace(profile.CatalogID), strings.TrimSpace(provider.ID)) || - strings.EqualFold(strings.TrimSpace(profile.Name), strings.TrimSpace(provider.ID)) { + if config.SameProviderIdentity(profile.Name, provider.Name) || + config.SameProviderIdentity(profile.CatalogID, provider.ID) || + config.SameProviderIdentity(profile.Name, provider.ID) { return profile.Name, true } } return "", false } +// applyProviderKeyRemovalToSession mirrors a stored-key removal into the live +// session: every in-memory profile that shares the removed credential identity +// drops its APIKeyStored marker, matching what +// ClearProviderKeyStoredCaseVariants just wrote to disk. +func (m model) applyProviderKeyRemovalToSession(name string) model { + // Copy before mutating: model is passed by value, but the slice header is + // shared, so an in-place write would reach every other copy of the model. + updated := make([]config.ProviderProfile, len(m.savedProviders)) + copy(updated, m.savedProviders) + for index := range updated { + if config.SameProviderIdentity(updated[index].Name, name) { + updated[index].APIKeyStored = false + } + } + m.savedProviders = updated + if config.SameProviderIdentity(m.providerProfile.Name, name) { + m.providerProfile.APIKeyStored = false + } + return m +} + // applyManageKeyChoice acts on the keep/replace/remove selection. Keep closes the // wizard (nothing changes); Replace routes to credential entry (overwrites on save); // Remove deletes the stored key and its marker. @@ -1329,13 +1388,31 @@ func (m model) applyManageKeyChoice() (model, tea.Cmd) { return m, nil case 2: // Remove if strings.TrimSpace(m.userConfigPath) != "" { - if store, err := config.ProviderKeyStoreAt(filepath.Dir(m.userConfigPath)); err == nil { - _, _ = store.Delete(name) + if err := config.PreflightUserConfig(m.userConfigPath); err != nil { + wizard.err = redaction.RedactString(err.Error(), redaction.Options{}) + return m, nil } - _, _ = config.ClearProviderKeyStored(m.userConfigPath, name) - } else { - _, _ = config.ForgetProviderKey(name) } + // Marker first, secret second. The reverse order (which logout already + // fixed) leaves apiKeyStored:true with no secret behind it if the marker + // write fails — a profile that claims a credential every lookup misses. + // Clearing first can at worst orphan a secret no profile reads. + if strings.TrimSpace(m.userConfigPath) != "" { + if _, err := m.clearProviderKeyStored(m.userConfigPath, name); err != nil { + wizard.err = "Stored key marker cleanup failed: " + redaction.ErrorMessage(err, redaction.Options{}) + return m, nil + } + } + if _, err := m.deleteProviderKey(m.userConfigPath, name); err != nil { + wizard.err = "Stored key removal failed: " + redaction.ErrorMessage(err, redaction.Options{}) + + " — the saved-key marker was already cleared, so no profile claims it, but the secret may still be in the credential store." + return m, nil + } + // Reconcile the live session with the disk write: savedProviders and + // providerProfile still carry APIKeyStored:true otherwise, so /providers + // and a re-entered wizard would offer keep/replace for a key that is gone + // until the next restart. + m = m.applyProviderKeyRemovalToSession(name) m.providerWizard = nil m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: "Provider\nRemoved the stored key for " + name + ". Re-add it any time with /provider."}) return m, nil @@ -1346,6 +1423,17 @@ func (m model) applyManageKeyChoice() (model, tea.Cmd) { } } +func deleteProviderKey(configPath, provider string) (bool, error) { + if strings.TrimSpace(configPath) == "" { + return config.ForgetProviderKey(provider) + } + store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) + if err != nil { + return false, err + } + return store.Delete(provider) +} + func providerWizardRuntimeProfile(profile config.ProviderProfile) config.ProviderProfile { runtimeProfile := profile if strings.TrimSpace(runtimeProfile.APIKey) == "" && strings.TrimSpace(runtimeProfile.APIKeyEnv) != "" { diff --git a/internal/tui/provider_wizard_discovery.go b/internal/tui/provider_wizard_discovery.go index b0dcada69..41d89e1da 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. @@ -99,7 +99,8 @@ func (m model) existingAimlapiConfiguration() (config.ProviderProfile, string, b activeName := strings.TrimSpace(m.providerProfile.Name) if activeName != "" { for index, profile := range profiles { - if strings.EqualFold(strings.TrimSpace(profile.Name), activeName) && aimlapiProfile(profile) { + // Selecting the live session's own row: exact persisted spelling. + if strings.TrimSpace(profile.Name) == activeName && aimlapiProfile(profile) { profiles[0], profiles[index] = profiles[index], profiles[0] break } diff --git a/internal/tui/provider_wizard_oauth_test.go b/internal/tui/provider_wizard_oauth_test.go index bd4ac4d14..ffae32c20 100644 --- a/internal/tui/provider_wizard_oauth_test.go +++ b/internal/tui/provider_wizard_oauth_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/oauth" "github.com/Gitlawb/zero/internal/providercatalog" ) @@ -420,6 +421,51 @@ func TestPersistOAuthLoginProviderWritesKeylessProfileWithoutStealingActive(t *t persistOAuthLoginProvider("", "chatgpt") } +func TestOAuthCommandsRejectInvalidConfigBeforeCredentialSideEffects(t *testing.T) { + t.Setenv("ZERO_OAUTH_STORAGE", "file") + t.Setenv("ZERO_OAUTH_TOKENS_PATH", filepath.Join(t.TempDir(), "oauth.json")) + configPath := filepath.Join(t.TempDir(), "config.json") + seed := []byte(`{"providers":[{"name":"xai"},{"name":"XAI"}]}`) + if err := os.WriteFile(configPath, seed, 0o600); err != nil { + t.Fatal(err) + } + store, err := oauth.NewStore(oauth.StoreOptions{}) + if err != nil { + t.Fatal(err) + } + previous := oauth.Token{AccessToken: "previous-access", RefreshToken: "previous-refresh"} + if err := store.Save(oauth.ProviderKey("xai"), previous); err != nil { + t.Fatal(err) + } + + for _, providerID := range []string{"openrouter", "chatgpt", "xai"} { + descriptor, ok := providercatalog.Get(providerID) + if !ok { + t.Fatalf("missing catalog provider %q", providerID) + } + msg, ok := providerWizardOAuthCmdFor(descriptor, 7, configPath)().(providerWizardOAuthMsg) + if !ok || msg.err == nil || !strings.Contains(msg.err.Error(), "ambiguous persisted provider names") { + t.Fatalf("wizard OAuth %s did not preflight: %#v", providerID, msg) + } + setupMsg, ok := setupOAuthCmd(descriptor, configPath)().(setupOAuthMsg) + if !ok || setupMsg.err == nil || !strings.Contains(setupMsg.err.Error(), "ambiguous persisted provider names") { + t.Fatalf("setup OAuth %s did not preflight: %#v", providerID, setupMsg) + } + } + deviceMsg, ok := providerWizardDevicePollCmd(configPath, "xai", 8, oauth.Config{}, oauth.DeviceAuth{})().(providerWizardOAuthMsg) + if !ok || deviceMsg.err == nil || !strings.Contains(deviceMsg.err.Error(), "ambiguous persisted provider names") { + t.Fatalf("device completion did not revalidate config: %#v", deviceMsg) + } + stored, ok, err := store.Load(oauth.ProviderKey("xai")) + if err != nil || !ok || stored.AccessToken != previous.AccessToken || stored.RefreshToken != previous.RefreshToken { + t.Fatalf("rejected TUI login changed previous token: ok=%v err=%v", ok, err) + } + after, err := os.ReadFile(configPath) + if err != nil || string(after) != string(seed) { + t.Fatalf("rejected TUI login changed config: readErr=%v", err) + } +} + func TestAppendOAuthLoginProfileAddsOnceAndRespectsRenames(t *testing.T) { saved := []config.ProviderProfile{{Name: "opengateway", ProviderKind: config.ProviderKindOpenAICompatible}} diff --git a/internal/tui/provider_wizard_test.go b/internal/tui/provider_wizard_test.go index 1013bd917..b0bd7dfe8 100644 --- a/internal/tui/provider_wizard_test.go +++ b/internal/tui/provider_wizard_test.go @@ -719,6 +719,66 @@ func TestProviderWizardPersistsPastedKeyToUserConfig(t *testing.T) { } } +func TestProviderWizardRejectsCaseVariantBeforeCredentialCapture(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(`{"activeProvider":"work","providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "OLD"); err != nil { + t.Fatal(err) + } + + m := newModel(context.Background(), Options{ + UserConfigPath: configPath, + NewProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return &fakeProvider{}, nil + }, + }) + m = openProviderWizardForTest(t, m) + m.providerWizard.selectedProvider = providerWizardProviderIndex(t, m.providerWizard, "ollama-cloud") + m.providerWizard.profileName = "WORK" + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + updated, _ = next.Update(testPaste("NEW")) + next = updated.(model) + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + next = finishProviderWizardModelDiscoveryForTest(t, next) + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + updated, _ = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + + if next.providerWizard == nil { + t.Fatal("rejected wizard unexpectedly closed") + } + if !strings.Contains(next.providerWizard.err, `provider "WORK" already exists as "work"`) { + t.Fatalf("wizard error = %q, want case-variant collision", next.providerWizard.err) + } + after, readErr := os.ReadFile(configPath) + if readErr != nil { + t.Fatal(readErr) + } + if string(after) != string(before) { + t.Fatalf("rejected wizard rewrote config\nbefore: %s\nafter: %s", before, after) + } + if key, ok, getErr := store.Get("work"); getErr != nil || !ok || key != "OLD" { + t.Fatalf("existing credential = %q,%v,%v; want OLD,true,nil", key, ok, getErr) + } +} + func TestProviderWizardUsesAPIKeyEnvForCurrentSessionWithoutPersistingSecret(t *testing.T) { const secret = "ollama-env-secret" t.Setenv("OLLAMA_API_KEY", secret) @@ -1138,26 +1198,73 @@ func TestProviderWizardManageKeyRemove(t *testing.T) { if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"acme","apiKeyStored":true}]}`), 0o600); err != nil { + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { t.Fatal(err) } store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath)) if err != nil { t.Fatal(err) } - if err := store.Set("acme", "sk-secret"); err != nil { + if err := store.Set("work", "sk-secret"); err != nil { t.Fatal(err) } m := newModel(context.Background(), Options{UserConfigPath: configPath}) - m.providerWizard = &providerWizardState{step: providerWizardStepManageKey, manageProviderName: "acme", manageKeyCursor: 2} + m.providerWizard = &providerWizardState{step: providerWizardStepManageKey, manageProviderName: "WORK", manageKeyCursor: 2} next, _ := m.applyManageKeyChoice() if next.providerWizard != nil { t.Fatal("remove should close the wizard") } - if _, ok, _ := store.Get("acme"); ok { - t.Fatal("remove should delete the key from the credential store") + if _, ok, _ := store.Get("work"); ok { + t.Fatal("remove should delete the normalized key from the credential store") } + if cfg := readProviderWizardConfigFixture(t, configPath); cfg.Providers[0].APIKeyStored { + t.Fatal("case-variant removal left apiKeyStored set") + } +} + +func TestProviderWizardManageKeyRemoveReportsCleanupFailures(t *testing.T) { + newRemovalModel := func(t *testing.T) model { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + m := newModel(context.Background(), Options{UserConfigPath: path}) + m.providerWizard = &providerWizardState{step: providerWizardStepManageKey, manageProviderName: "work", manageKeyCursor: 2} + return m + } + + t.Run("stored key deletion", func(t *testing.T) { + m := newRemovalModel(t) + m.deleteProviderKey = func(string, string) (bool, error) { + return false, errors.New("injected delete failure") + } + next, _ := m.applyManageKeyChoice() + if next.providerWizard == nil || !strings.Contains(next.providerWizard.err, "Stored key removal failed") { + t.Fatalf("wizard did not remain open with deletion error: %+v", next.providerWizard) + } + // Marker first, secret second: a failed delete leaves an orphaned secret + // that nothing reads, never a marker claiming a key that is gone. + if cfg := readProviderWizardConfigFixture(t, next.userConfigPath); cfg.Providers[0].APIKeyStored { + t.Fatal("marker must be cleared before the secret delete is attempted") + } + }) + + t.Run("persisted marker cleanup", func(t *testing.T) { + m := newRemovalModel(t) + m.deleteProviderKey = func(string, string) (bool, error) { return true, nil } + m.clearProviderKeyStored = func(string, string) (bool, error) { + return false, errors.New("injected marker failure") + } + next, _ := m.applyManageKeyChoice() + if next.providerWizard == nil || !strings.Contains(next.providerWizard.err, "Stored key marker cleanup failed") { + t.Fatalf("wizard did not remain open with marker error: %+v", next.providerWizard) + } + if cfg := readProviderWizardConfigFixture(t, next.userConfigPath); !cfg.Providers[0].APIKeyStored { + t.Fatal("injected marker failure unexpectedly changed config") + } + }) } func TestProviderWizardManageKeyReplaceAndKeep(t *testing.T) { @@ -1825,3 +1932,59 @@ func TestProviderWizardModelRowsStayDistinctWithProseDescriptions(t *testing.T) t.Errorf("prose blurb still used as a row label:\n%s", view) } } + +// The disk write is only half the removal: the live session still holds +// APIKeyStored:true until it is reconciled, so /providers and a re-entered +// wizard would keep offering keep/replace for a key that is gone. +func TestProviderWizardManageKeyRemoveSyncsSessionState(t *testing.T) { + t.Setenv("ZERO_CRED_STORAGE", "encrypted-file") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(`{"providers":[{"name":"work","apiKeyStored":true}]}`), 0o600); err != nil { + t.Fatal(err) + } + store, err := config.ProviderKeyStoreAt(dir) + if err != nil { + t.Fatal(err) + } + if err := store.Set("work", "sk-secret"); err != nil { + t.Fatal(err) + } + profile := config.ProviderProfile{Name: "work", APIKeyStored: true} + m := newModel(context.Background(), Options{ + UserConfigPath: configPath, + ProviderName: "work", + ProviderProfile: profile, + SavedProviders: []config.ProviderProfile{profile}, + }) + m.providerWizard = &providerWizardState{step: providerWizardStepManageKey, manageProviderName: "WORK", manageKeyCursor: 2} + + next, _ := m.applyManageKeyChoice() + + if next.providerProfile.APIKeyStored { + t.Fatal("live profile still claims a stored key after removal") + } + if len(next.savedProviders) != 1 || next.savedProviders[0].APIKeyStored { + t.Fatalf("saved providers not reconciled: %+v", next.savedProviders) + } + // The caller's copy must be untouched: savedProviders is mutated by copy. + if !m.savedProviders[0].APIKeyStored { + t.Fatal("session sync mutated the pre-removal model's slice in place") + } +} + +// wizardProviderStoredKey answers a credential question, so it must use the +// store's normalization: strings.EqualFold folds "s" and Unicode long-s into +// one identity that the store keeps apart. +func TestWizardProviderStoredKeyDistinguishesUnicodeIdentities(t *testing.T) { + m := model{savedProviders: []config.ProviderProfile{ + {Name: "ſ", APIKeyStored: true}, + }} + if name, ok := m.wizardProviderStoredKey(providercatalog.Descriptor{Name: "s", ID: "s"}); ok { + t.Fatalf("latin-s descriptor matched the long-s profile %q", name) + } + name, ok := m.wizardProviderStoredKey(providercatalog.Descriptor{Name: "ſ", ID: "ſ"}) + if !ok || name != "ſ" { + t.Fatalf("long-s descriptor = %q, %v; want its own profile", name, ok) + } +} diff --git a/internal/tui/session.go b/internal/tui/session.go index 4aec00e58..b978f9ac9 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -11,6 +11,7 @@ import ( "time" "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/config" "github.com/Gitlawb/zero/internal/execution" "github.com/Gitlawb/zero/internal/sandbox" "github.com/Gitlawb/zero/internal/sessions" @@ -333,7 +334,10 @@ func (m model) formatResumeSummary(session sessions.Metadata, eventCount int) st modelLine += " (recorded: " + recorded + ")" } providerLine := "provider: " + displayValue(m.providerName, "none") - if recorded := strings.TrimSpace(session.Provider); recorded != "" && !strings.EqualFold(recorded, m.providerName) { + // Provider names are compared with the credential store's rule, so a + // recorded spelling that is a genuinely different provider (Unicode long-s) + // is reported as a difference rather than folded into a silent match. + if recorded := strings.TrimSpace(session.Provider); recorded != "" && !config.SameProviderIdentity(recorded, m.providerName) { providerLine += " (recorded: " + recorded + ")" } lines := []string{