mirror of
https://github.com/openai/codex.git
synced 2026-09-14 11:57:03 +00:00
Fix Windows PowerShell parser stdin
Restore the parser subprocess stdin behavior that Command::output() provided so safe PowerShell wrapper commands do not time out on Windows, and add the argument-name comments required by the stale lint failure. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -71,6 +71,8 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron
|
||||
- Add a test-only `user_shell_override` seam through `ThreadManager`, `SessionConfiguration`, and `TestCodexBuilder` so the affected integration tests can pin `cmd.exe` on Windows without mutating live session state after startup.
|
||||
- Use that override in the Windows-flaky `apply_patch_cli` and websocket shell-chain tests, while still executing the file-read command under `powershell.exe` inside `cmd.exe` where the scenario needs PowerShell output rather than PowerShell process startup.
|
||||
- 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.
|
||||
- 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
|
||||
@@ -109,3 +111,5 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron
|
||||
| `5399ca70d` | Record validation pass 1 | failed | Run `23086309217` failed only in `Tests — windows-x64 - x86_64-pc-windows-msvc`. Nextest timed out in `codex-core exec_policy::tests::verify_approval_requirement_for_unsafe_powershell_command` after `30.015s`, while the rest of the matrix and all Bazel shards passed on the same SHA. The follow-up bounds the PowerShell AST parser subprocess to `5s` so stalled startup no longer hangs the shard. |
|
||||
| `83e7ab58a` | Bound Windows PowerShell parser hangs | no PR CI | GitHub created only the `CLA Assistant` check suite for this SHA and never enqueued any `pull_request` workflows after repeated Actions API polls by `head_sha`. The next commit retriggers the PR synchronize event without changing the flaky-test fix. |
|
||||
| `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. |
|
||||
|
||||
@@ -381,7 +381,7 @@ impl ThreadManager {
|
||||
persist_extended_history,
|
||||
metrics_service_name,
|
||||
parent_trace,
|
||||
None,
|
||||
/*user_shell_override*/ None,
|
||||
))
|
||||
.await
|
||||
}
|
||||
@@ -421,7 +421,7 @@ impl ThreadManager {
|
||||
persist_extended_history,
|
||||
/*metrics_service_name*/ None,
|
||||
parent_trace,
|
||||
None,
|
||||
/*user_shell_override*/ None,
|
||||
))
|
||||
.await
|
||||
}
|
||||
@@ -437,9 +437,9 @@ impl ThreadManager {
|
||||
Arc::clone(&self.state.auth_manager),
|
||||
self.agent_control(),
|
||||
Vec::new(),
|
||||
false,
|
||||
None,
|
||||
None,
|
||||
/*persist_extended_history*/ false,
|
||||
/*metrics_service_name*/ None,
|
||||
/*parent_trace*/ None,
|
||||
Some(user_shell_override),
|
||||
))
|
||||
.await
|
||||
@@ -459,9 +459,9 @@ impl ThreadManager {
|
||||
auth_manager,
|
||||
self.agent_control(),
|
||||
Vec::new(),
|
||||
false,
|
||||
None,
|
||||
None,
|
||||
/*persist_extended_history*/ false,
|
||||
/*metrics_service_name*/ None,
|
||||
/*parent_trace*/ None,
|
||||
Some(user_shell_override),
|
||||
))
|
||||
.await
|
||||
@@ -548,7 +548,7 @@ impl ThreadManager {
|
||||
persist_extended_history,
|
||||
/*metrics_service_name*/ None,
|
||||
parent_trace,
|
||||
None,
|
||||
/*user_shell_override*/ None,
|
||||
))
|
||||
.await
|
||||
}
|
||||
|
||||
@@ -144,6 +144,9 @@ fn parse_with_powershell_ast(executable: &str, script: &str) -> PowershellParseO
|
||||
encoded_parser_script,
|
||||
])
|
||||
.env("CODEX_POWERSHELL_PAYLOAD", &encoded_script)
|
||||
// Match `Command::output()` here so PowerShell does not stay alive waiting on inherited
|
||||
// stdin after the parser script has already finished.
|
||||
.stdin(Stdio::null())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped())
|
||||
.spawn()
|
||||
|
||||
Reference in New Issue
Block a user