diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index a57d27d9..9e52de2f 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -2162,7 +2162,24 @@ pub fn ensure_parent_dir(path: &Path) -> Result<()> { /// the caller can show a confirmation message without leaking the key. #[derive(Debug, Clone, PartialEq, Eq)] pub enum SavedCredential { - /// Stored in the deepseek config file at the given path. + /// Stored in **both** the OS keyring and the deepseek config file. + /// This is the default outcome on platforms with a working keyring + /// backend: writing both layers defeats the + /// `keyring → env → config-file` resolution-order shadow that + /// would otherwise let a stale OS-keyring entry from a previous + /// install hide the freshly-entered key (#593). The `backend` + /// label is the value of [`deepseek_secrets::Secrets::backend_name`] + /// at write time so the toast text can name the actual backend + /// (`"system keyring"`, `"file-based (~/.deepseek/secrets/)"`). + KeyringAndConfigFile { + /// `Secrets::backend_name()` at write time. + backend: String, + /// Absolute path to the config file that was also updated. + path: PathBuf, + }, + /// Stored in the deepseek config file only. Fallback when no + /// keyring backend is reachable, or under `cfg(test)` so unit + /// tests don't pollute the host keyring. ConfigFile(PathBuf), } @@ -2172,23 +2189,76 @@ impl SavedCredential { #[must_use] pub fn describe(&self) -> String { match self { + Self::KeyringAndConfigFile { backend, path } => { + format!("OS keyring ({backend}) and {}", path.display()) + } Self::ConfigFile(path) => path.display().to_string(), } } } -/// Save the active provider's API key to `~/.deepseek/config.toml`. +/// Save the active provider's API key. /// -/// v0.8.8 intentionally uses the shared config file as the default -/// setup path. It works in every folder, in npm installs, in IDE -/// terminals, and without platform credential prompts. +/// **Dual-write strategy (#593):** writes to `~/.deepseek/config.toml` +/// (always) and to the OS keyring via [`deepseek_secrets::Secrets`] +/// (when a backend is reachable). The runtime resolves credentials in +/// `keyring → env → config-file` order; writing to the config file +/// alone — as v0.8.8 through v0.8.10 did — let a stale keyring entry +/// from a prior install silently shadow the fresh value the user just +/// typed during in-TUI onboarding, producing the "no response" symptom +/// reported in #593. +/// +/// The config file remains the inspectable durable record (works in +/// npm installs, IDE terminals, and headless boxes alike), and the +/// keyring acts as the layered override that defeats stale-shadow on +/// the resolution path. When the keyring write fails (no backend, OS +/// permission denied, etc.) the config-file write still stands and +/// the function reports a [`SavedCredential::ConfigFile`] outcome — +/// callers should not treat that as a failure. +/// +/// Skipped under `cfg(test)` so the suite never touches the host +/// keyring. The `secrets` crate has its own test coverage for +/// keyring set/get. pub fn save_api_key(api_key: &str) -> Result { let trimmed = api_key.trim(); if trimmed.is_empty() { anyhow::bail!("Refusing to save an empty API key."); } + // Always write the inspectable copy first. The config file is the + // durable record everyone — including macOS Keychain-prompted + // first-run, headless CI, and IDE terminals — can rely on. let path = save_api_key_to_config_file(trimmed)?; + + // Then mirror to the OS keyring when one is reachable. This + // overwrites any stale entry from a prior install so + // `Secrets::resolve` (keyring → env → config-file) no longer + // shadows the fresh key. Skipped under `cfg(test)` so unit tests + // can't pollute the host keyring (macOS Always-Allow prompts, + // cross-test contamination). + #[cfg(not(test))] + { + let secrets = deepseek_secrets::Secrets::auto_detect(); + match secrets.set("deepseek", trimmed) { + Ok(()) => { + let backend = secrets.backend_name().to_string(); + log_sensitive_event( + "credential.save", + json!({ + "backend": backend.clone(), + "config_path": path.display().to_string(), + "dual_write": true, + }), + ); + return Ok(SavedCredential::KeyringAndConfigFile { backend, path }); + } + Err(err) => { + tracing::warn!("OS keyring write failed; key saved to config.toml only: {err}"); + // Fall through to the ConfigFile-only outcome below. + } + } + } + Ok(SavedCredential::ConfigFile(path)) } @@ -2349,7 +2419,8 @@ pub fn has_api_key_for(config: &Config, provider: ApiProvider) -> bool { pub fn save_api_key_for(provider: ApiProvider, api_key: &str) -> Result { if matches!(provider, ApiProvider::Deepseek | ApiProvider::DeepseekCN) { return match save_api_key(api_key)? { - SavedCredential::ConfigFile(path) => Ok(path), + SavedCredential::KeyringAndConfigFile { path, .. } + | SavedCredential::ConfigFile(path) => Ok(path), }; } @@ -2721,6 +2792,23 @@ mod tests { assert_eq!(cf.describe(), "/tmp/x.toml"); } + /// #593: the dual-write outcome describes both targets so the + /// onboarding toast (`API key saved to {describe}`) tells the user + /// the key landed in *both* the keyring and the config file — + /// which is the whole point of the fix (defeats stale-keyring + /// shadow while keeping the config file inspectable). + #[test] + fn saved_credential_describe_lists_both_targets_for_keyring_and_config() { + let dual = SavedCredential::KeyringAndConfigFile { + backend: "system keyring".to_string(), + path: PathBuf::from("/tmp/x.toml"), + }; + assert_eq!( + dual.describe(), + "OS keyring (system keyring) and /tmp/x.toml" + ); + } + #[test] fn has_api_key_detects_in_memory_override_and_env_var() -> Result<()> { // Pins the v0.8.8 contract: `has_api_key` covers the prompt-free diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 40b664df..a13a8392 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -1580,6 +1580,45 @@ async fn run_doctor(config: &Config, workspace: &Path, config_path_override: Opt } println!(" · credential sources: env, ~/.deepseek/config.toml"); + // #593: surface keyring/config disagreement explicitly. The runtime + // resolution order is `keyring → env → config-file`, so a stale + // keyring entry from a prior install can shadow the value the user + // sees in `~/.deepseek/config.toml`. We only check the DeepSeek + // slot — other providers don't write to the keyring today, and + // probing entries that aren't there triggers macOS keychain + // prompts for nothing. + let secrets = deepseek_secrets::Secrets::auto_detect(); + let keyring_key = secrets.get("deepseek").ok().flatten(); + let config_key = config + .api_key + .as_ref() + .filter(|v| !v.trim().is_empty() && v.as_str() != "__KEYRING__") + .map(|s| s.to_string()); + match (keyring_key.as_deref(), config_key.as_deref()) { + (Some(k), Some(c)) if k.trim() != c.trim() => { + println!(); + println!( + " {} `deepseek`: OS keyring and config.toml hold different values.", + "⚠".truecolor(red_r, red_g, red_b) + ); + println!( + " Resolution order is keyring → env → config-file, so the keyring value wins." + ); + println!(" Reconcile by overwriting both with the current key:"); + println!(" deepseek auth set --provider deepseek"); + println!( + " (Or paste the key into the in-TUI onboarding screen — it now writes both layers.)" + ); + } + (Some(_), None) => { + println!( + " {} `deepseek`: key is in OS keyring only (config.toml has no copy).", + "·".dimmed() + ); + } + _ => {} + } + let has_api_key = if config.deepseek_api_key().is_ok() { println!( " {} active provider key resolved",