diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 72b30a76..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,7 +191,18 @@ 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 (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 dd8fc375..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,7 +23,7 @@ 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), + shell: options.shell ?? (process.platform === "win32" ? resolveWindowsShell() : false), windowsHide: true });