fix: use a deterministic Windows shell for broker spawns (#236)#484
Open
nategarelik wants to merge 2 commits into
Open
fix: use a deterministic Windows shell for broker spawns (#236)#484nategarelik wants to merge 2 commits into
nategarelik wants to merge 2 commits into
Conversation
openai#236 reports the app-server broker hanging indefinitely at "Initializing..." on Windows with no error surfaced. The direct spawn path (used by the broker itself to launch `codex app-server`, and by runCommand for utilities like taskkill) wraps the child process with process.env.SHELL on win32, trusting whatever shell the user happens to have configured (git-bash, a WSL bash.exe shim, an unresolvable literal POSIX path, etc.) to correctly proxy a JSON-RPC stdio pipe. That is unpredictable: it can silently swallow the handshake, fail with ENOENT, or otherwise misbehave depending on what SHELL resolves to, and the broker's own cleanup logic already assumed the spawned child was cmd.exe (see the terminateProcessTree comment in app-server.mjs), which breaks when SHELL points elsewhere. process.env.SHELL was added in openai#178 to fix openai#138, but that fix targeted the wrong layer: the outer spawn wrapper only resolves the codex executable itself (npm installs ship a .cmd shim that CreateProcess cannot exec directly) and has no bearing on what shell Codex uses internally for its own command-execution tool, since SHELL is already passed straight through via the `env` option regardless of this wrapper. Reverting to a deterministic cmd.exe wrapper on Windows fixes the hang without reopening openai#138. Verified: baseline test suite has 6 pre-existing failures including "cancel sends turn interrupt to the shared app-server before killing a brokered task" (a taskkill/terminateProcessTree test whose process tree assumptions broke exactly the way described above); with this fix that test passes and no other test regresses (5 failures remain, all pre-existing and unrelated: platform-mismatch path assertions, npm/setup env assumptions, Claude-session-transfer path assumptions). A live headless round trip against `codex app-server` (broker start, initialize, thread/start, turn/start, turn/completed, clean shutdown) also passes end to end.
Cross-model adversarial review of 8b49e99 found a remaining gap: on Windows, `shell: true` doesn't invoke cmd.exe directly -- Node falls back to process.env.ComSpec (defaulting to cmd.exe only when ComSpec is unset). A nonstandard ComSpec (pointed at PowerShell, a shell shim, etc.) can therefore reproduce the exact openai#236 hang through ComSpec that 8b49e99 already fixed for SHELL. Add resolveWindowsShell() in process.mjs, which joins SystemRoot (set by the OS itself, not a dev-tooling convention like SHELL/ComSpec) with System32\cmd.exe, and use it at both spawn sites instead of the plain `shell: true` boolean. Neither SHELL nor ComSpec can redirect the wrapper now. runCommand's explicit `options.shell` override is preserved. Verified: full test suite matches the previous fix's pass/fail split (the two extra failures seen during one run were load-induced flakes under heavy concurrent-agent CPU contention on this machine -- 300+ node.exe processes at the time -- confirmed by re-running both in isolation, where they pass cleanly). Live headless round trip against codex app-server (thread/start, turn/start, turn/completed, clean shutdown) passes end to end with this build.
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows hangs when spawning codex app-server by making the spawn wrapper’s shell selection deterministic (avoiding arbitrary SHELL / ComSpec values that can wrap or break the stdio JSON-RPC pipe).
Changes:
- Introduces
resolveWindowsShell()to deterministically select%SystemRoot%\System32\cmd.exefor Windows shell-wrapped spawns. - Updates
runCommand()(spawnSync) to use the deterministic Windows shell by default (while preserving explicitoptions.shelloverrides). - Updates the direct
codex app-serverspawn to use the deterministic Windows shell wrapper instead ofprocess.env.SHELL || true.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/codex/scripts/lib/process.mjs | Adds resolveWindowsShell() and uses it as the default Windows shell for runCommand(). |
| plugins/codex/scripts/lib/app-server.mjs | Switches the Windows codex app-server spawn wrapper to use resolveWindowsShell() and documents the rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+13
to
+16
| export function resolveWindowsShell() { | ||
| const systemRoot = process.env.SystemRoot || "C:\\Windows"; | ||
| return path.join(systemRoot, "System32", "cmd.exe"); | ||
| } |
Comment on lines
+5
to
+16
| // Resolve the Windows command interpreter deterministically. Passing `shell: true` on | ||
| // Windows would otherwise let Node fall back to process.env.ComSpec, and this project | ||
| // intentionally never consults process.env.SHELL either -- both are arbitrary, | ||
| // user-configurable env vars that are not a safe or predictable wrapper for spawning a | ||
| // subprocess on Windows (#236: a nonstandard SHELL or ComSpec can silently break the | ||
| // spawn wrapper for a JSON-RPC stdio pipe). SystemRoot is set by the OS itself, so | ||
| // resolving System32\cmd.exe through it keeps this wrapper deterministic regardless of | ||
| // either env var. See app-server.mjs for the codex app-server spawn that shares this. | ||
| export function resolveWindowsShell() { | ||
| const systemRoot = process.env.SystemRoot || "C:\\Windows"; | ||
| return path.join(systemRoot, "System32", "cmd.exe"); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #236 (broker hangs at "Initializing..." on Windows).
app-server.mjsspawnscodex app-serverwithshell: process.env.SHELL || trueon win32 (andlib/process.mjsrunCommandhas the same pattern). An arbitrary, unvalidatedSHELL(git-bash, a WSL shim, an untranslated POSIX literal like/usr/bin/bash) then wraps the JSON-RPC stdio pipe. Depending on the value this yields ENOENT or a wrapped pipe, and it silently breaksterminateProcessTree, whose cleanup logic assumes the direct child is cmd.exe.Regression chain
shell: process.platform === "win32"(deterministic) to fix ENOENT on npm.cmdshims.process.env.SHELL || true. The outer spawnshelloption only resolves the codex executable; it has no bearing on the shell Codex uses internally for its command tool (SHELLalready flows throughenvpassthrough), so Codex review hangs indefinitely on Windows with Git Bash shell #138's actual concern was unaffected by this option — but the change exposed the wrapper to arbitrarySHELLvalues.shell: trueis itself not deterministic: Node resolves it viaprocess.env.ComSpec, so a nonstandard ComSpec (PowerShell, git-bash) reproduces the hang through a second env var.Fix (2 commits)
SHELLfor the spawn wrapper (revert to platform-boolean).resolveWindowsShell()returns%SystemRoot%\System32\cmd.exe(SystemRoot is kernel-set, not a dev-tooling convention like SHELL/ComSpec). Explicitoptions.shelloverrides inrunCommandare preserved; non-win32 behavior unchanged.SHELLstill reaches Codex via env passthrough, so internal shell selection (Codex review hangs indefinitely on Windows with Git Bash shell #138) is unaffected.Verification
SHELLunset / POSIX literal / translated path) demonstrated the failure modes before the fix.cancel sends turn interrupt to the shared app-server before killing a brokered task(taskkill/terminateProcessTree path) flips to passing; no other test status changes.ensureBrokerSession-> broker ->codex app-server-> initialize -> thread/start -> turn/start -> turn/completed -> clean shutdown, no orphaned processes.