diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 41c3034514..925f50e7ae 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/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("|") } diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index d83e3f515c..0c2c57809e 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/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)] diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index 83bcbdc0ae..28b0e0b344 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/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. diff --git a/codex-rs/protocol/src/shell_environment.rs b/codex-rs/protocol/src/shell_environment.rs index 0557327ddb..591ee43398 100644 --- a/codex-rs/protocol/src/shell_environment.rs +++ b/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()); + } }