fix: Windows SHELL env var breaks taskkill; handleCancel aborts before updating job state on a partial kill failure

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,7 +984,33 @@ async function handleCancel(argv) {
     );
   }
 
-  terminateProcessTree(job.pid ?? Number.NaN);
+  let terminationOutcomeKnown = true;
+  try {
+    terminateProcessTree(job.pid ?? Number.NaN);
+  } catch (error) {
+    // terminateProcessTree already treats "process already gone" as
+    // 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();

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

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

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

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

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