Address Codex's feedback

Signed-off-by: William Woodruff <ww@openai.com>

codex-rs/core/src/shell_snapshot.rs

@@ -43,7 +43,8 @@ pub(crate) struct ShellSnapshotFile {
 const SNAPSHOT_TIMEOUT: Duration = Duration::from_secs(10);
 const SNAPSHOT_RETENTION: Duration = Duration::from_secs(60 * 60 * 24 * 3); // 3 days retention.
 const SNAPSHOT_DIR: &str = "shell_snapshots";
-const EXCLUDED_EXPORT_VARS: &[&str] = &["PWD", "OLDPWD", "RUST_LOG"];
+const EXCLUDED_EXPORT_VARS: &[&str] = &["PWD", "OLDPWD"];
+const EXCLUDED_INHERITED_VARS: &[&str] = &["RUST_LOG"];
 
 impl ShellSnapshot {
     pub(crate) fn new(
@@ -287,6 +288,7 @@ async fn run_script_with_timeout(
     // returns a ref of handler.
     let mut handler = Command::new(&args[0]);
     handler.args(&args[1..]);
+    remove_inherited_snapshot_vars(&mut handler);
     handler.stdin(Stdio::null());
     handler.current_dir(cwd);
     #[cfg(unix)]
@@ -311,6 +313,12 @@ async fn run_script_with_timeout(
     Ok(String::from_utf8_lossy(&output.stdout).into_owned())
 }
 
+fn remove_inherited_snapshot_vars(handler: &mut Command) {
+    for name in EXCLUDED_INHERITED_VARS {
+        handler.env_remove(name);
+    }
+}
+
 fn excluded_exports_regex() -> String {
     EXCLUDED_EXPORT_VARS.join("|")
 }

codex-rs/core/src/shell_snapshot_tests.rs

@@ -148,44 +148,85 @@ fn bash_snapshot_filters_invalid_exports() -> Result<()> {
 }
 
 #[cfg(unix)]
-fn assert_snapshot_filters_rust_log(shell: &str, snapshot_script: String) -> Result<()> {
+fn assert_snapshot_preserves_profile_rust_log(
+    shell: &str,
+    snapshot_script: String,
+    profile_name: &str,
+) -> Result<()> {
     let home = tempdir()?;
-    let output = Command::new(shell)
+    let profile_path = home.path().join(profile_name);
+    std::fs::write(&profile_path, "export RUST_LOG=profile\n")?;
+
+    let mut snapshot = Command::new(shell);
+    snapshot
         .arg("-c")
         .arg(snapshot_script)
         .env("HOME", home.path())
-        .env("BASH_ENV", "/dev/null")
+        .env_remove("BASH_ENV")
         .env_remove("ENV")
         .env("RUST_LOG", "warn")
-        .output()?;
+        .env("RUST_LOG_STYLE", "always");
+    if shell == "/bin/sh" {
+        snapshot.env("ENV", &profile_path);
+    }
+    for name in EXCLUDED_INHERITED_VARS {
+        snapshot.env_remove(name);
+    }
+    let output = snapshot.output()?;
 
     assert!(output.status.success());
 
-    let stdout = String::from_utf8_lossy(&output.stdout);
-    assert!(
-        !stdout.contains("RUST_LOG"),
-        "snapshot should not capture app-server logging configuration"
-    );
+    let snapshot_path = home.path().join("snapshot.sh");
+    std::fs::write(&snapshot_path, &output.stdout)?;
+    let validate = Command::new(shell)
+        .arg("-c")
+        .arg(". \"$1\"; printf '%s' \"$RUST_LOG\"")
+        .arg(shell)
+        .arg(&snapshot_path)
+        .env("HOME", home.path())
+        .env("BASH_ENV", "/dev/null")
+        .env_remove("ENV")
+        .env_remove("RUST_LOG")
+        .output()?;
 
+    assert!(validate.status.success());
+    assert_eq!(String::from_utf8_lossy(&validate.stdout), "profile");
     Ok(())
 }
 
 #[cfg(unix)]
 #[test]
-fn bash_snapshot_filters_rust_log() -> Result<()> {
-    assert_snapshot_filters_rust_log("/bin/bash", bash_snapshot_script())
+fn bash_snapshot_preserves_profile_rust_log() -> Result<()> {
+    assert_snapshot_preserves_profile_rust_log("/bin/bash", bash_snapshot_script(), ".bashrc")
 }
 
 #[cfg(unix)]
 #[test]
-fn sh_snapshot_filters_rust_log() -> Result<()> {
-    assert_snapshot_filters_rust_log("/bin/sh", sh_snapshot_script())
+fn sh_snapshot_preserves_profile_rust_log() -> Result<()> {
+    assert_snapshot_preserves_profile_rust_log("/bin/sh", sh_snapshot_script(), ".profile")
 }
 
 #[cfg(target_os = "macos")]
 #[test]
-fn zsh_snapshot_filters_rust_log() -> Result<()> {
-    assert_snapshot_filters_rust_log("/bin/zsh", zsh_snapshot_script())
+fn zsh_snapshot_preserves_profile_rust_log() -> Result<()> {
+    assert_snapshot_preserves_profile_rust_log("/bin/zsh", zsh_snapshot_script(), ".zshrc")
+}
+
+#[cfg(unix)]
+#[tokio::test]
+async fn snapshot_shell_does_not_inherit_rust_log() -> Result<()> {
+    let mut command = tokio::process::Command::new("/bin/sh");
+    command
+        .arg("-c")
+        .arg("printf '%s' \"${RUST_LOG-unset}\"")
+        .env("RUST_LOG", "warn");
+    remove_inherited_snapshot_vars(&mut command);
+
+    let output = command.output().await?;
+
+    assert!(output.status.success());
+    assert_eq!(String::from_utf8_lossy(&output.stdout), "unset");
+    Ok(())
 }
 
 #[cfg(unix)]

codex-rs/protocol/src/config_types.rs

@@ -203,11 +203,12 @@ pub type EnvironmentVariablePattern = WildMatchPattern<'*', '?'>;
 
 /// Deriving the `env` based on this policy works as follows:
 /// 1. Create an initial map based on the `inherit` policy.
-/// 2. If `ignore_default_excludes` is false, filter the map using the default
+/// 2. Remove inherited `RUST_LOG` unless `include_only` explicitly retains it.
+/// 3. If `ignore_default_excludes` is false, filter the map using the default
 ///    exclude pattern(s), which are: `"*KEY*"`, `"*SECRET*"`, and `"*TOKEN*"`.
-/// 3. If `exclude` is not empty, filter the map using the provided patterns.
-/// 4. Insert any entries from `r#set` into the map.
-/// 5. If non-empty, filter the map using the `include_only` patterns.
+/// 4. If `exclude` is not empty, filter the map using the provided patterns.
+/// 5. Insert any entries from `r#set` into the map.
+/// 6. If non-empty, filter the map using the `include_only` patterns.
 #[derive(Debug, Clone, PartialEq)]
 pub struct ShellEnvironmentPolicy {
     /// Starting point when building the environment.

codex-rs/protocol/src/shell_environment.rs

@@ -72,18 +72,20 @@ where
         }
     };
 
-    // Remove RUST_LOG if present.
-    // Harnesses like Codex Desktop and the VS Code extension will
-    // spawn the app-server with `RUST_LOG=warn` by default, which can then cause
-    // unexpected behavior in subprocesses. Users can still explicitly include
-    // it via overrides.
-    env_map.retain(|key, _| !key.eq_ignore_ascii_case("RUST_LOG"));
-
     let matches_any = |name: &str, patterns: &[EnvironmentVariablePattern]| -> bool {
         patterns.iter().any(|pattern| pattern.matches(name))
     };
 
-    // Step 2 - Apply the default exclude if not disabled.
+    // Step 2 - Harnesses like Codex Desktop and the VS Code extension spawn the
+    // app-server with `RUST_LOG=warn` by default. Do not pass that inherited
+    // value to subprocesses unless the user explicitly includes it.
+    let include_inherited_rust_log =
+        !policy.include_only.is_empty() && matches_any("RUST_LOG", &policy.include_only);
+    if !include_inherited_rust_log {
+        env_map.retain(|key, _| !is_rust_log(key));
+    }
+
+    // Step 3 - Apply the default exclude if not disabled.
     if !policy.ignore_default_excludes {
         let default_excludes = vec![
             EnvironmentVariablePattern::new_case_insensitive("*KEY*"),
@@ -93,22 +95,22 @@ where
         env_map.retain(|k, _| !matches_any(k, &default_excludes));
     }
 
-    // Step 3 - Apply custom excludes.
+    // Step 4 - Apply custom excludes.
     if !policy.exclude.is_empty() {
         env_map.retain(|k, _| !matches_any(k, &policy.exclude));
     }
 
-    // Step 4 - Apply user-provided overrides.
+    // Step 5 - Apply user-provided overrides.
     for (key, val) in &policy.r#set {
         env_map.insert(key.clone(), val.clone());
     }
 
-    // Step 5 - If include_only is non-empty, keep only the matching vars.
+    // Step 6 - If include_only is non-empty, keep only the matching vars.
     if !policy.include_only.is_empty() {
         env_map.retain(|k, _| matches_any(k, &policy.include_only));
     }
 
-    // Step 6 - Populate the thread ID environment variable when provided.
+    // Step 7 - Populate the thread ID environment variable when provided.
     if let Some(thread_id) = thread_id {
         env_map.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string());
     }
@@ -116,6 +118,14 @@ where
     env_map
 }
 
+fn is_rust_log(name: &str) -> bool {
+    if cfg!(target_os = "windows") {
+        name.eq_ignore_ascii_case("RUST_LOG")
+    } else {
+        name == "RUST_LOG"
+    }
+}
+
 #[cfg(not(target_os = "windows"))]
 const UNIX_CORE_ENV_VARS: &[&str] = &[
     "PATH", "SHELL", "TMPDIR", "TEMP", "TMP", "HOME", "LANG", "LC_ALL", "LC_CTYPE", "LOGNAME",
@@ -163,7 +173,7 @@ mod tests {
     fn inherited_rust_log_is_removed() {
         let vars = vec![
             ("PATH".to_string(), "/usr/bin".to_string()),
-            ("rust_log".to_string(), "warn".to_string()),
+            ("RUST_LOG".to_string(), "warn".to_string()),
         ];
 
         let result = populate_env(
@@ -178,6 +188,19 @@ mod tests {
         );
     }
 
+    #[test]
+    fn explicitly_included_rust_log_is_preserved() {
+        let vars = vec![("RUST_LOG".to_string(), "codex_core=trace".to_string())];
+        let policy = ShellEnvironmentPolicy {
+            include_only: vec![EnvironmentVariablePattern::new_case_insensitive("RUST_LOG")],
+            ..Default::default()
+        };
+
+        let result = populate_env(vars.clone(), &policy, /*thread_id*/ None);
+
+        assert_eq!(result, vars.into_iter().collect());
+    }
+
     #[test]
     fn explicit_rust_log_override_is_preserved() {
         let vars = vec![("RUST_LOG".to_string(), "warn".to_string())];
@@ -294,4 +317,17 @@ mod non_windows_tests {
 
         assert_eq!(result, expected);
     }
+
+    #[test]
+    fn similarly_named_unix_env_vars_are_preserved() {
+        let vars = make_vars(&[("rust_log", "warn"), ("Rust_Log", "debug")]);
+
+        let result = populate_env(
+            vars.clone(),
+            &ShellEnvironmentPolicy::default(),
+            /*thread_id*/ None,
+        );
+
+        assert_eq!(result, vars.into_iter().collect());
+    }
 }