fix: SessionEnd/status lookups miss state written under a different CLAUDE_PLUGIN_DATA root, orphaning brokers

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);
   }
 }
 

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) {

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,57 @@ 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);
+      }
+    }
+  }
+
+  const [primary] = parsedCandidates;
+  return {
+    ...defaultState(),
+    ...primary,
+    config: {
+      ...defaultState().config,
+      ...(primary.config ?? {})
+    },
+    jobs: [...jobsById.values()]
+  };
 }
 
 function pruneJobs(jobs) {
@@ -107,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;
 }
 
@@ -189,3 +277,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`));
+}