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é\""