test: reap orphaned brokers and clean temp dirs on npm test exit (closes openai#163)

.github/workflows/pull-request-ci.yml

@@ -19,7 +19,7 @@ jobs:
       - name: Set up Node.js
         uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0
         with:
-          node-version: 22
+          node-version: 24
           cache: npm
 
       - name: Install dependencies

README.md

@@ -17,7 +17,7 @@ they already have.
 
 - **ChatGPT subscription (incl. Free) or OpenAI API key.**
   - Usage will contribute to your Codex usage limits. [Learn more](https://developers.openai.com/codex/pricing).
-- **Node.js 18.18 or later**
+- **Node.js 24 or later**
 
 ## Install
 

package.json

@@ -6,14 +6,14 @@
   "description": "Use Codex from Claude Code to review code or delegate tasks.",
   "license": "Apache-2.0",
   "engines": {
-    "node": ">=18.18.0"
+    "node": ">=24.0.0"
   },
   "scripts": {
     "bump-version": "node scripts/bump-version.mjs",
     "check-version": "node scripts/bump-version.mjs --check",
     "prebuild": "mkdir -p plugins/codex/.generated/app-server-types && codex app-server generate-ts --out plugins/codex/.generated/app-server-types",
     "build": "tsc -p tsconfig.app-server.json",
-    "test": "node --test tests/*.test.mjs"
+    "test": "node --test --test-global-setup=./tests/test-global-setup.mjs tests/*.test.mjs"
   },
   "devDependencies": {
     "@types/node": "^25.5.0",

tests/fake-codex-fixture.mjs

@@ -1,8 +1,9 @@
 import fs from "node:fs";
+import os from "node:os";
 import path from "node:path";
 import process from "node:process";
 
-import { writeExecutable } from "./helpers.mjs";
+import { registerPluginDataDir, writeExecutable } from "./helpers.mjs";
 
 export function installFakeCodex(binDir, behavior = "review-ok", version = "codex-cli test") {
   const statePath = path.join(binDir, "fake-codex-state.json");
@@ -764,11 +765,38 @@ rl.on("line", (line) => {
   }
 }
 
+// One ephemeral CLAUDE_PLUGIN_DATA per worker process. Without this, buildEnv
+// spreads ...process.env and inherits the real plugin data dir (or the
+// $TMPDIR/codex-companion fallback), so tests write broker.json/state.json
+// into the live plugin's data and leave them behind.
+//
+// We set process.env.CLAUDE_PLUGIN_DATA here (not just the returned env) so
+// the worker process itself resolves state to the same root as the companion
+// subprocesses it spawns. Tests like broker-lifecycle's "concurrent startup"
+// spawn a child that calls ensureBrokerSession with buildEnv()'s env (root A),
+// then call loadBrokerSession(repo) in-process — without this mutation that
+// in-process read would resolve through the unset fallback (root B) and miss
+// the broker.json the child wrote. Because the temp root lives under
+// os.tmpdir(), state.test.mjs's startsWith(os.tmpdir()) assertion still holds.
+let testPluginDataDir = null;
+export function getTestPluginDataDir() {
+  if (!testPluginDataDir) {
+    testPluginDataDir = fs.mkdtempSync(path.join(os.tmpdir(), "codex-plugin-test-data-"));
+    registerPluginDataDir(testPluginDataDir);
+    process.env.CLAUDE_PLUGIN_DATA = testPluginDataDir;
+  }
+  return testPluginDataDir;
+}
+
 export function buildEnv(binDir) {
   const sep = process.platform === "win32" ? ";" : ":";
   return {
     ...process.env,
     PATH: `${binDir}${sep}${process.env.PATH}`,
+    // Isolate plugin state from the host. Per-workspace state is still
+    // namespaced under this root by resolveStateDir (sha256(realpath(cwd))),
+    // so concurrent test workspaces do not collide.
+    CLAUDE_PLUGIN_DATA: getTestPluginDataDir(),
     // Production keeps an idle broker warm for 15 minutes. Tests only need a
     // brief reuse window and should not leave dozens of detached helpers.
     CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "2000"

tests/helpers.mjs

@@ -4,15 +4,34 @@ import path from "node:path";
 import process from "node:process";
 import { spawnSync } from "node:child_process";
 
+import {
+  loadBrokerSession,
+  sendBrokerShutdown,
+  waitForBrokerEndpoint,
+  teardownBrokerSession,
+  clearBrokerSession
+} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs";
+import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs";
+
 export function makeTempDir(prefix = "codex-plugin-test-") {
-  return fs.mkdtempSync(path.join(os.tmpdir(), prefix));
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
+  registerTrackedTempDir(dir);
+  ensureWorkerExitHandlers();
+  return dir;
 }
 
 export function writeExecutable(filePath, source) {
   fs.writeFileSync(filePath, source, { encoding: "utf8", mode: 0o755 });
 }
 
 export function run(command, args, options = {}) {
+  // Best-effort registration of broker-spawning workspaces. The companion and
+  // the SessionEnd hook both spawn the broker internally via production code,
+  // so the test process never sees the broker pid — but given the workspace
+  // cwd we can recover it deterministically from broker.json at teardown.
+  if (options.cwd && isUnderTestTempRoot(options.cwd)) {
+    registerBrokerWorkspace(options.cwd);
+  }
   return spawnSync(command, args, {
     cwd: options.cwd,
     env: options.env,
@@ -30,3 +49,170 @@ export function initGitRepo(cwd) {
   run("git", ["config", "commit.gpgsign", "false"], { cwd });
   run("git", ["config", "tag.gpgsign", "false"], { cwd });
 }
+
+// ---------------------------------------------------------------------------
+// Test-teardown registry + worker exit handlers
+//
+// `npm test` leaks detached `app-server-broker.mjs` processes (openai/codex-plugin-cc#163):
+// spawnBrokerProcess sets detached:true + unref() so the broker survives the
+// parent, and node:test does not forward signals to detached grandchildren.
+// We cannot capture the broker pid at spawn (it is born in a grandchild
+// subprocess), but given the workspace cwd we can read broker.json at teardown
+// and recover {endpoint, pid, sessionDir} deterministically. See the plan at
+// .claude/plans/506-swirling-bird.md.
+// ---------------------------------------------------------------------------
+
+const trackedTempDirs = new Set();
+// Plugin-data roots hold broker.json; removing them before the broker is
+// reaped would orphan a live process AND make it undiscoverable. So on the
+// sync 'exit' path (where we cannot await a reap) we must NOT remove these —
+// leave them for the parent global-teardown sweep. On the async signal path
+// we reap first, then remove them.
+const trackedPluginDataDirs = new Set();
+const trackedWorkspaces = new Set();
+let workerTeardownStarted = false;
+let workerHandlersRegistered = false;
+
+/** Register a non-broker temp path (workspace, binDir, home) for rm -rf at exit.
+ *  Plugin-data roots go through registerPluginDataDir (removed only after reap). */
+export function registerTrackedTempDir(dir) {
+  if (typeof dir === "string" && dir) {
+    trackedTempDirs.add(dir);
+  }
+}
+
+/** Register a workspace cwd that may have spawned a broker, for reaping at exit. */
+export function registerBrokerWorkspace(cwd) {
+  if (typeof cwd === "string" && cwd) {
+    trackedWorkspaces.add(cwd);
+  }
+}
+
+/** Register the per-worker ephemeral CLAUDE_PLUGIN_DATA root. Removed only AFTER
+ *  broker reaping (signal path); on sync exit it is left for the parent sweep. */
+export function registerPluginDataDir(dir) {
+  if (typeof dir === "string" && dir) {
+    trackedPluginDataDirs.add(dir);
+  }
+}
+
+function isUnderTestTempRoot(target) {
+  const tmp = os.tmpdir();
+  try {
+    const resolvedTarget = fs.realpathSync(target);
+    const resolvedTmp = fs.realpathSync(tmp);
+    return resolvedTarget.startsWith(resolvedTmp + path.sep);
+  } catch {
+    return target.startsWith(tmp + path.sep);
+  }
+}
+
+function ensureWorkerExitHandlers() {
+  if (workerHandlersRegistered) {
+    return;
+  }
+  workerHandlersRegistered = true;
+
+  // On a signal (SIGINT/SIGTERM) we CAN await — run the full reap (probe +
+  // shutdown + tree-kill + remove files) before exiting. On 'exit' we CANNOT
+  // await (sync-only); there we must NOT destroy broker.json / pid-file /
+  // sessionDir / the plugin-data root, because a still-running broker would be
+  // orphaned AND undiscoverable (the parent sweep keys on broker.json). Leave
+  // the broker metadata intact for the parent global-teardown to reap; only
+  // remove the non-broker temp dirs (workspace/binDir/home) we created.
+  const onSignal = (signal) => {
+    if (workerTeardownStarted) {
+      return;
+    }
+    workerTeardownStarted = true;
+    Promise.resolve(reapWorkerBrokers())
+      .catch(() => {})
+      .finally(() => {
+        try { rmTrackedTempDirs(); } catch {}
+        try { rmPluginDataDirs(); } catch {}
+        process.exit(signal === "SIGINT" ? 130 : 143);
+      });
+  };
+
+  process.on("exit", () => {
+    if (workerTeardownStarted) {
+      return;
+    }
+    workerTeardownStarted = true;
+    // Sync-only: do NOT touch broker metadata here (see onSignal comment).
+    try { rmTrackedTempDirs(); } catch {}
+  });
+
+  process.on("SIGINT", () => onSignal("SIGINT"));
+  process.on("SIGTERM", () => onSignal("SIGTERM"));
+}
+
+/**
+ * Reap every broker spawned for a tracked workspace. Mirrors the SessionEnd
+ * sequence (session-lifecycle-hook.mjs): loadBrokerSession → sendBrokerShutdown
+ * → teardownBrokerSession(killProcess: terminateProcessTree) → clearBrokerSession.
+ *
+ * Pid-reuse safety: only tree-kill when the endpoint probe confirms the broker
+ * is actually live. A stale broker.json may point at a pid the OS recycled into
+ * an unrelated process — tree-killing there would kill the wrong thing. Mirrors
+ * loadReusableBrokerSessionUnlocked (broker-lifecycle.mjs:193-216).
+ *
+ * Only called from the async signal path (onSignal) where awaits are safe.
+ * The sync 'exit' path deliberately does NOT call this — destroying broker
+ * metadata without first terminating the process would orphan a live broker
+ * and make it undiscoverable to the parent sweep.
+ */
+async function reapWorkerBrokers() {
+  for (const cwd of trackedWorkspaces) {
+    const session = loadBrokerSession(cwd);
+    if (!session) {
+      continue;
+    }
+    const ready = session.endpoint
+      ? await safeAwait(waitForBrokerEndpoint(session.endpoint, 150), false)
+      : false;
+    const killProcess = ready ? terminateProcessTree : null;
+    if (ready && session.endpoint) {
+      await safeAwait(sendBrokerShutdown(session.endpoint, 500));
+    }
+    teardownBrokerSession({
+      endpoint: session.endpoint ?? null,
+      pidFile: session.pidFile ?? null,
+      logFile: session.logFile ?? null,
+      sessionDir: session.sessionDir ?? null,
+      pid: session.pid ?? null,
+      killProcess
+    });
+    clearBrokerSession(cwd);
+  }
+}
+
+function rmTrackedTempDirs() {
+  for (const dir of trackedTempDirs) {
+    try {
+      fs.rmSync(dir, { recursive: true, force: true });
+    } catch {
+      // Best-effort.
+    }
+  }
+}
+
+// Plugin-data roots are removed ONLY after a reap, because they hold the
+// broker.json the parent sweep needs to find any broker this worker missed.
+function rmPluginDataDirs() {
+  for (const dir of trackedPluginDataDirs) {
+    try {
+      fs.rmSync(dir, { recursive: true, force: true });
+    } catch {
+      // Best-effort.
+    }
+  }
+}
+
+async function safeAwait(promise, fallback) {
+  try {
+    return await promise;
+  } catch {
+    return fallback;
+  }
+}