From 6b102b3d295d092e1c6866715b98b562b3d46d23 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 08:28:11 +0200 Subject: [PATCH 1/5] fix: SessionEnd/status lookups miss state written under a different CLAUDE_PLUGIN_DATA root, orphaning brokers resolveStateDir() picks the state root from CLAUDE_PLUGIN_DATA when set, falling back to $TMPDIR/codex-companion when it's absent -- same workspace slug/hash either way, only the root differs. State written under one root (e.g. a broker registered while the var was unset) becomes invisible to any later lookup that resolves to the other root, since nothing checked both. For a broker specifically, that means SessionEnd never finds it to shut down -- it's orphaned permanently, and since ensureBrokerSession also can't see it, the next session spawns a duplicate broker for the same workspace, compounding the leak. The same mechanism affects job/status state (state.json, individual job detail files), not just the broker. Reads now check every candidate root (current primary, then the tmpdir fallback), not just the current invocation's primary -- writes are unchanged, still going to the primary root. Applied to loadState() (job list, status, config), readStoredJob() (individual job detail lookups), and loadBrokerSession()/clearBrokerSession(). Known remaining asymmetry: this fixes the direction with concrete evidence in the issue -- state written while CLAUDE_PLUGIN_DATA was unset, later missed by a lookup that has it set. 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. Fixes #636 --- .../codex/scripts/lib/broker-lifecycle.mjs | 37 ++++++---- plugins/codex/scripts/lib/job-control.mjs | 11 +-- plugins/codex/scripts/lib/state.mjs | 59 ++++++++++++++-- tests/broker-lifecycle.test.mjs | 68 +++++++++++++++++++ tests/state.test.mjs | 61 ++++++++++++++++- 5 files changed, 212 insertions(+), 24 deletions(-) create mode 100644 tests/broker-lifecycle.test.mjs diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..9d90f4cf7 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,27 @@ 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; +export function loadBrokerSession(cwd) { + for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { + if (!fs.existsSync(stateFile)) { + continue; + } + try { + return JSON.parse(fs.readFileSync(stateFile, "utf8")); + } catch { + continue; + } } + return null; } export function saveBrokerSession(cwd, session) { @@ -93,9 +103,10 @@ export function saveBrokerSession(cwd, session) { } export function clearBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - if (fs.existsSync(stateFile)) { - fs.unlinkSync(stateFile); + for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { + if (fs.existsSync(stateFile)) { + fs.unlinkSync(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..c2c034f24 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,9 +83,19 @@ export function ensureStateDir(cwd) { fs.mkdirSync(resolveJobsDir(cwd), { recursive: true }); } +function resolveExistingStateFile(cwd) { + for (const stateDir of resolveStateDirCandidates(cwd)) { + const stateFile = path.join(stateDir, STATE_FILE_NAME); + if (fs.existsSync(stateFile)) { + return stateFile; + } + } + return null; +} + export function loadState(cwd) { - const stateFile = resolveStateFile(cwd); - if (!fs.existsSync(stateFile)) { + const stateFile = resolveExistingStateFile(cwd); + if (!stateFile) { return defaultState(); } @@ -189,3 +227,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/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..a4f70b0ed --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,68 @@ +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"; + +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); + }); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0f8f57cea..a088b9557 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,57 @@ 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; + } + } +}); + +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); From baa5ffdcfe7db046cbbe2ebeda39c4f95e691b69 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 08:36:48 +0200 Subject: [PATCH 2/5] fix: clearBrokerSession only clears the record loadBrokerSession() returned Both call sites (handleSessionEnd, ensureBrokerSession) act on whatever loadBrokerSession() returns -- tearing that broker down and clearing its record -- but clearBrokerSession deleted every candidate root's broker.json, not just the one that was actually torn down. That's reachable in practice: it's precisely the root-split bug's own historical fallout, where the old lookup could leave a broker registered under one root while a duplicate got spawned under the other. Deleting both records on the next cleanup erases the untorn broker's only metadata, making it permanently untrackable instead of leaving a stale-but-discoverable file behind. Thanks to Codex Review for catching this. --- .../codex/scripts/lib/broker-lifecycle.mjs | 10 +++++ tests/broker-lifecycle.test.mjs | 38 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 9d90f4cf7..c8a6fa8fe 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -102,10 +102,20 @@ export function saveBrokerSession(cwd, session) { fs.writeFileSync(resolveBrokerStateFile(cwd), `${JSON.stringify(session, null, 2)}\n`, "utf8"); } +// Removes only the record loadBrokerSession() would return (the first +// existing candidate), 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. export function clearBrokerSession(cwd) { for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { if (fs.existsSync(stateFile)) { fs.unlinkSync(stateFile); + return; } } } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index a4f70b0ed..68098a967 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -66,3 +66,41 @@ test("clearBrokerSession removes a session that was registered without CLAUDE_PL 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 }); + }); +}); From dcf9384f53612a259d98c6a48cbe6d2ddea4766f Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 09:24:06 +0200 Subject: [PATCH 3/5] fix: merge jobs across candidate roots; make clearBrokerSession agree with loadBrokerSession on malformed files Two more issues from Codex Review, both real: 1. loadState() returned only the first candidate state.json found, not merged. Unlike a broker session (at most one meaningful record, so 'first found' is correct), jobs are a growing collection -- a job started while CLAUDE_PLUGIN_DATA was set and a different job started while it was unset are both real and non-conflicting. Returning only the first root's job list silently hid whichever root wasn't picked, for every status/result/cancel lookup, any time both roots happened to have a state.json. Now merges jobs from every candidate, keeping the more recently updated copy if the same id somehow appears in more than one. 2. loadBrokerSession() skips a candidate it can't parse and moves on, so it can return a fallback session while a malformed primary file exists. clearBrokerSession() selected by existence alone, so it could delete the unrelated malformed primary while leaving the valid fallback record behind -- the one actually loaded and torn down by the caller. Both functions now share a single selectBrokerState() helper (exists AND parses), so they always agree on which candidate is the selected one. Verified the loadState() merge fix doesn't have a side effect on saveState()'s own previousJobs cleanup diff (its per-job file removal resolves paths against the current-root-only resolveJobFile(), so a job living in another root is a no-op there, not a deletion) -- confirmed empirically with a throwaway repro before concluding no further change was needed there. Thanks again to Codex Review. --- .../codex/scripts/lib/broker-lifecycle.mjs | 46 ++++++++----- plugins/codex/scripts/lib/state.mjs | 65 ++++++++++++------- tests/broker-lifecycle.test.mjs | 38 +++++++++++ tests/state.test.mjs | 39 +++++++++++ 4 files changed, 150 insertions(+), 38 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index c8a6fa8fe..4fd39296c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -82,13 +82,22 @@ function resolveBrokerStateFileCandidates(cwd) { return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, BROKER_STATE_FILE)); } -export function loadBrokerSession(cwd) { +// 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 { - return JSON.parse(fs.readFileSync(stateFile, "utf8")); + const session = JSON.parse(fs.readFileSync(stateFile, "utf8")); + return { stateFile, session }; } catch { continue; } @@ -96,27 +105,32 @@ export function loadBrokerSession(cwd) { return null; } +export function loadBrokerSession(cwd) { + return selectBrokerState(cwd)?.session ?? null; +} + export function saveBrokerSession(cwd, session) { const stateDir = resolveStateDir(cwd); fs.mkdirSync(stateDir, { recursive: true }); fs.writeFileSync(resolveBrokerStateFile(cwd), `${JSON.stringify(session, null, 2)}\n`, "utf8"); } -// Removes only the record loadBrokerSession() would return (the first -// existing candidate), 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. +// 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) { - for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { - if (fs.existsSync(stateFile)) { - fs.unlinkSync(stateFile); - return; - } + const selected = selectBrokerState(cwd); + if (selected) { + fs.unlinkSync(selected.stateFile); } } diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index c2c034f24..92cb1187c 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -83,36 +83,57 @@ export function ensureStateDir(cwd) { fs.mkdirSync(resolveJobsDir(cwd), { recursive: true }); } -function resolveExistingStateFile(cwd) { - for (const stateDir of resolveStateDirCandidates(cwd)) { - const stateFile = path.join(stateDir, STATE_FILE_NAME); - if (fs.existsSync(stateFile)) { - return stateFile; - } +function readStateFileIfValid(stateFile) { + if (!fs.existsSync(stateFile)) { + return null; + } + try { + return JSON.parse(fs.readFileSync(stateFile, "utf8")); + } catch { + return null; } - 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 stateFile = resolveExistingStateFile(cwd); - if (!stateFile) { + const parsedCandidates = resolveStateDirCandidates(cwd) + .map((stateDir) => readStateFileIfValid(path.join(stateDir, STATE_FILE_NAME))) + .filter((parsed) => parsed != null); + + if (parsedCandidates.length === 0) { return defaultState(); } - try { - const parsed = JSON.parse(fs.readFileSync(stateFile, "utf8")); - return { - ...defaultState(), - ...parsed, - config: { - ...defaultState().config, - ...(parsed.config ?? {}) - }, - jobs: Array.isArray(parsed.jobs) ? parsed.jobs : [] - }; - } catch { - 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); + } + } } + + const [primary] = parsedCandidates; + return { + ...defaultState(), + ...primary, + config: { + ...defaultState().config, + ...(primary.config ?? {}) + }, + jobs: [...jobsById.values()] + }; } function pruneJobs(jobs) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 68098a967..abb0555c3 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -1,3 +1,5 @@ +import fs from "node:fs"; +import path from "node:path"; import test from "node:test"; import assert from "node:assert/strict"; @@ -7,6 +9,7 @@ import { 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; @@ -104,3 +107,38 @@ test("clearBrokerSession does not delete a distinct session recorded under the o 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/state.test.mjs b/tests/state.test.mjs index a088b9557..371b1d454 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -76,6 +76,45 @@ test("loadState finds state written without CLAUDE_PLUGIN_DATA when the current } }); +// 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). +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 { + delete process.env.CLAUDE_PLUGIN_DATA; + saveState(workspace, { + config: {}, + jobs: [{ id: "job-fallback", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + saveState(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; + } + } +}); + 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(); From e349f1c09b102f2b1e9054e37976fb262f19686a Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 09:31:24 +0200 Subject: [PATCH 4/5] fix: persist job deletions across every candidate state root, not just the primary saveState() only ever wrote the new job list to the current primary root. A job that originated entirely in a different root (e.g. added while CLAUDE_PLUGIN_DATA was unset) and later gets filtered out -- cleanupSessionJobs() during SessionEnd loads the merged view, drops jobs for the ending session, and saves the remainder -- never actually disappeared: that other root's own state.json still held its own untouched copy, and the very next loadState() merged it right back in. A removed job could keep reporting as running indefinitely. saveState() now also prunes every other candidate root's own file down to the same retained job-id set (derived from this save's own merged previousJobs diff), so a deletion sticks everywhere. New and updated jobs are unaffected -- they still only ever get written to the primary root, exactly as before; this only ever removes. Also made the individual job-detail-file cleanup in the same loop candidate-aware (resolveJobFileCandidates instead of the primary-only resolveJobFile), for the same reason. Two of the existing tests had to seed their two-root fixtures via direct file writes instead of two independent saveState() calls -- every real caller (updateState()/cleanupSessionJobs()) always derives its job list from a prior loadState(), so seeding via two disjoint, non-full-list saveState() calls doesn't reflect any real call pattern, and (correctly, now) tripped this very fix's own deletion logic during test setup. Thanks again to Codex Review. --- plugins/codex/scripts/lib/state.mjs | 31 ++++++++++++- tests/state.test.mjs | 70 ++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 92cb1187c..c14e13e85 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -166,11 +166,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; } diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 371b1d454..14f4656f4 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -84,20 +84,28 @@ test("loadState finds state written without CLAUDE_PLUGIN_DATA when the current // 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; - saveState(workspace, { + writeStateFileDirectly(resolveStateDir(workspace), { config: {}, jobs: [{ id: "job-fallback", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }] }); process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; - saveState(workspace, { + writeStateFileDirectly(resolveStateDir(workspace), { config: {}, jobs: [{ id: "job-plugin-data", status: "running", updatedAt: "2026-08-19T00:01:00.000Z" }] }); @@ -115,6 +123,64 @@ test("loadState merges jobs from every candidate root instead of only the first } }); +// 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(); From a88f5d8105fb3a7ff0d2b0c7ef36c79bfa15d978 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Tue, 25 Aug 2026 00:03:40 +0200 Subject: [PATCH 5/5] fix: make SessionEnd check every state candidate, reconcile config across roots cleanupSessionJobs() checked only resolveStateFile()'s (the primary candidate's) existence before deciding whether to look for jobs to clean up. loadState() is candidate-aware, but 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) was silently skipped: the early check saw no primary file and returned before loadState() was ever called. Fixed by removing the redundant pre-check -- loadState() already returns an empty job list when nothing exists anywhere, and the existing removedJobs.length === 0 check already short-circuits correctly, without the primary-only blind spot. loadState()'s config merge also only ever read the primary candidate's config, unlike jobs (already merged across every root). A boolean flag like stopReviewGate is an opt-in toward stricter/safer behavior, so any candidate setting it true should win over a stale false elsewhere -- e.g. /codex:setup --enable-review-gate running without CLAUDE_PLUGIN_DATA writes it to the fallback root, invisible to a later invocation whose primary is the plugin-data root. Reconciling by "primary wins" could silently downgrade an explicitly-enabled gate. Both found via Codex Review on the PR. --- plugins/codex/scripts/lib/state.mjs | 24 ++++++++-- .../codex/scripts/session-lifecycle-hook.mjs | 11 ++--- tests/runtime.test.mjs | 48 ++++++++++++++++++- tests/state.test.mjs | 38 +++++++++++++++ 4 files changed, 110 insertions(+), 11 deletions(-) diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index c14e13e85..932f8c7c4 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -124,14 +124,30 @@ export function loadState(cwd) { } } + // 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: { - ...defaultState().config, - ...(primary.config ?? {}) - }, + config: mergedConfig, jobs: [...jobsById.values()] }; } 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/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 14f4656f4..0903e8dcf 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -123,6 +123,44 @@ test("loadState merges jobs from every candidate root instead of only the first } }); +// 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