fix: make cancel work under Git Bash on a non-English Windows (#423)

plugins/codex/scripts/codex-companion.mjs

@@ -983,7 +983,17 @@ async function handleCancel(argv) {
     );
   }
 
-  terminateProcessTree(job.pid ?? Number.NaN);
+  const termination = terminateProcessTree(job.pid ?? Number.NaN);
+  if (termination.attempted && !termination.treeConfirmed) {
+    // The job's own process is gone, but the kill could not account for what was under
+    // it. Say so rather than reporting a clean cancellation: a surviving codex or broker
+    // child keeps running, and nothing else makes a second pass at it.
+    appendLogLine(
+      job.logFile,
+      `Cancelled, but the process tree under pid ${job.pid} could not be fully confirmed stopped. ` +
+        "Check for leftover codex processes if the workspace behaves oddly."
+    );
+  }
   appendLogLine(job.logFile, "Cancelled by user.");
 
   const completedAt = nowIso();

plugins/codex/scripts/lib/process.mjs

@@ -8,6 +8,7 @@ export function runCommand(command, args = [], options = {}) {
     encoding: "utf8",
     input: options.input,
     maxBuffer: options.maxBuffer,
+    timeout: options.timeout,
     stdio: options.stdio ?? "pipe",
     shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false),
     windowsHide: true
@@ -54,9 +55,55 @@ function looksLikeMissingProcessMessage(text) {
   return /not found|no running instance|cannot find|does not exist|no such process/i.test(text);
 }
 
+// taskkill's exit code for "no such process". Locale-independent, unlike the message
+// text: on a non-English Windows the "not found" wording is translated and the regex
+// above never matches, so an already-dead pid would surface as a thrown error.
+const TASKKILL_PROCESS_NOT_FOUND = 128;
+
+// On Windows runCommand spawns through $SHELL, which is Git Bash when Claude Code runs
+// from it. MSYS then rewrites taskkill's `/PID` flag into a path (`C:/Program Files/Git/PID`)
+// and the call fails. Disable the conversion for this child only -- setting it on the
+// node process itself would break the `/c/...` paths node needs to resolve.
+function envWithoutMsysPathConversion(env) {
+  return {
+    ...(env ?? process.env),
+    MSYS_NO_PATHCONV: "1",
+    MSYS2_ARG_CONV_EXCL: "*"
+  };
+}
+
+// Signal 0 tests for existence without touching the process. EPERM means it is alive
+// under another owner, so anything but ESRCH counts as still running.
+function processIsGone(pid, killImpl) {
+  try {
+    killImpl(pid, 0);
+    return false;
+  } catch (error) {
+    return error?.code === "ESRCH";
+  }
+}
+
+// A process that is already on its way out -- taskkill losing the race with a turn that
+// was just interrupted reports "Access is denied", exit 1 -- is still briefly alive, so a
+// single instantaneous probe calls it a failure. Give it a bounded moment to finish.
+// This whole function is synchronous, hence the Atomics sleep rather than a timer.
+function waitForProcessGone(pid, killImpl, timeoutMs) {
+  const deadline = Date.now() + timeoutMs;
+  const idle = new Int32Array(new SharedArrayBuffer(4));
+  for (;;) {
+    if (processIsGone(pid, killImpl)) {
+      return true;
+    }
+    if (Date.now() >= deadline) {
+      return false;
+    }
+    Atomics.wait(idle, 0, 0, Math.min(50, Math.max(1, deadline - Date.now())));
+  }
+}
+
 export function terminateProcessTree(pid, options = {}) {
   if (!Number.isFinite(pid)) {
-    return { attempted: false, delivered: false, method: null };
+    return { attempted: false, delivered: false, treeConfirmed: true, method: null };
   }
 
   const platform = options.platform ?? process.platform;
@@ -66,25 +113,48 @@ export function terminateProcessTree(pid, options = {}) {
   if (platform === "win32") {
     const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], {
       cwd: options.cwd,
-      env: options.env
+      env: envWithoutMsysPathConversion(options.env)
     });
 
     if (!result.error && result.status === 0) {
-      return { attempted: true, delivered: true, method: "taskkill", result };
+      return { attempted: true, delivered: true, treeConfirmed: true, method: "taskkill", result };
     }
 
     const combinedOutput = `${result.stderr}\n${result.stdout}`.trim();
-    if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) {
-      return { attempted: true, delivered: false, method: "taskkill", result };
+    if (
+      !result.error &&
+      (result.status === TASKKILL_PROCESS_NOT_FOUND || looksLikeMissingProcessMessage(combinedOutput))
+    ) {
+      // "No such process" is about the root at this instant, not about what it spawned
+      // while it was alive. A worker that died mid-turn leaves its `codex app-server`
+      // child orphaned, and with the root gone `/T` has nothing left to walk from. That
+      // is not a corner case here: `cancel` only reaches this branch for a job the state
+      // still calls running, which is exactly the crashed-worker case.
+      return { attempted: true, delivered: false, treeConfirmed: false, method: "taskkill", result };
+    }
+
+    // taskkill exits non-zero both after failing to reap a descendant ("The operation
+    // attempted is not supported", 255) and when it races a root that was already
+    // terminating ("Access is denied", 1 -- what `cancel` hits after interrupting the
+    // turn). Both messages are localized, so the root's own liveness is the only usable
+    // signal: it is gone, and the kill should not throw for either case.
+    //
+    // But a non-zero taskkill never establishes that the REST of the tree went with it,
+    // and `/T` leaves no way to enumerate what survived once the root is gone. So the
+    // two facts are reported separately -- `delivered` is about the target, and
+    // `treeConfirmed` is about everything under it. Like the missing-root branch above,
+    // this one cannot account for the whole tree.
+    if (!result.error && waitForProcessGone(pid, killImpl, options.killWaitMs ?? 1000)) {
+      return { attempted: true, delivered: true, treeConfirmed: false, method: "taskkill", result };
     }
 
     if (result.error?.code === "ENOENT") {
       try {
         killImpl(pid);
-        return { attempted: true, delivered: true, method: "kill" };
+        return { attempted: true, delivered: true, treeConfirmed: true, method: "kill" };
       } catch (error) {
         if (error?.code === "ESRCH") {
-          return { attempted: true, delivered: false, method: "kill" };
+          return { attempted: true, delivered: false, treeConfirmed: true, method: "kill" };
         }
         throw error;
       }
@@ -99,21 +169,21 @@ export function terminateProcessTree(pid, options = {}) {
 
   try {
     killImpl(-pid, "SIGTERM");
-    return { attempted: true, delivered: true, method: "process-group" };
+    return { attempted: true, delivered: true, treeConfirmed: true, method: "process-group" };
   } catch (error) {
     if (error?.code !== "ESRCH") {
       try {
         killImpl(pid, "SIGTERM");
-        return { attempted: true, delivered: true, method: "process" };
+        return { attempted: true, delivered: true, treeConfirmed: true, method: "process" };
       } catch (innerError) {
         if (innerError?.code === "ESRCH") {
-          return { attempted: true, delivered: false, method: "process" };
+          return { attempted: true, delivered: false, treeConfirmed: true, method: "process" };
         }
         throw innerError;
       }
     }
 
-    return { attempted: true, delivered: false, method: "process-group" };
+    return { attempted: true, delivered: false, treeConfirmed: true, method: "process-group" };
   }
 }
 

tests/process.test.mjs

@@ -7,8 +7,9 @@ test("terminateProcessTree uses taskkill on Windows", () => {
   let captured = null;
   const outcome = terminateProcessTree(1234, {
     platform: "win32",
-    runCommandImpl(command, args) {
-      captured = { command, args };
+    env: { PATH: "C:\\Windows\\System32", SHELL: "C:\\Program Files\\Git\\bin\\bash.exe" },
+    runCommandImpl(command, args, options) {
+      captured = { command, args, options };
       return {
         command,
         args,
@@ -26,12 +27,131 @@ 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: {
+        PATH: "C:\\Windows\\System32",
+        SHELL: "C:\\Program Files\\Git\\bin\\bash.exe",
+        MSYS_NO_PATHCONV: "1",
+        MSYS2_ARG_CONV_EXCL: "*"
+      }
+    }
   });
   assert.equal(outcome.delivered, true);
+  assert.equal(outcome.treeConfirmed, true, "a clean taskkill accounts for the whole tree");
+  assert.equal(outcome.method, "taskkill");
+});
+
+test("terminateProcessTree treats exit code 128 as already stopped on a non-English Windows", () => {
+  // Korean Windows: taskkill translates "not found", so the English regex cannot match.
+  // Only the exit code identifies the case. Without it this call throws.
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      return {
+        command,
+        args,
+        status: 128,
+        signal: null,
+        stdout: "",
+        stderr: "오류: 프로세스 \"1234\"을(를) 찾을 수 없습니다.",
+        error: null
+      };
+    },
+    killImpl() {
+      throw new Error("kill fallback should not run");
+    }
+  });
+
+  assert.equal(outcome.attempted, true);
+  assert.equal(outcome.delivered, false);
+  assert.equal(
+    outcome.treeConfirmed,
+    false,
+    "a root that is already gone says nothing about the children it left behind"
+  );
   assert.equal(outcome.method, "taskkill");
 });
 
+// The real message, from a Korean Windows host: taskkill killed the target but could not
+// reap one descendant. Keying on the text would reintroduce the locale bug this file
+// exists for, so the target's own liveness has to decide it.
+test("terminateProcessTree reports a partial taskkill failure when only the root is known gone", () => {
+  let probedSignal;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      return {
+        command,
+        args,
+        status: 255,
+        signal: null,
+        stdout: "",
+        stderr:
+          "오류: PID 27048인 프로세스(PID 1234인 자식 프로세스)를 종료할 수 없습니다.\n오류: 시도한 작업은 지원되지 않습니다.",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      probedSignal = signal;
+      const error = new Error("no such process");
+      error.code = "ESRCH";
+      throw error;
+    }
+  });
+
+  assert.equal(outcome.attempted, true);
+  assert.equal(outcome.delivered, true);
+  assert.equal(outcome.treeConfirmed, false, "a partial kill must not claim the tree");
+  assert.equal(outcome.method, "taskkill");
+  assert.equal(probedSignal, 0, "the target must be probed, not signalled");
+});
+
+// The real failure behind `/codex:cancel`: the turn was just interrupted, so the worker
+// is already terminating and taskkill loses the race with "Access is denied". The target
+// is not gone at the instant taskkill returns -- only a moment later.
+test("terminateProcessTree waits out a target that is still exiting", () => {
+  let probes = 0;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    killWaitMs: 500,
+    runCommandImpl(command, args) {
+      return { command, args, status: 1, signal: null, stdout: "", stderr: "오류: 액세스가 거부되었습니다.", error: null };
+    },
+    killImpl() {
+      probes += 1;
+      if (probes < 3) {
+        return true; // still winding down
+      }
+      const error = new Error("no such process");
+      error.code = "ESRCH";
+      throw error;
+    }
+  });
+
+  assert.equal(outcome.delivered, true);
+  assert.equal(outcome.treeConfirmed, false, "a raced kill cannot account for the tree");
+  assert.ok(probes >= 3, "it gave up after a single probe");
+});
+
+test("terminateProcessTree still throws when taskkill fails and the target stays alive", () => {
+  assert.throws(
+    () =>
+      terminateProcessTree(1234, {
+        platform: "win32",
+        killWaitMs: 0,
+        runCommandImpl(command, args) {
+          return { command, args, status: 1, signal: null, stdout: "", stderr: "Access is denied.", error: null };
+        },
+        killImpl() {
+          return true; // never goes away
+        }
+      }),
+    /Access is denied|taskkill/
+  );
+});
+
 test("terminateProcessTree treats missing Windows processes as already stopped", () => {
   const outcome = terminateProcessTree(1234, {
     platform: "win32",