From 6a302cb2e30078c7af804d8e23e83a52c5dcd437 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim <219906144+aibrahim-oai@users.noreply.github.com> Date: Tue, 17 Mar 2026 02:55:21 +0000 Subject: [PATCH] Fix cmd-wrapped PowerShell test quoting Keep the Windows cmd.exe harness for apply_patch_cli, but encode the nested PowerShell read so cmd does not pass the script through as a quoted string literal. This preserves the test coverage while making the file-read path deterministic again. Update the flaky-test ledger with the c14493260 CI failure and the new quoting diagnosis. Co-authored-by: Codex --- .codex/flaky-test-triage.md | 4 ++++ codex-rs/core/tests/suite/apply_patch_cli.rs | 15 +++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.codex/flaky-test-triage.md b/.codex/flaky-test-triage.md index 1dca8cea6b..8543bee41c 100644 --- a/.codex/flaky-test-triage.md +++ b/.codex/flaky-test-triage.md @@ -73,6 +73,9 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron - Remove the live PowerShell executable discovery from `core/src/tools/handlers/shell_tests.rs`; the PowerShell wrapper safety coverage already lives in `codex-shell-command`, and this core handler regression test only needs to verify that derived commands still match `is_known_safe_command`. - Current follow-up: local reproduction of run `23172240730` with `./tools/argument-comment-lint/run.sh` showed the new test-only `spawn_thread` wrappers still violated the argument comment lint. The fix only adds explicit argument-name comments on the literal `false`/`None` values for `persist_extended_history`, `metrics_service_name`, `parent_trace`, and `user_shell_override`; it does not change runtime behavior. - The same stale `23172240730` run later exposed two branch-local Windows x64 failures in `codex-shell-command::command_safety::windows_safe_commands`: `recognizes_safe_powershell_wrappers` and `accepts_full_path_powershell_invocations`. The timeout refactor had switched from `Command::output()` to `spawn()` without also closing stdin, so `powershell.exe` could remain alive waiting on inherited stdin until the new `5s` timeout fired. The follow-up restores the `output()` stdin behavior with `Stdio::null()` while keeping the timeout cap. + - Run `23173027099` on head `c14493260` cleared the stale lint and PowerShell-parser regressions, but both Windows `Tests` shards failed in the same `codex-core` case: `all::suite::apply_patch_cli::apply_patch_cli_can_use_shell_command_output_as_patch_input`. + - CI annotations from check runs `67329068119` and `67329068114` showed the produced `target.txt` started with the literal `Get-Content -Encoding utf8 source.txt` instead of the source file contents. The branch-local `cmd.exe` harness change had wrapped the nested PowerShell read in `-Command "..."`, so `cmd.exe /c` passed the quotes through and PowerShell evaluated the payload as a string literal. + - The next follow-up keeps the `cmd.exe` harness but switches the nested read to a UTF-8 `-EncodedCommand` script, removing the extra quoting layer without changing the test's product coverage. - Rationale: these failures are test-harness flakes, not product behaviors. The fixes keep the assertions intact and remove environment-sensitive startup and ordering hazards instead of stretching timeouts. ## Constraints @@ -113,3 +116,4 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron | `9f21509ab` | Retrigger PR CI after dropped event | no PR CI | GitHub again created only the `CLA Assistant` suite for this SHA. The PR simultaneously flipped to `mergeable: false`, `rebaseable: false`, `mergeable_state: dirty`, so the next follow-up merges `origin/main` into the branch to clear the conflict and restore a clean PR head before retrying the full CI loop. | | `a2b78a660` | Stabilize Windows shell test harnesses | no PR CI | GitHub again created only the `CLA Assistant` suite for this SHA, and the PR was simultaneously `dirty`/`conflicting` after `main` advanced. No `pull_request` workflows ran on this head, so the next follow-up merged `origin/main` to restore a clean synchronize event. | | `27654cc6d` | Merge `origin/main` into flaky-test branch | failed | Run `23172240730` exposed a branch-local `Argument comment lint` failure in the new test-only `spawn_thread` wrappers. Local reproduction with `./tools/argument-comment-lint/run.sh` showed those wrappers needed explicit argument comments for `persist_extended_history`, `metrics_service_name`, `parent_trace`, and `user_shell_override`. The same stale run later surfaced two Windows x64 shell-command regressions, `recognizes_safe_powershell_wrappers` and `accepts_full_path_powershell_invocations`, caused by the PowerShell parser timeout refactor inheriting stdin instead of matching `Command::output()`. `Bazel (experimental)` run `23172240729` also failed on the two Linux shards on this stale SHA, so the next follow-up combines the lint cleanup with the minimal stdin fix and retriggers the full matrix. | +| `c14493260` | Fix Windows PowerShell parser stdin | failed | Run `23173027099` cleared the stale lint and `codex-shell-command` parser regressions, and `Bazel (experimental)` run `23173027042` finished green. The only remaining failures were `Tests — windows-x64 - x86_64-pc-windows-msvc` and `Tests — windows-arm64 - aarch64-pc-windows-msvc`, both in `all::suite::apply_patch_cli::apply_patch_cli_can_use_shell_command_output_as_patch_input`. CI annotations showed the `cmd.exe`-wrapped nested `powershell.exe -Command "Get-Content ..."` invocation printed the command string literal instead of reading the file, so the next follow-up replaces that nested command with a UTF-8 `-EncodedCommand` script. | diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 761c2c4b0e..eea315db90 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -1,6 +1,8 @@ #![allow(clippy::expect_used)] use anyhow::Result; +use base64::Engine; +use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use codex_test_macros::large_stack_test; use core_test_support::responses::ev_apply_patch_call; use core_test_support::responses::ev_apply_patch_custom_tool_call; @@ -788,9 +790,18 @@ async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result match call_num { 0 => { let command = if cfg!(windows) { - r#"powershell.exe -NoLogo -NoProfile -Command "Get-Content -Encoding utf8 source.txt""# + // Encode the nested PowerShell script so `cmd.exe /c` does not leave the + // read command wrapped in quotes and turn it into a printed string literal. + let script = "[Console]::OutputEncoding = [System.Text.UTF8Encoding]::new($false); Get-Content -Encoding utf8 source.txt"; + let encoded = BASE64_STANDARD.encode( + script + .encode_utf16() + .flat_map(u16::to_le_bytes) + .collect::>(), + ); + format!("powershell.exe -NoLogo -NoProfile -EncodedCommand {encoded}") } else { - "cat source.txt" + "cat source.txt".to_string() }; let args = json!({ "command": command,