diff --git a/plugins/codex/scripts/lib/broker-endpoint.mjs b/plugins/codex/scripts/lib/broker-endpoint.mjs index 8abdcc71a..664c58494 100644 --- a/plugins/codex/scripts/lib/broker-endpoint.mjs +++ b/plugins/codex/scripts/lib/broker-endpoint.mjs @@ -13,7 +13,7 @@ export function createBrokerEndpoint(sessionDir, platform = process.platform) { return `pipe:\\\\.\\pipe\\${pipeName}`; } - return `unix:${path.join(sessionDir, "broker.sock")}`; + return `unix:${path.posix.join(sessionDir, "broker.sock")}`; } export function parseBrokerEndpoint(endpoint) { diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..184e98aed 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -40,19 +40,30 @@ export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { return false; } -export async function sendBrokerShutdown(endpoint) { +export async function sendBrokerShutdown(endpoint, timeoutMs = 2000) { await new Promise((resolve) => { const socket = connectToEndpoint(endpoint); + const timer = setTimeout(() => { + socket.destroy(); + resolve(); + }, timeoutMs); socket.setEncoding("utf8"); socket.on("connect", () => { socket.write(`${JSON.stringify({ id: 1, method: "broker/shutdown", params: {} })}\n`); }); socket.on("data", () => { + clearTimeout(timer); socket.end(); resolve(); }); - socket.on("error", resolve); - socket.on("close", resolve); + socket.on("error", () => { + clearTimeout(timer); + resolve(); + }); + socket.on("close", () => { + clearTimeout(timer); + resolve(); + }); }); } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..ab09ec9f3 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -50,8 +50,13 @@ export function binaryAvailable(command, versionArgs = ["--version"], options = return { available: true, detail: result.stdout.trim() || result.stderr.trim() || "ok" }; } -function looksLikeMissingProcessMessage(text) { - return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); +function isProcessAlive(pid, killImpl) { + try { + killImpl(pid, 0); + return true; + } catch (error) { + return error?.code !== "ESRCH"; + } } export function terminateProcessTree(pid, options = {}) { @@ -64,6 +69,12 @@ export function terminateProcessTree(pid, options = {}) { const killImpl = options.killImpl ?? process.kill.bind(process); if (platform === "win32") { + // Probe the root before calling taskkill instead of parsing its localized "not found" + // message: a root that is already gone is reported as not delivered in every language. + if (!isProcessAlive(pid, killImpl)) { + return { attempted: false, delivered: false, method: null }; + } + const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], { cwd: options.cwd, env: options.env @@ -73,9 +84,13 @@ export function terminateProcessTree(pid, options = {}) { return { attempted: true, delivered: 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 }; + // taskkill /T walks the tree, then terminates each entry; a descendant that exits in + // between (short-lived git/cmd helpers) makes taskkill report "not supported" and a + // non-zero status even though the root was killed. Trust the root's liveness instead. + // `delivered` therefore describes the root only: like the process-group SIGTERM on + // other platforms, it does not prove that every descendant is gone. + if (!result.error && !isProcessAlive(pid, killImpl)) { + return { attempted: true, delivered: true, method: "taskkill", result }; } if (result.error?.code === "ENOENT") { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..e9e779d36 --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,43 @@ +import fs from "node:fs"; +import net from "node:net"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { sendBrokerShutdown } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { createBrokerEndpoint, parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; + +test("sendBrokerShutdown times out when the broker accepts but never responds", async (t) => { + const sessionDir = fs.mkdtempSync(path.join(os.tmpdir(), "codex-plugin-broker-test-")); + const endpoint = createBrokerEndpoint(sessionDir); + const { path: endpointPath } = parseBrokerEndpoint(endpoint); + const sockets = new Set(); + const server = net.createServer((socket) => { + sockets.add(socket); + socket.on("close", () => sockets.delete(socket)); + }); + + t.after(async () => { + for (const socket of sockets) { + socket.destroy(); + } + await new Promise((resolve) => server.close(resolve)); + if (process.platform !== "win32" && fs.existsSync(endpointPath)) { + fs.unlinkSync(endpointPath); + } + fs.rmSync(sessionDir, { recursive: true, force: true }); + }); + + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(endpointPath, resolve); + }); + + const startedAt = Date.now(); + await sendBrokerShutdown(endpoint, 50); + const elapsedMs = Date.now() - startedAt; + + assert.ok(elapsedMs >= 40, `expected timeout path, completed after ${elapsedMs}ms`); + assert.ok(elapsedMs < 1000, `shutdown timeout exceeded test budget: ${elapsedMs}ms`); +}); 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 80e0715b0..5e7d608f4 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -19,8 +19,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); } }); @@ -32,7 +33,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) { @@ -41,15 +43,202 @@ 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.match(outcome.result.stdout, /not found/i); + 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.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" + }); }); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..541834b2d 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -47,7 +47,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, @@ -220,6 +224,7 @@ test("transfer delegates the current Claude session directly to native import", env: { ...buildEnv(binDir), HOME: home, + USERPROFILE: home, CODEX_HOME: path.join(home, ".codex"), CODEX_COMPANION_TRANSCRIPT_PATH: sourcePath } @@ -265,6 +270,7 @@ test("transfer reports an actionable upgrade error when native import is unsuppo env: { ...buildEnv(binDir), HOME: home, + USERPROFILE: home, CODEX_HOME: path.join(home, ".codex") } }); @@ -295,6 +301,7 @@ test("transfer fails visibly when native import completes without a ledger recor env: { ...buildEnv(binDir), HOME: home, + USERPROFILE: home, CODEX_HOME: path.join(home, ".codex") } }); @@ -320,7 +327,7 @@ test("transfer rejects sources outside the Claude projects directory", () => { const result = run("node", [SCRIPT, "transfer", "--source", sourcePath], { cwd: repo, - env: { ...buildEnv(binDir), HOME: home } + env: { ...buildEnv(binDir), HOME: home, USERPROFILE: home } }); assert.notEqual(result.status, 0);