From faab4d39e18465f6111b9ba5b2ee0dbbae600bd2 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 3 Apr 2026 12:07:07 -0700 Subject: [PATCH] fix: preserve platform-specific core shell env vars (#16707) ## Why We were seeing failures in the following tests as part of trying to get all the tests running under Bazel on Windows in CI (https://github.com/openai/codex/pull/16528): ``` suite::shell_command::unicode_output::with_login suite::shell_command::unicode_output::without_login ``` Certainly `PATHEXT` should have been included in the extra `CORE_VARS` list, so we fix that up here, but also take things a step further for now by forcibly ensuring it is set on Windows in the return value of `create_env()`. Once we get the Windows Bazel build working reliably (i.e., after #16528 is merged), we should come back to this and confirm we can remove the special case in `create_env()`. ## What - Split core env inheritance into `COMMON_CORE_VARS` plus platform-specific allowlists for Windows and Unix in [`exec_env.rs`](https://github.com/openai/codex/blob/1b55c88fbf585b32cd553cb9d02ec817f2ad6ebc/codex-rs/core/src/exec_env.rs#L45-L81). - Preserve `PATHEXT`, `USERNAME`, and `USERPROFILE` on Windows, and `HOME` / locale vars on Unix. - Backfill a default `PATHEXT` in `create_env()` on Windows if the parent env does not provide one, so child process launch still works in stripped-down Bazel environments. - Extend the Windows exec-env test to assert mixed-case `PathExt` survives case-insensitive core filtering, and document why the shell-command Unicode test goes through a child process. ## Verification - `cargo test -p codex-core exec_env::tests` --- codex-rs/core/src/exec_env.rs | 52 +++++++++++++++++++--- codex-rs/core/src/exec_env_tests.rs | 44 ++++++++++++++++++ codex-rs/core/tests/suite/shell_command.rs | 8 +++- 3 files changed, 95 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/exec_env.rs b/codex-rs/core/src/exec_env.rs index 4dd5532d5..3feb531ea 100644 --- a/codex-rs/core/src/exec_env.rs +++ b/codex-rs/core/src/exec_env.rs @@ -21,9 +21,46 @@ pub fn create_env( policy: &ShellEnvironmentPolicy, thread_id: Option, ) -> HashMap { - populate_env(std::env::vars(), policy, thread_id) + create_env_from_vars(std::env::vars(), policy, thread_id) } +fn create_env_from_vars( + vars: I, + policy: &ShellEnvironmentPolicy, + thread_id: Option, +) -> HashMap +where + I: IntoIterator, +{ + let mut env_map = populate_env(vars, policy, thread_id); + + if cfg!(target_os = "windows") { + // This is a workaround to address the failures we are seeing in the + // following tests when run via Bazel on Windows: + // + // ``` + // suite::shell_command::unicode_output::with_login + // suite::shell_command::unicode_output::without_login + // ``` + // + // Currently, we can only reproduce these failures in CI, which makes + // iteration times long, so we include this quick fix for now to unblock + // getting the Windows Bazel build running. + if !env_map.keys().any(|k| k.eq_ignore_ascii_case("PATHEXT")) { + env_map.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); + } + } + env_map +} + +const COMMON_CORE_VARS: &[&str] = &["PATH", "SHELL", "TMPDIR", "TEMP", "TMP"]; + +#[cfg(target_os = "windows")] +const PLATFORM_CORE_VARS: &[&str] = { &["PATHEXT", "USERNAME", "USERPROFILE"] }; + +#[cfg(unix)] +const PLATFORM_CORE_VARS: &[&str] = &["HOME", "LANG", "LC_ALL", "LC_CTYPE", "LOGNAME", "USER"]; + fn populate_env( vars: I, policy: &ShellEnvironmentPolicy, @@ -38,17 +75,18 @@ where ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(), ShellEnvironmentPolicyInherit::None => HashMap::new(), ShellEnvironmentPolicyInherit::Core => { - const CORE_VARS: &[&str] = &[ - "HOME", "LOGNAME", "PATH", "SHELL", "USER", "USERNAME", "TMPDIR", "TEMP", "TMP", - ]; - let allow: HashSet<&str> = CORE_VARS.iter().copied().collect(); + let core_vars: HashSet<&str> = COMMON_CORE_VARS + .iter() + .copied() + .chain(PLATFORM_CORE_VARS.iter().copied()) + .collect(); let is_core_var = |name: &str| { if cfg!(target_os = "windows") { - CORE_VARS + core_vars .iter() .any(|allowed| allowed.eq_ignore_ascii_case(name)) } else { - allow.contains(name) + core_vars.contains(name) } }; vars.into_iter().filter(|(k, _)| is_core_var(k)).collect() diff --git a/codex-rs/core/src/exec_env_tests.rs b/codex-rs/core/src/exec_env_tests.rs index 1b486834e..81b5c0bb3 100644 --- a/codex-rs/core/src/exec_env_tests.rs +++ b/codex-rs/core/src/exec_env_tests.rs @@ -1,6 +1,7 @@ use super::*; use codex_config::types::ShellEnvironmentPolicyInherit; use maplit::hashmap; +use pretty_assertions::assert_eq; fn make_vars(pairs: &[(&str, &str)]) -> Vec<(String, String)> { pairs @@ -171,6 +172,7 @@ fn test_inherit_all_with_default_excludes() { fn test_core_inherit_respects_case_insensitive_names_on_windows() { let vars = make_vars(&[ ("Path", "C:\\Windows\\System32"), + ("PathExt", ".COM;.EXE;.BAT;.CMD"), ("TEMP", "C:\\Temp"), ("FOO", "bar"), ]); @@ -185,6 +187,7 @@ fn test_core_inherit_respects_case_insensitive_names_on_windows() { let result = populate_env(vars, &policy, Some(thread_id)); let mut expected: HashMap = hashmap! { "Path".to_string() => "C:\\Windows\\System32".to_string(), + "PathExt".to_string() => ".COM;.EXE;.BAT;.CMD".to_string(), "TEMP".to_string() => "C:\\Temp".to_string(), }; expected.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string()); @@ -192,6 +195,47 @@ fn test_core_inherit_respects_case_insensitive_names_on_windows() { assert_eq!(result, expected); } +#[test] +#[cfg(target_os = "windows")] +fn create_env_inserts_pathext_on_windows_when_missing() { + let vars = make_vars(&[]); + + let policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::None, + ignore_default_excludes: true, + ..Default::default() + }; + + let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); + + let expected: HashMap = hashmap! { + "PATHEXT".to_string() => ".COM;.EXE;.BAT;.CMD".to_string(), + }; + assert_eq!(result, expected); +} + +#[test] +#[cfg(target_os = "windows")] +fn create_env_preserves_existing_pathext_case_insensitively_on_windows() { + let vars = make_vars(&[("PathExt", ".COM;.EXE;.BAT;.CMD;.PS1")]); + + let policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::Core, + ignore_default_excludes: true, + ..Default::default() + }; + + let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); + + let pathext_vars = result + .iter() + .filter(|(key, _)| key.eq_ignore_ascii_case("PATHEXT")) + .collect::>(); + + assert_eq!(pathext_vars.len(), 1); + assert_eq!(pathext_vars[0].1, ".COM;.EXE;.BAT;.CMD;.PS1"); +} + #[test] fn test_inherit_none() { let vars = make_vars(&[("PATH", "/usr/bin"), ("HOME", "/home")]); diff --git a/codex-rs/core/tests/suite/shell_command.rs b/codex-rs/core/tests/suite/shell_command.rs index 705a06a4c..8734c3e69 100644 --- a/codex-rs/core/tests/suite/shell_command.rs +++ b/codex-rs/core/tests/suite/shell_command.rs @@ -256,6 +256,9 @@ async fn shell_command_times_out_with_timeout_ms() -> anyhow::Result<()> { Ok(()) } +/// This test verifies that a shell, particularly PowerShell, can correctly +/// handle unicode output when the UTF-8 BOM is used. See +/// https://github.com/openai/codex/pull/7902 for more context. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(true ; "with_login")] #[test_case(false ; "without_login")] @@ -264,10 +267,11 @@ async fn unicode_output(login: bool) -> anyhow::Result<()> { let harness = shell_command_harness_with(|builder| builder.with_model("gpt-5.2")).await?; - // We use a child process on windows instead of a direct builtin like 'echo' to ensure that Powershell - // config is actually being set correctly. let call_id = "unicode_output"; let command = if cfg!(windows) { + // We use a child process on Windows instead of a PowerShell command + // like `Write-Output` to ensure that the Powershell config is set + // correctly. "cmd.exe /c echo naïve_café" } else { "echo \"naïve_café\""