fix: remove unsafe set_var, use thread-safe overlay for injected secrets

Address PR #92 review comments:
- Replace all 5 unsafe `std::env::set_var()` calls with safe alternatives
- Add INJECTED_VARS OnceLock<HashMap> overlay in config.rs, checked by
  optional_env() before falling back to std::env::var()
- Cache wizard API key in SetupWizard.llm_api_key field instead of env
- Pass explicit key param to fetch_anthropic_models/fetch_openai_models
- Persist env-provided API keys to secrets store during onboarding

Co-Authored-By: Claude Opus 4.6 <[email protected]>

src/config.rs

@@ -5,7 +5,9 @@
 //! in startup). Everything else comes from env vars, the DB settings
 //! table, or auto-detection.
 
+use std::collections::HashMap;
 use std::path::PathBuf;
+use std::sync::OnceLock;
 use std::time::Duration;
 
 use secrecy::{ExposeSecret, SecretString};
@@ -13,6 +15,13 @@ use secrecy::{ExposeSecret, SecretString};
 use crate::error::ConfigError;
 use crate::settings::Settings;
 
+/// Thread-safe overlay for injected env vars (secrets loaded from DB).
+///
+/// Used by `inject_llm_keys_from_secrets()` to make API keys available to
+/// `optional_env()` without unsafe `set_var` calls. Read by `optional_env()`
+/// before falling back to `std::env::var()`.
+static INJECTED_VARS: OnceLock<HashMap<String, String>> = OnceLock::new();
+
 /// Main configuration for the agent.
 #[derive(Debug, Clone)]
 pub struct Config {
@@ -1358,11 +1367,12 @@ impl ClaudeCodeConfig {
     }
 }
 
-/// Load API keys from the encrypted secrets store into process env vars.
+/// Load API keys from the encrypted secrets store into a thread-safe overlay.
 ///
 /// This bridges the gap between secrets stored during onboarding and the
-/// env-var-first resolution in `LlmConfig::resolve()`. Only sets env vars
-/// that aren't already present, so explicit env vars always win.
+/// env-var-first resolution in `LlmConfig::resolve()`. Keys in the overlay
+/// are read by `optional_env()` before falling back to `std::env::var()`,
+/// so explicit env vars always win.
 pub async fn inject_llm_keys_from_secrets(
     secrets: &dyn crate::secrets::SecretsStore,
     user_id: &str,
@@ -1373,42 +1383,49 @@ pub async fn inject_llm_keys_from_secrets(
         ("llm_compatible_api_key", "LLM_API_KEY"),
     ];
 
+    let mut injected = HashMap::new();
+
     for (secret_name, env_var) in mappings {
         if std::env::var(env_var).is_ok() {
             continue;
         }
         match secrets.get_decrypted(user_id, secret_name).await {
             Ok(decrypted) => {
-                // SAFETY: Called from main() before any tokio::spawn(). The tokio
-                // worker threads exist but are idle (no tasks scheduled yet), so
-                // no concurrent std::env::var reads can race with this write.
-                unsafe {
-                    std::env::set_var(env_var, decrypted.expose());
-                }
-                tracing::debug!(
-                    "Injected secret '{}' into env var '{}'",
-                    secret_name,
-                    env_var
-                );
+                injected.insert(env_var.to_string(), decrypted.expose().to_string());
+                tracing::debug!("Loaded secret '{}' for env var '{}'", secret_name, env_var);
             }
             Err(_) => {
                 // Secret doesn't exist, that's fine
             }
         }
     }
+
+    let _ = INJECTED_VARS.set(injected);
 }
 
 // Helper functions
 
 fn optional_env(key: &str) -> Result<Option<String>, ConfigError> {
+    // Check real env vars first (always win over injected secrets)
     match std::env::var(key) {
-        Ok(val) if val.is_empty() => Ok(None),
-        Ok(val) => Ok(Some(val)),
-        Err(std::env::VarError::NotPresent) => Ok(None),
-        Err(e) => Err(ConfigError::ParseError(format!(
-            "failed to read {key}: {e}"
-        ))),
+        Ok(val) if val.is_empty() => {}
+        Ok(val) => return Ok(Some(val)),
+        Err(std::env::VarError::NotPresent) => {}
+        Err(e) => {
+            return Err(ConfigError::ParseError(format!(
+                "failed to read {key}: {e}"
+            )));
+        }
     }
+
+    // Fall back to thread-safe overlay (secrets injected from DB)
+    if let Some(map) = INJECTED_VARS.get() {
+        if let Some(val) = map.get(key) {
+            return Ok(Some(val.clone()));
+        }
+    }
+
+    Ok(None)
 }
 
 fn parse_optional_env<T>(key: &str, default: T) -> Result<T, ConfigError>

src/setup/wizard.rs

@@ -82,6 +82,8 @@ pub struct SetupWizard {
     db_backend: Option<crate::db::libsql_backend::LibSqlBackend>,
     /// Secrets crypto (created during setup).
     secrets_crypto: Option<Arc<SecretsCrypto>>,
+    /// Cached API key from provider setup (used by model fetcher without env mutation).
+    llm_api_key: Option<String>,
 }
 
 impl SetupWizard {
@@ -96,6 +98,7 @@ impl SetupWizard {
             #[cfg(feature = "libsql")]
             db_backend: None,
             secrets_crypto: None,
+            llm_api_key: None,
         }
     }
 
@@ -110,6 +113,7 @@ impl SetupWizard {
             #[cfg(feature = "libsql")]
             db_backend: None,
             secrets_crypto: None,
+            llm_api_key: None,
         }
     }
 
@@ -734,6 +738,14 @@ impl SetupWizard {
         if let Ok(existing) = std::env::var(env_var) {
             print_info(&format!("{env_var} found: {}", mask_api_key(&existing)));
             if confirm("Use this key?", true).map_err(SetupError::Io)? {
+                // Persist env-provided key to secrets store for future runs
+                if let Ok(ctx) = self.init_secrets_context().await {
+                    let key = SecretString::from(existing.clone());
+                    if let Err(e) = ctx.save_secret(secret_name, &key).await {
+                        tracing::warn!("Failed to persist env key to secrets: {}", e);
+                    }
+                }
+                self.llm_api_key = Some(existing);
                 print_success(&format!("{display_name} configured (from env)"));
                 return Ok(());
             }
@@ -762,10 +774,8 @@ impl SetupWizard {
             ));
         }
 
-        // SAFETY: Onboarding runs single-threaded before the async runtime spawns workers.
-        unsafe {
-            std::env::set_var(env_var, key_str);
-        }
+        // Cache key in memory for model fetching later in the wizard
+        self.llm_api_key = Some(key_str.to_string());
 
         print_success(&format!("{display_name} configured"));
         Ok(())
@@ -793,11 +803,6 @@ impl SetupWizard {
         let url = url_input.unwrap_or_else(|| default_url.to_string());
         self.settings.ollama_base_url = Some(url.clone());
 
-        // SAFETY: Onboarding runs single-threaded before the async runtime spawns workers.
-        unsafe {
-            std::env::set_var("OLLAMA_BASE_URL", &url);
-        }
-
         print_success(&format!("Ollama configured ({})", url));
         Ok(())
     }
@@ -830,10 +835,6 @@ impl SetupWizard {
         }
 
         self.settings.openai_compatible_base_url = Some(url.clone());
-        // SAFETY: Onboarding runs single-threaded before the async runtime spawns workers.
-        unsafe {
-            std::env::set_var("LLM_BASE_URL", &url);
-        }
 
         // Optional API key
         if confirm("Does this endpoint require an API key?", false).map_err(SetupError::Io)? {
@@ -851,11 +852,6 @@ impl SetupWizard {
                 } else {
                     print_info("Secrets not available. Set LLM_API_KEY in your environment.");
                 }
-
-                // SAFETY: Onboarding runs single-threaded before the async runtime spawns workers.
-                unsafe {
-                    std::env::set_var("LLM_API_KEY", key_str);
-                }
             }
         }
 
@@ -887,11 +883,11 @@ impl SetupWizard {
 
         match backend {
             "anthropic" => {
-                let models = fetch_anthropic_models().await;
+                let models = fetch_anthropic_models(self.llm_api_key.as_deref()).await;
                 self.select_from_model_list(&models)?;
             }
             "openai" => {
-                let models = fetch_openai_models().await;
+                let models = fetch_openai_models(self.llm_api_key.as_deref()).await;
                 self.select_from_model_list(&models)?;
             }
             "ollama" => {
@@ -1619,7 +1615,7 @@ fn mask_password_in_url(url: &str) -> String {
 /// Fetch models from the Anthropic API.
 ///
 /// Returns `(model_id, display_label)` pairs. Falls back to static defaults on error.
-async fn fetch_anthropic_models() -> Vec<(String, String)> {
+async fn fetch_anthropic_models(cached_key: Option<&str>) -> Vec<(String, String)> {
     let static_defaults = vec![
         ("claude-sonnet-4-20250514".into(), "Claude Sonnet 4".into()),
         ("claude-opus-4-20250514".into(), "Claude Opus 4".into()),
@@ -1629,9 +1625,14 @@ async fn fetch_anthropic_models() -> Vec<(String, String)> {
         ),
     ];
 
-    let api_key = match std::env::var("ANTHROPIC_API_KEY") {
-        Ok(k) if !k.is_empty() => k,
-        _ => return static_defaults,
+    let api_key = cached_key
+        .map(String::from)
+        .or_else(|| std::env::var("ANTHROPIC_API_KEY").ok())
+        .filter(|k| !k.is_empty());
+
+    let api_key = match api_key {
+        Some(k) => k,
+        None => return static_defaults,
     };
 
     let client = reqwest::Client::new();
@@ -1680,16 +1681,21 @@ async fn fetch_anthropic_models() -> Vec<(String, String)> {
 /// Fetch models from the OpenAI API.
 ///
 /// Returns `(model_id, display_label)` pairs. Falls back to static defaults on error.
-async fn fetch_openai_models() -> Vec<(String, String)> {
+async fn fetch_openai_models(cached_key: Option<&str>) -> Vec<(String, String)> {
     let static_defaults = vec![
         ("gpt-4o".into(), "GPT-4o".into()),
         ("gpt-4o-mini".into(), "GPT-4o Mini (fast)".into()),
         ("o3".into(), "o3 (reasoning)".into()),
     ];
 
-    let api_key = match std::env::var("OPENAI_API_KEY") {
-        Ok(k) if !k.is_empty() => k,
-        _ => return static_defaults,
+    let api_key = cached_key
+        .map(String::from)
+        .or_else(|| std::env::var("OPENAI_API_KEY").ok())
+        .filter(|k| !k.is_empty());
+
+    let api_key = match api_key {
+        Some(k) => k,
+        None => return static_defaults,
     };
 
     let client = reqwest::Client::new();
@@ -2060,7 +2066,7 @@ mod tests {
     async fn test_fetch_anthropic_models_static_fallback() {
         // With no API key, should return static defaults
         let _guard = EnvGuard::clear("ANTHROPIC_API_KEY");
-        let models = fetch_anthropic_models().await;
+        let models = fetch_anthropic_models(None).await;
         assert!(!models.is_empty());
         assert!(
             models.iter().any(|(id, _)| id.contains("claude")),
@@ -2071,7 +2077,7 @@ mod tests {
     #[tokio::test]
     async fn test_fetch_openai_models_static_fallback() {
         let _guard = EnvGuard::clear("OPENAI_API_KEY");
-        let models = fetch_openai_models().await;
+        let models = fetch_openai_models(None).await;
         assert!(!models.is_empty());
         assert!(
             models.iter().any(|(id, _)| id.contains("gpt")),