From 95433c7a93d9eb9e4dfd06dde163797eb260728b Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 16 Jun 2026 17:00:56 +0000 Subject: [PATCH] codex: address PR review feedback (#28463) --- bazel/rules/testing/wine/BUILD.bazel | 1 + bazel/rules/testing/wine/src/exec.rs | 38 +++++-- bazel/rules/testing/wine/src/lib.rs | 128 +++++++++++++++++++++- bazel/rules/testing/wine/src/lib_tests.rs | 125 ++++++++++----------- bazel/rules/testing/wine/wine.bzl | 3 +- codex-rs/core/tests/suite/remote_env.rs | 31 ++++-- 6 files changed, 238 insertions(+), 88 deletions(-) diff --git a/bazel/rules/testing/wine/BUILD.bazel b/bazel/rules/testing/wine/BUILD.bazel index 1f5d523a08..ed306ba661 100644 --- a/bazel/rules/testing/wine/BUILD.bazel +++ b/bazel/rules/testing/wine/BUILD.bazel @@ -39,6 +39,7 @@ rust_binary( deps = [ ":wine_test_support", "@crates//:anyhow", + "@crates//:tokio", ], ) diff --git a/bazel/rules/testing/wine/src/exec.rs b/bazel/rules/testing/wine/src/exec.rs index 8caa468c3e..39f7e8532f 100644 --- a/bazel/rules/testing/wine/src/exec.rs +++ b/bazel/rules/testing/wine/src/exec.rs @@ -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 [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 | ] [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::>>()?; + 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(()) } diff --git a/bazel/rules/testing/wine/src/lib.rs b/bazel/rules/testing/wine/src/lib.rs index 5fb706e5e5..264ca3fdf7 100644 --- a/bazel/rules/testing/wine/src/lib.rs +++ b/bazel/rules/testing/wine/src/lib.rs @@ -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, + /// Bytes captured from standard error. + pub stderr: Vec, + /// 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) -> Result { - 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 { + 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 { + 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 { + let mut env = std::env::vars().collect::>(); + 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, + runtime: &WineRuntimePaths, +) -> Result { + 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 { + 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, diff --git a/bazel/rules/testing/wine/src/lib_tests.rs b/bazel/rules/testing/wine/src/lib_tests.rs index 03cfeb92a9..e3241b53fa 100644 --- a/bazel/rules/testing/wine/src/lib_tests.rs +++ b/bazel/rules/testing/wine/src/lib_tests.rs @@ -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 { 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::>(); - 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::>(); - 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(), diff --git a/bazel/rules/testing/wine/wine.bzl b/bazel/rules/testing/wine/wine.bzl index 4030e2c3c2..d39bdbe081 100644 --- a/bazel/rules/testing/wine/wine.bzl +++ b/bazel/rules/testing/wine/wine.bzl @@ -22,7 +22,8 @@ def wine_rust_test( `CARGO_BIN_EXE_` 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 diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 628e9a40d1..538cf1d3c5 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -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(())