diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..4fd39296c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -6,7 +6,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; -import { resolveStateDir } from "./state.mjs"; +import { resolveStateDir, resolveStateDirCandidates } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; export const LOG_FILE_ENV = "CODEX_COMPANION_APP_SERVER_LOG_FILE"; @@ -73,17 +73,40 @@ function resolveBrokerStateFile(cwd) { return path.join(resolveStateDir(cwd), BROKER_STATE_FILE); } -export function loadBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - if (!fs.existsSync(stateFile)) { - return null; - } +// The state root is derived from ambient environment (CLAUDE_PLUGIN_DATA), +// which can differ between the invocation that registered a broker and a +// later one that looks it up -- checking every candidate root, not just the +// current invocation's primary, is what keeps a broker registered under one +// root from being orphaned by a lookup that resolves to the other. +function resolveBrokerStateFileCandidates(cwd) { + return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, BROKER_STATE_FILE)); +} - try { - return JSON.parse(fs.readFileSync(stateFile, "utf8")); - } catch { - return null; +// The single source of truth for which candidate is "the" active broker +// session: the first one that both exists *and* parses. loadBrokerSession() +// and clearBrokerSession() both build on this so they always agree -- if +// clearBrokerSession() instead selected by existence alone, a malformed +// primary file next to a valid fallback one would make it delete the +// (malformed, unused) primary while loadBrokerSession() actually returned +// and a caller tore down the fallback broker, leaving that broker's now- +// stale record behind. +function selectBrokerState(cwd) { + for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { + if (!fs.existsSync(stateFile)) { + continue; + } + try { + const session = JSON.parse(fs.readFileSync(stateFile, "utf8")); + return { stateFile, session }; + } catch { + continue; + } } + return null; +} + +export function loadBrokerSession(cwd) { + return selectBrokerState(cwd)?.session ?? null; } export function saveBrokerSession(cwd, session) { @@ -92,10 +115,22 @@ export function saveBrokerSession(cwd, session) { fs.writeFileSync(resolveBrokerStateFile(cwd), `${JSON.stringify(session, null, 2)}\n`, "utf8"); } +// Removes only the record loadBrokerSession() would return, not every +// candidate. Both call sites act on whatever loadBrokerSession() returned -- +// tearing that broker down and clearing its record -- so clearing every +// candidate here would delete an *other* root's broker.json for a broker +// that was never torn down (a real reachable case: this is precisely the +// root-split bug's own historical fallout, where the old lookup spawned a +// duplicate broker under the other root). Erasing that record makes the +// still-running duplicate permanently untrackable, which is worse than +// leaving a stale-but-discoverable file behind. Built on the same +// selectBrokerState() loadBrokerSession() uses, rather than its own +// existence-only scan, so the two never disagree about which candidate is +// "the" selected one when a malformed file sits in front of a valid one. export function clearBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - if (fs.existsSync(stateFile)) { - fs.unlinkSync(stateFile); + const selected = selectBrokerState(cwd); + if (selected) { + fs.unlinkSync(selected.stateFile); } } diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index ad152c157..f0638bf61 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -1,7 +1,7 @@ import fs from "node:fs"; import { getSessionRuntimeStatus } from "./codex.mjs"; -import { getConfig, listJobs, readJobFile, resolveJobFile } from "./state.mjs"; +import { getConfig, listJobs, readJobFile, resolveJobFileCandidates } from "./state.mjs"; import { SESSION_ID_ENV } from "./tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./workspace.mjs"; @@ -181,11 +181,12 @@ export function enrichJob(job, options = {}) { } export function readStoredJob(workspaceRoot, jobId) { - const jobFile = resolveJobFile(workspaceRoot, jobId); - if (!fs.existsSync(jobFile)) { - return null; + for (const jobFile of resolveJobFileCandidates(workspaceRoot, jobId)) { + if (fs.existsSync(jobFile)) { + return readJobFile(jobFile); + } } - return readJobFile(jobFile); + return null; } function matchJobReference(jobs, reference, predicate = () => true) { diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 2da23498f..932f8c7c4 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -26,7 +26,7 @@ function defaultState() { }; } -export function resolveStateDir(cwd) { +function workspaceStateDirName(cwd) { const workspaceRoot = resolveWorkspaceRoot(cwd); let canonicalWorkspaceRoot = workspaceRoot; try { @@ -38,9 +38,37 @@ export function resolveStateDir(cwd) { const slugSource = path.basename(workspaceRoot) || "workspace"; const slug = slugSource.replace(/[^a-zA-Z0-9._-]+/g, "-").replace(/^-+|-+$/g, "") || "workspace"; const hash = createHash("sha256").update(canonicalWorkspaceRoot).digest("hex").slice(0, 16); + return `${slug}-${hash}`; +} + +// CLAUDE_PLUGIN_DATA is only present when the current invocation runs as a +// plugin hook; a directly-invoked CLI call (or a hook whose env didn't +// propagate it) resolves to the tmpdir fallback instead. Since the state +// root is derived from ambient environment rather than anything persisted, +// two invocations for the *same* workspace can land on different roots -- +// the primary root is still the write target for new/updated state, but +// reads check every candidate so state written under one root is never +// invisible to a later invocation that resolves to the other. +function stateRootCandidates() { const pluginDataDir = process.env[PLUGIN_DATA_ENV]; - const stateRoot = pluginDataDir ? path.join(pluginDataDir, "state") : FALLBACK_STATE_ROOT_DIR; - return path.join(stateRoot, `${slug}-${hash}`); + return pluginDataDir + ? [path.join(pluginDataDir, "state"), FALLBACK_STATE_ROOT_DIR] + : [FALLBACK_STATE_ROOT_DIR]; +} + +export function resolveStateDir(cwd) { + const [primaryRoot] = stateRootCandidates(); + return path.join(primaryRoot, workspaceStateDirName(cwd)); +} + +/** + * All directories that could hold this workspace's state, primary root + * first. Use for reads that must not miss state written under a different + * root than the current invocation resolves to. + */ +export function resolveStateDirCandidates(cwd) { + const dirName = workspaceStateDirName(cwd); + return stateRootCandidates().map((root) => path.join(root, dirName)); } export function resolveStateFile(cwd) { @@ -55,26 +83,73 @@ export function ensureStateDir(cwd) { fs.mkdirSync(resolveJobsDir(cwd), { recursive: true }); } -export function loadState(cwd) { - const stateFile = resolveStateFile(cwd); +function readStateFileIfValid(stateFile) { if (!fs.existsSync(stateFile)) { - return defaultState(); + return null; } - try { - const parsed = JSON.parse(fs.readFileSync(stateFile, "utf8")); - return { - ...defaultState(), - ...parsed, - config: { - ...defaultState().config, - ...(parsed.config ?? {}) - }, - jobs: Array.isArray(parsed.jobs) ? parsed.jobs : [] - }; + return JSON.parse(fs.readFileSync(stateFile, "utf8")); } catch { + return null; + } +} + +// Unlike the broker session (at most one meaningful record per workspace, +// so "first candidate found" is a correct selection), jobs are a growing +// collection that can genuinely differ across roots -- a job started while +// CLAUDE_PLUGIN_DATA was set and another started while it was unset are +// both real and non-conflicting. Returning only the first candidate's job +// list would silently hide whichever root wasn't picked, leaving the exact +// cross-root invisibility this fix targets for status/result/cancel +// whenever *both* roots happen to have a state.json (a reachable legacy +// state after invocations alternated). So every candidate's jobs are +// merged instead, keeping the more recently updated copy if the same job +// id somehow appears in more than one. +export function loadState(cwd) { + const parsedCandidates = resolveStateDirCandidates(cwd) + .map((stateDir) => readStateFileIfValid(path.join(stateDir, STATE_FILE_NAME))) + .filter((parsed) => parsed != null); + + if (parsedCandidates.length === 0) { return defaultState(); } + + const jobsById = new Map(); + for (const parsed of parsedCandidates) { + for (const job of Array.isArray(parsed.jobs) ? parsed.jobs : []) { + const existing = jobsById.get(job.id); + if (!existing || String(job.updatedAt ?? "") > String(existing.updatedAt ?? "")) { + jobsById.set(job.id, job); + } + } + } + + // Like jobs, config can genuinely differ across roots depending on which + // invocation wrote it -- e.g. `/codex:setup --enable-review-gate` running + // without CLAUDE_PLUGIN_DATA writes stopReviewGate to the fallback root, + // which a later invocation with CLAUDE_PLUGIN_DATA set would never see if + // only the primary candidate's config were read. A boolean flag here is + // an opt-in toward stricter/safer behavior, so any candidate setting it + // true wins over a stale false elsewhere -- reconciling by "primary wins" + // could silently downgrade an explicitly-enabled gate. + const mergedConfig = { ...defaultState().config }; + for (const parsed of parsedCandidates) { + for (const [key, value] of Object.entries(parsed.config ?? {})) { + if (typeof value === "boolean") { + mergedConfig[key] = mergedConfig[key] === true || value === true; + } else if (mergedConfig[key] === undefined) { + mergedConfig[key] = value; + } + } + } + + const [primary] = parsedCandidates; + return { + ...defaultState(), + ...primary, + config: mergedConfig, + jobs: [...jobsById.values()] + }; } function pruneJobs(jobs) { @@ -107,11 +182,40 @@ export function saveState(cwd, state) { if (retainedIds.has(job.id)) { continue; } - removeJobFile(resolveJobFile(cwd, job.id)); + for (const jobFile of resolveJobFileCandidates(cwd, job.id)) { + removeJobFile(jobFile); + } removeFileIfExists(job.logFile); } fs.writeFileSync(resolveStateFile(cwd), `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); + + // previousJobs is the merged view across every candidate root (see + // loadState()), so a job dropped from state.jobs here may have + // originated entirely in a root other than the one just written above. + // Without this, that root's own state.json still holds its own + // untouched copy, and the very next loadState() merges it right back in + // -- deletions could never actually stick for a job that lives only in a + // non-primary root. Prune every other candidate root's own file down to + // the same retained set; new/updated jobs still only ever get written to + // the primary root, above -- this only ever removes, never adds or + // rewrites in place. + const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); + for (const otherStateDir of otherStateDirs) { + const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); + const otherParsed = readStateFileIfValid(otherStateFile); + const otherJobs = Array.isArray(otherParsed?.jobs) ? otherParsed.jobs : []; + const prunedOtherJobs = otherJobs.filter((job) => retainedIds.has(job.id)); + if (prunedOtherJobs.length === otherJobs.length) { + continue; + } + fs.writeFileSync( + otherStateFile, + `${JSON.stringify({ ...otherParsed, jobs: prunedOtherJobs }, null, 2)}\n`, + "utf8" + ); + } + return nextState; } @@ -189,3 +293,14 @@ export function resolveJobFile(cwd, jobId) { ensureStateDir(cwd); return path.join(resolveJobsDir(cwd), `${jobId}.json`); } + +/** + * Every path a job's detail file could be at, primary root first. A job + * listed via loadState()/listJobs() (which already searches every + * candidate root) may have had its detail file written under a different + * root than resolveJobFile()'s current primary; read lookups should not + * miss it just because it isn't in the root a fresh call resolves to. + */ +export function resolveJobFileCandidates(cwd, jobId) { + return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, JOBS_DIR_NAME, `${jobId}.json`)); +} diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 778571e6c..e49964559 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -13,7 +13,7 @@ import { sendBrokerShutdown, teardownBrokerSession } from "./lib/broker-lifecycle.mjs"; -import { loadState, resolveStateFile, saveState } from "./lib/state.mjs"; +import { loadState, saveState } from "./lib/state.mjs"; import { TRANSCRIPT_PATH_ENV } from "./lib/claude-session-transfer.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; @@ -45,11 +45,10 @@ function cleanupSessionJobs(cwd, sessionId) { } const workspaceRoot = resolveWorkspaceRoot(cwd); - const stateFile = resolveStateFile(workspaceRoot); - if (!fs.existsSync(stateFile)) { - return; - } - + // loadState() is candidate-aware and already returns an empty job list + // when nothing exists in any root; a raw existsSync() against just the + // primary candidate would miss a session whose jobs only live in the + // fallback root. const state = loadState(workspaceRoot); const removedJobs = state.jobs.filter((job) => job.sessionId === sessionId); if (removedJobs.length === 0) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..abb0555c3 --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,144 @@ +import fs from "node:fs"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { makeTempDir } from "./helpers.mjs"; +import { + clearBrokerSession, + loadBrokerSession, + saveBrokerSession +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; + +function withPluginDataDir(pluginDataDir, fn) { + const previous = process.env.CLAUDE_PLUGIN_DATA; + if (pluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + } + try { + return fn(); + } finally { + if (previous == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previous; + } + } +} + +// A broker registered while CLAUDE_PLUGIN_DATA is unset (the tmpdir +// fallback) can later be looked up by an invocation where it's set, and +// resolves the same workspace slug/hash -- only the root differs, and a +// lookup that only checks the current invocation's root orphans the broker. +// This is the direction with concrete real-world evidence in the issue. The +// reverse isn't fixable this way: an unset env var carries no trace of what +// value it previously held, so there's nothing to check beyond the +// always-known tmpdir fallback. +test("loadBrokerSession finds a session registered without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "test-endpoint", pid: 1234 }); + }); + + const session = withPluginDataDir(pluginDataDir, () => loadBrokerSession(workspace)); + + assert.deepEqual(session, { endpoint: "test-endpoint", pid: 1234 }); +}); + +test("clearBrokerSession removes a session that was registered without CLAUDE_PLUGIN_DATA, from an invocation that has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "test-endpoint", pid: 1234 }); + }); + + withPluginDataDir(pluginDataDir, () => { + clearBrokerSession(workspace); + assert.equal(loadBrokerSession(workspace), null); + }); + + // Confirm it's gone from the root it was actually written under too, not + // just invisible from the other one. + withPluginDataDir(null, () => { + assert.equal(loadBrokerSession(workspace), null); + }); +}); + +// Caught in review: this is a real reachable state, not a hypothetical -- +// it's precisely what the old (pre-fix) lookup behavior could leave behind: +// a broker registered under one root, then a *different* broker later +// registered under the other root because the old code couldn't see the +// first one. Only one of the two brokers is ever the one actually acted on +// (whichever loadBrokerSession() returns) and torn down; clearBrokerSession +// must not delete the other root's record too, since that broker was never +// shut down and losing its record would make it permanently untrackable. +test("clearBrokerSession does not delete a distinct session recorded under the other root", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "fallback-endpoint", pid: 1111 }); + }); + withPluginDataDir(pluginDataDir, () => { + saveBrokerSession(workspace, { endpoint: "plugin-data-endpoint", pid: 2222 }); + }); + + withPluginDataDir(pluginDataDir, () => { + // loadBrokerSession() would return (and a caller would tear down) the + // plugin-data-root session, since it's checked first. + clearBrokerSession(workspace); + }); + + // The fallback-root session must survive untouched -- visible whether + // checked directly (env unset) or as the sole remaining candidate (env + // set, since the plugin-data one is now gone). If clearBrokerSession had + // wrongly deleted it too, this would come back null or the check with the + // env set would find nothing. + withPluginDataDir(null, () => { + assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); + }); + withPluginDataDir(pluginDataDir, () => { + assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); + }); +}); + +// Caught in review: loadBrokerSession() skips a candidate it can't parse and +// moves on to the next one, so it can return a *fallback* session while a +// *primary* file exists but is malformed. clearBrokerSession() must select +// by the same rule (exists AND parses), not existence alone -- otherwise it +// deletes the unrelated malformed primary while leaving the valid fallback +// record behind, even though a caller just tore down the broker that record +// points to. +test("clearBrokerSession deletes the same record loadBrokerSession() returned, not just the first existing file", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "fallback-endpoint", pid: 1111 }); + }); + + withPluginDataDir(pluginDataDir, () => { + const primaryBrokerFile = path.join(resolveStateDir(workspace), "broker.json"); + fs.mkdirSync(path.dirname(primaryBrokerFile), { recursive: true }); + fs.writeFileSync(primaryBrokerFile, "{not valid json", "utf8"); + + // loadBrokerSession() skips the malformed primary and returns the valid + // fallback session. + assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); + + clearBrokerSession(workspace); + + // The malformed primary file is untouched (clearBrokerSession() doesn't + // garbage-collect unrelated corrupt files, only the selected record)... + assert.equal(fs.existsSync(primaryBrokerFile), true); + // ...but the valid fallback session -- the one actually loaded and torn + // down -- is gone. + assert.equal(loadBrokerSession(workspace), null); + }); +}); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..a3c525c49 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -8,7 +8,7 @@ import { fileURLToPath } from "node:url"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; import { loadBrokerSession, saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; -import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; +import { loadState, resolveStateDir, saveState } from "../plugins/codex/scripts/lib/state.mjs"; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const PLUGIN_ROOT = path.join(ROOT, "plugins", "codex"); @@ -2257,3 +2257,49 @@ test("setup and status honor --cwd when reading shared session runtime", () => { assert.equal(payload.sessionRuntime.mode, "shared"); assert.equal(payload.sessionRuntime.endpoint, "unix:/tmp/fake-broker.sock"); }); + +// Caught in review: cleanupSessionJobs() checked only resolveStateFile()'s +// (the primary candidate's) existence before deciding whether to look for +// jobs to clean up -- but loadState() is candidate-aware, so a session +// whose jobs live only in the fallback root (e.g. started without +// CLAUDE_PLUGIN_DATA, with SessionEnd later running with it set, flipping +// which root is primary) would be silently skipped: the early check saw no +// primary file and returned before loadState() was ever called. +test("SessionEnd cleans up a session's jobs even when they exist only in the fallback root", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + saveState(workspace, { + config: {}, + jobs: [{ id: "job-fallback-only", sessionId: "sess-under-test", status: "completed", updatedAt: "2026-08-19T00:00:00.000Z" }] + }); + + const env = { ...process.env, CLAUDE_PLUGIN_DATA: pluginDataDir }; + const cleanup = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + cwd: workspace, + session_id: "sess-under-test" + }) + }); + assert.equal(cleanup.status, 0, cleanup.stderr); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const state = loadState(workspace); + assert.equal( + state.jobs.some((job) => job.id === "job-fallback-only"), + false + ); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0f8f57cea..0903e8dcf 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -5,7 +5,15 @@ import test from "node:test"; import assert from "node:assert/strict"; import { makeTempDir } from "./helpers.mjs"; -import { resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs"; +import { + loadState, + resolveJobFile, + resolveJobLogFile, + resolveStateDir, + resolveStateFile, + saveState +} from "../plugins/codex/scripts/lib/state.mjs"; +import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; test("resolveStateDir uses a temp-backed per-workspace directory", () => { const workspace = makeTempDir(); @@ -40,6 +48,200 @@ test("resolveStateDir uses CLAUDE_PLUGIN_DATA when it is provided", () => { } }); +// The reverse (state written *with* CLAUDE_PLUGIN_DATA set, later read with +// it unset) isn't fixable this way: an unset env var carries no trace of +// what value it previously held, so there's nothing to check beyond the +// always-known tmpdir fallback. This direction is the one with concrete +// real-world evidence in the issue (a broker registered under the tmpdir +// fallback, later orphaned by a lookup that ran with CLAUDE_PLUGIN_DATA set). +test("loadState finds state written without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + saveState(workspace, { config: { stopReviewGate: true }, jobs: [] }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const state = loadState(workspace); + + assert.equal(state.config.stopReviewGate, true); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +// Caught in review: jobs are a growing collection, not a single pointer like +// the broker session -- a job started while CLAUDE_PLUGIN_DATA was set and a +// different job started while it was unset are both real and non- +// conflicting, so loadState() must merge every candidate's jobs rather than +// returning only the first state.json found (which would silently hide +// whichever root wasn't picked, for every status/result/cancel lookup, any +// time both roots happen to have a state.json -- a reachable legacy state +// after invocations alternated). +function writeStateFileDirectly(stateDir, state) { + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify(state, null, 2)}\n`, "utf8"); +} + +test("loadState merges jobs from every candidate root instead of only the first found", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + // Written directly (not via saveState()) so this test exercises only + // loadState()'s read-side merge, independent of saveState()'s own + // write/deletion-propagation behavior (covered separately below). + delete process.env.CLAUDE_PLUGIN_DATA; + writeStateFileDirectly(resolveStateDir(workspace), { + config: {}, + jobs: [{ id: "job-fallback", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + writeStateFileDirectly(resolveStateDir(workspace), { + config: {}, + jobs: [{ id: "job-plugin-data", status: "running", updatedAt: "2026-08-19T00:01:00.000Z" }] + }); + + const state = loadState(workspace); + const jobIds = state.jobs.map((job) => job.id).sort(); + + assert.deepEqual(jobIds, ["job-fallback", "job-plugin-data"]); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +// Caught in review: config (like jobs) can genuinely differ across roots -- +// e.g. `/codex:setup --enable-review-gate` running without CLAUDE_PLUGIN_DATA +// writes stopReviewGate to the fallback root, which a later invocation with +// CLAUDE_PLUGIN_DATA set (a different primary) would never see if only the +// primary candidate's config were read. Unlike the sibling test above (only +// one root has state.json, so "primary" trivially picks the only candidate +// available either way), this exercises the actual bug: *both* roots have +// state, and the non-primary one is the one with the flag enabled. +test("loadState merges config across roots, preferring an enabled boolean over a stale disabled one", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + writeStateFileDirectly(resolveStateDir(workspace), { + config: { stopReviewGate: true }, + jobs: [] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + writeStateFileDirectly(resolveStateDir(workspace), { + config: { stopReviewGate: false }, + jobs: [] + }); + + const state = loadState(workspace); + + assert.equal(state.config.stopReviewGate, true); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +// Caught in review: merging reads across roots (the previous test) isn't +// enough on its own -- saveState() only ever wrote the new job list to the +// current primary root, so a job that originated in a *different* root and +// gets filtered out (e.g. cleanupSessionJobs() during SessionEnd, which +// loads the merged view, drops jobs for the ending session, and saves the +// remainder) never actually disappears: the other root's own state.json +// still has its own untouched copy, and the next loadState() merges it +// right back in. A "removed" job could keep reporting as running forever. +test("saveState persists a job removal across every candidate root, not just the current primary", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + // Setup writes both roots directly (not via saveState()), exactly like + // the previous test -- a real caller always derives saveState()'s job + // list from a prior loadState() (see updateState()/cleanupSessionJobs() + // themselves), so seeding two roots via two independent, non-full-list + // saveState() calls wouldn't reflect any real call pattern and would + // trip the very deletion-propagation behavior under test here. + delete process.env.CLAUDE_PLUGIN_DATA; + writeStateFileDirectly(resolveStateDir(workspace), { + config: {}, + jobs: [ + { id: "job-fallback-keep", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }, + { id: "job-fallback-remove", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" } + ] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + writeStateFileDirectly(resolveStateDir(workspace), { + config: {}, + jobs: [{ id: "job-plugin-data", status: "running", updatedAt: "2026-08-19T00:01:00.000Z" }] + }); + + // Mirrors cleanupSessionJobs(): load the merged view, drop one job that + // originated entirely in the fallback root, save the remainder -- still + // with CLAUDE_PLUGIN_DATA set, the same as a real SessionEnd hook. + const merged = loadState(workspace); + saveState(workspace, { + ...merged, + jobs: merged.jobs.filter((job) => job.id !== "job-fallback-remove") + }); + + const jobIdsAfterRemoval = loadState(workspace) + .jobs.map((job) => job.id) + .sort(); + + assert.deepEqual(jobIdsAfterRemoval, ["job-fallback-keep", "job-plugin-data"]); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("readStoredJob finds a job's detail file written without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + const jobFile = resolveJobFile(workspace, "job-1"); + fs.writeFileSync(jobFile, JSON.stringify({ id: "job-1", status: "completed" }), "utf8"); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const job = readStoredJob(workspace, "job-1"); + + assert.deepEqual(job, { id: "job-1", status: "completed" }); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + test("saveState prunes dropped job artifacts when indexed jobs exceed the cap", () => { const workspace = makeTempDir(); const stateFile = resolveStateFile(workspace);