Make the plugin actually run on Windows: import #725 and #656, then fix what the audit found

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
 
@@ -417,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:
 
@@ -457,6 +462,11 @@ 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
+- 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
 
 Each of those came out of an adversarial review of the merges, re-run after every round of fixes;

plugins/codex/scripts/codex-companion.mjs

@@ -43,7 +43,8 @@ import {
   reconcileJobsLiveness,
   resolveCancelableJob,
   resolveResultJob,
-  sortJobsNewestFirst
+  sortJobsNewestFirst,
+  wasCancellationConfirmed
 } from "./lib/job-control.mjs";
 import {
   appendLogLine,
@@ -1369,6 +1370,12 @@ async function handleCancel(argv) {
   const effectiveStatus = orphanAdopted
     ? readStoredJob(workspaceRoot, job.id)?.status ?? "cancelled"
     : "cancelled";
+  // Neither the turn interrupt nor the worker kill proved the job stopped:
+  // a partial `taskkill /T` on Windows can refuse a subset of the tree. The
+  // record is already written (record-first, above), so the cancellation is
+  // not rewound — but the command must not exit as though it had worked while
+  // a write-capable task may still be editing the workspace.
+  const cancellationConfirmed = wasCancellationConfirmed(interrupt, workerKillError == null);
   const payload = {
     jobId: job.id,
     status: effectiveStatus,
@@ -1377,14 +1384,22 @@ async function handleCancel(argv) {
     turnInterrupted: interrupt.interrupted,
     // A failed kill leaves the worker alive even though the record is
     // cancelled; the caller must be able to tell that from a clean cancel.
-    workerTerminated: workerKillError == null
+    workerTerminated: workerKillError == null,
+    cancellationConfirmed
   };
 
   outputCommandResult(
     payload,
     renderCancelReport({ ...nextJob, status: effectiveStatus }, { workerTerminated: workerKillError == null }),
     options.json
   );
+
+  if (!cancellationConfirmed) {
+    throw new Error(
+      `Could not confirm job ${job.id} stopped: the turn interrupt did not succeed and terminating the worker process tree failed. ` +
+        "The job is recorded as cancelled, but its worker may still be running — check it before relying on the workspace."
+    );
+  }
 }
 
 async function main() {

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, isBrokerEndpointReady, 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"));
@@ -216,11 +216,15 @@ 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,
+      // See runCommand(): SHELL is a POSIX convention and is never consulted
+      // for Windows process creation. `codex` is a .cmd shim there, so the
+      // invocation above wraps it in an explicit cmd.exe call instead.
+      shell: invocation.shell,
       windowsHide: true
     });
 
@@ -274,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);
@@ -306,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");

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 });

plugins/codex/scripts/lib/job-control.mjs

@@ -377,3 +377,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);
+}

plugins/codex/scripts/lib/process.mjs

@@ -15,7 +15,13 @@ export function runCommand(command, args = [], options = {}) {
     timeout: options.timeout,
     killSignal: options.killSignal,
     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 routed commands through whatever
+    // POSIX shell happened to be set (Git Bash, which Claude Code's own Bash
+    // tool sets), and MSYS path conversion then mangled Windows-style flags
+    // like `/PID`. Nothing is spawned through a shell here; a Windows `.cmd`
+    // shim goes through commandWithWindowsShim() instead.
+    shell: options.shell ?? false,
     windowsHide: true
   });
 
@@ -43,8 +49,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" };
   }
@@ -389,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.
  *
@@ -419,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);
 }
@@ -440,15 +515,16 @@ 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) {
       return { attempted: true, delivered: true, method: "taskkill", result };
     }
 
     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 };
     }
 

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));