diff --git a/README.md b/README.md index 862064378..d8758ee55 100644 --- a/README.md +++ b/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: diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 914c7387a..f970af9ef 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/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") { diff --git a/tests/git.test.mjs b/tests/git.test.mjs index 5b5c266ee..09e1c367a 100644 --- a/tests/git.test.mjs +++ b/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); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index c9f7f2924..da7620296 100644 --- a/tests/process.test.mjs +++ b/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, diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index b26579775..ddb9f7620 100644 --- a/tests/runtime.test.mjs +++ b/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,