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..e8df4a6f 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,7 +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 = resolveShellOption(options.shell); const result = spawnSync(command, args, { cwd: options.cwd, env: options.env, @@ -9,7 +24,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 +79,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); +});