From 673a46fb6064b096e7d2edc6f51da20be1a096a3 Mon Sep 17 00:00:00 2001 From: Honey Tyagi Date: Fri, 3 Jul 2026 22:58:00 +0530 Subject: [PATCH 1/2] fix: cancel dead pids on non-English Windows (#423) terminateProcessTree() threw when taskkill reported the target process was already gone on non-English Windows, because the localized stderr never matched the English-only "not found" patterns (and is mojibake once a non-UTF-8 console codepage is decoded as utf8). That crash happened before handleCancel() persisted the cancelled state, so the job stayed "running" forever and blocked --resume-last. Changes: - Treat taskkill exit code 128 as process-not-found in the win32 branch. The exit code is the only locale-independent signal; the message check stays as a fallback. - Invoke taskkill with shell:false so MSYS/Git-Bash argument conversion no longer mangles "/PID", and to avoid the DEP0190 warning. runCommand now accepts a shell override. - Persist the cancelled state even if terminateProcessTree() throws, so abandoning a job can never leave the state machine stuck. - Add tests for the exit-128/mojibake case and the shell:false invocation. --- plugins/codex/scripts/codex-companion.mjs | 12 ++++++- plugins/codex/scripts/lib/process.mjs | 20 +++++++++-- tests/process.test.mjs | 44 +++++++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468a..2b9d37f7 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -983,7 +983,17 @@ async function handleCancel(argv) { ); } - terminateProcessTree(job.pid ?? Number.NaN); + // Termination failures must not leave the job stuck as "running": the user is + // abandoning this job, so persist the cancelled state even if the (possibly + // already-dead) worker cannot be terminated. + try { + terminateProcessTree(job.pid ?? Number.NaN); + } catch (error) { + appendLogLine( + job.logFile, + `Process termination failed during cancel${error?.message ? `: ${error.message}` : "."}` + ); + } appendLogLine(job.logFile, "Cancelled by user."); const completedAt = nowIso(); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index af28d1cf..e1455233 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -2,6 +2,12 @@ import { spawnSync } from "node:child_process"; import process from "node:process"; export function runCommand(command, args = [], options = {}) { + const shell = + options.shell !== undefined + ? options.shell + : process.platform === "win32" + ? process.env.SHELL || true + : false; const result = spawnSync(command, args, { cwd: options.cwd, env: options.env, @@ -9,7 +15,7 @@ export function runCommand(command, args = [], options = {}) { input: options.input, maxBuffer: options.maxBuffer, stdio: options.stdio ?? "pipe", - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + shell, windowsHide: true }); @@ -64,15 +70,25 @@ export function terminateProcessTree(pid, options = {}) { const killImpl = options.killImpl ?? process.kill.bind(process); if (platform === "win32") { + // taskkill needs no shell; forcing shell:false avoids MSYS/Git-Bash argument + // conversion mangling "/PID" into a path, and sidesteps the DEP0190 warning. const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], { cwd: options.cwd, - env: options.env + env: options.env, + shell: false }); if (!result.error && result.status === 0) { return { attempted: true, delivered: true, method: "taskkill", result }; } + // taskkill exits 128 when the target process does not exist. Its stderr is + // localized (and becomes mojibake when a non-UTF-8 console codepage is + // decoded as utf8), so the exit code is the only locale-independent signal. + if (!result.error && result.status === 128) { + return { attempted: true, delivered: false, method: "taskkill", result }; + } + const combinedOutput = `${result.stderr}\n${result.stdout}`.trim(); if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) { return { attempted: true, delivered: false, method: "taskkill", result }; diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b..ac59873e 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -53,3 +53,47 @@ test("terminateProcessTree treats missing Windows processes as already stopped", assert.equal(outcome.result.status, 128); assert.match(outcome.result.stdout, /not found/i); }); + +test("terminateProcessTree treats exit 128 as not-found even with localized/mojibake output", () => { + // cp950 rendering of a localized "process not found" message decoded as utf8. + const outcome = terminateProcessTree(36564, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 128, + signal: null, + stdout: "", + stderr: "\uFFFD\uFFFD: \uFFFD\uFFFD\uFFFD\uFFFD\uFFFD", + error: null + }; + } + }); + + assert.equal(outcome.attempted, true); + assert.equal(outcome.delivered, false); + assert.equal(outcome.method, "taskkill"); + assert.equal(outcome.result.status, 128); +}); + +test("terminateProcessTree invokes taskkill without a shell", () => { + let capturedOptions = null; + terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args, options) { + capturedOptions = options; + return { + command, + args, + status: 0, + signal: null, + stdout: "", + stderr: "", + error: null + }; + } + }); + + assert.equal(capturedOptions.shell, false); +}); From 23a9f27618d11da1cf2ab13d2d800448e94a54a3 Mon Sep 17 00:00:00 2001 From: Honey Tyagi Date: Sat, 4 Jul 2026 12:30:31 +0530 Subject: [PATCH 2/2] fix: normalize and validate runCommand shell option Normalize null/undefined shell to the platform default and reject non-boolean/non-string values with a clear TypeError, instead of forwarding them to spawnSync and triggering ERR_INVALID_ARG_TYPE. Addresses Copilot review on #424. --- plugins/codex/scripts/lib/process.mjs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index e1455233..e8df4a6f 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,13 +1,22 @@ import { spawnSync } from "node:child_process"; import process from "node:process"; +function resolveShellOption(shell) { + // Normalize null/undefined to the platform default so callers can pass + // through optional values without tripping spawnSync's ERR_INVALID_ARG_TYPE. + if (shell === undefined || shell === null) { + return process.platform === "win32" ? process.env.SHELL || true : false; + } + if (typeof shell !== "boolean" && typeof shell !== "string") { + throw new TypeError( + `runCommand "shell" option must be a boolean or string, received ${typeof shell}` + ); + } + return shell; +} + export function runCommand(command, args = [], options = {}) { - const shell = - options.shell !== undefined - ? options.shell - : process.platform === "win32" - ? process.env.SHELL || true - : false; + const shell = resolveShellOption(options.shell); const result = spawnSync(command, args, { cwd: options.cwd, env: options.env,