fix: address latest PR review comments (SecretString, empty env, docs, embeddings)

- Change wizard llm_api_key from String to SecretString to prevent
  accidental logging of API keys
- Fix inject_llm_keys_from_secrets skipping when env var is set but
  empty, matching optional_env's treatment of empty as unset
- Fix inverted doc comment on INJECTED_VARS (env checked first, overlay
  is the fallback, not the other way around)
- Update stale "env vars" comments in main.rs to reflect overlay pattern
- Fix step_embeddings not seeing cached OpenAI key from wizard session

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

src/config.rs

@@ -18,8 +18,8 @@ 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()`.
+/// `optional_env()` without unsafe `set_var` calls. `optional_env()` checks
+/// real env vars first, then falls back to this overlay.
 static INJECTED_VARS: OnceLock<HashMap<String, String>> = OnceLock::new();
 
 /// Main configuration for the agent.
@@ -1385,8 +1385,9 @@ pub async fn inject_llm_keys_from_secrets(
     let mut injected = HashMap::new();
 
     for (secret_name, env_var) in mappings {
-        if std::env::var(env_var).is_ok() {
-            continue;
+        match std::env::var(env_var) {
+            Ok(val) if !val.is_empty() => continue,
+            _ => {}
         }
         match secrets.get_decrypted(user_id, secret_name).await {
             Ok(decrypted) => {

src/main.rs

@@ -488,14 +488,14 @@ async fn main() -> anyhow::Result<()> {
             None
         };
 
-    // Inject LLM API keys from the encrypted secrets store into env vars so that
-    // LlmConfig::resolve() picks them up. Then re-resolve LlmConfig with the
-    // newly available keys (backend may have been set during onboarding but the
-    // API key is in the secrets store, not in env vars).
+    // Inject LLM API keys from the encrypted secrets store into a thread-safe
+    // overlay so that optional_env() (used by LlmConfig::resolve()) picks them
+    // up. Then re-resolve LlmConfig with the newly available keys (backend may
+    // have been set during onboarding but the API key is in the secrets store).
     if let Some(ref secrets) = secrets_store {
         ironclaw::config::inject_llm_keys_from_secrets(secrets.as_ref(), "default").await;
 
-        // Re-resolve LlmConfig now that env vars may have been populated
+        // Re-resolve LlmConfig now that secrets overlay has been populated
         if let Some(ref db_ref) = db {
             match Config::from_db(db_ref.as_ref(), "default").await {
                 Ok(refreshed) => {

src/setup/wizard.rs

@@ -83,7 +83,7 @@ pub struct SetupWizard {
     /// 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>,
+    llm_api_key: Option<SecretString>,
 }
 
 impl SetupWizard {
@@ -745,7 +745,7 @@ impl SetupWizard {
                         tracing::warn!("Failed to persist env key to secrets: {}", e);
                     }
                 }
-                self.llm_api_key = Some(existing);
+                self.llm_api_key = Some(SecretString::from(existing));
                 print_success(&format!("{display_name} configured (from env)"));
                 return Ok(());
             }
@@ -775,7 +775,7 @@ impl SetupWizard {
         }
 
         // Cache key in memory for model fetching later in the wizard
-        self.llm_api_key = Some(key_str.to_string());
+        self.llm_api_key = Some(SecretString::from(key_str.to_string()));
 
         print_success(&format!("{display_name} configured"));
         Ok(())
@@ -883,11 +883,19 @@ impl SetupWizard {
 
         match backend {
             "anthropic" => {
-                let models = fetch_anthropic_models(self.llm_api_key.as_deref()).await;
+                let cached = self
+                    .llm_api_key
+                    .as_ref()
+                    .map(|k| k.expose_secret().to_string());
+                let models = fetch_anthropic_models(cached.as_deref()).await;
                 self.select_from_model_list(&models)?;
             }
             "openai" => {
-                let models = fetch_openai_models(self.llm_api_key.as_deref()).await;
+                let cached = self
+                    .llm_api_key
+                    .as_ref()
+                    .map(|k| k.expose_secret().to_string());
+                let models = fetch_openai_models(cached.as_deref()).await;
                 self.select_from_model_list(&models)?;
             }
             "ollama" => {
@@ -1034,7 +1042,8 @@ impl SetupWizard {
         }
 
         let backend = self.settings.llm_backend.as_deref().unwrap_or("nearai");
-        let has_openai_key = std::env::var("OPENAI_API_KEY").is_ok();
+        let has_openai_key = std::env::var("OPENAI_API_KEY").is_ok()
+            || (backend == "openai" && self.llm_api_key.is_some());
         let has_nearai = backend == "nearai" || self.session_manager.is_some();
 
         // If the LLM backend is OpenAI and we already have a key, default to OpenAI embeddings