From bd6bdcc9cd32f0dfc72f616ddc1bf00e3c0afa2a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 2 Jun 2026 12:32:49 -0400 Subject: [PATCH] Honor the user's shell env policy for RUST_LOG Signed-off-by: William Woodruff --- codex-rs/core/src/session/session.rs | 1 + codex-rs/core/src/shell_snapshot.rs | 80 ++++++++++++++++++---- codex-rs/core/src/shell_snapshot_tests.rs | 70 +++++++++++++++++-- codex-rs/protocol/src/shell_environment.rs | 13 +++- 4 files changed, 142 insertions(+), 22 deletions(-) diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 96dffed888..dbb6a7ec60 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -854,6 +854,7 @@ impl Session { ShellSnapshot::new( config.codex_home.clone(), thread_id, + config.permissions.shell_environment_policy.clone(), session_telemetry.clone(), state_db_ctx.clone(), ) diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 925f50e7ae..c18cb3543b 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -17,6 +17,8 @@ use anyhow::anyhow; use anyhow::bail; use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; +use codex_protocol::config_types::ShellEnvironmentPolicy; +use codex_protocol::shell_environment::policy_explicitly_includes_inherited_rust_log; use codex_utils_absolute_path::AbsolutePathBuf; use tokio::fs; use tokio::process::Command; @@ -32,6 +34,7 @@ pub(crate) struct ShellSnapshot { struct ShellSnapshotConfig { codex_home: AbsolutePathBuf, session_id: ThreadId, + shell_environment_policy: ShellEnvironmentPolicy, session_telemetry: SessionTelemetry, state_db: Option, } @@ -44,12 +47,12 @@ 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"]; -const EXCLUDED_INHERITED_VARS: &[&str] = &["RUST_LOG"]; impl ShellSnapshot { pub(crate) fn new( codex_home: AbsolutePathBuf, session_id: ThreadId, + shell_environment_policy: ShellEnvironmentPolicy, session_telemetry: SessionTelemetry, state_db: Option, ) -> Self { @@ -57,6 +60,7 @@ impl ShellSnapshot { config: Some(Arc::new(ShellSnapshotConfig { codex_home, session_id, + shell_environment_policy, session_telemetry, state_db, })), @@ -98,6 +102,7 @@ impl ShellSnapshot { config.session_id, &cwd, &shell, + &config.shell_environment_policy, config.state_db.clone(), ) .await; @@ -121,6 +126,7 @@ impl ShellSnapshot { session_id: ThreadId, session_cwd: &AbsolutePathBuf, shell: &Shell, + shell_environment_policy: &ShellEnvironmentPolicy, state_db: Option, ) -> std::result::Result { // File to store the snapshot @@ -151,7 +157,14 @@ impl ShellSnapshot { }); // Make the new snapshot. - if let Err(err) = write_shell_snapshot(shell.shell_type, &temp_path, session_cwd).await { + if let Err(err) = write_shell_snapshot( + shell.shell_type, + &temp_path, + session_cwd, + shell_environment_policy, + ) + .await + { tracing::warn!( "Failed to create shell snapshot for {}: {err:?}", shell.name() @@ -163,7 +176,9 @@ impl ShellSnapshot { temp_path.display() ); - if let Err(err) = validate_snapshot(shell, &temp_path, session_cwd).await { + if let Err(err) = + validate_snapshot(shell, &temp_path, session_cwd, shell_environment_policy).await + { tracing::error!("Shell snapshot validation failed: {err:?}"); remove_snapshot_file(&temp_path).await; return Err("validation_failed"); @@ -200,6 +215,7 @@ async fn write_shell_snapshot( shell_type: ShellType, output_path: &AbsolutePathBuf, cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, ) -> Result<()> { if shell_type == ShellType::PowerShell || shell_type == ShellType::Cmd { bail!("Shell snapshot not supported yet for {shell_type:?}"); @@ -207,7 +223,7 @@ async fn write_shell_snapshot( let shell = get_shell(shell_type, /*path*/ None) .with_context(|| format!("No available shell for {shell_type:?}"))?; - let raw_snapshot = capture_snapshot(&shell, cwd).await?; + let raw_snapshot = capture_snapshot(&shell, cwd, shell_environment_policy).await?; let snapshot = strip_snapshot_preamble(&raw_snapshot)?; if let Some(parent) = output_path.parent() { @@ -225,13 +241,37 @@ async fn write_shell_snapshot( Ok(()) } -async fn capture_snapshot(shell: &Shell, cwd: &AbsolutePathBuf) -> Result { +async fn capture_snapshot( + shell: &Shell, + cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, +) -> Result { let shell_type = shell.shell_type; match shell_type { - ShellType::Zsh => run_shell_script(shell, &zsh_snapshot_script(), cwd).await, - ShellType::Bash => run_shell_script(shell, &bash_snapshot_script(), cwd).await, - ShellType::Sh => run_shell_script(shell, &sh_snapshot_script(), cwd).await, - ShellType::PowerShell => run_shell_script(shell, powershell_snapshot_script(), cwd).await, + ShellType::Zsh => { + run_shell_script(shell, &zsh_snapshot_script(), cwd, shell_environment_policy).await + } + ShellType::Bash => { + run_shell_script( + shell, + &bash_snapshot_script(), + cwd, + shell_environment_policy, + ) + .await + } + ShellType::Sh => { + run_shell_script(shell, &sh_snapshot_script(), cwd, shell_environment_policy).await + } + ShellType::PowerShell => { + run_shell_script( + shell, + powershell_snapshot_script(), + cwd, + shell_environment_policy, + ) + .await + } ShellType::Cmd => bail!("Shell snapshotting is not yet supported for {shell_type:?}"), } } @@ -249,12 +289,14 @@ async fn validate_snapshot( shell: &Shell, snapshot_path: &AbsolutePathBuf, cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, ) -> Result<()> { let snapshot_path_display = snapshot_path.display(); let script = format!("set -e; . \"{snapshot_path_display}\""); run_script_with_timeout( shell, &script, + shell_environment_policy, SNAPSHOT_TIMEOUT, /*use_login_shell*/ false, cwd, @@ -263,10 +305,16 @@ async fn validate_snapshot( .map(|_| ()) } -async fn run_shell_script(shell: &Shell, script: &str, cwd: &AbsolutePathBuf) -> Result { +async fn run_shell_script( + shell: &Shell, + script: &str, + cwd: &AbsolutePathBuf, + shell_environment_policy: &ShellEnvironmentPolicy, +) -> Result { run_script_with_timeout( shell, script, + shell_environment_policy, SNAPSHOT_TIMEOUT, /*use_login_shell*/ true, cwd, @@ -277,6 +325,7 @@ async fn run_shell_script(shell: &Shell, script: &str, cwd: &AbsolutePathBuf) -> async fn run_script_with_timeout( shell: &Shell, script: &str, + shell_environment_policy: &ShellEnvironmentPolicy, snapshot_timeout: Duration, use_login_shell: bool, cwd: &AbsolutePathBuf, @@ -288,7 +337,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); + apply_inherited_rust_log_policy(&mut handler, shell_environment_policy); handler.stdin(Stdio::null()); handler.current_dir(cwd); #[cfg(unix)] @@ -313,9 +362,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 apply_inherited_rust_log_policy( + handler: &mut Command, + shell_environment_policy: &ShellEnvironmentPolicy, +) { + if !policy_explicitly_includes_inherited_rust_log(shell_environment_policy) { + handler.env_remove("RUST_LOG"); } } diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index 0c2c57809e..18cb7ac2b7 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -1,4 +1,6 @@ use super::*; +#[cfg(unix)] +use codex_protocol::config_types::EnvironmentVariablePattern; use core_test_support::PathBufExt; use core_test_support::PathExt; use pretty_assertions::assert_eq; @@ -83,7 +85,14 @@ fn assert_posix_snapshot_sections(snapshot: &str) { async fn get_snapshot(shell_type: ShellType) -> Result { let dir = tempdir()?; let path = dir.path().join("snapshot.sh"); - write_shell_snapshot(shell_type, &path.abs(), &dir.path().abs()).await?; + let shell_environment_policy = ShellEnvironmentPolicy::default(); + write_shell_snapshot( + shell_type, + &path.abs(), + &dir.path().abs(), + &shell_environment_policy, + ) + .await?; let content = fs::read_to_string(&path).await?; Ok(content) } @@ -169,9 +178,7 @@ fn assert_snapshot_preserves_profile_rust_log( if shell == "/bin/sh" { snapshot.env("ENV", &profile_path); } - for name in EXCLUDED_INHERITED_VARS { - snapshot.env_remove(name); - } + snapshot.env_remove("RUST_LOG"); let output = snapshot.output()?; assert!(output.status.success()); @@ -220,7 +227,7 @@ async fn snapshot_shell_does_not_inherit_rust_log() -> Result<()> { .arg("-c") .arg("printf '%s' \"${RUST_LOG-unset}\"") .env("RUST_LOG", "warn"); - remove_inherited_snapshot_vars(&mut command); + apply_inherited_rust_log_policy(&mut command, &ShellEnvironmentPolicy::default()); let output = command.output().await?; @@ -229,6 +236,50 @@ async fn snapshot_shell_does_not_inherit_rust_log() -> Result<()> { Ok(()) } +#[cfg(unix)] +#[tokio::test] +async fn explicitly_included_rust_log_is_visible_to_snapshot_profile() -> Result<()> { + let home = tempdir()?; + let profile_path = home.path().join(".bashrc"); + std::fs::write( + &profile_path, + "if [ \"${RUST_LOG-}\" = warn ]; then export PROFILE_SAW_RUST_LOG=yes; fi\n", + )?; + let shell_environment_policy = ShellEnvironmentPolicy { + include_only: vec![EnvironmentVariablePattern::new_case_insensitive("RUST_LOG")], + ..Default::default() + }; + + let mut snapshot = tokio::process::Command::new("/bin/bash"); + snapshot + .arg("-c") + .arg(bash_snapshot_script()) + .env("HOME", home.path()) + .env_remove("BASH_ENV") + .env("RUST_LOG", "warn"); + apply_inherited_rust_log_policy(&mut snapshot, &shell_environment_policy); + let output = snapshot.output().await?; + + assert!(output.status.success()); + + let snapshot_path = home.path().join("snapshot.sh"); + std::fs::write(&snapshot_path, &output.stdout)?; + let validate = tokio::process::Command::new("/bin/bash") + .arg("-c") + .arg(". \"$1\"; printf '%s' \"${PROFILE_SAW_RUST_LOG-unset}\"") + .arg("bash") + .arg(&snapshot_path) + .env("HOME", home.path()) + .env("BASH_ENV", "/dev/null") + .env_remove("RUST_LOG") + .output() + .await?; + + assert!(validate.status.success()); + assert_eq!(String::from_utf8_lossy(&validate.stdout), "yes"); + Ok(()) +} + #[cfg(unix)] #[test] fn bash_snapshot_preserves_multiline_exports() -> Result<()> { @@ -277,12 +328,14 @@ async fn try_create_creates_and_deletes_snapshot_file() -> Result<()> { shell_type: ShellType::Bash, shell_path: PathBuf::from("/bin/bash"), }; + let shell_environment_policy = ShellEnvironmentPolicy::default(); let snapshot = ShellSnapshot::try_create( &dir.path().abs(), ThreadId::new(), &dir.path().abs(), &shell, + &shell_environment_policy, /*state_db*/ None, ) .await @@ -306,12 +359,14 @@ async fn try_create_uses_distinct_generation_paths() -> Result<()> { shell_type: ShellType::Bash, shell_path: PathBuf::from("/bin/bash"), }; + let shell_environment_policy = ShellEnvironmentPolicy::default(); let initial_snapshot = ShellSnapshot::try_create( &dir.path().abs(), session_id, &dir.path().abs(), &shell, + &shell_environment_policy, /*state_db*/ None, ) .await @@ -321,6 +376,7 @@ async fn try_create_uses_distinct_generation_paths() -> Result<()> { session_id, &dir.path().abs(), &shell, + &shell_environment_policy, /*state_db*/ None, ) .await @@ -367,9 +423,11 @@ async fn snapshot_shell_does_not_inherit_stdin() -> Result<()> { "HOME=\"{home_display}\"; export HOME; {}", bash_snapshot_script() ); + let shell_environment_policy = ShellEnvironmentPolicy::default(); let output = run_script_with_timeout( &shell, &script, + &shell_environment_policy, Duration::from_secs(2), /*use_login_shell*/ true, &home, @@ -409,10 +467,12 @@ async fn timed_out_snapshot_shell_is_terminated() -> Result<()> { shell_type: ShellType::Sh, shell_path: PathBuf::from("/bin/sh"), }; + let shell_environment_policy = ShellEnvironmentPolicy::default(); let err = run_script_with_timeout( &shell, &script, + &shell_environment_policy, Duration::from_secs(1), /*use_login_shell*/ true, &dir.path().abs(), diff --git a/codex-rs/protocol/src/shell_environment.rs b/codex-rs/protocol/src/shell_environment.rs index 591ee43398..ce8a167dfc 100644 --- a/codex-rs/protocol/src/shell_environment.rs +++ b/codex-rs/protocol/src/shell_environment.rs @@ -43,6 +43,15 @@ where env_map } +/// Returns whether the policy explicitly opts into inheriting `RUST_LOG`. +pub fn policy_explicitly_includes_inherited_rust_log(policy: &ShellEnvironmentPolicy) -> bool { + !policy.include_only.is_empty() + && policy + .include_only + .iter() + .any(|pattern| pattern.matches("RUST_LOG")) +} + pub fn populate_env( vars: I, policy: &ShellEnvironmentPolicy, @@ -79,9 +88,7 @@ where // 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 { + if !policy_explicitly_includes_inherited_rust_log(policy) { env_map.retain(|key, _| !is_rust_log(key)); }