mirror of
https://github.com/openai/codex.git
synced 2026-09-04 15:08:45 +00:00
Honor the user's shell env policy for RUST_LOG
Signed-off-by: William Woodruff <ww@openai.com>
This commit is contained in:
@@ -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(),
|
||||
)
|
||||
|
||||
@@ -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<StateDbHandle>,
|
||||
}
|
||||
@@ -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<StateDbHandle>,
|
||||
) -> 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<StateDbHandle>,
|
||||
) -> std::result::Result<ShellSnapshotFile, &'static str> {
|
||||
// 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<String> {
|
||||
async fn capture_snapshot(
|
||||
shell: &Shell,
|
||||
cwd: &AbsolutePathBuf,
|
||||
shell_environment_policy: &ShellEnvironmentPolicy,
|
||||
) -> Result<String> {
|
||||
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<String> {
|
||||
async fn run_shell_script(
|
||||
shell: &Shell,
|
||||
script: &str,
|
||||
cwd: &AbsolutePathBuf,
|
||||
shell_environment_policy: &ShellEnvironmentPolicy,
|
||||
) -> Result<String> {
|
||||
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");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<String> {
|
||||
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(),
|
||||
|
||||
@@ -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<I>(
|
||||
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));
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user