From dc937b282499b14f7294169ff22ba364df112bbb Mon Sep 17 00:00:00 2001
From: axisrow <axisrow@users.noreply.github.com>
Date: Sun, 19 Jul 2026 09:06:20 +0800
Subject: [PATCH 1/3] test: reap orphaned brokers and clean temp dirs on npm
 test exit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes openai/codex-plugin-cc#163.

`node --test tests/*.test.mjs` was leaving behind detached
app-server-broker.mjs processes (all reparented to PID 1) plus thousands of
temp dirs. Root cause: production spawnBrokerProcess spawns the broker
`detached:true` + `unref()` so it survives the parent — correct in production,
fatal in tests, because node:test does not forward signals/exit to detached
grandchildren.

The test process cannot capture the broker pid at spawn (it is born inside a
grandchild subprocess via production code), so the harness tracks workspace
cws instead and recovers {endpoint, pid, sessionDir} deterministically from
broker.json at teardown. This matches the sketch Wintersta7e proposed in #163.

Two-layer teardown, forced by node:test's default --test-isolation=process
(each test file is its own worker):

- tests/helpers.mjs: a registry of temp dirs and broker-spawning workspaces.
  makeTempDir registers the dir; run() registers its cwd when it lives under
  the temp root. Worker process.on('exit')/SIGINT/SIGTERM handlers reap every
  tracked broker (loadBrokerSession -> sendBrokerShutdown ->
  teardownBrokerSession -> clearBrokerSession, the SessionEnd sequence) and
  rm -rf the tracked dirs. Pid-reuse safety: only tree-kill when the endpoint
  probe confirms the broker is live — mirrors loadReusableBrokerSessionUnlocked
  (broker-lifecycle.mjs:193-216), so a stale broker.json whose pid the OS
  recycled into an unrelated process cannot trigger killing the wrong process.

- tests/test-global-setup.mjs: a --test-global-setup module that runs once in
  the parent after all workers exit. Sweeps broker.json under every
  codex-plugin-test-data-* root for brokers a crashed worker missed, then
  removes those roots.

- tests/fake-codex-fixture.mjs: buildEnv now points CLAUDE_PLUGIN_DATA at one
  ephemeral per-worker temp root (codex-plugin-test-data-*), so tests no
  longer inherit the real plugin data dir via `...process.env`. This is the
  env-inheritance leak axisrow flagged in #163 and #487 part 1. process.env is
  set too, so the worker's own resolveStateDir agrees with the companions it
  spawns.

- package.json: test script gains --test-global-setup and --test-force-exit
  (the latter so process.exit fires and the exit handlers run).

Verified: a full `npm test` run adds 0 new broker processes, 0 new cxc-*
dirs, and 0 leftover codex-plugin-test-data-* dirs (measured before/after).
Same under SIGINT mid-run. Production code (plugins/codex/scripts/) is
untouched — this is test-only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
---
 package.json                 |   2 +-
 tests/fake-codex-fixture.mjs |  43 +++++++++-
 tests/helpers.mjs            | 152 ++++++++++++++++++++++++++++++++++-
 tests/test-global-setup.mjs  | 129 +++++++++++++++++++++++++++++
 4 files changed, 323 insertions(+), 3 deletions(-)
 create mode 100644 tests/test-global-setup.mjs

diff --git a/package.json b/package.json
index 7fbe71e2..7883a840 100644
--- a/package.json
+++ b/package.json
@@ -13,7 +13,7 @@
     "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 tests/*.test.mjs --test-global-setup=./tests/test-global-setup.mjs --test-force-exit"
   },
   "devDependencies": {
     "@types/node": "^25.5.0",
diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs
index f6552e43..21ee071e 100644
--- a/tests/fake-codex-fixture.mjs
+++ b/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 { registerTrackedTempDir, 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,51 @@ 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. Pointing state at a
+// known temp root is also what lets the parent sweep find every broker.json
+// spawned by any worker.
+//
+// We deliberately do NOT mutate process.env here: some tests spawn the
+// companion without passing env (so it inherits process.env) and read state
+// back through resolveStateDir in the same process — both paths must agree.
+// Setting CLAUDE_PLUGIN_DATA only on the returned env object keeps that
+// invariant for tests that go through buildEnv, while tests that manage their
+// own env (or rely on the unset fallback) keep working unchanged.
+// 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-"));
+    registerTrackedTempDir(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"
diff --git a/tests/helpers.mjs b/tests/helpers.mjs
index d6981197..c0d28e0f 100644
--- a/tests/helpers.mjs
+++ b/tests/helpers.mjs
@@ -4,8 +4,20 @@ 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) {
@@ -13,6 +25,13 @@ export function writeExecutable(filePath, source) {
 }
 
 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,134 @@ 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();
+const trackedWorkspaces = new Set();
+let workerTeardownStarted = false;
+let workerHandlersRegistered = false;
+
+/** Register any temp path (workspace, binDir, plugin-data root) for rm -rf at exit. */
+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);
+  }
+}
+
+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;
+
+  const reapAndClean = (signal) => {
+    if (workerTeardownStarted) {
+      return;
+    }
+    workerTeardownStarted = true;
+    try {
+      reapWorkerBrokers(signal != null);
+    } catch {
+      // Best-effort: never let teardown mask the original failure.
+    }
+    try {
+      rmTrackedTempDirs();
+    } catch {}
+  };
+
+  // 'exit' is synchronous-only — no awaits, no RPC. We can only unlink files
+  // and rm dirs here. Process reaping is left to the async signal handlers
+  // (when a signal arrives) and to the parent global-teardown sweep.
+  process.on("exit", () => reapAndClean(null));
+
+  process.on("SIGINT", () => {
+    reapAndClean("SIGINT");
+    process.exit(130);
+  });
+  process.on("SIGTERM", () => {
+    reapAndClean("SIGTERM");
+    process.exit(143);
+  });
+}
+
+/**
+ * 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).
+ */
+async function reapWorkerBrokers(canAwait) {
+  for (const cwd of trackedWorkspaces) {
+    const session = loadBrokerSession(cwd);
+    if (!session) {
+      continue;
+    }
+    const ready = canAwait && session.endpoint
+      ? 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.
+    }
+  }
+}
+
+async function safeAwait(promise, fallback) {
+  try {
+    return await promise;
+  } catch {
+    return fallback;
+  }
+}
diff --git a/tests/test-global-setup.mjs b/tests/test-global-setup.mjs
new file mode 100644
index 00000000..d8215b2a
--- /dev/null
+++ b/tests/test-global-setup.mjs
@@ -0,0 +1,129 @@
+// Global test setup/teardown for the parent test-runner process.
+//
+// node:test defaults to --test-isolation=process, so each *.test.mjs runs in
+// its own worker child. Workers spawn detached app-server-broker.mjs processes
+// (via production spawnBrokerProcess) and register their own process.exit /
+// SIGINT handlers in tests/helpers.mjs to reap what they spawned. This module
+// is the catch-all that runs ONCE in the parent after all workers exit: it
+// sweeps broker.json files left on disk for any broker a worker missed
+// (crashed worker, wedged broker, abnormal exit that beat the worker handler)
+// and removes the ephemeral plugin-data roots.
+//
+// See .claude/plans/506-swirling-bird.md and openai/codex-plugin-cc#163.
+
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import process from "node:process";
+
+import {
+  sendBrokerShutdown,
+  waitForBrokerEndpoint,
+  teardownBrokerSession
+} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs";
+import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs";
+
+const TEST_DATA_PREFIX = "codex-plugin-test-data-";
+
+export async function globalSetup() {
+  registerParentHandlers();
+  return async function globalTeardown() {
+    await sweepOrphanedBrokers();
+    rmTestPluginDataDirs();
+  };
+}
+
+function registerParentHandlers() {
+  // On abnormal exit (SIGINT/SIGTERM), run the best-effort sync-ish sweep before
+  // exiting. Workers also reap on their own SIGINT, but a worker may die before
+  // its handler runs; this is the parent's safety net.
+  const onSignal = (signal) => {
+    sweepOrphanedBrokers().catch(() => {}).finally(() => process.exit(0));
+  };
+  process.on("SIGINT", () => onSignal("SIGINT"));
+  process.on("SIGTERM", () => onSignal("SIGTERM"));
+}
+
+/**
+ * Sweep every ephemeral plugin-data root under os.tmpdir() for broker.json
+ * files and reap the brokers they point at. Mirrors the SessionEnd sequence
+ * (session-lifecycle-hook.mjs) and loadReusableBrokerSessionUnlocked's
+ * pid-reuse safety: only tree-kill when the endpoint probe confirms the
+ * broker is live (broker-lifecycle.mjs:193-216).
+ */
+async function sweepOrphanedBrokers() {
+  for (const pluginDataDir of listTestPluginDataDirs()) {
+    const stateRoot = path.join(pluginDataDir, "state");
+    let workspaces;
+    try {
+      workspaces = fs.readdirSync(stateRoot, { withFileTypes: true })
+        .filter((entry) => entry.isDirectory())
+        .map((entry) => path.join(stateRoot, entry.name));
+    } catch {
+      continue;
+    }
+    for (const stateDir of workspaces) {
+      await reapStateDirBroker(stateDir);
+    }
+  }
+}
+
+async function reapStateDirBroker(stateDir) {
+  const brokerFile = path.join(stateDir, "broker.json");
+  let session;
+  try {
+    session = JSON.parse(fs.readFileSync(brokerFile, "utf8"));
+  } catch {
+    return; // no broker.json here — nothing to reap
+  }
+
+  const endpoint = session.endpoint ?? null;
+  const ready = endpoint ? await safeAwait(waitForBrokerEndpoint(endpoint, 150), false) : false;
+  // Only trust the recorded pid for tree-kill when the endpoint probe confirms
+  // the broker is live — a stale session may point at a recycled pid.
+  const killProcess = ready ? terminateProcessTree : null;
+  if (ready && endpoint) {
+    await safeAwait(sendBrokerShutdown(endpoint, 500));
+  }
+  teardownBrokerSession({
+    endpoint,
+    pidFile: session.pidFile ?? null,
+    logFile: session.logFile ?? null,
+    sessionDir: session.sessionDir ?? null,
+    pid: session.pid ?? null,
+    killProcess
+  });
+  try {
+    fs.unlinkSync(brokerFile);
+  } catch {}
+}
+
+function rmTestPluginDataDirs() {
+  for (const dir of listTestPluginDataDirs()) {
+    try {
+      fs.rmSync(dir, { recursive: true, force: true });
+    } catch {
+      // Best-effort.
+    }
+  }
+}
+
+function listTestPluginDataDirs() {
+  let entries;
+  try {
+    entries = fs.readdirSync(os.tmpdir(), { withFileTypes: true });
+  } catch {
+    return [];
+  }
+  return entries
+    .filter((entry) => entry.isDirectory() && entry.name.startsWith(TEST_DATA_PREFIX))
+    .map((entry) => path.join(os.tmpdir(), entry.name));
+}
+
+async function safeAwait(promise, fallback) {
+  try {
+    return await promise;
+  } catch {
+    return fallback;
+  }
+}

From 9c798cb3182650e8ca5c28892b48df21517b7a55 Mon Sep 17 00:00:00 2001
From: axisrow <axisrow@users.noreply.github.com>
Date: Sun, 19 Jul 2026 11:01:37 +0800
Subject: [PATCH 2/3] fix(test-teardown): activate parent sweep, await worker
 reap, preserve broker state
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses Codex review of PR #5 (verdict needs-attention / NO-SHIP). All three
critical findings verified empirically and fixed:

1. Parent sweep was never activated. `--test-global-setup` placed AFTER the
   `tests/*.test.mjs` glob was silently ignored by node (the shell-expanded
   positional paths come first, so the flag never reaches the option parser).
   Reordered: flags first, paths last. Verified with a bad-path smoke check
   (before: exit 0 / ignored; after: fails to resolve the module).

2. globalTeardown was returned from globalSetup, but node:test looks for a
   NAMED `globalTeardown` export — the returned fn is never called. Split into
   two named exports. Verified both fire (instrumented markers).

3. Worker sync `exit` path destroyed broker.json / pid-file / sessionDir /
   plugin-data-root WITHOUT terminating the broker first. A live (busy/wedged)
   broker would be orphaned AND undiscoverable by the parent sweep. The sync
   exit path now leaves all broker metadata and the plugin-data root intact
   for the parent globalTeardown to reap; only non-broker temp dirs (workspace,
   binDir, home) are removed on sync exit. The async signal path (SIGINT/
   SIGTERM) was also not awaiting reapWorkerBrokers before process.exit — now
   awaits the full probe→shutdown→tree-kill→remove sequence, then exits.

Also:
- Removed --test-force-exit: it aborted globalTeardown mid-await (trace showed
  START but never rmData OK). Without it, node awaits the async teardown and
  the sweep completes — verified: parent reaped all brokers and removed all
  codex-plugin-test-data-* roots.
- Raised engines.node to >=24.0.0 (--test-global-setup was added in Node 24).
- Removed a duplicated stale comment block in fake-codex-fixture.mjs.

Verification (full npm test): brokers 150→150 (0 leaked), test-data roots
removed entirely (parent sweep ran to completion). SIGINT mid-run: 0 leaked.
globalSetup + globalTeardown both confirmed firing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
---
 package.json                 |  4 +-
 tests/fake-codex-fixture.mjs | 17 +-------
 tests/helpers.mjs            | 81 ++++++++++++++++++++++++++----------
 tests/test-global-setup.mjs  | 13 ++++--
 4 files changed, 71 insertions(+), 44 deletions(-)

diff --git a/package.json b/package.json
index 7883a840..7efba001 100644
--- a/package.json
+++ b/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-global-setup=./tests/test-global-setup.mjs --test-force-exit"
+    "test": "node --test --test-global-setup=./tests/test-global-setup.mjs tests/*.test.mjs"
   },
   "devDependencies": {
     "@types/node": "^25.5.0",
diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs
index 21ee071e..7f2f2940 100644
--- a/tests/fake-codex-fixture.mjs
+++ b/tests/fake-codex-fixture.mjs
@@ -3,7 +3,7 @@ import os from "node:os";
 import path from "node:path";
 import process from "node:process";
 
-import { registerTrackedTempDir, 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");
@@ -765,19 +765,6 @@ 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. Pointing state at a
-// known temp root is also what lets the parent sweep find every broker.json
-// spawned by any worker.
-//
-// We deliberately do NOT mutate process.env here: some tests spawn the
-// companion without passing env (so it inherits process.env) and read state
-// back through resolveStateDir in the same process — both paths must agree.
-// Setting CLAUDE_PLUGIN_DATA only on the returned env object keeps that
-// invariant for tests that go through buildEnv, while tests that manage their
-// own env (or rely on the unset fallback) keep working unchanged.
 // 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
@@ -795,7 +782,7 @@ let testPluginDataDir = null;
 export function getTestPluginDataDir() {
   if (!testPluginDataDir) {
     testPluginDataDir = fs.mkdtempSync(path.join(os.tmpdir(), "codex-plugin-test-data-"));
-    registerTrackedTempDir(testPluginDataDir);
+    registerPluginDataDir(testPluginDataDir);
     process.env.CLAUDE_PLUGIN_DATA = testPluginDataDir;
   }
   return testPluginDataDir;
diff --git a/tests/helpers.mjs b/tests/helpers.mjs
index c0d28e0f..77d6ab35 100644
--- a/tests/helpers.mjs
+++ b/tests/helpers.mjs
@@ -63,6 +63,12 @@ export function initGitRepo(cwd) {
 // ---------------------------------------------------------------------------
 
 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;
@@ -81,6 +87,14 @@ export function registerBrokerWorkspace(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 {
@@ -98,34 +112,38 @@ function ensureWorkerExitHandlers() {
   }
   workerHandlersRegistered = true;
 
-  const reapAndClean = (signal) => {
+  // 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;
-    try {
-      reapWorkerBrokers(signal != null);
-    } catch {
-      // Best-effort: never let teardown mask the original failure.
-    }
-    try {
-      rmTrackedTempDirs();
-    } catch {}
+    Promise.resolve(reapWorkerBrokers(true))
+      .catch(() => {})
+      .finally(() => {
+        try { rmTrackedTempDirs(); } catch {}
+        try { rmPluginDataDirs(); } catch {}
+        process.exit(signal === "SIGINT" ? 130 : 143);
+      });
   };
 
-  // 'exit' is synchronous-only — no awaits, no RPC. We can only unlink files
-  // and rm dirs here. Process reaping is left to the async signal handlers
-  // (when a signal arrives) and to the parent global-teardown sweep.
-  process.on("exit", () => reapAndClean(null));
-
-  process.on("SIGINT", () => {
-    reapAndClean("SIGINT");
-    process.exit(130);
-  });
-  process.on("SIGTERM", () => {
-    reapAndClean("SIGTERM");
-    process.exit(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"));
 }
 
 /**
@@ -137,14 +155,19 @@ function ensureWorkerExitHandlers() {
  * 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(canAwait) {
+async function reapWorkerBrokers() {
   for (const cwd of trackedWorkspaces) {
     const session = loadBrokerSession(cwd);
     if (!session) {
       continue;
     }
-    const ready = canAwait && session.endpoint
+    const ready = session.endpoint
       ? safeAwait(waitForBrokerEndpoint(session.endpoint, 150), false)
       : false;
     const killProcess = ready ? terminateProcessTree : null;
@@ -173,6 +196,18 @@ function rmTrackedTempDirs() {
   }
 }
 
+// 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;
diff --git a/tests/test-global-setup.mjs b/tests/test-global-setup.mjs
index d8215b2a..39898cbb 100644
--- a/tests/test-global-setup.mjs
+++ b/tests/test-global-setup.mjs
@@ -25,12 +25,17 @@ import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs";
 
 const TEST_DATA_PREFIX = "codex-plugin-test-data-";
 
+// node:test's global teardown contract: the runner looks for a NAMED
+// `globalTeardown` export on the module — the function RETURNED by globalSetup
+// is NOT called (verified empirically against Node 25.9.0). Both must be
+// named exports; returning teardown from setup is a no-op.
 export async function globalSetup() {
   registerParentHandlers();
-  return async function globalTeardown() {
-    await sweepOrphanedBrokers();
-    rmTestPluginDataDirs();
-  };
+}
+
+export async function globalTeardown() {
+  await sweepOrphanedBrokers();
+  rmTestPluginDataDirs();
 }
 
 function registerParentHandlers() {

From 667eb3d66b67b165547efb7ebce039a0ab568f17 Mon Sep 17 00:00:00 2001
From: axisrow <axisrow@users.noreply.github.com>
Date: Sun, 19 Jul 2026 11:38:01 +0800
Subject: [PATCH 3/3] fix(test-teardown): restore await on endpoint probe +
 bump CI/Node to 24
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Cycle-2 review findings (Codex + Claude subagent):

1. pid-reuse guard regressed in cycle 1: reapWorkerBrokers dropped the `await`
   on `safeAwait(waitForBrokerEndpoint(...))`, so `ready` was an unresolved
   Promise (always truthy) → killProcess selected `terminateProcessTree`
   unconditionally, even for a dead endpoint whose pid the OS may have
   recycled into an unrelated process. The exact invariant the PR claims to
   protect. `await` restored; killProcess once again only set when the probe
   confirms the broker is live.

2. --test-global-setup needs Node 24, but CI ran Node 22 and engines/lockfile/
   README still said 18.18. Bumped all four in lockstep: CI workflow 22→24,
   README "Node.js 18.18 or later" → "Node.js 24 or later", package-lock
   engines mirrored (already >=24 in package.json from cycle 1).

Cycle-2 cleanup (non-blocking, applied in the final pass):
- reapWorkerBrokers() called with a stray `true` arg from the old signature;
  removed.
- registerTrackedTempDir docstring clarified: it covers non-broker temp dirs
  only; plugin-data roots go through registerPluginDataDir.

Verified: full npm test → 0 broker leak, 0 leftover test-data dirs; SIGINT
mid-run → 0 leftover. Both reviewers (Codex + Claude subagent) ship.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
---
 .github/workflows/pull-request-ci.yml | 2 +-
 README.md                             | 2 +-
 package-lock.json                     | 2 +-
 tests/helpers.mjs                     | 7 ++++---
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/.github/workflows/pull-request-ci.yml b/.github/workflows/pull-request-ci.yml
index ebcff0b6..51a8d872 100644
--- a/.github/workflows/pull-request-ci.yml
+++ b/.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
diff --git a/README.md b/README.md
index ee16ff99..12e59baf 100644
--- a/README.md
+++ b/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
 
diff --git a/package-lock.json b/package-lock.json
index 9a1ee704..49646cf3 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -13,7 +13,7 @@
         "typescript": "^6.0.2"
       },
       "engines": {
-        "node": ">=18.18.0"
+        "node": ">=24.0.0"
       }
     },
     "node_modules/@types/node": {
diff --git a/tests/helpers.mjs b/tests/helpers.mjs
index 77d6ab35..26ea26c3 100644
--- a/tests/helpers.mjs
+++ b/tests/helpers.mjs
@@ -73,7 +73,8 @@ const trackedWorkspaces = new Set();
 let workerTeardownStarted = false;
 let workerHandlersRegistered = false;
 
-/** Register any temp path (workspace, binDir, plugin-data root) for rm -rf at exit. */
+/** 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);
@@ -124,7 +125,7 @@ function ensureWorkerExitHandlers() {
       return;
     }
     workerTeardownStarted = true;
-    Promise.resolve(reapWorkerBrokers(true))
+    Promise.resolve(reapWorkerBrokers())
       .catch(() => {})
       .finally(() => {
         try { rmTrackedTempDirs(); } catch {}
@@ -168,7 +169,7 @@ async function reapWorkerBrokers() {
       continue;
     }
     const ready = session.endpoint
-      ? safeAwait(waitForBrokerEndpoint(session.endpoint, 150), false)
+      ? await safeAwait(waitForBrokerEndpoint(session.endpoint, 150), false)
       : false;
     const killProcess = ready ? terminateProcessTree : null;
     if (ready && session.endpoint) {