From c79ac1c1335ae99e23cde8173534c10af76bc66d Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Tue, 18 Aug 2026 00:18:44 +0200 Subject: [PATCH 1/9] fix: Windows SHELL env var breaks taskkill; handleCancel aborts before updating job state on a partial kill failure Bug 1: runCommand and SpawnedCodexAppServerClient.initialize both consulted process.env.SHELL when deciding the shell: option on Windows. SHELL is a POSIX convention with no meaning for native Windows process creation -- on a machine where it's set to a POSIX shell path (e.g. Git Bash, which Claude Code's own Bash tool sets), spawn/spawnSync routes commands through that shell instead of cmd.exe, and MSYS's automatic POSIX-path conversion mangles Windows-style flags like taskkill's /PID. Fixed by always using true on win32, never consulting SHELL. Bug 2: terminateProcessTree throws whenever taskkill exits non-zero for a reason other than "process not found". handleCancel called it unguarded, so a partial /T tree-kill failure on Windows aborted the whole cancel before the job's on-disk status was ever updated, leaving it stuck at running/finalizing forever even though the turn interrupt had already succeeded. Fixed by wrapping the call in try/catch and logging-but-continuing, matching terminateProcessTree's own best-effort semantics for the already-gone case. Fixes #647 --- plugins/codex/scripts/codex-companion.mjs | 14 +++++++++++- plugins/codex/scripts/lib/app-server.mjs | 5 +++- plugins/codex/scripts/lib/process.mjs | 7 +++++- tests/process.test.mjs | 28 +++++++++++++++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..d3ede4627 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -983,7 +983,19 @@ async function handleCancel(argv) { ); } - terminateProcessTree(job.pid ?? Number.NaN); + try { + terminateProcessTree(job.pid ?? Number.NaN); + } catch (error) { + // terminateProcessTree already treats "process already gone" as + // best-effort/non-fatal; a partial `taskkill /T` tree-kill failure + // (e.g. Windows refusing to kill a subset of grandchild processes) + // deserves the same treatment rather than aborting the cancel before + // the job's on-disk status is ever updated, leaving it stuck at + // "running"/"finalizing" forever even though the turn interrupt above + // already succeeded. + const detail = error instanceof Error ? error.message : String(error); + appendLogLine(job.logFile, `Process tree termination failed (continuing cancel): ${detail}`); + } appendLogLine(job.logFile, "Cancelled by user."); const completedAt = nowIso(); diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 72b30a764..b37daf0c3 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -191,7 +191,10 @@ 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, + // See the matching comment in lib/process.mjs's runCommand: `SHELL` is + // a POSIX convention and must not be consulted for native Windows + // process creation. + shell: process.platform === "win32" ? true : false, windowsHide: true }); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..6b489e5a3 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -9,7 +9,12 @@ 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), + // `process.env.SHELL` is a POSIX convention with no meaning for native + // Windows process creation; consulting it here routes commands through + // whatever POSIX shell happens to be set (e.g. Git Bash, which Claude + // Code's own Bash tool sets), which mangles Windows-style flags like + // `/PID` via MSYS's automatic POSIX-path conversion. + shell: options.shell ?? (process.platform === "win32" ? true : false), windowsHide: true }); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..6af8cbfb2 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -53,3 +53,31 @@ 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 throws on a genuine Windows taskkill failure, not just a missing-process one", () => { + // A partial `taskkill /T` tree-kill failure (Windows refusing to kill a + // subset of grandchild processes) does not match the "already gone" + // regex, so it must still surface as a thrown error here -- callers like + // handleCancel are responsible for deciding whether that's fatal to them, + // not terminateProcessTree itself. + assert.throws( + () => + terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 128, + signal: null, + stdout: "", + stderr: + "ERROR: The process with PID 25692 (child process of PID 27196) could not be terminated.\n" + + "Reason: This operation is not supported.", + error: null + }; + } + }), + /could not be terminated/i + ); +}); From 5d8496113589aa67ad45291a031d0a4d086c6a0b Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Tue, 25 Aug 2026 00:09:20 +0200 Subject: [PATCH 2/9] fix: do not report cancellation confirmed when neither interrupt nor termination succeeded handleCancel's catch around terminateProcessTree unconditionally continued and reported the job as cancelled, clearing its pid, even when the turn interrupt was also unavailable/failed. terminateProcessTree already treats "process already gone" as non-fatal without throwing, so reaching the catch means the outcome is genuinely unknown, not just "already stopped." In that case, with the interrupt also not confirmed, nothing had actually proven the worker stopped -- reporting cancelled and clearing pid would let a write-capable task keep modifying the workspace unsupervised, with its later completion able to overwrite the fabricated cancelled status. Extracted the decision (wasCancellationConfirmed) into job-control.mjs so it's directly unit-testable, since codex-companion.mjs's handlers aren't exported and forcing terminateProcessTree to genuinely throw via a real subprocess integration test isn't reliably engineerable. When neither path confirms the stop, the job's status/pid are left unchanged and the command throws instead of reporting a cancellation that may not have happened. Found via Codex Review on the PR. --- plugins/codex/scripts/codex-companion.mjs | 29 +++++++++++++++++------ plugins/codex/scripts/lib/job-control.mjs | 19 +++++++++++++++ tests/job-control.test.mjs | 26 ++++++++++++++++++++ 3 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 tests/job-control.test.mjs diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index d3ede4627..980414a09 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -40,7 +40,8 @@ import { readStoredJob, resolveCancelableJob, resolveResultJob, - sortJobsNewestFirst + sortJobsNewestFirst, + wasCancellationConfirmed } from "./lib/job-control.mjs"; import { appendLogLine, @@ -983,19 +984,33 @@ async function handleCancel(argv) { ); } + let terminationOutcomeKnown = true; try { terminateProcessTree(job.pid ?? Number.NaN); } catch (error) { // terminateProcessTree already treats "process already gone" as - // best-effort/non-fatal; a partial `taskkill /T` tree-kill failure - // (e.g. Windows refusing to kill a subset of grandchild processes) - // deserves the same treatment rather than aborting the cancel before - // the job's on-disk status is ever updated, leaving it stuck at - // "running"/"finalizing" forever even though the turn interrupt above - // already succeeded. + // best-effort/non-fatal (it doesn't throw for that case), so reaching + // this catch means the outcome is genuinely unknown -- e.g. a partial + // `taskkill /T` tree-kill failure (Windows refusing to kill a subset of + // grandchild processes). That's still not fatal to the cancel attempt + // itself when the turn interrupt above already succeeded (Codex has + // already stopped acting on this turn either way), but if the interrupt + // *also* didn't succeed, nothing here has actually proven the worker + // stopped -- reporting "cancelled" and clearing pid in that case would + // let a write-capable task keep modifying the workspace unsupervised, + // and its later completion could overwrite the fabricated cancelled + // status. const detail = error instanceof Error ? error.message : String(error); appendLogLine(job.logFile, `Process tree termination failed (continuing cancel): ${detail}`); + terminationOutcomeKnown = false; } + + if (!wasCancellationConfirmed(interrupt, terminationOutcomeKnown)) { + throw new Error( + `Could not confirm job ${job.id} was stopped: the turn interrupt did not succeed and process tree termination failed. The job's status was left unchanged rather than reporting a cancellation that may not have happened.` + ); + } + appendLogLine(job.logFile, "Cancelled by user."); const completedAt = nowIso(); diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index ad152c157..a2a5a646f 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -306,3 +306,22 @@ export function resolveCancelableJob(cwd, reference, options = {}) { throw new Error("No active Codex jobs to cancel."); } + +/** + * Whether a cancel attempt actually stopped the job's work, and it's safe + * to record the job as cancelled and clear its pid. + * + * A successful turn interrupt is sufficient on its own -- Codex has already + * stopped acting on this turn regardless of what process-tree termination + * does afterward. Otherwise, termination must have completed without an + * unexpected throw: terminateProcessTree() already treats "process already + * gone" as non-fatal without throwing, so a throw here means the outcome is + * genuinely unknown, not just "already stopped." Reporting cancellation + * confirmed in that case (interrupt didn't succeed, and termination outcome + * is unknown) would let a write-capable task keep modifying the workspace + * unsupervised, with its later completion able to overwrite the fabricated + * cancelled status. + */ +export function wasCancellationConfirmed(interrupt, terminationOutcomeKnown) { + return Boolean(interrupt?.interrupted) || Boolean(terminationOutcomeKnown); +} diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs new file mode 100644 index 000000000..eb9de1aae --- /dev/null +++ b/tests/job-control.test.mjs @@ -0,0 +1,26 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { wasCancellationConfirmed } from "../plugins/codex/scripts/lib/job-control.mjs"; + +// Regression tests for a P1 finding on PR #656: handleCancel unconditionally +// reported a job as cancelled even when neither the turn interrupt nor +// process-tree termination could confirm the worker actually stopped. + +test("wasCancellationConfirmed is true when the turn interrupt succeeded, regardless of termination outcome", () => { + assert.equal(wasCancellationConfirmed({ interrupted: true }, false), true); + assert.equal(wasCancellationConfirmed({ interrupted: true }, true), true); +}); + +test("wasCancellationConfirmed is true when termination completed without throwing, even if the interrupt did not succeed", () => { + assert.equal(wasCancellationConfirmed({ interrupted: false }, true), true); +}); + +test("wasCancellationConfirmed is false when neither the interrupt succeeded nor termination's outcome is known", () => { + assert.equal(wasCancellationConfirmed({ interrupted: false }, false), false); +}); + +test("wasCancellationConfirmed treats a missing interrupt result as not interrupted", () => { + assert.equal(wasCancellationConfirmed(null, false), false); + assert.equal(wasCancellationConfirmed(undefined, true), true); +}); From d7480fc95178e1bd5c986f3e63b9262ab59579b8 Mon Sep 17 00:00:00 2001 From: Verso Labs Date: Fri, 4 Sep 2026 06:21:30 -0300 Subject: [PATCH 3/9] fix: avoid shell true for Windows process launches --- plugins/codex/scripts/lib/app-server.mjs | 7 ++-- plugins/codex/scripts/lib/process.mjs | 32 +++++++++++++-- tests/process.test.mjs | 51 ++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 10 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 72b30a764..2d7332005 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 { commandWithWindowsShim, 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")); @@ -187,11 +187,12 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { } async initialize() { - this.proc = spawn("codex", ["app-server"], { + const invocation = commandWithWindowsShim("codex", ["app-server"]); + this.proc = spawn(invocation.command, invocation.args, { cwd: this.cwd, env: this.options.env ?? process.env, stdio: ["pipe", "pipe", "pipe"], - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + shell: invocation.shell, windowsHide: true }); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..651f34a23 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -9,7 +9,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 ?? false, windowsHide: true }); @@ -35,8 +35,33 @@ export function runCommandChecked(command, args = [], options = {}) { return result; } +export function commandWithWindowsShim(command, args = [], options = {}) { + const platform = options.platform ?? process.platform; + if (platform !== "win32") { + return { command, args, shell: false }; + } + + return { + command: options.comspec ?? process.env.ComSpec ?? "cmd.exe", + args: ["/d", "/s", "/c", "call", command, ...args], + shell: false + }; +} + export function binaryAvailable(command, versionArgs = ["--version"], options = {}) { - const result = runCommand(command, versionArgs, options); + const runCommandImpl = options.runCommandImpl ?? runCommand; + let result; + + if (options.shell !== undefined) { + result = runCommandImpl(command, versionArgs, options); + } else { + const invocation = commandWithWindowsShim(command, versionArgs, options); + result = runCommandImpl(invocation.command, invocation.args, { + cwd: options.cwd, + env: options.env, + shell: invocation.shell + }); + } if (result.error && /** @type {NodeJS.ErrnoException} */ (result.error).code === "ENOENT") { return { available: false, detail: "not found" }; } @@ -66,7 +91,8 @@ export function terminateProcessTree(pid, options = {}) { if (platform === "win32") { 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) { diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..0388e9281 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,14 +1,56 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { binaryAvailable, commandWithWindowsShim, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; + + +test("commandWithWindowsShim avoids shell:true on Windows", () => { + assert.deepEqual( + commandWithWindowsShim("codex", ["app-server"], { + platform: "win32", + comspec: "C:\\Windows\\System32\\cmd.exe" + }), + { + command: "C:\\Windows\\System32\\cmd.exe", + args: ["/d", "/s", "/c", "call", "codex", "app-server"], + shell: false + } + ); +}); + +test("binaryAvailable uses cmd.exe explicitly for Windows command shims", () => { + let captured = null; + const outcome = binaryAvailable("npm", ["--version"], { + platform: "win32", + comspec: "C:\\Windows\\System32\\cmd.exe", + runCommandImpl(command, args, options) { + captured = { command, args, options }; + return { + command, + args, + status: 0, + signal: null, + stdout: "11.16.0\n", + stderr: "", + error: null + }; + } + }); + + assert.deepEqual(captured, { + command: "C:\\Windows\\System32\\cmd.exe", + args: ["/d", "/s", "/c", "call", "npm", "--version"], + options: { cwd: undefined, env: undefined, shell: false } + }); + assert.deepEqual(outcome, { available: true, detail: "11.16.0" }); +}); test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; const outcome = terminateProcessTree(1234, { platform: "win32", - runCommandImpl(command, args) { - captured = { command, args }; + runCommandImpl(command, args, options) { + captured = { command, args, options }; return { command, args, @@ -26,7 +68,8 @@ test("terminateProcessTree uses taskkill on Windows", () => { assert.deepEqual(captured, { command: "taskkill", - args: ["/PID", "1234", "/T", "/F"] + args: ["/PID", "1234", "/T", "/F"], + options: { cwd: undefined, env: undefined, shell: false } }); assert.equal(outcome.delivered, true); assert.equal(outcome.method, "taskkill"); From 2eadfabc9c90ee901e52f0f442bda279b793dd85 Mon Sep 17 00:00:00 2001 From: Verso Labs Date: Fri, 4 Sep 2026 13:14:40 -0300 Subject: [PATCH 4/9] fix: handle localized taskkill missing-process result --- plugins/codex/scripts/lib/process.mjs | 2 +- tests/process.test.mjs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 651f34a23..abcf13373 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -100,7 +100,7 @@ export function terminateProcessTree(pid, options = {}) { } const combinedOutput = `${result.stderr}\n${result.stdout}`.trim(); - if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) { + if (!result.error && (result.status === 128 || looksLikeMissingProcessMessage(combinedOutput))) { return { attempted: true, delivered: false, method: "taskkill", result }; } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 0388e9281..4748805fe 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -84,7 +84,7 @@ test("terminateProcessTree treats missing Windows processes as already stopped", args, status: 128, signal: null, - stdout: "ERROR: The process \"1234\" not found.", + stdout: "ERROR: no se encontró el proceso \"1234\".", stderr: "", error: null }; @@ -94,5 +94,5 @@ test("terminateProcessTree treats missing Windows processes as already stopped", assert.equal(outcome.attempted, true); assert.equal(outcome.method, "taskkill"); assert.equal(outcome.result.status, 128); - assert.match(outcome.result.stdout, /not found/i); + assert.equal(outcome.delivered, false); }); From 1670db7943df2d4bc71741a88846c7ab49fc51ee Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 22:09:32 +0000 Subject: [PATCH 5/9] fix(comments): the Windows tree kill is not about shell: true any more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both Windows branches in app-server.mjs justified the tree kill with "with shell: true the direct child is cmd.exe". Since #725 nothing is spawned through a shell, so that reasoning reads as obsolete — and the natural conclusion from it (drop the tree kill, there is no shell child) is wrong: `codex` is a .cmd shim, so commandWithWindowsShim() still puts a real cmd.exe between us and the app-server. Say that instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- plugins/codex/scripts/lib/app-server.mjs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 8b848bdb8..91961e0ee 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -278,9 +278,10 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { this.proc.stdin.end(); setTimeout(() => { if (this.proc && !this.proc.killed && this.proc.exitCode === null) { - // On Windows with shell: true, the direct child is cmd.exe. - // Use terminateProcessTree to kill the entire tree including - // the grandchild node process. + // On Windows the direct child is cmd.exe — `codex` is a .cmd shim, so + // commandWithWindowsShim() spawns `cmd.exe /d /s /c call codex …`. + // Killing that child alone would leave the app-server grandchild + // running, so tear down the whole tree. if (process.platform === "win32") { try { terminateProcessTree(this.proc.pid); @@ -310,9 +311,9 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { if (this.proc && !this.proc.killed) { try { if (process.platform === "win32") { - // With shell: true the direct child is cmd.exe; kill the whole - // tree so the codex app-server grandchild does not survive the - // timeout (mirrors the graceful close() path). + // Same as close(): on Windows the direct child is the cmd.exe that + // runs the `codex` shim, so the whole tree has to go or the + // app-server grandchild survives the timeout. terminateProcessTree(this.proc.pid); } else { this.proc.kill("SIGKILL"); From 5740e3018b39469dbc2200f4cc0112b01b1209a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 22:09:32 +0000 Subject: [PATCH 6/9] docs: say what running on Windows requires and what changed there Requirements now name Git Bash (the hooks launch scripts/run-node.sh through it) and spell out that nothing else goes through a shell, with the taskkill failure as the reason it matters. Differences From Upstream gains the two imports of this batch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/README.md b/README.md index 3534a799f..39b615a73 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,9 @@ say what bounds those windows and where the files are. - Usage will contribute to your Codex usage limits. [Learn more](https://developers.openai.com/codex/pricing). - **Node.js 18.18 or later.** - It does not have to be on the system PATH: the hooks resolve Node through `scripts/run-node.sh`, which also looks in nvm, fnm, asdf, mise, Volta and Homebrew toolchains, preferring one that ships `codex` alongside it. Set `CODEX_COMPANION_NODE` to an executable path to pin a specific one. +- **On Windows: Git Bash** (the shell Git for Windows installs), because the hooks run `scripts/run-node.sh` through it. + - Nothing else is spawned through a shell. `codex` and `npm` are `.cmd` shims, so they are invoked as an explicit `cmd.exe /d /s /c call`; everything else — `git`, `taskkill`, `powershell.exe`, the background worker — is spawned directly. That matters: routing through a POSIX shell makes MSYS rewrite Windows-style switches, which is why `taskkill /PID … /T /F` used to fail and background workers could not be stopped. + - The shared broker listens on a named pipe rather than a Unix socket, and has no filesystem artifact to clean up. ## Install @@ -457,6 +460,13 @@ Beyond the imports, this fork carries fixes for defects the imports themselves s - disabling the review gate is not outvoted by a stale enable left under another plugin-data root - `CLAUDE_ENV_FILE` is only ever appended to: it is shared with other plugins' hooks, and rewriting it dropped whatever they had just written +- on Windows nothing is spawned through the user's shell any more, so MSYS path conversion can no + longer mangle a switch like `taskkill /PID` — which had left background workers unkillable under + Git Bash ([#725](https://github.com/openai/codex-plugin-cc/pull/725), which also supersedes + [#735](https://github.com/openai/codex-plugin-cc/pull/735)) +- `/codex:cancel` exits non-zero when neither the turn interrupt nor the worker kill confirmed the + job stopped, instead of reporting a cancellation nothing proved + ([#656](https://github.com/openai/codex-plugin-cc/pull/656)) - the app-server typecheck (`npm run build`) passes Each of those came out of an adversarial review of the merges, re-run after every round of fixes; From b38a589e0e7d663e2b1a31f00972f7f457548cde Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 22:20:49 +0000 Subject: [PATCH 7/9] fix(windows): stop force-killing through a negative pid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `process.kill(-pid, "SIGKILL")` addresses a process group, which only POSIX has. On Windows a negative pid is an invalid handle: OpenProcess fails, the call throws, and the catch-and-retry underneath it quietly degrades to killing the single process — leaving the worker's app-server, and every MCP server under it, running. That is the leak the SessionEnd hook exists to prevent, and it happened on exactly the stragglers that had already ignored SIGTERM. forceKillProcessTree() makes the platform choice once: SIGKILL the group on POSIX (with the non-leader fallback), taskkill's own tree walk on Windows, where /F is already the hardest stop available. The SessionEnd hook and the escalation inside terminateProcessTreeAndExit() both go through it. A guard test keeps the rule: no plugin script calls process.kill() with a negative pid, and group signalling stays inside process.mjs's platform-guarded helpers, which take an injectable killImpl. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- plugins/codex/scripts/lib/process.mjs | 54 +++++++- .../codex/scripts/session-lifecycle-hook.mjs | 14 +- tests/process.test.mjs | 124 ++++++++++++++++++ 3 files changed, 177 insertions(+), 15 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 13f0b0920..914c7387a 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -420,6 +420,52 @@ export function processHasLaunchToken(pid, token, options = {}) { return commandLine.includes(marker) && commandLine.includes(token); } +/** + * Force-kill a process and everything under it, for callers that have already tried a graceful stop. + * + * POSIX has a harder signal to escalate to, and the process group addresses the descendants: + * SIGKILL the group, falling back to the process alone for a caller that never was a group leader. + * + * Windows has neither. There are no process groups — a negative pid is just an invalid handle, so + * `process.kill(-pid)` throws ESRCH and a naive catch-and-retry silently degrades to killing the + * direct process while its descendants (the app-server, and every MCP server under it) keep + * running. taskkill's own tree walk is the only way to reach them, and `/F` is already the hardest + * stop there is, so the Windows branch is the same call as the graceful one. + */ +/** + * @param {number} pid + * @param {{ platform?: string, killImpl?: Function, runCommandImpl?: Function }} [options] + * @returns {{ attempted: boolean, delivered: boolean, method: string | null }} + */ +export function forceKillProcessTree(pid, options = {}) { + if (!isValidPid(pid)) { + return { attempted: false, delivered: false, method: null }; + } + + if ((options.platform ?? process.platform) === "win32") { + try { + const outcome = terminateProcessTree(pid, options); + return { attempted: outcome.attempted, delivered: outcome.delivered, method: outcome.method }; + } catch { + // Teardown is best effort: a taskkill that fails outright must not throw at the caller. + return { attempted: true, delivered: false, method: "taskkill" }; + } + } + + const killImpl = options.killImpl ?? process.kill.bind(process); + try { + killImpl(-pid, "SIGKILL"); + return { attempted: true, delivered: true, method: "process-group" }; + } catch { + try { + killImpl(pid, "SIGKILL"); + return { attempted: true, delivered: true, method: "process" }; + } catch { + return { attempted: true, delivered: false, method: "process" }; + } + } +} + /** * Terminate a process tree and make sure it is actually gone. * @@ -450,11 +496,9 @@ export function terminateProcessTreeAndExit(pid, { graceMs = 5000, exitCode = 1, } catch { // Never let bookkeeping stop the kill. } - try { - process.kill(-pid, "SIGKILL"); - } catch { - // Nothing left in the group, or the caller was never its leader. - } + // Nothing left in the group, a caller that was never its leader, or a Windows tree that + // taskkill could not reach: none of them may stop us from exiting. + forceKillProcessTree(pid); process.exit(exitCode); }, graceMs); } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 114fcfc98..28e0b16d6 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -3,7 +3,7 @@ import fs from "node:fs"; import process from "node:process"; -import { isPidAlive, terminateProcessTree } from "./lib/process.mjs"; +import { forceKillProcessTree, isPidAlive, terminateProcessTree } from "./lib/process.mjs"; import { reconcileJobLiveness } from "./lib/job-control.mjs"; import { brokerIdleShutdownMs } from "./lib/lifecycle-limits.mjs"; import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; @@ -50,15 +50,9 @@ async function waitForWorkerExits(pids) { return []; } for (const pid of stragglers) { - try { - process.kill(-pid, "SIGKILL"); - } catch { - try { - process.kill(pid, "SIGKILL"); - } catch { - // Ignore missing process. - } - } + // Not `process.kill(-pid)`: on Windows that is an invalid handle rather than a process group, + // and falling back to the bare pid would leave the worker's app-server subtree behind. + forceKillProcessTree(pid); } // Give the forced kill a beat to release the sockets. await new Promise((resolve) => setTimeout(resolve, 100)); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 5a5a76195..c9f7f2924 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,11 +1,15 @@ +import fs from "node:fs"; +import path from "node:path"; import process from "node:process"; import test from "node:test"; import assert from "node:assert/strict"; import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; import { binaryAvailable, commandWithWindowsShim, + forceKillProcessTree, getProcessIdentity, isProcessRunning, isProcessTreeRunning, @@ -16,6 +20,8 @@ import { waitForProcessExit } from "../plugins/codex/scripts/lib/process.mjs"; +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); + const SELF_TERMINATING_SCRIPT = "process.kill(process.pid, 'SIGTERM'); setInterval(() => {}, 1000);"; test("runCommand reports a signal-terminated process as a failure", { skip: process.platform === "win32" }, () => { @@ -335,3 +341,121 @@ test("binaryAvailable uses cmd.exe explicitly for Windows command shims", () => }); assert.deepEqual(outcome, { available: true, detail: "11.16.0" }); }); + +test("forceKillProcessTree reaches descendants on Windows instead of signalling a negative pid", () => { + let captured = null; + const outcome = forceKillProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args, options) { + captured = { command, args, shell: options?.shell }; + return { command, args, status: 0, signal: null, stdout: "", stderr: "", error: null }; + }, + killImpl(pid) { + // Windows has no process groups: OpenProcess on a negative pid fails, so a + // catch-and-retry would degrade to killing the worker alone and orphan the + // app-server (and every MCP server) underneath it. + throw new Error(`process.kill must not be used on Windows (called with ${pid})`); + } + }); + + assert.deepEqual(captured, { + command: "taskkill", + args: ["/PID", "1234", "/T", "/F"], + shell: false + }); + assert.deepEqual(outcome, { attempted: true, delivered: true, method: "taskkill" }); +}); + +test("forceKillProcessTree never throws when the Windows tree cannot be reached", () => { + const outcome = forceKillProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 1, + signal: null, + stdout: "", + stderr: "ERROR: Access is denied.", + error: null + }; + } + }); + + assert.deepEqual(outcome, { attempted: true, delivered: false, method: "taskkill" }); +}); + +test("forceKillProcessTree SIGKILLs the process group on POSIX", () => { + const signalled = []; + const outcome = forceKillProcessTree(1234, { + platform: "linux", + killImpl(pid, signal) { + signalled.push({ pid, signal }); + } + }); + + assert.deepEqual(signalled, [{ pid: -1234, signal: "SIGKILL" }]); + assert.equal(outcome.method, "process-group"); + assert.equal(outcome.delivered, true); +}); + +test("forceKillProcessTree falls back to the process when it leads no group", () => { + const signalled = []; + const outcome = forceKillProcessTree(1234, { + platform: "linux", + killImpl(pid, signal) { + signalled.push({ pid, signal }); + if (pid < 0) { + const error = new Error("No such process"); + error.code = "ESRCH"; + throw error; + } + } + }); + + assert.deepEqual(signalled, [ + { pid: -1234, signal: "SIGKILL" }, + { pid: 1234, signal: "SIGKILL" } + ]); + assert.equal(outcome.method, "process"); + assert.equal(outcome.delivered, true); +}); + +test("no plugin script force-kills through a negative pid outside the platform-guarded helpers", () => { + // `kill(-pid, …)` addresses a process group, which exists only on POSIX; on Windows it is + // an invalid handle, and a catch-and-retry on the bare pid silently orphans the descendants. + // So a direct `process.kill(-pid)` is banned outright: group signalling belongs to the + // platform-guarded helpers in process.mjs, which send it through an injectable `killImpl` + // and pick taskkill on Windows. + const scriptsDir = path.join(ROOT, "plugins", "codex", "scripts"); + const direct = new Set(); + const viaKillImpl = new Set(); + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!entry.name.endsWith(".mjs")) { + continue; + } + const relative = path.relative(ROOT, full).split(path.sep).join("/"); + for (const line of fs.readFileSync(full, "utf8").split("\n")) { + const code = line.trim(); + if (code.startsWith("//") || code.startsWith("*") || code.startsWith("/*")) { + continue; + } + if (/\bprocess\.kill\(\s*-/.test(code)) { + direct.add(relative); + } else if (/\bkill\w*\(\s*-/.test(code)) { + viaKillImpl.add(relative); + } + } + } + }; + walk(scriptsDir); + + assert.deepEqual([...direct], []); + assert.deepEqual([...viaKillImpl], ["plugins/codex/scripts/lib/process.mjs"]); +}); From e0c3c805a9a24139ac72a42af6884ef9a8030e74 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 22:20:49 +0000 Subject: [PATCH 8/9] fix(windows): retry an atomic write the filesystem briefly refuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replacing a file through rename can fail on Windows while something else holds the target open — an on-access scanner or the search indexer is enough — and it surfaces as EPERM/EACCES/EBUSY. Every job and state write goes through writeJsonFileAtomic(), so one unlucky moment threw the record away instead of recording it. renameReplacing() waits the sharing violation out: four short waits, then the error is real and is thrown. POSIX has no such failure mode, so the retry is Windows-only and any other error code fails on the first attempt. A guard test keeps bare fs.renameSync() out of the plugin scripts, except in locking.mjs, where EPERM already means "someone else holds the lock". README documents both Windows fixes and what running on Windows requires. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- README.md | 5 ++ plugins/codex/scripts/lib/fs.mjs | 39 +++++++++- tests/state.test.mjs | 125 +++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 39b615a73..fcebd5f63 100644 --- a/README.md +++ b/README.md @@ -467,6 +467,11 @@ Beyond the imports, this fork carries fixes for defects the imports themselves s - `/codex:cancel` exits non-zero when neither the turn interrupt nor the worker kill confirmed the job stopped, instead of reporting a cancellation nothing proved ([#656](https://github.com/openai/codex-plugin-cc/pull/656)) +- teardown on Windows no longer signals a negative pid (a process group is POSIX-only; there it is + just an invalid handle, and the fallback killed the worker while its app-server subtree kept + running) — `taskkill /T /F` walks the tree instead +- a state write that Windows briefly refuses — a scanner or indexer holding the file open, which + surfaces as `EPERM`/`EBUSY` on the replacing rename — is retried instead of losing the record - the app-server typecheck (`npm run build`) passes Each of those came out of an adversarial review of the merges, re-run after every round of fixes; diff --git a/plugins/codex/scripts/lib/fs.mjs b/plugins/codex/scripts/lib/fs.mjs index 730259831..92363c09c 100644 --- a/plugins/codex/scripts/lib/fs.mjs +++ b/plugins/codex/scripts/lib/fs.mjs @@ -37,6 +37,43 @@ export function writePrivateFile(filePath, value) { setMode(filePath, PRIVATE_FILE_MODE); } +// Windows can refuse to replace a file that something else has open — an on-access virus scanner +// or a search indexer opening the target for a moment is enough, and it surfaces as EPERM/EACCES/ +// EBUSY rather than as anything the caller could act on. POSIX rename has no such failure mode, so +// this retry is Windows-only: a handful of short waits, after which the error is real and is thrown. +const WINDOWS_RENAME_RETRY_DELAYS_MS = [5, 15, 40, 100]; +const WINDOWS_RENAME_RETRY_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); + +function sleepSync(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * @param {string} from + * @param {string} to + * @param {{ platform?: string, renameImpl?: Function, sleepImpl?: Function }} [options] + */ +export function renameReplacing(from, to, options = {}) { + const renameImpl = options.renameImpl ?? fs.renameSync; + if ((options.platform ?? process.platform) !== "win32") { + renameImpl(from, to); + return; + } + + const sleepImpl = options.sleepImpl ?? sleepSync; + for (let attempt = 0; ; attempt += 1) { + try { + renameImpl(from, to); + return; + } catch (error) { + if (attempt >= WINDOWS_RENAME_RETRY_DELAYS_MS.length || !WINDOWS_RENAME_RETRY_CODES.has(error?.code)) { + throw error; + } + sleepImpl(WINDOWS_RENAME_RETRY_DELAYS_MS[attempt]); + } + } +} + export function writeJsonFileAtomic(filePath, value) { const temporaryFile = `${filePath}.${process.pid}.${randomUUID()}.tmp`; try { @@ -52,7 +89,7 @@ export function writeJsonFileAtomic(filePath, value) { } finally { fs.closeSync(fd); } - fs.renameSync(temporaryFile, filePath); + renameReplacing(temporaryFile, filePath); setMode(filePath, PRIVATE_FILE_MODE); } catch (error) { fs.rmSync(temporaryFile, { force: true }); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0097b069a..51b3c0ccd 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -9,6 +9,9 @@ import { fileURLToPath, pathToFileURL } from "node:url"; import { makeTempDir } from "./helpers.mjs"; import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; +import { renameReplacing } from "../plugins/codex/scripts/lib/fs.mjs"; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); import { acquireLockSync, releaseLock } from "../plugins/codex/scripts/lib/locking.mjs"; import { getConfig, @@ -720,3 +723,125 @@ test("disabling the review gate is not outvoted by a stranded enable in another assert.equal(getConfig(workspace).stopReviewGate, false); }); }); + +test("an atomic write retries a Windows sharing violation instead of losing the record", () => { + const attempts = []; + const waits = []; + renameReplacing("state.json.tmp", "state.json", { + platform: "win32", + renameImpl(from, to) { + attempts.push({ from, to }); + if (attempts.length <= 2) { + // What a scanner or indexer holding the target open looks like from here. + const error = new Error("EPERM: operation not permitted, rename"); + error.code = "EPERM"; + throw error; + } + }, + sleepImpl(ms) { + waits.push(ms); + } + }); + + assert.equal(attempts.length, 3); + assert.deepEqual(attempts.at(-1), { from: "state.json.tmp", to: "state.json" }); + assert.deepEqual(waits, [5, 15]); +}); + +test("an atomic write gives up on a Windows sharing violation that never clears", () => { + let attempts = 0; + assert.throws( + () => + renameReplacing("state.json.tmp", "state.json", { + platform: "win32", + renameImpl() { + attempts += 1; + const error = new Error("EBUSY: resource busy or locked, rename"); + error.code = "EBUSY"; + throw error; + }, + sleepImpl() {} + }), + /EBUSY/ + ); + + // Four waits, five attempts: the budget is bounded, and the last failure is the caller's. + assert.equal(attempts, 5); +}); + +test("an atomic write does not retry errors that are not sharing violations", () => { + for (const platform of ["win32", "linux"]) { + let attempts = 0; + assert.throws( + () => + renameReplacing("state.json.tmp", "state.json", { + platform, + renameImpl() { + attempts += 1; + const error = new Error("ENOENT: no such file or directory, rename"); + error.code = "ENOENT"; + throw error; + }, + sleepImpl() { + throw new Error("must not wait"); + } + }), + /ENOENT/ + ); + assert.equal(attempts, 1, `${platform} must fail on the first attempt`); + } +}); + +test("outside Windows an atomic write renames once and reports the failure", () => { + let attempts = 0; + assert.throws( + () => + renameReplacing("state.json.tmp", "state.json", { + platform: "linux", + renameImpl() { + attempts += 1; + const error = new Error("EPERM: operation not permitted, rename"); + error.code = "EPERM"; + throw error; + }, + sleepImpl() { + throw new Error("POSIX rename has no sharing violation to wait out"); + } + }), + /EPERM/ + ); + assert.equal(attempts, 1); +}); + +test("plugin scripts replace files through the Windows-aware rename", () => { + // A bare fs.renameSync() over an existing file is the call that Windows can refuse with a + // sharing violation, so file replacement goes through renameReplacing(), which retries it. + // locking.mjs is the exception: its directory rename already reads EPERM as "someone else + // holds the lock", which is exactly what it means there. + const scriptsDir = path.join(REPO_ROOT, "plugins", "codex", "scripts"); + const callers = new Set(); + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!entry.name.endsWith(".mjs")) { + continue; + } + for (const line of fs.readFileSync(full, "utf8").split("\n")) { + const code = line.trim(); + if (code.startsWith("//") || code.startsWith("*") || code.startsWith("/*")) { + continue; + } + if (/\bfs\.renameSync\s*\(/.test(code)) { + callers.add(path.relative(REPO_ROOT, full).split(path.sep).join("/")); + } + } + } + }; + walk(scriptsDir); + + assert.deepEqual([...callers].sort(), ["plugins/codex/scripts/lib/locking.mjs"]); +}); From 9bff91005c171dd3a4f23598492da1fb99207161 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 22:21:35 +0000 Subject: [PATCH 9/9] docs: list the two Windows imports with the other imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They were described in the fork-fix list, which is for defects the imports surfaced — not for the imports themselves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- README.md | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index fcebd5f63..e0ea536dc 100644 --- a/README.md +++ b/README.md @@ -420,6 +420,8 @@ Broker and background-job lifecycle: | [#659](https://github.com/openai/codex-plugin-cc/pull/659) | state written under one `CLAUDE_PLUGIN_DATA` root is no longer invisible to an invocation that resolves to another, which orphaned brokers and hid jobs | | [#707](https://github.com/openai/codex-plugin-cc/pull/707) | the broker releases its app-server thread subscriptions when a client disconnects, instead of leaking them for its whole lifetime | | [#728](https://github.com/openai/codex-plugin-cc/pull/728) | a job whose worker died no longer reads as "running" forever; `/codex:status` reconciles the record against the live process | +| [#725](https://github.com/openai/codex-plugin-cc/pull/725) | nothing is spawned through the user's shell on Windows, where MSYS path conversion mangled switches like `taskkill /PID` and left background workers unkillable under Git Bash (this supersedes [#735](https://github.com/openai/codex-plugin-cc/pull/735)) | +| [#656](https://github.com/openai/codex-plugin-cc/pull/656) | `/codex:cancel` exits non-zero when neither the turn interrupt nor the worker kill confirmed the job stopped, instead of reporting a cancellation nothing proved | Commands and flags: @@ -460,16 +462,9 @@ Beyond the imports, this fork carries fixes for defects the imports themselves s - disabling the review gate is not outvoted by a stale enable left under another plugin-data root - `CLAUDE_ENV_FILE` is only ever appended to: it is shared with other plugins' hooks, and rewriting it dropped whatever they had just written -- on Windows nothing is spawned through the user's shell any more, so MSYS path conversion can no - longer mangle a switch like `taskkill /PID` — which had left background workers unkillable under - Git Bash ([#725](https://github.com/openai/codex-plugin-cc/pull/725), which also supersedes - [#735](https://github.com/openai/codex-plugin-cc/pull/735)) -- `/codex:cancel` exits non-zero when neither the turn interrupt nor the worker kill confirmed the - job stopped, instead of reporting a cancellation nothing proved - ([#656](https://github.com/openai/codex-plugin-cc/pull/656)) -- teardown on Windows no longer signals a negative pid (a process group is POSIX-only; there it is - just an invalid handle, and the fallback killed the worker while its app-server subtree kept - running) — `taskkill /T /F` walks the tree instead +- teardown never force-kills through a negative pid: a process group is POSIX-only, so on Windows + that was an invalid handle and the fallback killed the worker alone, leaving its app-server — and + every MCP server under it — running. `taskkill /T /F` walks the tree there instead - a state write that Windows briefly refuses — a scanner or indexer holding the file open, which surfaces as `EPERM`/`EBUSY` on the replacing rename — is retried instead of losing the record - the app-server typecheck (`npm run build`) passes