Merge pull request #594 from Hmbown/fix/593-keyring-shadow

fix(auth): dual-write API key to keyring + config to stop stale-keyring shadow (#593)

crates/tui/src/config.rs

@@ -2168,7 +2168,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),
 }
 
@@ -2178,23 +2195,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<SavedCredential> {
     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))
 }
 
@@ -2355,7 +2425,8 @@ pub fn has_api_key_for(config: &Config, provider: ApiProvider) -> bool {
 pub fn save_api_key_for(provider: ApiProvider, api_key: &str) -> Result<PathBuf> {
     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),
         };
     }
 
@@ -2727,6 +2798,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

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",