From 8b49e9913ccf63efaae67f32086fa31b991ef8b2 Mon Sep 17 00:00:00 2001 From: nategarelik Date: Sat, 11 Jul 2026 19:02:52 -0500 Subject: [PATCH 1/2] fix: stop wrapping Windows spawns in process.env.SHELL #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 #178 to fix #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 #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. --- plugins/codex/scripts/lib/app-server.mjs | 10 +++++++++- plugins/codex/scripts/lib/process.mjs | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 72b30a76..4dd59b2c 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -191,7 +191,15 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { cwd: this.cwd, env: this.options.env ?? process.env, stdio: ["pipe", "pipe", "pipe"], - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + // Always use the deterministic Windows shell (cmd.exe) to wrap this spawn, never + // process.env.SHELL. This wrapper only resolves the codex executable (npm installs + // ship a .cmd shim that CreateProcess cannot exec directly); it has no bearing on + // what shell Codex uses internally for its own command-execution tool, since SHELL + // is passed through via `env` above regardless of this option. Trusting an arbitrary + // user-configured SHELL here (git-bash, WSL's bash.exe shim, an unresolvable literal + // path, etc.) to wrap a JSON-RPC stdio pipe is unpredictable and can hang the + // handshake indefinitely with no surfaced error (#236). See also #138/#178. + shell: process.platform === "win32", windowsHide: true }); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc375..0fa6fcac 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -9,7 +9,10 @@ export function runCommand(command, args = [], options = {}) { input: options.input, maxBuffer: options.maxBuffer, stdio: options.stdio ?? "pipe", - shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false), + // See app-server.mjs for why this stays off process.env.SHELL: an arbitrary + // user-configured shell is not a safe or predictable wrapper for spawning + // subprocesses on Windows (#236). + shell: options.shell ?? process.platform === "win32", windowsHide: true }); From 31090c056bb84f68edead339835aaee582a89819 Mon Sep 17 00:00:00 2001 From: nategarelik Date: Sat, 11 Jul 2026 19:15:45 -0500 Subject: [PATCH 2/2] fix: resolve cmd.exe via SystemRoot instead of trusting ComSpec 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 #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. --- plugins/codex/scripts/lib/app-server.mjs | 23 +++++++++++++---------- plugins/codex/scripts/lib/process.mjs | 19 +++++++++++++++---- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 4dd59b2c..80c60342 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -14,7 +14,7 @@ import { spawn } from "node:child_process"; import readline from "node:readline"; import { parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { ensureBrokerSession, loadBrokerSession } from "./broker-lifecycle.mjs"; -import { terminateProcessTree } from "./process.mjs"; +import { resolveWindowsShell, terminateProcessTree } from "./process.mjs"; const PLUGIN_MANIFEST_URL = new URL("../../.claude-plugin/plugin.json", import.meta.url); const PLUGIN_MANIFEST = JSON.parse(fs.readFileSync(PLUGIN_MANIFEST_URL, "utf8")); @@ -191,15 +191,18 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { cwd: this.cwd, env: this.options.env ?? process.env, stdio: ["pipe", "pipe", "pipe"], - // Always use the deterministic Windows shell (cmd.exe) to wrap this spawn, never - // process.env.SHELL. This wrapper only resolves the codex executable (npm installs - // ship a .cmd shim that CreateProcess cannot exec directly); it has no bearing on - // what shell Codex uses internally for its own command-execution tool, since SHELL - // is passed through via `env` above regardless of this option. Trusting an arbitrary - // user-configured SHELL here (git-bash, WSL's bash.exe shim, an unresolvable literal - // path, etc.) to wrap a JSON-RPC stdio pipe is unpredictable and can hang the - // handshake indefinitely with no surfaced error (#236). See also #138/#178. - shell: process.platform === "win32", + // Always use the deterministic Windows shell (System32\cmd.exe, resolved via + // resolveWindowsShell) to wrap this spawn, never process.env.SHELL or + // process.env.ComSpec (plain `shell: true` falls back to the latter). This wrapper + // only resolves the codex executable (npm installs ship a .cmd shim that + // CreateProcess cannot exec directly); it has no bearing on what shell Codex uses + // internally for its own command-execution tool, since SHELL is passed through via + // `env` above regardless of this option. Trusting an arbitrary user-configured + // SHELL or ComSpec here (git-bash, WSL's bash.exe shim, an unresolvable literal + // path, a non-cmd.exe ComSpec, etc.) to wrap a JSON-RPC stdio pipe is unpredictable + // and can hang the handshake indefinitely with no surfaced error (#236). See also + // #138/#178. + shell: process.platform === "win32" ? resolveWindowsShell() : false, windowsHide: true }); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 0fa6fcac..409c732d 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,6 +1,20 @@ import { spawnSync } from "node:child_process"; +import path from "node:path"; import process from "node:process"; +// 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"); +} + export function runCommand(command, args = [], options = {}) { const result = spawnSync(command, args, { cwd: options.cwd, @@ -9,10 +23,7 @@ export function runCommand(command, args = [], options = {}) { input: options.input, maxBuffer: options.maxBuffer, stdio: options.stdio ?? "pipe", - // See app-server.mjs for why this stays off process.env.SHELL: an arbitrary - // user-configured shell is not a safe or predictable wrapper for spawning - // subprocesses on Windows (#236). - shell: options.shell ?? process.platform === "win32", + shell: options.shell ?? (process.platform === "win32" ? resolveWindowsShell() : false), windowsHide: true });