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/package.json b/package.json index 7fbe71e2..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": "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 f6552e43..7f2f2940 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 { 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" diff --git a/tests/helpers.mjs b/tests/helpers.mjs index d6981197..26ea26c3 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,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; + } +} diff --git a/tests/test-global-setup.mjs b/tests/test-global-setup.mjs new file mode 100644 index 00000000..39898cbb --- /dev/null +++ b/tests/test-global-setup.mjs @@ -0,0 +1,134 @@ +// 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-"; + +// 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(); +} + +export 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; + } +}