Import #776's teardown half: decide on the root's liveness, not on taskkill's message

README.md

@@ -427,6 +427,7 @@ Broker and background-job lifecycle:
 | [#772](https://github.com/openai/codex-plugin-cc/pull/772) | the stop-review gate keeps a minute of headroom under the Stop hook's budget, so a timed-out review can still say so instead of ending the turn silently |
 | [#774](https://github.com/openai/codex-plugin-cc/pull/774) | `status --wait` prints its timeout and exits non-zero, instead of looking like a finished status check |
 | [#773](https://github.com/openai/codex-plugin-cc/pull/773) | a broker connect that never completes is given up on after 2s and falls back to a direct app-server (the probe half of that PR is not taken: ours already bounds each attempt *and* reports why it failed) |
+| [#776](https://github.com/openai/codex-plugin-cc/pull/776) | Windows teardown decides on the root's liveness instead of taskkill's message: a process already gone costs no `taskkill` at all, and a `taskkill` that reports failure only because a short-lived descendant exited mid-walk no longer throws at the caller (its broker-endpoint and shutdown-timeout changes are not taken — one is a no-op here, the other is behind what this fork already does) |
 
 Commands and flags:
 

plugins/codex/scripts/lib/process.mjs

@@ -105,8 +105,24 @@ export function isPidAlive(pid, killImpl = process.kill.bind(process)) {
   }
 }
 
-function looksLikeMissingProcessMessage(text) {
-  return /not found|no running instance|cannot find|does not exist|no such process/i.test(text);
+/**
+ * Whether a pid is *provably* gone, as opposed to merely unreadable.
+ *
+ * Deliberately narrower than !isPidAlive(): only ESRCH proves absence. Any other failure — EPERM,
+ * or whatever code a platform reports for a handle it will not open — leaves the question open,
+ * and the teardown below must still try to kill, because skipping the kill on a live root leaks
+ * its whole tree.
+ */
+function isProvablyGone(pid, killImpl) {
+  if (!Number.isFinite(pid) || pid <= 0) {
+    return true;
+  }
+  try {
+    killImpl(pid, 0);
+    return false;
+  } catch (error) {
+    return /** @type {NodeJS.ErrnoException} */ (error)?.code === "ESRCH";
+  }
 }
 
 export function isValidPid(pid) {
@@ -513,6 +529,13 @@ export function terminateProcessTree(pid, options = {}) {
   const killImpl = options.killImpl ?? process.kill.bind(process);
 
   if (platform === "win32") {
+    // Probe the root before spawning taskkill at all, rather than parsing its localized "not
+    // found" message afterwards: a root that is provably gone is reported the same way in every
+    // system language.
+    if (isProvablyGone(pid, killImpl)) {
+      return { attempted: false, delivered: false, method: null };
+    }
+
     const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], {
       cwd: options.cwd,
       env: options.env,
@@ -523,9 +546,14 @@ export function terminateProcessTree(pid, options = {}) {
       return { attempted: true, delivered: true, method: "taskkill", result };
     }
 
-    const combinedOutput = `${result.stderr}\n${result.stdout}`.trim();
-    if (!result.error && (result.status === 128 || looksLikeMissingProcessMessage(combinedOutput))) {
-      return { attempted: true, delivered: false, method: "taskkill", result };
+    // A non-zero status: taskkill /T walks the tree and then terminates each entry, so a
+    // descendant that exits in between — a short-lived git or cmd helper — makes it report a
+    // failure although the root did die. The root's own liveness is the fact; its message is not,
+    // and matching that message only ever worked in English. `delivered` therefore describes the
+    // root only, exactly like the process-group SIGTERM on other platforms: it does not prove
+    // that every descendant is gone.
+    if (!result.error && isProvablyGone(pid, killImpl)) {
+      return { attempted: true, delivered: true, method: "taskkill", result };
     }
 
     if (result.error?.code === "ENOENT") {

tests/git.test.mjs

@@ -132,13 +132,27 @@ test("collectReviewContext skips untracked directories in working tree review",
   assert.match(context.content, /### \.claude\/worktrees\/agent-test\/\n\(skipped: directory\)/);
 });
 
-test("collectReviewContext skips broken untracked symlinks instead of crashing", () => {
+test("collectReviewContext skips broken untracked symlinks instead of crashing", (t) => {
   const cwd = makeTempDir();
   initGitRepo(cwd);
   fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n");
   run("git", ["add", "app.js"], { cwd });
   run("git", ["commit", "-m", "init"], { cwd });
-  fs.symlinkSync("missing-target", path.join(cwd, "broken-link"));
+  try {
+    fs.symlinkSync("missing-target", path.join(cwd, "broken-link"));
+  } catch (error) {
+    if (process.platform === "win32" && error?.code === "EPERM") {
+      t.skip("Windows requires Developer Mode or elevated privileges to create this symlink fixture.");
+      return;
+    }
+    throw error;
+  }
+
+  const untracked = run("git", ["ls-files", "--others", "--exclude-standard"], { cwd }).stdout;
+  if (!untracked.split(/\r?\n/).includes("broken-link")) {
+    t.skip("Git does not report broken symlinks as untracked in this environment.");
+    return;
+  }
 
   const target = resolveReviewTarget(cwd, {});
   const context = collectReviewContext(cwd, target);

tests/process.test.mjs

@@ -198,8 +198,9 @@ test("terminateProcessTree uses taskkill on Windows", () => {
         error: null
       };
     },
-    killImpl() {
-      throw new Error("kill fallback should not run");
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
     }
   });
 
@@ -215,7 +216,8 @@ test("terminateProcessTree uses taskkill on Windows", () => {
   assert.equal(outcome.method, "taskkill");
 });
 
-test("terminateProcessTree treats missing Windows processes as already stopped", () => {
+test("terminateProcessTree uses liveness instead of localized taskkill output", () => {
+  let livenessChecks = 0;
   const outcome = terminateProcessTree(1234, {
     platform: "win32",
     runCommandImpl(command, args) {
@@ -224,17 +226,204 @@ test("terminateProcessTree treats missing Windows processes as already stopped",
         args,
         status: 128,
         signal: null,
-        stdout: "ERROR: The process \"1234\" not found.",
-        stderr: "",
+        stdout: "",
+        stderr: "Erreur : le processus \"1234\" est introuvable.",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      livenessChecks += 1;
+      if (livenessChecks === 1) {
+        return;
+      }
+      const error = new Error("ESRCH");
+      error.code = "ESRCH";
+      throw error;
+    }
+  });
+
+  assert.equal(outcome.attempted, true);
+  assert.equal(outcome.delivered, true);
+  assert.equal(outcome.method, "taskkill");
+  assert.equal(outcome.result.status, 128);
+  assert.equal(livenessChecks, 2);
+});
+
+test("terminateProcessTree reports delivery when taskkill only failed on already-exiting descendants", () => {
+  let livenessChecks = 0;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      return {
+        command,
+        args,
+        status: 128,
+        signal: null,
+        stdout: "SUCCESS: The process with PID 1234 has been terminated.",
+        stderr:
+          "ERROR: The process with PID 5678 (child process of PID 1234) could not be terminated.\n" +
+          "Reason: The operation attempted is not supported.",
         error: null
       };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      livenessChecks += 1;
+      if (livenessChecks === 1) {
+        return;
+      }
+      const error = new Error("ESRCH");
+      error.code = "ESRCH";
+      throw error;
     }
   });
 
   assert.equal(outcome.attempted, true);
+  assert.equal(outcome.delivered, true);
   assert.equal(outcome.method, "taskkill");
   assert.equal(outcome.result.status, 128);
-  assert.match(outcome.result.stdout, /not found/i);
+  assert.equal(livenessChecks, 2);
+});
+
+test("terminateProcessTree still throws when taskkill fails and the root process survives", () => {
+  assert.throws(
+    () =>
+      terminateProcessTree(1234, {
+        platform: "win32",
+        runCommandImpl(command, args) {
+          return {
+            command,
+            args,
+            status: 128,
+            signal: null,
+            stdout: "",
+            stderr: "ERROR: The process with PID 1234 could not be terminated.\nReason: Access is denied.",
+            error: null
+          };
+        },
+        killImpl(pid, signal) {
+          assert.equal(pid, 1234);
+          assert.equal(signal, 0);
+        }
+      }),
+    /could not be terminated/
+  );
+});
+
+test("terminateProcessTree skips taskkill when the Windows process is already absent", () => {
+  let taskkillCalled = false;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      taskkillCalled = true;
+      return {
+        command,
+        args,
+        status: 128,
+        signal: null,
+        stdout: "",
+        stderr: "Erreur : le processus \"1234\" est introuvable.",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      const error = new Error("ESRCH");
+      error.code = "ESRCH";
+      throw error;
+    }
+  });
+
+  assert.equal(taskkillCalled, false);
+  assert.deepEqual(outcome, {
+    attempted: false,
+    delivered: false,
+    method: null
+  });
+});
+
+test("terminateProcessTree does not treat a Windows preflight permission error as missing", () => {
+  let taskkillCalled = false;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      taskkillCalled = true;
+      return {
+        command,
+        args,
+        status: 0,
+        signal: null,
+        stdout: "",
+        stderr: "",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      const error = new Error("EPERM");
+      error.code = "EPERM";
+      throw error;
+    }
+  });
+
+  assert.equal(taskkillCalled, true);
+  assert.equal(outcome.delivered, true);
+  assert.equal(outcome.method, "taskkill");
+});
+
+test("terminateProcessTree preserves the Windows ENOENT fallback", () => {
+  const killCalls = [];
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      const error = new Error("ENOENT");
+      error.code = "ENOENT";
+      return {
+        command,
+        args,
+        status: 0,
+        signal: null,
+        stdout: "",
+        stderr: "",
+        error
+      };
+    },
+    killImpl(pid, signal) {
+      killCalls.push([pid, signal]);
+    }
+  });
+
+  assert.deepEqual(killCalls, [
+    [1234, 0],
+    [1234, undefined]
+  ]);
+  assert.deepEqual(outcome, {
+    attempted: true,
+    delivered: true,
+    method: "kill"
+  });
+});
+
+test("terminateProcessTree leaves the non-Windows process-group path unchanged", () => {
+  const killCalls = [];
+  const outcome = terminateProcessTree(1234, {
+    platform: "linux",
+    killImpl(pid, signal) {
+      killCalls.push([pid, signal]);
+    }
+  });
+
+  assert.deepEqual(killCalls, [[-1234, "SIGTERM"]]);
+  assert.deepEqual(outcome, {
+    attempted: true,
+    delivered: true,
+    method: "process-group"
+  });
 });
 
 test("a dead group leader with a surviving descendant still counts as a running tree", { skip: process.platform === "win32" }, async () => {
@@ -369,6 +558,10 @@ test("forceKillProcessTree reaches descendants on Windows instead of signalling
 test("forceKillProcessTree never throws when the Windows tree cannot be reached", () => {
   const outcome = forceKillProcessTree(1234, {
     platform: "win32",
+    // A root that reads as live, so the pre-kill probe does not short-circuit the taskkill this
+    // test is about. Injected rather than real: otherwise the outcome would depend on whether
+    // pid 1234 happens to exist on the machine running the suite.
+    killImpl() {},
     runCommandImpl(command, args) {
       return {
         command,

tests/runtime.test.mjs

@@ -48,7 +48,11 @@ test("setup reports ready when fake codex is installed and authenticated", () =>
 test("setup is ready without npm when Codex is already installed and authenticated", () => {
   const binDir = makeTempDir();
   installFakeCodex(binDir);
-  fs.symlinkSync(process.execPath, path.join(binDir, "node"));
+  if (process.platform === "win32") {
+    fs.writeFileSync(path.join(binDir, "node.cmd"), `@echo off\r\n"${process.execPath}" %*\r\n`, "utf8");
+  } else {
+    fs.symlinkSync(process.execPath, path.join(binDir, "node"));
+  }
 
   const result = run("node", [SCRIPT, "setup", "--json"], {
     cwd: ROOT,