mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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`
This commit is contained in:
committed by
GitHub
Unverified
parent
0ab8eda375
commit
faab4d39e1
@@ -21,9 +21,46 @@ pub fn create_env(
|
||||
policy: &ShellEnvironmentPolicy,
|
||||
thread_id: Option<ThreadId>,
|
||||
) -> HashMap<String, String> {
|
||||
populate_env(std::env::vars(), policy, thread_id)
|
||||
create_env_from_vars(std::env::vars(), policy, thread_id)
|
||||
}
|
||||
|
||||
fn create_env_from_vars<I>(
|
||||
vars: I,
|
||||
policy: &ShellEnvironmentPolicy,
|
||||
thread_id: Option<ThreadId>,
|
||||
) -> HashMap<String, String>
|
||||
where
|
||||
I: IntoIterator<Item = (String, String)>,
|
||||
{
|
||||
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<I>(
|
||||
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()
|
||||
|
||||
@@ -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<String, String> = 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<String, String> = 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::<Vec<_>>();
|
||||
|
||||
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")]);
|
||||
|
||||
@@ -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é\""
|
||||
|
||||
Reference in New Issue
Block a user