openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-04 15:08:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address PR review feedback (#28463)

Browse Source
This commit is contained in:
Adam Perry
2026-06-16 17:00:56 +00:00
parent 47a38cdde3
commit 95433c7a93
6 changed files with 238 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
bazel/rules/testing/wine/BUILD.bazel
View File

@@ -39,6 +39,7 @@ rust_binary(
deps = [
":wine_test_support",
"@crates//:anyhow",
"@crates//:tokio",
],
)

38
bazel/rules/testing/wine/src/exec.rs
View File

@@ -1,17 +1,39 @@
use anyhow::Context;
use anyhow::Result;
use std::ffi::OsStr;
use std::io::Write;
fn main() -> Result<()> {
#[tokio::main]
async fn main() -> Result<()> {
let mut args = std::env::args_os().skip(1);
let executable = args
.next()
.context("usage: wine-test-exec <windows-executable> [args...]")?;
let status = wine_test_support::ambient_wine_command(executable)?
.args(args)
.status()
.context("run Windows command in shared Wine test prefix")?;
if !status.success() {
std::process::exit(status.code().unwrap_or(1));
.context("usage: wine-test-exec [--powershell | <windows-executable>] [args...]")?;
if executable.as_os_str() == OsStr::new("--powershell") {
let args = args
.map(|arg| {
arg.into_string()
.map_err(|arg| anyhow::anyhow!("PowerShell argument is not UTF-8: {arg:?}"))
})
.collect::<Result<Vec<_>>>()?;
let output = wine_test_support::run_ambient_powershell(&args).await?;
let mut stdout = std::io::stdout().lock();
stdout.write_all(&output.stdout)?;
stdout.flush()?;
let mut stderr = std::io::stderr().lock();
stderr.write_all(&output.stderr)?;
stderr.flush()?;
if output.exit_code != 0 {
std::process::exit(output.exit_code);
}
} else {
let status = wine_test_support::ambient_wine_command(executable)?
.args(args)
.status()
.context("run Windows command in shared Wine test prefix")?;
if !status.success() {
std::process::exit(status.code().unwrap_or(1));
}
}
Ok(())
}

128
bazel/rules/testing/wine/src/lib.rs
View File

@@ -1,6 +1,7 @@
#[cfg(not(target_os = "linux"))]
compile_error!("wine_test_support can only run on Linux");
use std::collections::HashMap;
use std::ffi::OsStr;
use std::ffi::OsString;
use std::fs;
@@ -14,6 +15,8 @@ use std::time::Duration;
use anyhow::Context;
use anyhow::Result;
use codex_utils_pty::SpawnedProcess;
use codex_utils_pty::TerminalSize;
use tempfile::TempDir;
use tokio::process::Child;
use tokio::process::ChildStdout;
@@ -47,6 +50,17 @@ pub struct WineTestCommandContext {
prefix: PathBuf,
}
/// Captured output and exit status from a Wine test command.
#[derive(Debug, PartialEq, Eq)]
pub struct WineCommandOutput {
/// Bytes captured from standard output.
pub stdout: Vec<u8>,
/// Bytes captured from standard error.
pub stderr: Vec<u8>,
/// Process exit code reported by the PTY backend.
pub exit_code: i32,
}
struct WineProcesses {
child: Child,
cleanup_complete: bool,
@@ -56,6 +70,7 @@ struct WineProcesses {
struct WineRuntimePaths {
dll_path: PathBuf,
powershell_executable: PathBuf,
powershell_runtime: PathBuf,
wine: PathBuf,
wineserver: PathBuf,
@@ -180,17 +195,118 @@ impl WineTestCommandContext {
/// Builds a Wine command that joins the prefix exported by [`WineTestCommandContext`].
pub fn ambient_wine_command(executable: impl AsRef<OsStr>) -> Result<StdCommand> {
let prefix = PathBuf::from(
std::env::var_os(WINE_TEST_PREFIX_ENV_VAR)
.with_context(|| format!("{WINE_TEST_PREFIX_ENV_VAR} must be set"))?,
);
let runtime = WineRuntimePaths::from_runfiles()?;
ambient_wine_command_with_runtime(executable, &runtime)
}
/// Runs pinned PowerShell in the exported Wine test prefix.
///
/// PowerShell runs through a PTY because it is a Windows console application
/// under Wine. The returned output is complete when this function returns.
pub async fn run_ambient_powershell(args: &[String]) -> Result<WineCommandOutput> {
let prefix = ambient_wine_prefix()?;
let runtime = WineRuntimePaths::from_runfiles()?;
let env = wine_pty_environment(&runtime, &prefix);
let mut powershell_args = Vec::with_capacity(args.len() + 1);
powershell_args.push(runtime.powershell_executable.to_string_lossy().into_owned());
powershell_args.extend_from_slice(args);
let spawned = codex_utils_pty::spawn_pty_process(
runtime.wine.to_string_lossy().as_ref(),
&powershell_args,
&prefix,
&env,
/*arg0*/ &None,
TerminalSize::default(),
)
.await?;
collect_wine_command_output(spawned).await
}
async fn collect_wine_command_output(spawned: SpawnedProcess) -> Result<WineCommandOutput> {
let SpawnedProcess {
session,
mut stdout_rx,
mut stderr_rx,
exit_rx,
} = spawned;
let stdout = async {
let mut output = Vec::new();
while let Some(chunk) = stdout_rx.recv().await {
output.extend(chunk);
}
output
};
let stderr = async {
let mut output = Vec::new();
while let Some(chunk) = stderr_rx.recv().await {
output.extend(chunk);
}
output
};
let (stdout, stderr, exit_code) = tokio::join!(stdout, stderr, exit_rx);
drop(session);
Ok(WineCommandOutput {
stdout,
stderr,
exit_code: exit_code.context("wait for PowerShell")?,
})
}
fn wine_pty_environment(runtime: &WineRuntimePaths, prefix: &Path) -> HashMap<String, String> {
let mut env = std::env::vars().collect::<HashMap<_, _>>();
env.remove("DISPLAY");
env.extend([
("HOME".to_string(), prefix.to_string_lossy().into_owned()),
(
"XDG_RUNTIME_DIR".to_string(),
prefix.to_string_lossy().into_owned(),
),
("WINEARCH".to_string(), "win64".to_string()),
(
"WINEPREFIX".to_string(),
prefix.to_string_lossy().into_owned(),
),
(
"WINEDLLPATH".to_string(),
runtime.dll_path.to_string_lossy().into_owned(),
),
(
"WINESERVER".to_string(),
runtime.wineserver.to_string_lossy().into_owned(),
),
("WINEDEBUG".to_string(), "-all".to_string()),
(
"WINEDLLOVERRIDES".to_string(),
"mscoree,mshtml,winegstreamer=".to_string(),
),
("LANG".to_string(), "C.UTF-8".to_string()),
("LC_ALL".to_string(), "C.UTF-8".to_string()),
("LC_CTYPE".to_string(), "C.UTF-8".to_string()),
("TEMP".to_string(), r"C:\windows\temp".to_string()),
("TMP".to_string(), r"C:\windows\temp".to_string()),
]);
env
}
fn ambient_wine_command_with_runtime(
executable: impl AsRef<OsStr>,
runtime: &WineRuntimePaths,
) -> Result<StdCommand> {
let executable = executable.as_ref();
let prefix = ambient_wine_prefix()?;
let mut command = StdCommand::new(&runtime.wine);
configure_wine_environment(&mut command, &runtime, &prefix);
configure_wine_environment(&mut command, runtime, &prefix);
command.arg(executable);
Ok(command)
}
fn ambient_wine_prefix() -> Result<PathBuf> {
Ok(PathBuf::from(
std::env::var_os(WINE_TEST_PREFIX_ENV_VAR)
.with_context(|| format!("{WINE_TEST_PREFIX_ENV_VAR} must be set"))?,
))
}
impl Drop for WineTestProcess {
fn drop(&mut self) {
// Panicking here starts unwinding, after which WineProcesses performs
@@ -210,12 +326,14 @@ impl WineRuntimePaths {
.context("locate Wine runtime directory")?
.to_path_buf();
let wineserver = codex_utils_cargo_bin::cargo_bin("wineserver")?;
let powershell_executable = codex_utils_cargo_bin::cargo_bin("pwsh")?;
let powershell_runtime = codex_utils_cargo_bin::cargo_bin("pwsh-runtime-marker")?
.parent()
.context("locate PowerShell runtime directory")?
.to_path_buf();
Ok(Self {
dll_path,
powershell_executable,
powershell_runtime,
wine,
wineserver,

125
bazel/rules/testing/wine/src/lib_tests.rs
View File

@@ -1,7 +1,6 @@
use std::any::Any;
use std::collections::HashMap;
use std::future::Future;
use std::fs;
use std::future::Future;
use std::panic::AssertUnwindSafe;
use std::path::Path;
use std::path::PathBuf;
@@ -10,7 +9,6 @@ use std::time::Duration;
use anyhow::Context;
use anyhow::Result;
use anyhow::anyhow;
use codex_utils_pty::SpawnedProcess;
use codex_utils_pty::TerminalSize;
use futures::FutureExt;
use pretty_assertions::assert_eq;
@@ -20,10 +18,13 @@ use tokio::io::BufReader;
use tokio::process::Command as TokioCommand;
use tokio::time::timeout;
use super::WineCommandOutput;
use super::WineRuntimePaths;
use super::WineTestCommand;
use super::WineTestProcess;
use super::WineRuntimePaths;
use super::collect_wine_command_output;
use super::install_powershell_runtime;
use super::wine_pty_environment;
async fn waiting_smoke_process() -> Result<WineTestProcess> {
let executable = codex_utils_cargo_bin::cargo_bin("wine-smoke")?;
@@ -149,18 +150,21 @@ async fn scope_returns_value_and_tears_down() -> Result<()> {
}
#[tokio::test]
async fn exported_command_context_reuses_active_prefix() -> Result<()> {
async fn exported_command_context_runs_commands_in_active_prefix() -> Result<()> {
let process = waiting_smoke_process().await?;
let prefix = prefix_path(&process);
let prefix_after_scope = prefix.clone();
let marker = prefix.join("drive_c").join("shared-prefix-marker");
let powershell_marker = prefix
.join("drive_c")
.join("shared-powershell-prefix-marker");
let context = process.command_context();
let wrapper = codex_utils_cargo_bin::cargo_bin("wine-test-exec")?;
let smoke = codex_utils_cargo_bin::cargo_bin("wine-smoke")?;
process
.scope(async move {
let mut command = TokioCommand::new(wrapper);
let mut command = TokioCommand::new(&wrapper);
context.apply_to_command(&mut command);
let output = command
.arg(smoke)
@@ -175,6 +179,37 @@ async fn exported_command_context_reuses_active_prefix() -> Result<()> {
String::from_utf8_lossy(&output.stderr).trim(),
);
assert_eq!(fs::read(&marker)?, b"shared prefix");
let mut command = TokioCommand::new(wrapper);
context.apply_to_command(&mut command);
let output = command
.args([
"--powershell",
"-NoLogo",
"-NoProfile",
"-NonInteractive",
"-Command",
r#"[System.IO.File]::WriteAllText('C:\shared-powershell-prefix-marker', 'shared powershell prefix')"#,
])
.output()
.await
.context("run PowerShell through exported Wine test context")?;
anyhow::ensure!(
output.status.success(),
"shared-prefix PowerShell command failed: stdout={} stderr={}",
String::from_utf8_lossy(&output.stdout).trim(),
String::from_utf8_lossy(&output.stderr).trim(),
);
assert_eq!(
fs::read(&powershell_marker).with_context(|| {
format!(
"read PowerShell marker; stdout={} stderr={}",
String::from_utf8_lossy(&output.stdout).trim(),
String::from_utf8_lossy(&output.stderr).trim(),
)
})?,
b"shared powershell prefix"
);
Ok(())
})
.await?;
@@ -345,41 +380,9 @@ async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> {
);
let runtime = WineRuntimePaths::from_runfiles()?;
let prefix = TempDir::new()?;
install_powershell_runtime(prefix.path(), &runtime.powershell_runtime)?;
let mut env = std::env::vars().collect::<HashMap<_, _>>();
env.remove("DISPLAY");
env.extend([
("HOME".to_string(), prefix.path().to_string_lossy().into_owned()),
(
"XDG_RUNTIME_DIR".to_string(),
prefix.path().to_string_lossy().into_owned(),
),
("WINEARCH".to_string(), "win64".to_string()),
(
"WINEPREFIX".to_string(),
prefix.path().to_string_lossy().into_owned(),
),
(
"WINEDLLPATH".to_string(),
runtime.dll_path.to_string_lossy().into_owned(),
),
(
"WINESERVER".to_string(),
runtime.wineserver.to_string_lossy().into_owned(),
),
("WINEDEBUG".to_string(), "-all".to_string()),
(
"WINEDLLOVERRIDES".to_string(),
"mscoree,mshtml,winegstreamer=".to_string(),
),
("LANG".to_string(), "C.UTF-8".to_string()),
("LC_ALL".to_string(), "C.UTF-8".to_string()),
("LC_CTYPE".to_string(), "C.UTF-8".to_string()),
("TEMP".to_string(), r"C:\windows\temp".to_string()),
("TMP".to_string(), r"C:\windows\temp".to_string()),
]);
let env = wine_pty_environment(&runtime, prefix.path());
let args = [
r"C:\Program Files\PowerShell\7\pwsh.exe".to_string(),
runtime.powershell_executable.to_string_lossy().into_owned(),
"-NoLogo".to_string(),
"-NoProfile".to_string(),
"-NonInteractive".to_string(),
@@ -387,12 +390,7 @@ async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> {
POWERSHELL_SMOKE_SCRIPT.to_string(),
];
let wine = runtime.wine.to_string_lossy().into_owned();
let SpawnedProcess {
session,
mut stdout_rx,
mut stderr_rx,
exit_rx,
} = codex_utils_pty::spawn_pty_process(
let spawned = codex_utils_pty::spawn_pty_process(
&wine,
&args,
prefix.path(),
@@ -401,28 +399,13 @@ async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> {
TerminalSize::default(),
)
.await?;
let command_result = timeout(Duration::from_secs(30), async {
let stdout = async {
let mut output = Vec::new();
while let Some(chunk) = stdout_rx.recv().await {
output.extend(chunk);
}
output
};
let stderr = async {
let mut output = Vec::new();
while let Some(chunk) = stderr_rx.recv().await {
output.extend(chunk);
}
output
};
let (stdout, stderr, exit_code) = tokio::join!(stdout, stderr, exit_rx);
Ok::<_, anyhow::Error>((stdout, stderr, exit_code.context("wait for PowerShell")?))
})
let command_result = timeout(
Duration::from_secs(30),
collect_wine_command_output(spawned),
)
.await
.context("PowerShell smoke test timed out")
.and_then(std::convert::identity);
drop(session);
let shutdown_result = timeout(Duration::from_secs(10), async {
let mut command = TokioCommand::new(&runtime.wineserver);
command
@@ -441,7 +424,11 @@ async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> {
.await
.context("stop isolated wineserver timed out")
.and_then(std::convert::identity);
let (stdout, stderr, exit_code) = match (command_result, shutdown_result) {
let WineCommandOutput {
stdout,
stderr,
exit_code,
} = match (command_result, shutdown_result) {
(Ok(output), Ok(())) => output,
(Err(error), Ok(())) => return Err(error),
(Ok(_), Err(error)) => return Err(error),
@@ -465,7 +452,11 @@ async fn pinned_powershell_runs_under_wine_with_a_pty() -> Result<()> {
.context("PowerShell smoke marker line was incomplete")?
.trim_end_matches('\r');
let fields = smoke.split('|').collect::<Vec<_>>();
assert_eq!(fields.len(), 5, "unexpected PowerShell smoke output: {smoke}");
assert_eq!(
fields.len(),
5,
"unexpected PowerShell smoke output: {smoke}"
);
assert_eq!(fields[0], POWERSHELL_SMOKE_MARKER);
assert_eq!(
fields[1].split('.').next(),

3
bazel/rules/testing/wine/wine.bzl
View File

@@ -22,7 +22,8 @@ def wine_rust_test(
`CARGO_BIN_EXE_<binary_name>` for its executable.
* `CARGO_BIN_EXE_wine` and `CARGO_BIN_EXE_wineserver` identify Wine tools.
* `CARGO_BIN_EXE_wine-test-exec` identifies the host wrapper for running an
additional Windows command in an exported test prefix.
additional Windows command in an exported test prefix. Pass
`--powershell` as its first argument to use the pinned PowerShell runtime.
* `CARGO_BIN_EXE_wine-runtime-marker` identifies a file whose parent is the
Wine DLL directory to use as `WINEDLLPATH`.
* `CARGO_BIN_EXE_pwsh` identifies the pinned PowerShell executable and

31
codex-rs/core/tests/suite/remote_env.rs
View File

@@ -28,6 +28,8 @@ use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_protocol::request_permissions::RequestPermissionsResponse;
use codex_protocol::user_input::UserInput;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::ApiPathString;
use codex_utils_path_uri::PathConvention;
use codex_utils_path_uri::PathUri;
use core_test_support::PathBufExt;
use core_test_support::PathExt;
@@ -298,7 +300,7 @@ fn remote_exec_wine_powershell(command: &str) -> Result<()> {
let wrapper = codex_utils_cargo_bin::cargo_bin("wine-test-exec")?;
let output = Command::new(wrapper)
.args([
r"C:\Program Files\PowerShell\7\pwsh.exe",
"--powershell",
"-NoLogo",
"-NoProfile",
"-NonInteractive",
@@ -1080,6 +1082,15 @@ async fn remote_test_env_sandboxed_read_rejects_symlink_parent_dotdot_escape() -
let outside_dir_uri = PathUri::from_abs_path(&outside_dir);
let secret_path_uri = PathUri::from_abs_path(&secret_path);
let symlink_path_uri = allowed_dir_uri.join("link")?;
let windows_root = ApiPathString::from_path_uri(&root_uri, PathConvention::Windows)?;
let windows_allowed_dir =
ApiPathString::from_path_uri(&allowed_dir_uri, PathConvention::Windows)?;
let windows_outside_dir =
ApiPathString::from_path_uri(&outside_dir_uri, PathConvention::Windows)?;
let windows_secret_path =
ApiPathString::from_path_uri(&secret_path_uri, PathConvention::Windows)?;
let windows_symlink_path =
ApiPathString::from_path_uri(&symlink_path_uri, PathConvention::Windows)?;
let linux_setup_command = format!(
"rm -rf {root}; mkdir -p {allowed} {outside}; printf nope > {secret}; ln -s {outside} {allowed}/link",
root = root.display(),
@@ -1089,16 +1100,21 @@ async fn remote_test_env_sandboxed_read_rejects_symlink_parent_dotdot_escape() -
);
let windows_setup_command = format!(
r#"$ErrorActionPreference = 'Stop'
$root = ([Uri]'{root_uri}').LocalPath
$allowed = ([Uri]'{allowed_dir_uri}').LocalPath
$outside = ([Uri]'{outside_dir_uri}').LocalPath
$secret = ([Uri]'{secret_path_uri}').LocalPath
$link = ([Uri]'{symlink_path_uri}').LocalPath
$root = '{root}'
$allowed = '{allowed}'
$outside = '{outside}'
$secret = '{secret}'
$link = '{link}'
Remove-Item -LiteralPath $root -Recurse -Force -ErrorAction SilentlyContinue
New-Item -ItemType Directory -Path $allowed -Force | Out-Null
New-Item -ItemType Directory -Path $outside -Force | Out-Null
[System.IO.File]::WriteAllText($secret, 'nope')
New-Item -ItemType SymbolicLink -Path $link -Target $outside | Out-Null"#,
root = windows_root.as_str(),
allowed = windows_allowed_dir.as_str(),
outside = windows_outside_dir.as_str(),
secret = windows_secret_path.as_str(),
link = windows_symlink_path.as_str(),
);
remote_exec(&linux_setup_command, &windows_setup_command)?;
@@ -1113,8 +1129,9 @@ New-Item -ItemType SymbolicLink -Path $link -Target $outside | Out-Null"#,
let linux_cleanup_command = format!("rm -rf {}", root.display());
let windows_cleanup_command = format!(
r#"$ErrorActionPreference = 'Stop'
$root = ([Uri]'{root_uri}').LocalPath
$root = '{root}'
Remove-Item -LiteralPath $root -Recurse -Force -ErrorAction SilentlyContinue"#,
root = windows_root.as_str(),
);
remote_exec(&linux_cleanup_command, &windows_cleanup_command)?;
Ok(())