diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 870246242..f4356e9fb 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -4,13 +4,13 @@ "name": "OpenAI" }, "metadata": { - "description": "Codex plugins to use in Claude Code for delegation and code review.", + "description": "Codex plugins to use in Claude Code for delegation and code review. Fork of openai/codex-plugin-cc carrying open upstream fixes.", "version": "1.0.6" }, "plugins": [ { "name": "codex", - "description": "Use Codex from Claude Code to review code or delegate tasks.", + "description": "Use Codex from Claude Code to review code or delegate tasks. Fork of openai/codex-plugin-cc with open upstream fixes merged.", "version": "1.0.6", "author": { "name": "OpenAI" diff --git a/README.md b/README.md index 710040311..3534a799f 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,29 @@ Accepted `--effort` values are `none`, `minimal`, `low`, `medium`, `high`, and ` unrecognised `--flag` is not silently swallowed into the prompt: the plugin warns on stderr and passes the token through as text. +### What The Background Runtime Guarantees + +Delegated work outlives the Claude session that started it, so most of this fork's divergence from +upstream is about what happens when something dies at the wrong moment. In short: + +- **A job's state is checked, not trusted.** Every status, result and cancel reconciles the record + against the worker process, so a run whose worker died does not read as running forever. +- **Ending a session never reports an outcome that did not happen.** In-flight jobs are interrupted + and recorded as cancelled; a turn that cannot be interrupted keeps its runtime alive instead, on a + bounded window. +- **One broker per workspace, shut down by the last session out.** It is never torn down while + another session has work in flight, and a departed client's Codex threads are unsubscribed rather + than left to leak notifications into the next one. +- **A finished job keeps a record, with its cause.** Sessions end without erasing what ran, so + `/codex:status` answers with an outcome rather than "No job found". +- **Nothing the plugin writes is world-readable, and nothing is half-written.** State, job files, + logs and the review-gate config are 0600 and written through a temp file and a rename. +- **Node is found where you actually installed it.** Hooks resolve a supported toolchain through + `scripts/run-node.sh`, preferring your version manager over a system install. + +[Background Runtime Limits](#background-runtime-limits) and [Where State Lives](#where-state-lives) +say what bounds those windows and where the files are. + ## Requirements - **ChatGPT subscription (incl. Free) or OpenAI API key.** @@ -234,11 +257,20 @@ Use it to: - see the latest completed job - confirm whether a task is still running +A job's status is reconciled against its worker process before it is reported, so a background run whose worker died is not shown as running forever: + +- worker gone, no Codex thread started → `terminated-unknown` +- worker gone while a Codex turn may still be running → still active, with the phase `worker-exited-turn-unknown` + +That second state is the one case the plugin cannot resolve on its own: the turn is server-side and the process that knew its id is gone. It clears when the turn's runtime does — see [Background Runtime Limits](#background-runtime-limits). + ### `/codex:result` Shows the final stored Codex output for a finished job. When available, it also includes the Codex session ID so you can reopen that run directly in Codex with `codex resume `. +A run that failed without crashing — a rejected model, an unsupported parameter — stores the error text too, so a failed job says *why* it failed rather than only that it did. + Examples: ```bash @@ -248,7 +280,7 @@ Examples: ### `/codex:cancel` -Cancels an active background Codex job. +Cancels an active background Codex job: it records the cancellation, interrupts the Codex turn, and then kills the worker — in that order, so a crash midway never leaves an interrupted turn with no recorded outcome. Examples: @@ -257,6 +289,12 @@ Examples: /codex:cancel task-abc123 ``` +**Notes:** + +- a job that finished while you were typing is reported as already finished rather than failing the command +- naming a job whose record went stale (its worker died mid-write) repairs the record and reports the real outcome +- a cancel is refused when the worker exited after Codex accepted the turn but before it recorded the turn id: that turn may still be running and there is nothing to address it by, so reporting it cancelled would be a lie. The job stays active until its runtime goes. + ### `/codex:setup` Checks whether Codex is installed and authenticated. @@ -343,8 +381,20 @@ Set any of these in the environment before starting Claude Code, for example: export CODEX_BROKER_IDLE_SHUTDOWN_MS=1800000 # 30 minutes ``` +`CODEX_BROKER_IDLE_SHUTDOWN_MS` does double duty: it also bounds how long a job whose worker died with a turn still possibly running keeps the runtime up (the `worker-exited-turn-unknown` state above). Session end leaves the broker alive for such a job instead of killing the turn under it; once that window passes, the next session end reclaims both. With the timer disabled (`0`), the fallback is 24 hours rather than forever. + These are advanced knobs for tuning resource usage in long-running or resource-constrained environments; most users will never need to touch them. +### Where State Lives + +| What | Where | Notes | +| --- | --- | --- | +| jobs, logs, broker record | `$CLAUDE_PLUGIN_DATA/state//`, or a temp-dir fallback | `CLAUDE_PLUGIN_DATA` is only set when the plugin runs as a hook, so both locations are real. Reads check both, writes go to the current one. | +| review-gate flag | `$CODEX_HOME/plugin-cc/config/.json` | Durable on purpose: clearing the state dir, or a different `CLAUDE_PLUGIN_DATA`, must not silently turn the gate off. Written privately (0600) and atomically. | +| Codex threads and auth | wherever your Codex CLI keeps them | The plugin never holds a second copy — see the [FAQ](#does-the-plugin-use-a-separate-codex-runtime). | + +One broker serves every session in a workspace. It is shut down by the last session out, never by a session that still has another's work in flight, and it releases its Codex thread subscriptions as clients disconnect, so a departed session's notifications never reach the next one. + ### Moving The Work Over To Codex Delegated tasks and any [stop gate](#what-does-the-review-gate-do) run can also be directly resumed inside Codex by running `codex resume` either with the specific session ID you received from running `/codex:result` or `/codex:status` or by selecting it from the list. @@ -364,6 +414,9 @@ Broker and background-job lifecycle: | [#541](https://github.com/openai/codex-plugin-cc/pull/541) | broker leaks, state races, and signal-masked command failures in the test runtime | | [#623](https://github.com/openai/codex-plugin-cc/pull/623) | session end no longer tears down the shared broker while another session's jobs are still using it | | [#652](https://github.com/openai/codex-plugin-cc/pull/652) | bounds the lifetime of detached brokers and task workers (see [Background Runtime Limits](#background-runtime-limits)) | +| [#659](https://github.com/openai/codex-plugin-cc/pull/659) | state written under one `CLAUDE_PLUGIN_DATA` root is no longer invisible to an invocation that resolves to another, which orphaned brokers and hid jobs | +| [#707](https://github.com/openai/codex-plugin-cc/pull/707) | the broker releases its app-server thread subscriptions when a client disconnects, instead of leaking them for its whole lifetime | +| [#728](https://github.com/openai/codex-plugin-cc/pull/728) | a job whose worker died no longer reads as "running" forever; `/codex:status` reconciles the record against the live process | Commands and flags: @@ -375,7 +428,7 @@ Commands and flags: | [#729](https://github.com/openai/codex-plugin-cc/pull/729) | `/codex:transfer` works with a relocated `CLAUDE_CONFIG_DIR` | | [#742](https://github.com/openai/codex-plugin-cc/pull/742) | `--sandbox ` on `task` and `/codex:rescue` | | [#746](https://github.com/openai/codex-plugin-cc/pull/746) | `--model`/`--effort` on the review commands, and a warning for unrecognised options | -| [#748](https://github.com/openai/codex-plugin-cc/pull/748) | `CLAUDE_ENV_FILE` keeps one export per key instead of growing on every session | +| [#748](https://github.com/openai/codex-plugin-cc/pull/748) | `CLAUDE_ENV_FILE` skips re-exporting an unchanged value (its rewrite-the-file mechanism is not used: the file is shared with other plugins' hooks, so this fork only ever appends to it) | | [#731](https://github.com/openai/codex-plugin-cc/pull/731) | the review-gate flag is persisted outside the transient state dir, so a different `CLAUDE_PLUGIN_DATA` no longer silently disables it | | [#737](https://github.com/openai/codex-plugin-cc/pull/737) | hooks resolve Node through `scripts/run-node.sh`, so nvm/fnm/asdf/mise/Volta/Homebrew toolchains work under the minimal hook PATH | | [#747](https://github.com/openai/codex-plugin-cc/pull/747) | `runCommand` sets an explicit 256 MiB `maxBuffer`, so a large `git diff` is no longer truncated at Node's 1 MiB default | @@ -384,6 +437,37 @@ Commands and flags: Where two of these PRs disagreed, the merge commit says which side won and why. The plugin version is deliberately left at the upstream number: these merges do not cut a release. +Beyond the imports, this fork carries fixes for defects the imports themselves surfaced: + +- a busy broker refusing shutdown is reported as a refusal, not an identity rejection, so SessionEnd + leaves a shared runtime to the sessions still using it instead of exiting with an error +- `scripts/run-node.sh` prefers a user-managed toolchain over a system install, which [#737](https://github.com/openai/codex-plugin-cc/pull/737) + had inverted (see the note under [Requirements](#requirements)) +- the durable review-gate config is written privately and atomically, so an interrupted write cannot + silently disable the gate +- `--read-root` works on a resumed thread: the scoped run sends a permission profile rather than a + sandbox mode, so asserting the mode had refused every scoped resume — and its advice dropped the + write grant. A scoped run on a thread started with the sandbox disabled is still refused +- cancelling refuses *before* taking the job's terminal claim, so a refused cancel leaves nothing + behind for the next one to turn into a bogus `cancelled` record +- a job retained because its turn may still be running keeps the runtime up, stays readable in + `/codex:status`, and expires on a bounded window instead of pinning the broker +- the state a job is deleted from is the state its files are deleted from, so a prune another + process is holding can no longer leave a record without its log and detail file +- disabling the review gate is not outvoted by a stale enable left under another plugin-data root +- `CLAUDE_ENV_FILE` is only ever appended to: it is shared with other plugins' hooks, and rewriting + it dropped whatever they had just written +- the app-server typecheck (`npm run build`) passes + +Each of those came out of an adversarial review of the merges, re-run after every round of fixes; +the reasoning behind each is in its commit message, and each has a regression test that fails +without it. + +Not imported: [#733](https://github.com/openai/codex-plugin-cc/pull/733) (durable startup +cancellation) — its behavior is already covered here by the terminal-claim mechanism, and its marker +files would add a second source of truth for the same decision. [#761](https://github.com/openai/codex-plugin-cc/pull/761) +(`max`/`ultra` reasoning efforts) — the same proposal was closed upstream as [#648](https://github.com/openai/codex-plugin-cc/pull/648). + ## FAQ ### Do I need a separate Codex account for this plugin? diff --git a/package.json b/package.json index b1d984d1a..4216d1040 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "version": "1.0.6", "private": true, "type": "module", - "description": "Use Codex from Claude Code to review code or delegate tasks.", + "description": "Use Codex from Claude Code to review code or delegate tasks. Fork of openai/codex-plugin-cc with open upstream fixes merged.", "license": "Apache-2.0", "engines": { "node": ">=18.18.0" diff --git a/plugins/codex/.claude-plugin/plugin.json b/plugins/codex/.claude-plugin/plugin.json index e91e5238c..bc9450598 100644 --- a/plugins/codex/.claude-plugin/plugin.json +++ b/plugins/codex/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "codex", "version": "1.0.6", - "description": "Use Codex from Claude Code to review code or delegate tasks.", + "description": "Use Codex from Claude Code to review code or delegate tasks. Fork of openai/codex-plugin-cc with open upstream fixes merged.", "author": { "name": "OpenAI" } diff --git a/plugins/codex/CHANGELOG.md b/plugins/codex/CHANGELOG.md index 7905d0fe6..9f6918001 100644 --- a/plugins/codex/CHANGELOG.md +++ b/plugins/codex/CHANGELOG.md @@ -13,6 +13,9 @@ - Persist the failure text of a turn that fails without throwing, and shorten job summaries to 96 characters. - Keep the review-gate flag in a durable per-workspace file under `CODEX_HOME`, written privately and atomically. - Resolve Node through `scripts/run-node.sh` in the hooks, preferring a user-managed toolchain over a system install. +- Release the broker's app-server thread subscriptions when a client disconnects, so a departed client no longer leaks them for the broker's lifetime (openai/codex-plugin-cc#707). +- Read state from every candidate `CLAUDE_PLUGIN_DATA` root, so jobs and broker records written by one invocation are not invisible to another (openai/codex-plugin-cc#659). +- Reconcile a job against its worker process, so a job whose worker died stops reading as running (openai/codex-plugin-cc#728). ## 1.0.0 diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 18ab8ed14..1a207e7c5 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -22,6 +22,66 @@ import { import { terminateProcessTreeAndExit } from "./lib/process.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); +const SUBSCRIBING_METHODS = new Set(["thread/start", "thread/resume", "thread/fork"]); +const UNSUBSCRIBE_RETRY_DELAYS_MS = [100, 500, 2000]; +// Upper bound on how long a request waits for an in-flight thread/unsubscribe of +// the same thread. A hung cleanup request must not wedge the shared broker. +const UNSUBSCRIBE_WAIT_TIMEOUT_MS = 5000; + +// Resolves true once the promise settles, or false if the timeout expires first. +function settleWithin(promise, timeoutMs) { + let timer; + return Promise.race([ + promise.then( + () => true, + () => true + ), + new Promise((resolve) => { + timer = setTimeout(() => resolve(false), timeoutMs); + timer.unref?.(); + }) + ]).finally(() => clearTimeout(timer)); +} + +function buildSubscriptionThreadIds(method, result) { + const threadIds = new Set(); + if (SUBSCRIBING_METHODS.has(method) && result?.thread?.id) { + threadIds.add(result.thread.id); + } + if (method === "review/start" && result?.reviewThreadId) { + threadIds.add(result.reviewThreadId); + } + return threadIds; +} + +function buildProvisionalSubscriptionThreadIds(method, params) { + const threadIds = new Set(); + if (method === "thread/resume" && params?.threadId) { + threadIds.add(params.threadId); + } + if (method === "review/start" && params?.threadId) { + threadIds.add(params.threadId); + } + return threadIds; +} + +function buildNotificationSubscriptionRelationships(message) { + const relationships = []; + const thread = message?.method === "thread/started" ? message.params?.thread : null; + if (thread?.id && thread?.parentThreadId) { + relationships.push({ sourceThreadId: thread.parentThreadId, subscribedThreadId: thread.id }); + } + + const item = message?.params?.item; + if (item?.type === "collabAgentToolCall" && item?.senderThreadId && Array.isArray(item.receiverThreadIds)) { + for (const threadId of item.receiverThreadIds) { + if (threadId) { + relationships.push({ sourceThreadId: item.senderThreadId, subscribedThreadId: threadId }); + } + } + } + return relationships; +} /** How long each step of a shutdown waits before giving up and going down without it. */ const SHUTDOWN_GRACE_MS = 5000; @@ -147,6 +207,232 @@ async function main() { let shuttingDown = false; let shutdownPromise = null; + // App-server subscriptions belong to the broker's single upstream connection. + // Mirror downstream ownership so one client cannot release another client's thread. + const socketThreadIds = new Map(); + const threadSockets = new Map(); + const pendingUnsubscribes = new Map(); + const unsubscribeRetryTimers = new Map(); + // Threads a socket claimed for a request that has not succeeded yet, plus any + // child threads it inherited through those claims while the request was open. + const provisionalClaims = new Map(); + + function cancelUnsubscribeRetry(threadId) { + const retry = unsubscribeRetryTimers.get(threadId); + if (!retry) { + return; + } + clearTimeout(retry.timer); + unsubscribeRetryTimers.delete(threadId); + } + + function addThreadOwner(socket, threadId) { + cancelUnsubscribeRetry(threadId); + let ownedThreadIds = socketThreadIds.get(socket); + if (!ownedThreadIds) { + ownedThreadIds = new Set(); + socketThreadIds.set(socket, ownedThreadIds); + } + if (ownedThreadIds.has(threadId)) { + return false; + } + ownedThreadIds.add(threadId); + + let owners = threadSockets.get(threadId); + if (!owners) { + owners = new Set(); + threadSockets.set(threadId, owners); + } + owners.add(socket); + return true; + } + + function removeThreadOwner(socket, threadId) { + const ownedThreadIds = socketThreadIds.get(socket); + if (!ownedThreadIds?.delete(threadId)) { + return false; + } + if (ownedThreadIds.size === 0) { + socketThreadIds.delete(socket); + } + + const owners = threadSockets.get(threadId); + owners?.delete(socket); + if (owners?.size === 0) { + threadSockets.delete(threadId); + } + return true; + } + + function requestThreadUnsubscribe(threadId) { + // A request that has already been sent is never reused: the thread may have + // been reacquired and released again while it was in flight. A request that + // is still queued behind an earlier one re-checks ownership when it sends, so + // every release until then can share it instead of growing the chain. + const previous = pendingUnsubscribes.get(threadId); + if (previous && !previous.sent) { + return previous.request; + } + const entry = { request: null, sent: false }; + const execute = async () => { + if (previous) { + // Wait without a bound. The pending entry must not settle while any + // earlier upstream unsubscribe for this thread is still outstanding, + // otherwise a retried claim could slip past a hung cleanup request. + await previous.request.then( + () => {}, + () => {} + ); + } + if (threadSockets.has(threadId)) { + return { result: null, error: null, skipped: true }; + } + entry.sent = true; + const result = await appClient.request("thread/unsubscribe", { threadId }); + return { result, error: null }; + }; + entry.request = execute().then( + (outcome) => { + if (pendingUnsubscribes.get(threadId) === entry) { + pendingUnsubscribes.delete(threadId); + } + return outcome; + }, + (error) => { + if (pendingUnsubscribes.get(threadId) === entry) { + pendingUnsubscribes.delete(threadId); + } + process.stderr.write( + `Failed to unsubscribe Codex thread ${threadId}: ${error instanceof Error ? error.message : String(error)}\n` + ); + return { result: null, error }; + } + ); + pendingUnsubscribes.set(threadId, entry); + return entry.request; + } + + function scheduleUnsubscribeRetry(threadId, retryIndex) { + if ( + retryIndex >= UNSUBSCRIBE_RETRY_DELAYS_MS.length || + unsubscribeRetryTimers.has(threadId) || + threadSockets.has(threadId) || + appClient.closed + ) { + return; + } + const timer = setTimeout(() => { + unsubscribeRetryTimers.delete(threadId); + void unsubscribeIfUnowned(threadId, { retryOnFailure: true, retryIndex: retryIndex + 1 }); + }, UNSUBSCRIBE_RETRY_DELAYS_MS[retryIndex]); + timer.unref?.(); + unsubscribeRetryTimers.set(threadId, { timer, retryIndex }); + } + + async function unsubscribeIfUnowned(threadId, { retryOnFailure = false, retryIndex = 0 } = {}) { + if (threadSockets.has(threadId) || appClient.closed) { + cancelUnsubscribeRetry(threadId); + return null; + } + const outcome = await requestThreadUnsubscribe(threadId); + if (outcome.error && retryOnFailure) { + scheduleUnsubscribeRetry(threadId, retryIndex); + } else if (!outcome.error) { + cancelUnsubscribeRetry(threadId); + } + return outcome; + } + + async function releaseThreadOwners(socket, threadIds = socketThreadIds.get(socket) ?? new Set()) { + const releasedThreadIds = []; + for (const threadId of [...threadIds]) { + if (removeThreadOwner(socket, threadId) && !threadSockets.has(threadId)) { + releasedThreadIds.push(threadId); + } + } + await Promise.all( + releasedThreadIds.map((threadId) => unsubscribeIfUnowned(threadId, { retryOnFailure: true })) + ); + } + + function trackSubscriptionResults(socket, method, result) { + for (const threadId of buildSubscriptionThreadIds(method, result)) { + if (socket.destroyed || !sockets.has(socket)) { + void unsubscribeIfUnowned(threadId, { retryOnFailure: true }); + continue; + } + addThreadOwner(socket, threadId); + } + } + + function trackNotificationSubscriptions(message) { + // App-server can auto-subscribe its connection to subagent threads. Attribute + // each child to the downstream owners of its causal parent, not the client + // that happens to be active when a delayed notification arrives. + for (const { sourceThreadId, subscribedThreadId } of buildNotificationSubscriptionRelationships(message)) { + const sourceOwners = [...(threadSockets.get(sourceThreadId) ?? [])].filter( + (socket) => !socket.destroyed && sockets.has(socket) + ); + if (sourceOwners.length === 0) { + void unsubscribeIfUnowned(subscribedThreadId, { retryOnFailure: true }); + continue; + } + if (pendingUnsubscribes.has(subscribedThreadId)) { + // A cleanup request for this child is still outstanding, so its upstream + // subscription is going away. Do not hand it to new owners. + continue; + } + for (const socket of sourceOwners) { + if (addThreadOwner(socket, subscribedThreadId)) { + // Ownership inherited through a claim that has not succeeded yet is + // rolled back with that claim. The source may itself be an inherited + // child, so nested descendants are recorded too. + const claim = provisionalClaims.get(socket); + if (claim && (claim.threadIds.has(sourceThreadId) || claim.inheritedThreadIds.has(sourceThreadId))) { + claim.inheritedThreadIds.add(subscribedThreadId); + } + } + } + } + } + + async function handleThreadUnsubscribe(socket, params) { + const threadId = params?.threadId; + if (typeof threadId !== "string") { + return appClient.request("thread/unsubscribe", params ?? {}); + } + + const ownedThreadIds = socketThreadIds.get(socket); + if (!ownedThreadIds?.has(threadId)) { + if (threadSockets.has(threadId)) { + return { status: "notSubscribed" }; + } + const outcome = await unsubscribeIfUnowned(threadId); + if (outcome?.error) { + throw outcome.error; + } + return outcome?.result ?? { status: "notSubscribed" }; + } + + removeThreadOwner(socket, threadId); + if (threadSockets.has(threadId)) { + return { status: "unsubscribed" }; + } + + const outcome = await unsubscribeIfUnowned(threadId); + if (outcome?.error) { + if (!socket.destroyed && sockets.has(socket)) { + addThreadOwner(socket, threadId); + } else { + // The requester is gone, so nobody will retry on its behalf. Fall back + // to the automatic cleanup path for the now-unowned thread. + scheduleUnsubscribeRetry(threadId, 0); + } + throw outcome.error; + } + return outcome?.result ?? { status: "unsubscribed" }; + } + function clearSocketOwnership(socket) { if (activeRequestSocket === socket) { activeRequestSocket = null; @@ -208,6 +494,7 @@ async function main() { } const target = activeRequestSocket ?? activeStreamSocket; + trackNotificationSubscriptions(message); if (!target) { return; } @@ -269,6 +556,12 @@ async function main() { async function runShutdown(server, responseSocket) { cancelIdleShutdown(); + // Pending unsubscribe retries are timers on a broker that is going away: left + // armed they would fire against a closed app-server. + for (const { timer } of unsubscribeRetryTimers.values()) { + clearTimeout(timer); + } + unsubscribeRetryTimers.clear(); // Stop accepting before the first await. Otherwise a client can connect while we are closing // the app-server, get a broker that looks alive but has no backend, and hold server.close() // open on a socket nobody will serve. @@ -348,16 +641,20 @@ async function main() { socket.setEncoding("utf8"); let buffer = ""; - socket.on("data", async (chunk) => { - buffer += chunk; - let newlineIndex = buffer.indexOf("\n"); - while (newlineIndex !== -1) { - const line = buffer.slice(0, newlineIndex); - buffer = buffer.slice(newlineIndex + 1); - newlineIndex = buffer.indexOf("\n"); + + let processing = Promise.resolve(); + + // Every line is handled to completion before the next one starts. Without this a second + // data event can interleave with an await inside the first, and the per-request ownership + // claims below stop being a correct picture of who holds which thread. + async function handleLine(line) { + if (!line.trim() || socket.destroyed || !sockets.has(socket)) { + return; + } + if (!line.trim()) { - continue; + return; } let message; @@ -368,7 +665,7 @@ async function main() { id: null, error: buildJsonRpcError(-32700, `Invalid JSON: ${error.message}`) }); - continue; + return; } // `null`, a bare number and an array are all valid JSON. Dereferencing them below would @@ -379,7 +676,7 @@ async function main() { id: null, error: buildJsonRpcError(-32600, "Invalid JSON-RPC message: expected an object.") }); - continue; + return; } if (message.id !== undefined && message.method === "initialize") { @@ -389,11 +686,11 @@ async function main() { userAgent: "codex-companion-broker" } }); - continue; + return; } if (message.method === "initialized" && message.id === undefined) { - continue; + return; } if (message.id !== undefined && message.method === "broker/shutdown") { @@ -403,7 +700,7 @@ async function main() { id: message.id, error: buildJsonRpcError(-32003, "Broker shutdown identity did not match this instance.") }); - continue; + return; } // Teardown must be atomic with client admission: a client that // connected between a session-end guard check and this shutdown @@ -423,7 +720,7 @@ async function main() { id: message.id, error: buildJsonRpcError(BROKER_BUSY_RPC_CODE, "Shared Codex broker is busy.") }); - continue; + return; } send(socket, { id: message.id, @@ -434,7 +731,7 @@ async function main() { } if (message.id === undefined) { - continue; + return; } const allowInterruptDuringActiveStream = @@ -448,7 +745,7 @@ async function main() { id: message.id, error: buildJsonRpcError(BROKER_BUSY_RPC_CODE, "Shared Codex broker is busy.") }); - continue; + return; } if (allowInterruptDuringActiveStream) { @@ -461,12 +758,65 @@ async function main() { error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) }); } - continue; + return; } const isStreaming = STREAMING_METHODS.has(message.method); + + // Claim the thread ids this request already names before awaiting the + // app-server, so another client's close handler cannot unsubscribe a + // thread this one is concurrently resuming. + const provisionalThreadIds = buildProvisionalSubscriptionThreadIds(message.method, message.params ?? {}); + const addedProvisionalThreadIds = new Set(); + for (const threadId of provisionalThreadIds) { + if (addThreadOwner(socket, threadId)) { + addedProvisionalThreadIds.add(threadId); + } + } + const claim = { threadIds: addedProvisionalThreadIds, inheritedThreadIds: new Set() }; + if (addedProvisionalThreadIds.size > 0) { + provisionalClaims.set(socket, claim); + } + const rollBackClaim = () => { + if (provisionalClaims.get(socket) === claim) { + provisionalClaims.delete(socket); + } + void releaseThreadOwners(socket, new Set([...claim.threadIds, ...claim.inheritedThreadIds])); + }; + activeRequestSocket = socket; + // Let an in-flight unsubscribe for the same thread settle first so it cannot overtake the + // new subscription. The wait is bounded: a hung cleanup request must not hold this client + // or keep the broker busy for everyone else. + const gatedThreadIds = new Set(provisionalThreadIds); + if (message.method === "thread/unsubscribe" && typeof message.params?.threadId === "string") { + gatedThreadIds.add(message.params.threadId); + } + const settled = await Promise.all( + [...gatedThreadIds].map((threadId) => { + const pending = pendingUnsubscribes.get(threadId); + return pending ? settleWithin(pending.request, UNSUBSCRIBE_WAIT_TIMEOUT_MS) : true; + }) + ); + if (settled.includes(false)) { + send(socket, { + id: message.id, + error: buildJsonRpcError( + -32000, + "Codex thread is still being released upstream; retry the request shortly." + ) + }); + if (activeRequestSocket === socket) { + activeRequestSocket = null; + // This request never reached the app-server, so no notification follows to arm the + // timer later. + armIdleShutdown(); + } + rollBackClaim(); + return; + } + // A detached review streams on a thread it only names in its response, so a completion // racing that response cannot be recognised by thread id in advance. Record completions // for the duration of the start instead, and match them once the response tells us which @@ -476,7 +826,14 @@ async function main() { } try { - const result = await appClient.request(message.method, message.params ?? {}); + const result = + message.method === "thread/unsubscribe" + ? await handleThreadUnsubscribe(socket, message.params ?? {}) + : await appClient.request(message.method, message.params ?? {}); + trackSubscriptionResults(socket, message.method, result); + if (provisionalClaims.get(socket) === claim) { + provisionalClaims.delete(socket); + } send(socket, { id: message.id, result }); if (isStreaming) { const threadIds = buildStreamThreadIds(message.method, message.params ?? {}, result); @@ -520,6 +877,9 @@ async function main() { activeStreamThreadIds = null; } armIdleShutdown(); + // Released after replying: a hung upstream unsubscribe must not withhold the error or + // leave the broker busy for other clients. + rollBackClaim(); } finally { // The handoff is over either way. Once no start is in flight, a completion belongs to a // running turn rather than to a race, so nothing recorded here may survive. @@ -530,6 +890,20 @@ async function main() { } } } + } + + socket.on("data", (chunk) => { + buffer += chunk; + let newlineIndex = buffer.indexOf("\n"); + while (newlineIndex !== -1) { + const line = buffer.slice(0, newlineIndex); + buffer = buffer.slice(newlineIndex + 1); + newlineIndex = buffer.indexOf("\n"); + processing = processing.then(() => handleLine(line)).catch((error) => { + process.stderr.write( + `Failed to process broker request: ${error instanceof Error ? error.message : String(error)}\n` + ); + }); } }); @@ -537,6 +911,7 @@ async function main() { sockets.delete(socket); shutdownRequesters.delete(socket); clearSocketOwnership(socket); + void releaseThreadOwners(socket); armIdleShutdown(); }); @@ -544,6 +919,7 @@ async function main() { sockets.delete(socket); shutdownRequesters.delete(socket); clearSocketOwnership(socket); + void releaseThreadOwners(socket); armIdleShutdown(); }); }); diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index d54b1855d..fc4fe1c56 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -39,6 +39,8 @@ import { buildSingleJobSnapshot, buildStatusSnapshot, readStoredJob, + reconcileJobLiveness, + reconcileJobsLiveness, resolveCancelableJob, resolveResultJob, sortJobsNewestFirst @@ -392,7 +394,7 @@ async function waitForSingleJobSnapshot(cwd, reference, options = {}) { async function resolveLatestTrackedTaskThread(cwd, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); const sessionId = getCurrentClaudeSessionId(); - const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)).filter((job) => job.id !== options.excludeJobId); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot))).filter((job) => job.id !== options.excludeJobId); const visibleJobs = filterJobsForCurrentClaudeSession(jobs); const activeTask = visibleJobs.find((job) => job.jobClass === "task" && (job.status === "queued" || job.status === "running")); if (activeTask) { @@ -1149,7 +1151,7 @@ function handleTaskResumeCandidate(argv) { const cwd = resolveCommandCwd(options); const workspaceRoot = resolveCommandWorkspace(options); const sessionId = getCurrentClaudeSessionId(); - const jobs = filterJobsForCurrentClaudeSession(sortJobsNewestFirst(listJobs(workspaceRoot))); + const jobs = filterJobsForCurrentClaudeSession(sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot)))); const candidate = findLatestResumableTaskJob(jobs); const payload = { @@ -1192,6 +1194,21 @@ async function handleCancel(argv) { // writes below null the pid field, so capture the fresher value first. let workerPid = existing.pid ?? job.pid ?? Number.NaN; + // A worker that exited after turn/start was accepted, but before it recorded + // a turn id, leaves a Codex turn that may still be running and nothing to + // address it by, so the cancel refuses. It refuses here, before the terminal + // claim below: the claim is never released, and a leaked cancel-intent claim + // makes the next cancel (or SessionEnd) reassert it into a cancelled record + // for the turn that is still running — the very outcome this refusal exists + // to prevent. No identity wait can help either: the worker that would + // publish the turn id is already gone. + const reconciledForCancel = reconcileJobLiveness({ ...job, ...existing }); + if (reconciledForCancel.workerExited && threadId && !turnId) { + throw new Error( + `Cannot safely cancel ${job.id}: the worker exited after turn/start was accepted, but the turn id is not yet known. The Codex turn may still be running.` + ); + } + // Claim the terminal status first: if the worker finished in the meantime, // its completed/failed record stands and there is nothing left to cancel. // That race is benign, so report the job's terminal outcome as a normal diff --git a/plugins/codex/scripts/lib/app-server-protocol.d.ts b/plugins/codex/scripts/lib/app-server-protocol.d.ts index f61a4588e..e023b324f 100644 --- a/plugins/codex/scripts/lib/app-server-protocol.d.ts +++ b/plugins/codex/scripts/lib/app-server-protocol.d.ts @@ -21,6 +21,8 @@ import type { ThreadSetNameResponse, ThreadStartParams as RawThreadStartParams, ThreadStartResponse, + ThreadUnsubscribeParams, + ThreadUnsubscribeResponse, Turn, TurnInterruptParams, TurnInterruptResponse, @@ -63,6 +65,7 @@ export interface AppServerMethodMap { "thread/resume": { params: ThreadResumeParams; result: ThreadResumeResponse }; "thread/name/set": { params: ThreadSetNameParams; result: ThreadSetNameResponse }; "thread/list": { params: ThreadListParams; result: ThreadListResponse }; + "thread/unsubscribe": { params: ThreadUnsubscribeParams; result: ThreadUnsubscribeResponse }; "review/start": { params: ReviewStartParams; result: ReviewStartResponse }; "turn/start": { params: TurnStartParams; result: TurnStartResponse }; "turn/interrupt": { params: TurnInterruptParams; result: TurnInterruptResponse }; diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 397d552e9..3d7912afb 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -27,7 +27,7 @@ import { terminateProcessTree, waitForProcessExit } from "./process.mjs"; -import { resolveStateDir } from "./state.mjs"; +import { resolveStateDir, resolveStateDirCandidates } from "./state.mjs"; const BROKER_STATE_FILE = "broker.json"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; @@ -234,12 +234,40 @@ function resolveBrokerStateFile(cwd) { return path.join(resolveStateDir(cwd), BROKER_STATE_FILE); } -export function loadBrokerSession(cwd) { - try { - return JSON.parse(fs.readFileSync(resolveBrokerStateFile(cwd), "utf8")); - } catch { - 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)); +} + +// 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) { @@ -248,8 +276,23 @@ export function saveBrokerSession(cwd, session) { writeJsonFileAtomic(resolveBrokerStateFile(cwd), session); } +// 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) { - removeFileIfExists(resolveBrokerStateFile(cwd)); + const selected = selectBrokerState(cwd); + if (selected) { + fs.unlinkSync(selected.stateFile); + } } function resolveBrokerPid(session) { diff --git a/plugins/codex/scripts/lib/claude-session-transfer.mjs b/plugins/codex/scripts/lib/claude-session-transfer.mjs index ae228a80c..029673eec 100644 --- a/plugins/codex/scripts/lib/claude-session-transfer.mjs +++ b/plugins/codex/scripts/lib/claude-session-transfer.mjs @@ -156,25 +156,43 @@ function acquireStagingLease(stagedPath, staged) { } else { managed = markerMatches; } - if (managed) fs.writeFileSync(leasePath, "", { flag: "wx" }); + // The lease is taken whatever `managed` says. A process that attaches to a + // staged copy the creator has not yet marked would otherwise hold no lease, + // and the creator's release() -- seeing no leases -- deletes the file out + // from under it. `managed` only decides whether we may write the marker. + fs.writeFileSync(leasePath, "", { flag: "wx" }); }); return { release() { - if (!managed) return; - withStagingLock(stagedPath, () => { + // Called from a finally: a staging lock that is busy (5s) or unusable + // must never surface in place of the import error that is unwinding. + // Dropping our own lease needs no lock — the path is unique to us — so + // the fallback still frees the staged copy for whoever leaves last. + try { + releaseLocked(); + } catch { + try { fs.unlinkSync(leasePath); } catch (error) { if (error?.code !== "ENOENT") { /* nothing left to do */ } } + } + } + }; + + function releaseLocked() { + withStagingLock(stagedPath, () => { try { fs.unlinkSync(leasePath); } catch (error) { if (error?.code !== "ENOENT") throw error; } const activeLeases = fs.readdirSync(directory).filter((name) => name.startsWith(leasePrefix)); + // Cleanup belongs to whoever leaves last, not to whoever created the + // copy: the marker is what proves the staging is the plugin's, and the + // lease count is what proves nobody is still reading it. if (activeLeases.length > 0 || !managedMarkerMatches(markerPath)) return; if (fs.existsSync(stagedPath) && fileSha256(stagedPath) !== staged.sourceSha256) { fs.unlinkSync(markerPath); return; } try { fs.unlinkSync(stagedPath); } catch (error) { if (error?.code !== "ENOENT") throw error; } - try { fs.unlinkSync(markerPath); } catch (error) { if (error?.code !== "ENOENT") throw error; } - }); - } - }; + try { fs.unlinkSync(markerPath); } catch (error) { if (error?.code !== "ENOENT") throw error; } + }); + } } function isWithin(root, candidate) { diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index c60536369..37380bc28 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -120,6 +120,26 @@ function sandboxModeForPolicy(policy) { return null; } +// A scoped run constrains reads through a permission profile, which cannot +// take back what a thread started with `danger-full-access` already has: the +// sandbox is off for that thread, so the scope would be a promise this plugin +// cannot keep. +function assertScopedNotEscalated(threadId, response, { resumed }) { + const effectiveMode = sandboxModeForPolicy(response?.sandbox); + if (effectiveMode !== "danger-full-access") { + return; + } + throw new Error( + `Thread ${threadId} runs with the Codex sandbox disabled (danger-full-access), so --read-root cannot scope it. ` + + (resumed + ? "Start a fresh thread with --fresh to run scoped." + : // A fresh thread is what just started, so "--fresh" would only repeat + // this. The mode came from the Codex config that applies here. + "It was started that way by the Codex config in effect (sandbox_mode in config.toml), " + + "so drop --read-root or change that default to run scoped.") + ); +} + function assertResumedSandbox(threadId, requestedMode, response) { if (!requestedMode || !SANDBOX_POLICY_TYPES.has(requestedMode)) { return; @@ -1335,7 +1355,18 @@ export async function runAppServerTurn(cwd, options = {}) { write: options.write, ephemeral: false }); - assertResumedSandbox(options.resumeThreadId, options.sandbox, response); + // With read roots, buildThreadAccessParams() deliberately sends a + // scoped permission profile and no `sandbox`, so the app-server's + // reported mode is not the one this turn requested: asserting it + // refused every `--read-root` resume, and the error's own advice + // (resume with the reported mode) silently dropped the write grant. + // The escalation half of that check still applies, though — a thread + // started with the sandbox disabled is not scoped by any profile. + if (options.readRoots?.length > 0) { + assertScopedNotEscalated(options.resumeThreadId, response, { resumed: true }); + } else { + assertResumedSandbox(options.resumeThreadId, options.sandbox, response); + } threadId = response.thread.id; } else { emitProgress(options.onProgress, "Starting Codex task thread.", "starting"); @@ -1347,6 +1378,12 @@ export async function runAppServerTurn(cwd, options = {}) { ephemeral: options.persistThread ? false : true, threadName: options.persistThread ? options.threadName : options.threadName ?? null }); + // A default config can start a thread with the sandbox disabled, in + // which case the scope this run asked for would not hold here either — + // and "start a fresh thread" is the advice the resume path gives. + if (options.readRoots?.length > 0) { + assertScopedNotEscalated(response.thread.id, response, { resumed: false }); + } threadId = response.thread.id; } } catch (error) { diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index ad152c157..68dd0150f 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"; @@ -24,6 +24,53 @@ function filterJobsForCurrentSession(jobs, options = {}) { return jobs.filter((job) => job.sessionId === sessionId); } +function isActiveJob(job) { + return job.status === "queued" || job.status === "running"; +} + +export function reconcileJobLiveness(job, options = {}) { + if (!isActiveJob(job)) { + return job; + } + // A record whose worker was already found gone carries the verdict itself: + // the pid is dropped when that is persisted (a dead pid would let the + // dead-worker reaper fail the job and stop it pinning the broker, under a + // turn that may still be running), so there is nothing left to probe. + if (job.workerExited === true && job.threadId) { + return { ...job, status: "running", phase: "worker-exited-turn-unknown", pid: null, workerExited: true }; + } + if (!Number.isSafeInteger(job.pid) || job.pid <= 0) { + return job; + } + + const killImpl = options.killImpl ?? process.kill.bind(process); + try { + killImpl(job.pid, 0); + return job; + } catch (error) { + if (error?.code === "EPERM") { + return job; + } + if (error?.code === "ESRCH") { + if (job.threadId) { + return { + ...job, + status: "running", + phase: "worker-exited-turn-unknown", + pid: null, + workerExited: true + }; + } + return { ...job, status: "terminated-unknown", phase: "worker-exited" }; + } + return job; + } +} + +export function reconcileJobsLiveness(jobs, options = {}) { + return jobs.map((job) => reconcileJobLiveness(job, options)); +} + function getJobTypeLabel(job) { if (typeof job.kindLabel === "string" && job.kindLabel) { return job.kindLabel; @@ -181,11 +228,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) { @@ -213,7 +261,9 @@ function matchJobReference(jobs, reference, predicate = () => true) { export function buildStatusSnapshot(cwd, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); const config = getConfig(workspaceRoot); - const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(listJobs(workspaceRoot), options)); + const jobs = sortJobsNewestFirst( + reconcileJobsLiveness(filterJobsForCurrentSession(listJobs(workspaceRoot), options), options) + ); const maxJobs = options.maxJobs ?? DEFAULT_MAX_STATUS_JOBS; const maxProgressLines = options.maxProgressLines ?? DEFAULT_MAX_PROGRESS_LINES; @@ -241,7 +291,7 @@ export function buildStatusSnapshot(cwd, options = {}) { export function buildSingleJobSnapshot(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); - const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot), options)); const selected = matchJobReference(jobs, reference); if (!selected) { throw new Error(`No job found for "${reference}". Run /codex:status to inspect known jobs.`); @@ -253,13 +303,18 @@ export function buildSingleJobSnapshot(cwd, reference, options = {}) { }; } -export function resolveResultJob(cwd, reference) { +export function resolveResultJob(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); - const jobs = sortJobsNewestFirst(reference ? listJobs(workspaceRoot) : filterJobsForCurrentSession(listJobs(workspaceRoot))); + const scopedJobs = reference ? listJobs(workspaceRoot) : filterJobsForCurrentSession(listJobs(workspaceRoot), options); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(scopedJobs, options)); const selected = matchJobReference( jobs, reference, - (job) => job.status === "completed" || job.status === "failed" || job.status === "cancelled" + (job) => + job.status === "completed" || + job.status === "failed" || + job.status === "cancelled" || + (Boolean(reference) && job.status === "terminated-unknown") ); if (selected) { @@ -280,15 +335,31 @@ export function resolveResultJob(cwd, reference) { export function resolveCancelableJob(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); - const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)); + const storedJobs = sortJobsNewestFirst(listJobs(workspaceRoot)); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot), options)); const activeJobs = jobs.filter((job) => job.status === "queued" || job.status === "running"); if (reference) { - const selected = matchJobReference(activeJobs, reference); - if (!selected) { - throw new Error(`No active job found for "${reference}".`); + const selected = activeJobs.some((job) => job.id === reference || job.id.startsWith(reference)) + ? matchJobReference(activeJobs, reference) + : null; + if (selected) { + return { workspaceRoot, job: selected }; } - return { workspaceRoot, job: selected }; + // Reconciliation reports a job whose worker is gone as terminated-unknown, + // which would end the cancel here. When the worker did leave a detail file + // behind, that record is exactly what the cancel path repairs and reports + // ("already finished", or a claim it adopts), so resolve against the stored + // record instead. With no file on disk there is genuinely nothing to act + // on, and the error below stands. + const storedActiveJobs = storedJobs.filter((job) => job.status === "queued" || job.status === "running"); + const storedSelected = storedActiveJobs.some((job) => job.id === reference || job.id.startsWith(reference)) + ? matchJobReference(storedActiveJobs, reference) + : null; + if (storedSelected && readStoredJob(workspaceRoot, storedSelected.id)) { + return { workspaceRoot, job: storedSelected }; + } + throw new Error(`No active job found for "${reference}".`); } const sessionScopedActiveJobs = filterJobsForCurrentSession(activeJobs, options); diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index c3469432b..f9464f037 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -17,6 +17,7 @@ const CODEX_HOME_ENV = "CODEX_HOME"; const CONFIG_DIR_NAME = path.join("plugin-cc", "config"); const FALLBACK_STATE_ROOT_DIR = path.join(os.tmpdir(), "codex-companion"); const STATE_FILE_NAME = "state.json"; +const STATE_LOCK_DIR_NAME = ".state.lock"; const JOBS_DIR_NAME = "jobs"; const MAX_JOBS = 50; const MIN_TERMINAL_JOBS = 10; @@ -49,10 +50,34 @@ function resolveWorkspaceKey(cwd) { return `${slug}-${hash}`; } -export function resolveStateDir(cwd) { +// 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, resolveWorkspaceKey(cwd)); + 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, resolveWorkspaceKey(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 = resolveWorkspaceKey(cwd); + return stateRootCandidates().map((root) => path.join(root, dirName)); } export function resolveConfigFile(cwd) { @@ -76,26 +101,79 @@ export function ensureStateDir(cwd) { } } -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. + // + // Candidates are folded in reverse (fallback first, primary last) so the + // primary root wins for anything that is not a boolean. The previous + // "first writer wins" rule was dead for every key defaults already define — + // which is all of them — so a non-boolean setting could never be read back + // from any root. + const mergedConfig = { ...defaultState().config }; + for (const parsed of [...parsedCandidates].reverse()) { + for (const [key, value] of Object.entries(parsed.config ?? {})) { + if (typeof value === "boolean") { + mergedConfig[key] = mergedConfig[key] === true || value === true; + } else { + mergedConfig[key] = value; + } + } + } + + const [primary] = parsedCandidates; + return { + ...defaultState(), + ...primary, + config: mergedConfig, + jobs: [...jobsById.values()] + }; } // Shared by the pruner, the session-end broker guard, and the dead-worker @@ -131,10 +209,59 @@ function pruneJobs(jobs) { } function resolveStateLockDir(cwd) { - return path.join(resolveStateDir(cwd), ".state.lock"); + return path.join(resolveStateDir(cwd), STATE_LOCK_DIR_NAME); } -function saveStateLocked(cwd, state) { +// Rewriting another root's state.json is a read-modify-write on a file whose +// own lock is not the one saveState() holds: a process whose primary IS that +// root can be mid-update, and the rename would drop everything it just wrote. +// So take that root's lock too -- but never wait for it. Two processes holding +// each other's primary lock would deadlock, and this prune is not urgent: the +// pruned ids stay pruned in this root, and the next save re-runs it. +// Everything a single root holds for one job: its detail file, its terminal +// claim, and its log when the log lives in that root. +function removeJobArtifacts(stateDir, job) { + const jobsDir = path.join(stateDir, JOBS_DIR_NAME); + removeFileIfExists(path.join(jobsDir, `${job.id}.json`)); + removeFileIfExists(path.join(jobsDir, `${job.id}.terminal`)); + if (typeof job.logFile === "string" && job.logFile.startsWith(`${stateDir}${path.sep}`)) { + removeFileIfExists(job.logFile); + } +} + +function pruneOtherStateRoot(otherStateDir, retainedIds, knownIds) { + const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); + if (!fs.existsSync(otherStateFile)) { + return; + } + try { + withLockSync( + path.join(otherStateDir, STATE_LOCK_DIR_NAME), + () => { + // Re-read under the lock: the copy this decision was made from could + // have been replaced while the lock was being taken. + const otherParsed = readStateFileIfValid(otherStateFile); + const otherJobs = Array.isArray(otherParsed?.jobs) ? otherParsed.jobs : []; + const dropped = (job) => !retainedIds.has(job.id) && knownIds.has(job.id); + const prunedOtherJobs = otherJobs.filter((job) => !dropped(job)); + if (prunedOtherJobs.length === otherJobs.length) { + return; + } + writeJsonFileAtomic(otherStateFile, { ...otherParsed, jobs: prunedOtherJobs }); + for (const job of otherJobs) { + if (dropped(job)) { + removeJobArtifacts(otherStateDir, job); + } + } + }, + { timeoutMs: 0 } + ); + } catch { + // Busy or unlockable: leave that root alone rather than racing its owner. + } +} + +function saveStateLocked(cwd, state, options = {}) { const previousJobs = loadState(cwd).jobs; const nextJobs = pruneJobs(state.jobs ?? []); const nextState = { @@ -147,16 +274,40 @@ function saveStateLocked(cwd, state) { }; const retainedIds = new Set(nextJobs.map((job) => job.id)); + const primaryStateDir = resolveStateDir(cwd); for (const job of previousJobs) { if (retainedIds.has(job.id)) { continue; } - removeFileIfExists(resolveJobFile(cwd, job.id)); - removeFileIfExists(resolveJobClaimFile(cwd, job.id)); - removeFileIfExists(job.logFile); + // Only this root's copies. Pruning another root's state.json is + // best-effort (it needs that root's lock), so deleting its files here + // would leave a record that is merged back in — and rewritten into the + // primary — with its detail file, claim and log already gone. Each root's + // files go when its own record does. + removeJobArtifacts(primaryStateDir, job); } writeJsonFileAtomic(resolveStateFile(cwd), nextState); + + // 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 stick for a job that lives + // only in a non-primary root. Prune every other candidate root down to the same + // retained set; new and updated jobs are still only ever written to the primary + // root, above. This only ever removes. + // Ids the caller actually decided about. updateState() hands down the snapshot + // it mutated, which closes the window between its read and the re-read above; + // a direct saveState() has no such snapshot to offer, so the re-read is the + // best available. Anything in another root outside this set arrived after the + // caller looked and is nobody's to drop here. + const knownIds = options.knownIds ?? new Set(previousJobs.map((job) => job.id)); + const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); + for (const otherStateDir of otherStateDirs) { + pruneOtherStateRoot(otherStateDir, retainedIds, knownIds); + } + return nextState; } @@ -169,8 +320,9 @@ export function updateState(cwd, mutate) { ensureStateDir(cwd); return withLockSync(resolveStateLockDir(cwd), () => { const state = loadState(cwd); + const knownIds = new Set(state.jobs.map((job) => job.id)); mutate(state); - return saveStateLocked(cwd, state); + return saveStateLocked(cwd, state, { knownIds }); }); } @@ -227,11 +379,49 @@ function writeDurableConfig(cwd, config) { return nextConfig; } +// Same lock discipline as pruneOtherStateRoot(): that root's own lock, never +// waited on. A config copy this misses is corrected by the next write, and by +// the durable config, which is the authority. +function syncOtherStateRootConfig(otherStateDir, config) { + const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); + if (!fs.existsSync(otherStateFile)) { + return; + } + try { + withLockSync( + path.join(otherStateDir, STATE_LOCK_DIR_NAME), + () => { + const otherParsed = readStateFileIfValid(otherStateFile); + if (!otherParsed) { + return; + } + writeJsonFileAtomic(otherStateFile, { + ...otherParsed, + config: { ...(otherParsed.config ?? {}), ...config } + }); + }, + { timeoutMs: 0 } + ); + } catch { + // Busy or unlockable: leave that root alone rather than racing its owner. + } +} + export function setConfig(cwd, key, value) { const nextConfig = writeDurableConfig(cwd, { ...getConfig(cwd), [key]: value }); updateState(cwd, (state) => { state.config = { ...state.config, ...nextConfig }; }); + // The cached copy in every other root has to follow. loadState() merges + // booleans with OR — deliberately, so a gate enabled under one root is not + // downgraded by a stale false under another — which also means a stranded + // true would outvote this write forever whenever the durable config cannot + // be read. Writing the new value everywhere keeps that safety direction + // without making "disable" unreachable. + const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); + for (const otherStateDir of otherStateDirs) { + syncOtherStateRootConfig(otherStateDir, nextConfig); + } return nextConfig; } @@ -268,3 +458,23 @@ export function resolveJobClaimFile(cwd, jobId) { ensureStateDir(cwd); return path.join(resolveJobsDir(cwd), `${jobId}.terminal`); } + +/** + * Every path a job's terminal-claim file could be at, primary root first. The + * claim lives beside the job's detail file, so it follows the same candidate + * roots; unlike resolveJobClaimFile() this never creates a directory. + */ +export function resolveJobClaimFileCandidates(cwd, jobId) { + return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, JOBS_DIR_NAME, `${jobId}.terminal`)); +} + +/** + * 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 4a3f0906d..114fcfc98 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -4,6 +4,8 @@ import fs from "node:fs"; import process from "node:process"; import { isPidAlive, terminateProcessTree } from "./lib/process.mjs"; +import { reconcileJobLiveness } from "./lib/job-control.mjs"; +import { brokerIdleShutdownMs } from "./lib/lifecycle-limits.mjs"; import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; import { LOG_FILE_ENV, @@ -83,6 +85,16 @@ function setEnv(name, value) { const prefix = `export ${name}=`; const line = `${prefix}${shellEscape(value)}`; + // CLAUDE_ENV_FILE is shared with every other plugin's SessionStart hook and is + // append-only by convention. Rewriting it (read, filter, rename) drops any + // export another hook appended between the read and the rename, and the + // rename replaces the file, discarding its mode along with it. So append. + // + // That means a value that changes every session (the session id, the + // transcript path) adds a line every session: the shell takes the last + // export for a key, so the file stays correct while it grows. Only an + // unchanged value is skipped. Bounded growth is the price of never + // destroying another plugin's export. let content = ""; try { content = fs.readFileSync(envFile, "utf8"); @@ -90,12 +102,19 @@ function setEnv(name, value) { if (err.code !== "ENOENT") throw err; } - const lines = content.split(/\r?\n/).filter((l) => l && !l.startsWith(prefix)); - lines.push(line); + const existing = content + .split(/\r?\n/) + .filter((entry) => entry.startsWith(prefix)) + .at(-1); + if (existing === line) { + return; + } - const tmp = `${envFile}.${process.pid}.tmp`; - fs.writeFileSync(tmp, lines.join("\n") + "\n", "utf8"); - fs.renameSync(tmp, envFile); + // Always open with a newline rather than deciding from the read above: a + // hook appending an unterminated line between that read and this write would + // otherwise run into ours, losing both exports. A blank line costs nothing to + // the shell that sources this file. + fs.appendFileSync(envFile, `\n${line}\n`, "utf8"); } // A pid-less active record has no liveness signal at all (current code @@ -114,6 +133,33 @@ function isStaleJobRecord(job) { return Date.now() - timestamp > ACTIVE_JOB_STALENESS_MS; } +// A record retained because its worker died with a turn still possibly running +// has no pid to probe, so the generic pid-less rule (a day) would keep it +// "running" — and keep it pinning the broker — for a day. The turn it protects +// cannot outlive the broker anyway: its client is gone, so the broker idles out +// on its own timer. Bound the protection to that same window. +function retainedOrphanExpired(job, env = process.env) { + if (job?.workerExited !== true) { + return false; + } + // This record is why SessionEnd declines to tear the broker down, so here + // "cannot tell" must not mean "protected forever" — isStaleJobRecord()'s + // conservative reading of an unparseable timestamp would do exactly that. A + // record with no readable timestamp has outlived anything it could protect. + const reference = job.updatedAt ?? job.createdAt ?? null; + const timestamp = reference ? Date.parse(reference) : Number.NaN; + if (!Number.isFinite(timestamp)) { + return true; + } + // The broker's idle timer is what actually ends the turn, so it sets the + // window. With that timer disabled (CODEX_BROKER_IDLE_SHUTDOWN_MS=0) nothing + // would end it, so fall back to the generic staleness bound rather than + // pinning the broker — and its app-server and MCP servers — indefinitely. + const idleMs = brokerIdleShutdownMs(env); + const windowMs = Number.isFinite(idleMs) && idleMs > 0 ? idleMs : ACTIVE_JOB_STALENESS_MS; + return Date.now() - timestamp > windowMs; +} + // Nothing else transitions the record of a worker that died without its // SessionEnd ever running (SIGKILL, OOM, reboot): reap it to failed here so // the broker guard, the pruner, and status queries all agree, instead of a @@ -147,7 +193,12 @@ async function reapDeadWorkerJobs(workspaceRoot, { excludeSessionId = null, cwd continue; } jobsAwaitingInterrupt -= 1; - const workerDead = job.pid != null ? !isPidAlive(job.pid) : isStaleJobRecord(job); + const workerDead = + job.pid != null + ? !isPidAlive(job.pid) + : job.workerExited === true + ? retainedOrphanExpired(job) + : isStaleJobRecord(job); if (!workerDead) { continue; } @@ -222,24 +273,25 @@ function hasActiveJobsFromOtherSessions(workspaceRoot, sessionId) { if (job.pid != null) { return isPidAlive(job.pid); } + if (job.workerExited === true) { + return !retainedOrphanExpired(job); + } return !isStaleJobRecord(job); }); } async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, interruptDeadline = null } = {}) { if (!cwd || !sessionId) { - return; + return { retainedOrphans: 0 }; } 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 the primary candidate + // alone would miss a session whose jobs only live in the fallback root. const sessionJobs = loadState(workspaceRoot).jobs.filter((job) => job.sessionId === sessionId); if (sessionJobs.length === 0) { - return; + return { retainedOrphans: 0 }; } const completedAt = new Date().toISOString(); @@ -251,6 +303,10 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte errorMessage: "Cancelled: the Claude session ended while the job was still running." }; const cancelledIds = new Set(); + // Jobs left active because their turn may still be running: the broker must + // outlive this hook run for them, and hasActiveJobsFromOtherSessions() will + // not speak for them — they belong to the session that is ending. + let retainedOrphans = 0; const killedPids = []; const finishingJobs = []; interruptDeadline = interruptDeadline ?? Date.now() + TURN_INTERRUPT_BUDGET_MS; @@ -266,7 +322,6 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte if (!stillRunning) { continue; } - jobsAwaitingInterrupt -= 1; // The state snapshot can carry the queued record's pid: null while the // worker has since written its real pid (and turn identity) to the job // file — and the terminal patches below null the pid field, destroying @@ -283,6 +338,35 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte } catch { // Keep the snapshot values. } + // A dead worker still gets its terminal record below — that is what keeps + // /codex:status from answering "No job found" for the session that just + // ended. Reconciliation is consulted only for the one case where writing + // that record would be a lie, and it reads the values captured above + // rather than the snapshot: the snapshot's pid can be null while the + // worker has long since published its real pid and turn identity, which + // would both skip this check and misjudge the turn as unidentified. + const reconciled = reconcileJobLiveness({ ...job, pid: workerPid, threadId, turnId }); + if (reconciled.workerExited && threadId && !turnId) { + // The worker died after turn/start was accepted but before it recorded a + // turn id, so the Codex turn may still be running and there is nothing to + // interrupt it by. Cancelling the record here would claim an outcome that + // did not happen; leave it active (its phase says why) for the next + // status query. The verdict is persisted instead of the pid: writing a + // dead pid back would let another session's dead-worker reaper fail the + // record and stop it pinning the broker — tearing the runtime down under + // the very turn this retain protects. A pid-less active record keeps the + // broker up (bounded by the staleness rule) and reconciles from the flag. + upsertJob(workspaceRoot, { + id: job.id, + phase: reconciled.phase, + pid: null, + threadId, + workerExited: true + }); + retainedOrphans += 1; + continue; + } + jobsAwaitingInterrupt -= 1; // The worker may be recording its own terminal outcome right now; only // cancel jobs whose terminal status this hook wins — unless the claim is // orphaned (its owner died before writing a terminal record), in which @@ -430,6 +514,8 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte return isActiveJob(job); }); }); + + return { retainedOrphans }; } function handleSessionStart(input) { @@ -462,7 +548,20 @@ async function handleSessionEnd(input) { interruptTurns, interruptDeadline }); - await cleanupSessionJobs(cwd, sessionId, { interruptTurns, interruptDeadline }); + const cleanup = await cleanupSessionJobs(cwd, sessionId, { interruptTurns, interruptDeadline }); + + // A turn this session could not interrupt — its worker died before publishing + // a turn id — may still be running on this broker. The guard below speaks + // only for other sessions' work, so without this the same hook run would + // tear the runtime down under the turn the retain exists to protect. + // + // What ends the wait: normally the broker's own idle timer, since the turn's + // client is gone. With that timer disabled the next session end in this + // workspace reclaims the broker once the record passes the staleness bound + // (see retainedOrphanExpired), so the runtime is held, never stranded. + if (cleanup?.retainedOrphans > 0) { + return; + } // The broker and state dir are workspace-shared, not session-owned. If any // other session still has work in flight, tearing the broker down would diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index 8d480cac0..e2d53b9c7 100644 --- a/plugins/codex/scripts/stop-review-gate-hook.mjs +++ b/plugins/codex/scripts/stop-review-gate-hook.mjs @@ -9,7 +9,7 @@ import { fileURLToPath } from "node:url"; import { getCodexAvailability } from "./lib/codex.mjs"; import { loadPromptTemplate, interpolateTemplate } from "./lib/prompts.mjs"; import { getConfig, listJobs } from "./lib/state.mjs"; -import { sortJobsNewestFirst } from "./lib/job-control.mjs"; +import { reconcileJobsLiveness, sortJobsNewestFirst } from "./lib/job-control.mjs"; import { SESSION_ID_ENV } from "./lib/tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; @@ -145,7 +145,7 @@ function main() { const workspaceRoot = resolveWorkspaceRoot(cwd); const config = getConfig(workspaceRoot); - const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(listJobs(workspaceRoot), input)); + const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(reconcileJobsLiveness(listJobs(workspaceRoot)), input)); const runningJob = jobs.find((job) => job.status === "queued" || job.status === "running"); const runningTaskNote = runningJob ? `Codex task ${runningJob.id} is still running. Check /codex:status and use /codex:cancel ${runningJob.id} if you want to stop it before ending the session.` diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 4b8a09dbe..18b95c4ec 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -11,6 +11,7 @@ import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { makeTempDir } from "./helpers.mjs"; import { createBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; import { + clearBrokerSession, ensureBrokerSession, loadBrokerSession, saveBrokerSession, @@ -1164,3 +1165,135 @@ test("a legacy orphaned group still preserves state", { skip: process.platform = await cleanupOrphanGroup(leaderPid, grandchildPid); } }); + +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/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs new file mode 100644 index 000000000..cd8eef5cb --- /dev/null +++ b/tests/broker-subscriptions.test.mjs @@ -0,0 +1,798 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import net from "node:net"; +import path from "node:path"; +import test from "node:test"; +import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; +import { makeTempDir } from "./helpers.mjs"; + +const ROOT = path.resolve(fileURLToPath(new URL("..", import.meta.url))); +const BROKER = path.join(ROOT, "plugins", "codex", "scripts", "app-server-broker.mjs"); + +function delay(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +async function waitWithTimeout(promise, timeoutMs) { + let timer; + try { + return await Promise.race([ + promise, + new Promise((resolve) => { + timer = setTimeout(() => resolve(null), timeoutMs); + }) + ]); + } finally { + clearTimeout(timer); + } +} + +async function waitFor(predicate, { timeoutMs = 10000, intervalMs = 25 } = {}) { + const startedAt = Date.now(); + while (Date.now() - startedAt < timeoutMs) { + if (await predicate()) { + return true; + } + await delay(intervalMs); + } + return false; +} + +function readState(statePath) { + try { + return JSON.parse(fs.readFileSync(statePath, "utf8")); + } catch { + return null; + } +} + +function startBroker(behavior = "review-ok") { + const binDir = makeTempDir("codex-broker-bin-"); + installFakeCodex(binDir, behavior); + const sessionDir = makeTempDir("codex-broker-subscriptions-"); + const cwd = makeTempDir("codex-broker-cwd-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + const statePath = path.join(binDir, "fake-codex-state.json"); + const tempDirs = [binDir, sessionDir, cwd]; + + // This fork's broker authenticates shutdown against a per-instance token and + // refuses to start without one. + const instanceToken = `subscriptions-test-${process.pid}-${Math.random().toString(36).slice(2)}`; + const child = spawn( + process.execPath, + [ + BROKER, + "serve", + "--endpoint", + `unix:${socketPath}`, + "--cwd", + cwd, + "--pid-file", + pidFile, + "--instance-token", + instanceToken + ], + { env: buildEnv(binDir), stdio: ["ignore", "pipe", "pipe"] } + ); + let stderr = ""; + child.stderr.on("data", (chunk) => { + stderr += chunk; + }); + const exited = new Promise((resolve) => { + child.on("exit", (code, signal) => resolve({ code, signal })); + }); + + async function stop() { + try { + if (child.exitCode !== null || child.signalCode !== null) { + await exited; + return; + } + child.kill("SIGTERM"); + const result = await waitWithTimeout(exited, 5000); + if (!result) { + child.kill("SIGKILL"); + await exited; + } + } finally { + for (const tempDir of tempDirs) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + } + } + + return { + socketPath, + statePath, + stderr: () => stderr, + listening: () => waitFor(() => fs.existsSync(socketPath)), + stop + }; +} + +async function connectClient(socketPath) { + const socket = await new Promise((resolve, reject) => { + const candidate = net.createConnection({ path: socketPath }); + candidate.on("connect", () => resolve(candidate)); + candidate.on("error", reject); + }); + socket.setEncoding("utf8"); + + let nextId = 1; + let buffer = ""; + const pending = new Map(); + const notifications = []; + const notificationWaiters = new Set(); + const closed = new Promise((resolve) => socket.on("close", resolve)); + + function notifyWaiters(message) { + for (const waiter of [...notificationWaiters]) { + if (waiter.predicate(message)) { + waiter.settle(message); + } + } + } + + socket.on("data", (chunk) => { + buffer += chunk; + let newlineIndex = buffer.indexOf("\n"); + while (newlineIndex !== -1) { + const line = buffer.slice(0, newlineIndex); + buffer = buffer.slice(newlineIndex + 1); + newlineIndex = buffer.indexOf("\n"); + if (!line.trim()) { + continue; + } + const message = JSON.parse(line); + if (message.id !== undefined) { + const request = pending.get(message.id); + if (request) { + pending.delete(message.id); + if (message.error) { + request.reject(new Error(message.error.message)); + } else { + request.resolve(message.result); + } + } + continue; + } + notifications.push(message); + notifyWaiters(message); + } + }); + + socket.on("error", (error) => { + for (const request of pending.values()) { + request.reject(error); + } + pending.clear(); + }); + + function request(method, params = {}) { + const id = nextId++; + const response = new Promise((resolve, reject) => { + pending.set(id, { resolve, reject }); + }); + socket.write(`${JSON.stringify({ id, method, params })}\n`); + return response; + } + + async function waitForNotification(predicate, timeoutMs = 10000) { + const existing = notifications.find(predicate); + if (existing) { + return existing; + } + return new Promise((resolve) => { + let timer; + const waiter = { + predicate, + settle(message) { + clearTimeout(timer); + notificationWaiters.delete(waiter); + resolve(message); + } + }; + timer = setTimeout(() => waiter.settle(null), timeoutMs); + notificationWaiters.add(waiter); + }); + } + + await request("initialize", {}); + return { + request, + waitForNotification, + async end() { + socket.end(); + await closed; + }, + destroy() { + socket.destroy(); + } + }; +} + +async function waitForUnsubscribes(statePath, expectedThreadIds) { + const expected = [...expectedThreadIds].sort(); + const found = await waitFor(() => { + const actual = [...(readState(statePath)?.unsubscribeRequests ?? [])].sort(); + return actual.length === expected.length && actual.every((threadId, index) => threadId === expected[index]); + }); + assert.equal(found, true, `expected unsubscribe requests for ${expected.join(", ")}`); +} + +test("broker unsubscribes a completed task thread when its client closes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const started = await client.request("thread/start", { cwd: process.cwd(), ephemeral: true }); + const threadId = started.thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "test normal completion" }] + }); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === threadId + ); + assert.ok(completed, "task never completed"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker keeps a resumed thread subscribed until its final client closes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + + await firstClient.end(); + await delay(250); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); +}); + +test("broker serializes a resume behind an in-flight unsubscribe", async (t) => { + const broker = startBroker("unsubscribe-delayed"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + assert.deepEqual(readState(broker.statePath).requestOrder, ["unsubscribe:response", "thread/resume"]); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId, threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker cancels a retry when a client reacquires the thread", async (t) => { + const broker = startBroker("unsubscribe-fails-once"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const firstAttemptObserved = await waitFor( + () => readState(broker.statePath)?.unsubscribeRequests?.length === 1 + ); + assert.equal(firstAttemptObserved, true, "initial unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + await delay(3000); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + await secondClient.end(); + const unsubscribed = await waitFor(() => readState(broker.statePath)?.subscriptions?.length === 0); + assert.equal(unsubscribed, true, "reacquired thread was not unsubscribed after its final owner closed"); + assert.equal(readState(broker.statePath).unsubscribeRequests.at(-1), threadId); +}); + +test("broker unsubscribes source and detached review threads", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const sourceThreadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + const review = await client.request("review/start", { + threadId: sourceThreadId, + delivery: "detached", + target: { type: "uncommittedChanges" } + }); + assert.notEqual(review.reviewThreadId, sourceThreadId); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === review.reviewThreadId + ); + assert.ok(completed, "review never completed"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [sourceThreadId, review.reviewThreadId]); +}); + +test("broker unsubscribes a forked thread when its client closes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const sourceThreadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const forkThreadId = (await client.request("thread/fork", { threadId: sourceThreadId, ephemeral: true })).thread.id; + + await client.end(); + await waitForUnsubscribes(broker.statePath, [sourceThreadId, forkThreadId]); +}); + +test("broker unsubscribes when a client disconnects during an active turn", async (t) => { + const broker = startBroker("slow-task"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "disconnect this client" }] + }); + client.destroy(); + + await waitForUnsubscribes(broker.statePath, [threadId]); +}); + +test("broker unsubscribes auto-subscribed subagent threads", async (t) => { + const broker = startBroker("with-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "delegate this task" }] + }); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === threadId + ); + assert.ok(completed, "task never completed"); + + const subagentThread = readState(broker.statePath).threads.find((thread) => thread.name === "design-challenger"); + assert.ok(subagentThread, "subagent thread was not created"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [threadId, subagentThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker tracks subagent subscriptions from collaboration items", async (t) => { + const broker = startBroker("with-receiver-only-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "delegate without a thread-started notification" }] + }); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === threadId + ); + assert.ok(completed, "task never completed"); + + const subagentThread = readState(broker.statePath).threads.find((thread) => thread.name === "design-challenger"); + assert.ok(subagentThread, "subagent thread was not created"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [threadId, subagentThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker unsubscribes a child thread created after its client disconnects", async (t) => { + const broker = startBroker("with-delayed-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "disconnect before delegation" }] + }); + client.destroy(); + + const childCreated = await waitFor(() => + readState(broker.statePath)?.threads.some((thread) => thread.name === "delayed-design-challenger") + ); + assert.equal(childCreated, true, "delayed child thread was not created"); + const childThread = readState(broker.statePath).threads.find( + (thread) => thread.name === "delayed-design-challenger" + ); + + await waitForUnsubscribes(broker.statePath, [threadId, childThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker does not assign a delayed child thread to an unrelated active client", async (t) => { + const broker = startBroker("with-delayed-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const firstThreadId = ( + await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: true }) + ).thread.id; + await firstClient.request("turn/start", { + threadId: firstThreadId, + input: [{ type: "text", text: "create a delayed child" }] + }); + firstClient.destroy(); + + const secondClient = await connectClient(broker.socketPath); + const secondThreadId = ( + await secondClient.request("thread/start", { cwd: process.cwd(), ephemeral: true }) + ).thread.id; + await secondClient.request("turn/start", { + threadId: secondThreadId, + input: [{ type: "text", text: "remain active while the first child arrives" }] + }); + + const childrenCreated = await waitFor( + () => readState(broker.statePath)?.threads.filter((thread) => thread.parentThreadId).length === 2 + ); + assert.equal(childrenCreated, true, "delayed child threads were not created"); + + const state = readState(broker.statePath); + const firstChild = state.threads.find((thread) => thread.parentThreadId === firstThreadId); + const secondChild = state.threads.find((thread) => thread.parentThreadId === secondThreadId); + assert.ok(firstChild, "the first client's child thread was not recorded"); + assert.ok(secondChild, "the second client's child thread was not recorded"); + + const firstReleased = await waitFor(() => { + const requests = readState(broker.statePath)?.unsubscribeRequests ?? []; + return requests.includes(firstThreadId) && requests.includes(firstChild.id); + }); + assert.equal(firstReleased, true, "the disconnected client's subscriptions were not released"); + const requestsBeforeSecondClientCloses = readState(broker.statePath).unsubscribeRequests; + assert.equal(requestsBeforeSecondClientCloses.includes(secondThreadId), false); + assert.equal(requestsBeforeSecondClientCloses.includes(secondChild.id), false); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [firstThreadId, firstChild.id, secondThreadId, secondChild.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker serializes subscription requests from one downstream client", async (t) => { + const broker = startBroker("overlapping-resume"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const secondClient = await connectClient(broker.socketPath); + const results = await Promise.allSettled([ + secondClient.request("thread/resume", { threadId, persistFullHistory: true }), + secondClient.request("thread/resume", { threadId }) + ]); + assert.equal(results[0].status, "rejected"); + assert.match(results[0].reason.message, /forced delayed resume failure/); + assert.equal(results[1].status, "fulfilled"); + + await firstClient.end(); + await delay(250); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); +}); + +test("broker replies to a failed resume even when the upstream unsubscribe hangs", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + // Nobody owns this thread, so the failed provisional claim releases it upstream. + const threadId = "thr_unowned"; + const firstClient = await connectClient(broker.socketPath); + const failedResume = await waitWithTimeout( + firstClient.request("thread/resume", { threadId, persistFullHistory: true }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 2000 + ); + assert.notEqual(failedResume, null, "failed resume did not receive a response"); + assert.equal(failedResume.status, "rejected"); + assert.match(failedResume.error.message, /forced resume failure/); + + const secondClient = await connectClient(broker.socketPath); + const secondStarted = await waitWithTimeout( + secondClient.request("thread/start", { cwd: process.cwd(), ephemeral: false }), + 2000 + ); + assert.notEqual(secondStarted, null, "broker remained busy after the failed resume"); + + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "the released thread was not unsubscribed upstream"); + await secondClient.end(); + await firstClient.end(); +}); + +test("broker keeps shared upstream subscriptions when one client explicitly unsubscribes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + + assert.deepEqual(await firstClient.request("thread/unsubscribe", { threadId }), { status: "unsubscribed" }); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + const thirdClient = await connectClient(broker.socketPath); + assert.deepEqual(await thirdClient.request("thread/unsubscribe", { threadId }), { status: "notSubscribed" }); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + + await firstClient.end(); + await thirdClient.end(); + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker does not reclaim ownership from notifications during explicit unsubscribe", async (t) => { + const broker = startBroker("unsubscribe-notifies"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + assert.deepEqual(await client.request("thread/unsubscribe", { threadId }), { status: "unsubscribed" }); + await client.end(); + await delay(250); + + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker logs upstream unsubscribe failures", async (t) => { + const broker = startBroker("unsubscribe-fails"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.end(); + + const requestObserved = await waitFor(() => readState(broker.statePath)?.unsubscribeRequests?.includes(threadId)); + assert.equal(requestObserved, true, "unsubscribe request was not observed"); + const warningObserved = await waitFor( + () => broker.stderr().includes(`Failed to unsubscribe Codex thread ${threadId}: thread unsubscribe failed`) + ); + assert.equal(warningObserved, true, "unsubscribe failure was not logged"); + const retryBoundReached = await waitFor( + () => readState(broker.statePath)?.unsubscribeRequests?.length === 4, + { timeoutMs: 6000 } + ); + assert.equal(retryBoundReached, true, "unsubscribe retries did not reach the expected bound"); + await delay(500); + assert.equal(readState(broker.statePath).unsubscribeRequests.length, 4); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); +}); + +test("broker retries a transient upstream unsubscribe failure", async (t) => { + const broker = startBroker("unsubscribe-fails-once"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.end(); + + const retried = await waitFor(() => { + const state = readState(broker.statePath); + return state?.unsubscribeRequests?.length === 2 && state.subscriptions.length === 0; + }); + assert.equal(retried, true, "the failed unsubscribe was not retried"); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId, threadId]); + assert.match(broker.stderr(), new RegExp(`Failed to unsubscribe Codex thread ${threadId}`)); +}); + +test("broker fails a resume when an in-flight unsubscribe outlives the bounded wait", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + const startedAt = Date.now(); + const resumed = await waitWithTimeout( + secondClient.request("thread/resume", { threadId }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 8000 + ); + assert.notEqual(resumed, null, "resume never settled while the upstream unsubscribe hung"); + assert.equal(resumed.status, "rejected"); + assert.match(resumed.error.message, /still being released upstream/); + assert.ok(Date.now() - startedAt >= 4000, "resume did not wait for the in-flight unsubscribe"); + // The resume was never sent upstream, so the fake still lists only the original subscription. + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + const thirdClient = await connectClient(broker.socketPath); + const thirdStarted = await waitWithTimeout( + thirdClient.request("thread/start", { cwd: process.cwd(), ephemeral: false }), + 2000 + ); + assert.notEqual(thirdStarted, null, "broker remained busy after the bounded wait"); + await thirdClient.end(); + await secondClient.end(); +}); + +test("broker keeps rejecting claims until a hung unsubscribe settles", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const attempt = async () => { + const client = await connectClient(broker.socketPath); + const outcome = await waitWithTimeout( + client.request("thread/resume", { threadId }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 8000 + ); + await client.end(); + return outcome; + }; + + // The first claim times out on the hung unsubscribe. Its release installs a + // follow-up cleanup that must stay pending; a retry must still be rejected. + const first = await attempt(); + assert.equal(first?.status, "rejected"); + assert.match(first.error.message, /still being released upstream/); + const retry = await attempt(); + assert.equal(retry?.status, "rejected", "a retry slipped past the still-outstanding unsubscribe"); + assert.match(retry.error.message, /still being released upstream/); + assert.deepEqual(readState(broker.statePath).requestOrder, []); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); +}); + +test("broker rejects an explicit unsubscribe that would queue behind a hung cleanup", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + const explicit = await waitWithTimeout( + secondClient.request("thread/unsubscribe", { threadId }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 8000 + ); + assert.notEqual(explicit, null, "explicit unsubscribe never settled behind the hung cleanup"); + assert.equal(explicit.status, "rejected"); + assert.match(explicit.error.message, /still being released upstream/); + + const thirdClient = await connectClient(broker.socketPath); + const thirdStarted = await waitWithTimeout( + thirdClient.request("thread/start", { cwd: process.cwd(), ephemeral: false }), + 2000 + ); + assert.notEqual(thirdStarted, null, "broker remained busy after the rejected explicit unsubscribe"); + // Only the original hung request reached upstream; nothing else was queued. + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId]); + await thirdClient.end(); + await secondClient.end(); +}); + +test("broker rolls back child threads inherited through a failed provisional claim", async (t) => { + const broker = startBroker("with-delayed-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.request("turn/start", { + threadId, + input: [{ type: "text", text: "spawn a child while another client is resuming" }] + }); + firstClient.destroy(); + await waitForUnsubscribes(broker.statePath, [threadId]); + + // The resume fails after 250 ms; the delayed child arrives at 100 ms while the + // claim is still open, so the claiming socket inherits it. + const secondClient = await connectClient(broker.socketPath); + await assert.rejects( + secondClient.request("thread/resume", { threadId, persistFullHistory: true }), + /forced resume failure after child arrival/ + ); + const childThread = readState(broker.statePath).threads.find( + (thread) => thread.name === "delayed-design-challenger" + ); + assert.ok(childThread, "delayed child thread was not created"); + const grandchildCreated = await waitFor(() => + readState(broker.statePath)?.threads.some((thread) => thread.name === "delayed-design-grandchild") + ); + assert.equal(grandchildCreated, true, "delayed grandchild thread was not created"); + const grandchildThread = readState(broker.statePath).threads.find( + (thread) => thread.name === "delayed-design-grandchild" + ); + + // The parent, the inherited child, and the nested grandchild are all released + // while the client stays connected. + await waitForUnsubscribes(broker.statePath, [threadId, threadId, childThread.id, grandchildThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); + await secondClient.end(); +}); + +test("broker retries cleanup when a client disconnects during a failing explicit unsubscribe", async (t) => { + const broker = startBroker("unsubscribe-fails-once-delayed"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + // Send the explicit unsubscribe and drop the socket before the upstream reply. + void client.request("thread/unsubscribe", { threadId }).catch(() => {}); + await waitFor(() => readState(broker.statePath)?.unsubscribeRequests?.length === 1); + client.destroy(); + + // The first attempt fails 300 ms later, after the requester is gone; the broker must retry on its own. + await waitForUnsubscribes(broker.statePath, [threadId, threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 6b17012d4..448adb8c9 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -19,13 +19,16 @@ const readline = require("node:readline"); function loadState() { if (!fs.existsSync(STATE_PATH)) { - return { nextThreadId: 1, nextTurnId: 1, appServerStarts: 0, threads: [], capabilities: null, lastInterrupt: null }; + return { nextThreadId: 1, nextTurnId: 1, appServerStarts: 0, threads: [], subscriptions: [], unsubscribeRequests: [], requestOrder: [], capabilities: null, lastInterrupt: null }; } return JSON.parse(fs.readFileSync(STATE_PATH, "utf8")); } function saveState(state) { - fs.writeFileSync(STATE_PATH, JSON.stringify(state, null, 2)); + // Write atomically so a test reading the file never sees a partial document. + const tmpPath = STATE_PATH + ".tmp"; + fs.writeFileSync(tmpPath, JSON.stringify(state, null, 2)); + fs.renameSync(tmpPath, STATE_PATH); } function requiresExperimental(field, message, state) { @@ -42,6 +45,8 @@ function now() { function buildThread(thread) { return { id: thread.id, + forkedFromId: thread.forkedFromId || null, + parentThreadId: thread.parentThreadId || null, preview: thread.preview || "", ephemeral: Boolean(thread.ephemeral), modelProvider: "openai", @@ -126,9 +131,11 @@ function sandboxPolicy(mode) { return { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }; } -function nextThread(state, cwd, ephemeral, sandbox) { +function nextThread(state, cwd, ephemeral, sandbox, { forkedFromId = null, parentThreadId = null } = {}) { const thread = { id: "thr_" + state.nextThreadId++, + forkedFromId, + parentThreadId, cwd: cwd || process.cwd(), name: null, preview: "", @@ -332,9 +339,10 @@ rl.on("line", (line) => { threadId: thread.id, approvalPolicy: message.params.approvalPolicy ?? null }; + state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; saveState(state); send({ id: message.id, result: { thread: buildThread(thread), model: message.params.model || "gpt-5.4", modelProvider: "openai", serviceTier: null, cwd: thread.cwd, approvalPolicy: "never", sandbox: sandboxPolicy(thread.sandbox), reasoningEffort: null } }); - send({ method: "thread/started", params: { thread: { id: thread.id } } }); + send({ method: "thread/started", params: { thread: buildThread(thread) } }); break; } @@ -364,6 +372,26 @@ rl.on("line", (line) => { if (BEHAVIOR === "permission-profiles-unsupported" && message.params.config) { throw new Error("unknown field config.default_permissions"); } + if (BEHAVIOR === "with-delayed-subagent" && message.params.persistFullHistory === true) { + state.nestedSubagentRequested = true; + saveState(state); + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "forced resume failure after child arrival" } }); + }, 250); + break; + } + if (BEHAVIOR === "resume-fails-unsubscribe-hangs" && message.params.persistFullHistory === true) { + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "forced resume failure" } }); + }, 50); + break; + } + if (BEHAVIOR === "overlapping-resume" && message.params.persistFullHistory === true) { + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "forced delayed resume failure" } }); + }, 100); + break; + } if (requiresExperimental("persistExtendedHistory", message, state) || requiresExperimental("persistFullHistory", message, state)) { throw new Error("thread/resume.persistFullHistory requires experimentalApi capability"); } @@ -377,11 +405,78 @@ rl.on("line", (line) => { ...message.params, threadId: thread.id }; + if (BEHAVIOR === "unsubscribe-delayed" || BEHAVIOR === "resume-fails-unsubscribe-hangs") { + state.requestOrder = [...(state.requestOrder || []), "thread/resume"]; + } + state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; saveState(state); send({ id: message.id, result: { thread: buildThread(thread), model: message.params.model || "gpt-5.4", modelProvider: "openai", serviceTier: null, cwd: thread.cwd, approvalPolicy: "never", sandbox: resumedPolicy, reasoningEffort: null } }); break; } + case "thread/fork": { + const sourceThread = ensureThread(state, message.params.threadId); + const thread = nextThread(state, sourceThread.cwd, message.params.ephemeral, sourceThread.sandbox, { forkedFromId: sourceThread.id }); + state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; + saveState(state); + send({ id: message.id, result: { thread: buildThread(thread) } }); + send({ method: "thread/started", params: { thread: buildThread(thread) } }); + break; + } + + case "thread/unsubscribe": { + const subscriptions = state.subscriptions || []; + const wasSubscribed = subscriptions.includes(message.params.threadId); + const wasLoaded = state.threads.some((thread) => thread.id === message.params.threadId); + state.unsubscribeRequests = [...(state.unsubscribeRequests || []), message.params.threadId]; + if (BEHAVIOR === "resume-fails-unsubscribe-hangs") { + saveState(state); + break; + } + if (BEHAVIOR === "unsubscribe-fails-once-delayed" && state.unsubscribeRequests.length === 1) { + saveState(state); + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "thread unsubscribe failed" } }); + }, 300); + break; + } + if ( + BEHAVIOR === "unsubscribe-fails" || + (BEHAVIOR === "unsubscribe-fails-once" && state.unsubscribeRequests.length === 1) + ) { + saveState(state); + send({ id: message.id, error: { code: -32000, message: "thread unsubscribe failed" } }); + break; + } + if (BEHAVIOR === "unsubscribe-delayed") { + state.subscriptions = subscriptions.filter((threadId) => threadId !== message.params.threadId); + saveState(state); + setTimeout(() => { + const delayedState = loadState(); + delayedState.requestOrder = [...(delayedState.requestOrder || []), "unsubscribe:response"]; + saveState(delayedState); + send({ + id: message.id, + result: { status: wasSubscribed ? "unsubscribed" : wasLoaded ? "notSubscribed" : "notLoaded" } + }); + }, 300); + break; + } + if (BEHAVIOR === "unsubscribe-notifies") { + send({ + method: "thread/status/changed", + params: { threadId: message.params.threadId, status: { type: "idle" } } + }); + } + state.subscriptions = subscriptions.filter((threadId) => threadId !== message.params.threadId); + saveState(state); + send({ + id: message.id, + result: { status: wasSubscribed ? "unsubscribed" : wasLoaded ? "notSubscribed" : "notLoaded" } + }); + break; + } + case "externalAgentConfig/import": { if (BEHAVIOR === "external-import-unsupported") { send({ id: message.id, error: { code: -32601, message: "Unsupported method: externalAgentConfig/import" } }); @@ -440,6 +535,8 @@ rl.on("line", (line) => { let reviewThread = thread; if (message.params.delivery === "detached") { reviewThread = nextThread(state, thread.cwd, true); + state.subscriptions = [...new Set([...(state.subscriptions || []), reviewThread.id])]; + saveState(state); send({ method: "thread/started", params: { thread: { id: reviewThread.id } } }); } const turnId = nextTurnId(state); @@ -489,18 +586,49 @@ rl.on("line", (line) => { ? structuredReviewPayload(prompt) : taskPayload(prompt, thread.name && thread.name.startsWith("Codex Companion Task") && prompt.includes("Continue from the current thread state")); + if (BEHAVIOR === "with-delayed-subagent") { + setTimeout(() => { + const delayedState = loadState(); + const subThread = nextThread(delayedState, thread.cwd, true, thread.sandbox, { parentThreadId: thread.id }); + const subThreadRecord = ensureThread(delayedState, subThread.id); + subThreadRecord.name = "delayed-design-challenger"; + delayedState.subscriptions = [...new Set([...(delayedState.subscriptions || []), subThread.id])]; + saveState(delayedState); + const subTurnId = nextTurnId(delayedState); + send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: subThreadRecord.name, agentNickname: subThreadRecord.name } } }); + send({ method: "turn/started", params: { threadId: subThread.id, turn: buildTurn(subTurnId) } }); + send({ method: "turn/completed", params: { threadId: subThread.id, turn: buildTurn(subTurnId, "completed") } }); + if (delayedState.nestedSubagentRequested) { + setTimeout(() => { + const nestedState = loadState(); + const grandchild = nextThread(nestedState, thread.cwd, true, thread.sandbox, { parentThreadId: subThread.id }); + const grandchildRecord = ensureThread(nestedState, grandchild.id); + grandchildRecord.name = "delayed-design-grandchild"; + nestedState.subscriptions = [...new Set([...(nestedState.subscriptions || []), grandchild.id])]; + saveState(nestedState); + send({ method: "thread/started", params: { thread: { ...buildThread(grandchildRecord), name: grandchildRecord.name, agentNickname: grandchildRecord.name } } }); + }, 100); + } + }, 100); + break; + } + if ( BEHAVIOR === "with-subagent" || + BEHAVIOR === "with-receiver-only-subagent" || BEHAVIOR === "with-late-subagent-message" || BEHAVIOR === "with-subagent-no-main-turn-completed" ) { - const subThread = nextThread(state, thread.cwd, true); + const subThread = nextThread(state, thread.cwd, true, thread.sandbox, { parentThreadId: thread.id }); const subThreadRecord = ensureThread(state, subThread.id); subThreadRecord.name = "design-challenger"; + state.subscriptions = [...new Set([...(state.subscriptions || []), subThread.id])]; saveState(state); const subTurnId = nextTurnId(state); - send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: "design-challenger", agentNickname: "design-challenger" } } }); + if (BEHAVIOR !== "with-receiver-only-subagent") { + send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: "design-challenger", agentNickname: "design-challenger" } } }); + } send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(turnId) } }); send({ method: "item/started", diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs new file mode 100644 index 000000000..576be7246 --- /dev/null +++ b/tests/job-control.test.mjs @@ -0,0 +1,201 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { saveState } from "../plugins/codex/scripts/lib/state.mjs"; + +import { + buildStatusSnapshot, + reconcileJobLiveness, + resolveCancelableJob, + resolveResultJob +} from "../plugins/codex/scripts/lib/job-control.mjs"; + +function activeJob(overrides = {}) { + return { + id: "task-dead-worker", + status: "running", + phase: "editing", + pid: 999999, + ...overrides + }; +} + +test("reconcileJobLiveness marks an absent worker as terminated-unknown", () => { + const job = reconcileJobLiveness(activeJob(), { + killImpl() { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + }); + + assert.equal(job.status, "terminated-unknown"); + assert.equal(job.phase, "worker-exited"); + assert.equal(job.pid, 999999); +}); + +test("reconcileJobLiveness keeps a dead worker active when its Codex turn may still run", () => { + const job = reconcileJobLiveness( + activeJob({ threadId: "thr_live", turnId: "turn_live" }), + { + killImpl() { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + } + ); + + assert.equal(job.status, "running"); + assert.equal(job.phase, "worker-exited-turn-unknown"); + assert.equal(job.threadId, "thr_live"); + assert.equal(job.turnId, "turn_live"); +}); + +test("reconcileJobLiveness keeps the turn-start response window active", () => { + const job = reconcileJobLiveness(activeJob({ threadId: "thr_pending", turnId: null }), { + killImpl() { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + }); + assert.equal(job.status, "running"); + assert.equal(job.phase, "worker-exited-turn-unknown"); + assert.equal(job.pid, null); + assert.equal(job.workerExited, true); +}); + +test("reconcileJobLiveness treats EPERM as alive", () => { + const original = activeJob(); + const job = reconcileJobLiveness(original, { + killImpl() { + const error = new Error("not permitted"); + error.code = "EPERM"; + throw error; + } + }); + assert.deepEqual(job, original); +}); + +test("reconcileJobLiveness leaves terminal and pid-less jobs untouched", () => { + let calls = 0; + const killImpl = () => { calls += 1; }; + const completed = activeJob({ status: "completed" }); + const noPid = activeJob({ pid: null }); + + assert.deepEqual(reconcileJobLiveness(completed, { killImpl }), completed); + assert.deepEqual(reconcileJobLiveness(noPid, { killImpl }), noPid); + assert.equal(calls, 0); +}); + +test("dead wrapper with a live turn stays active and cancelable", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-turn-unknown-")); + const workspace = path.join(root, "workspace"); + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + fs.mkdirSync(workspace, { recursive: true }); + process.env.CLAUDE_PLUGIN_DATA = path.join(root, "plugin-data"); + + try { + saveState(workspace, { + config: { stopReviewGate: false }, + jobs: [activeJob({ + threadId: "thr_live", + turnId: "turn_live", + sessionId: "sess-current", + createdAt: "2026-09-04T10:00:00.000Z", + updatedAt: "2026-09-04T10:01:00.000Z" + })] + }); + const killImpl = () => { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + }; + const options = { env: { CODEX_COMPANION_SESSION_ID: "sess-current" }, killImpl }; + const report = buildStatusSnapshot(workspace, options); + assert.equal(report.running.length, 1); + assert.equal(report.running[0].phase, "worker-exited-turn-unknown"); + assert.equal(report.running[0].pid, null); + assert.equal(resolveCancelableJob(workspace, "task-dead-worker", options).job.turnId, "turn_live"); + assert.throws(() => resolveResultJob(workspace, "task-dead-worker", options)); + } finally { + if (previousPluginData === undefined) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + fs.rmSync(root, { recursive: true, force: true }); + } +}); + +test("buildStatusSnapshot moves a dead worker out of the active queue", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-control-")); + const workspace = path.join(root, "workspace"); + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + fs.mkdirSync(workspace, { recursive: true }); + process.env.CLAUDE_PLUGIN_DATA = path.join(root, "plugin-data"); + + try { + saveState(workspace, { + config: { stopReviewGate: false }, + jobs: [{ + id: "task-dead-worker", + status: "running", + phase: "editing", + pid: 999999, + createdAt: "2026-09-04T10:00:00.000Z", + updatedAt: "2026-09-04T10:01:00.000Z" + }] + }); + const reportKill = () => { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + }; + const report = buildStatusSnapshot(workspace, { env: {}, killImpl: reportKill }); + assert.equal(report.running.length, 0); + assert.equal(report.latestFinished.status, "terminated-unknown"); + assert.equal(report.latestFinished.phase, "worker-exited"); + assert.equal( + resolveResultJob(workspace, "task-dead-worker", { killImpl: reportKill }).job.status, + "terminated-unknown" + ); + assert.throws( + () => resolveCancelableJob(workspace, "task-dead-worker", { killImpl: reportKill }), + /No job found|No active job/ + ); + } finally { + if (previousPluginData === undefined) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + fs.rmSync(root, { recursive: true, force: true }); + } +}); + + +test("implicit result ignores a newer dead worker in favor of stored final output", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-result-")); + const workspace = path.join(root, "workspace"); + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + fs.mkdirSync(workspace, { recursive: true }); + process.env.CLAUDE_PLUGIN_DATA = path.join(root, "plugin-data"); + + try { + saveState(workspace, { + config: { stopReviewGate: false }, + jobs: [ + activeJob({ sessionId: "sess-current", updatedAt: "2026-09-04T10:02:00.000Z" }), + { id: "task-completed", status: "completed", sessionId: "sess-current", result: { rawOutput: "done" }, updatedAt: "2026-09-04T10:01:00.000Z" } + ] + }); + const killImpl = () => { const error = new Error("no such process"); error.code = "ESRCH"; throw error; }; + const options = { env: { CODEX_COMPANION_SESSION_ID: "sess-current" }, killImpl }; + + assert.equal(resolveResultJob(workspace, null, options).job.id, "task-completed"); + assert.equal(resolveResultJob(workspace, "task-dead-worker", options).job.status, "terminated-unknown"); + } finally { + if (previousPluginData === undefined) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + fs.rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/tests/locking.test.mjs b/tests/locking.test.mjs index 37eba6e4b..eebc69559 100644 --- a/tests/locking.test.mjs +++ b/tests/locking.test.mjs @@ -74,13 +74,22 @@ test("lock immediately reclaims an owner process that exited", async () => { }); }); + // acquireLock() throws once its own deadline passes without acquiring, so a + // successful return under a 500ms budget with staleMs at 30s is itself the + // assertion: the lock was reclaimed because its owner is gone, not because it + // aged out. Measuring wall-clock here instead added nothing and made the test + // flaky -- the deadline is only checked after a failed attempt, so one slow + // attempt on a loaded runner returns successfully at over 500ms. const startedAt = Date.now(); const successor = await acquireLock(lockDir, { timeoutMs: 500, staleMs: 30000, retryDelayMs: 5 }); - assert.ok(Date.now() - startedAt < 500, "dead owner should be reclaimed before stale timeout"); + assert.ok( + Date.now() - startedAt < 30000, + "reclaiming a dead owner must not wait for the stale timeout" + ); releaseLock(successor); }); diff --git a/tests/node-launcher.test.mjs b/tests/node-launcher.test.mjs index e51128241..9ea6234d9 100644 --- a/tests/node-launcher.test.mjs +++ b/tests/node-launcher.test.mjs @@ -417,3 +417,96 @@ test("portable launcher aligns Node with the supported toolchain that contains c fs.rmSync(home, { recursive: true, force: true }); } }); + + +// Regression for the precedence bug this fork fixed in find_managed_node(): the +// hardcoded system paths (/usr/local/bin, /opt/homebrew, ...) used to be tried +// before every version-manager root, so on any machine carrying a system node +// the managed toolchains were shadowed. The assertion below is only meaningful +// where such a node exists (it is what the launcher would otherwise pick); where +// it does not, the test still passes and costs nothing. +function installManagedNode(binDir, marker) { + fs.mkdirSync(binDir, { recursive: true }); + const nodePath = path.join(binDir, "node"); + fs.writeFileSync( + nodePath, + `#!/bin/sh\nif [ "\${1:-}" = "-e" ]; then exit 0; fi\nprintf "${marker}:%s\\n" "$*"\n`, + "utf8" + ); + fs.chmodSync(nodePath, 0o755); + return nodePath; +} + +function installEveryManager(home) { + const nvmDir = path.join(home, "custom-nvm"); + const fnmDir = path.join(home, "custom-fnm"); + const asdfDir = path.join(home, "custom-asdf"); + const miseDir = path.join(home, "custom-mise"); + installManagedNode(path.join(nvmDir, "versions", "node", "v22.0.0", "bin"), "MANAGED_NVM"); + installManagedNode(path.join(fnmDir, "node-versions", "v22.1.0", "installation", "bin"), "MANAGED_FNM"); + installManagedNode(path.join(asdfDir, "installs", "nodejs", "22.2.0", "bin"), "MANAGED_ASDF"); + installManagedNode(path.join(miseDir, "installs", "node", "22.3.0", "bin"), "MANAGED_MISE"); + return { + NVM_DIR: nvmDir.replaceAll("\\", "/"), + FNM_DIR: fnmDir.replaceAll("\\", "/"), + ASDF_DATA_DIR: asdfDir.replaceAll("\\", "/"), + MISE_DATA_DIR: miseDir.replaceAll("\\", "/") + }; +} + +test("portable launcher prefers a managed toolchain over a system install with several managers present", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "codex-node-multi-manager-")); + const emptyBin = path.join(home, "empty-bin"); + fs.mkdirSync(emptyBin, { recursive: true }); + const managerEnv = installEveryManager(home); + try { + const result = spawnSync(BASH, [LAUNCHER.replaceAll("\\", "/"), "companion.mjs", "status", "--json"], { + encoding: "utf8", + env: { + ...cleanVersionManagerEnv(), + HOME: home.replaceAll("\\", "/"), + PATH: emptyBin.replaceAll("\\", "/"), + CODEX_COMPANION_NODE: "", + ...managerEnv + } + }); + + assert.equal(result.status, 0, result.stderr); + // Which manager wins is deliberately not pinned: the launcher has no way to + // tell which one the project or the user actually selected (no .nvmrc, + // .tool-versions or mise config is read), so asserting one would freeze an + // arbitrary order. What must hold is that a managed toolchain is chosen at + // all rather than a system install. + assert.match(result.stdout, /MANAGED_(NVM|FNM|ASDF|MISE):/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } +}); + +test("portable launcher lets CODEX_COMPANION_NODE override every installed manager", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "codex-node-multi-manager-pinned-")); + const emptyBin = path.join(home, "empty-bin"); + fs.mkdirSync(emptyBin, { recursive: true }); + const managerEnv = installEveryManager(home); + const pinnedNode = installManagedNode(path.join(home, "pinned", "bin"), "PINNED_NODE"); + try { + const result = spawnSync(BASH, [LAUNCHER.replaceAll("\\", "/"), "companion.mjs", "status", "--json"], { + encoding: "utf8", + env: { + ...cleanVersionManagerEnv(), + HOME: home.replaceAll("\\", "/"), + PATH: emptyBin.replaceAll("\\", "/"), + CODEX_COMPANION_NODE: pinnedNode.replaceAll("\\", "/"), + ...managerEnv + } + }); + + assert.equal(result.status, 0, result.stderr); + // The one way to pin a runtime today, and the answer to "which manager?" + // until the launcher learns to read the active one. + assert.match(result.stdout, /PINNED_NODE:/); + assert.doesNotMatch(result.stdout, /MANAGED_(NVM|FNM|ASDF|MISE):/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } +}); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index da3e5d5a1..8c9dff126 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -9,7 +9,7 @@ import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; import { CodexAppServerClient } from "../plugins/codex/scripts/lib/app-server.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"); @@ -889,9 +889,17 @@ test("session start hook exports the Claude session id, transcript path, and plu }); assert.equal(result.status, 0, result.stderr); - assert.equal( - fs.readFileSync(envFile, "utf8"), - `export CODEX_COMPANION_SESSION_ID='sess-current'\nexport CODEX_COMPANION_TRANSCRIPT_PATH='${transcriptPath}'\nexport CLAUDE_PLUGIN_DATA='${pluginDataDir}'\n` + // Each export opens with its own newline, so a line another plugin's hook + // appended without one cannot run into ours. Blank lines are nothing to the + // shell that sources this file, so the contract is the exports and their + // order, not byte-for-byte content. + assert.deepEqual( + fs.readFileSync(envFile, "utf8").split("\n").filter((line) => line !== ""), + [ + "export CODEX_COMPANION_SESSION_ID='sess-current'", + `export CODEX_COMPANION_TRANSCRIPT_PATH='${transcriptPath}'`, + `export CLAUDE_PLUGIN_DATA='${pluginDataDir}'` + ] ); }); @@ -5436,3 +5444,386 @@ 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; + } + } +}); + +test("task --resume-last ignores a stale current-session worker and resumes the prior completed thread", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-current" }; + const first = run("node", [SCRIPT, "task", "initial task"], { cwd: repo, env }); + assert.equal(first.status, 0, first.stderr); + const statePath = path.join(resolveStateDir(repo), "state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.jobs.push({ id: "task-stale", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, updatedAt: "2099-01-01T00:00:00.000Z" }); + fs.writeFileSync(statePath, `${JSON.stringify(state, null, 2)}\n`, "utf8"); + const resumed = run("node", [SCRIPT, "task", "--resume-last", "follow up"], { cwd: repo, env }); + assert.equal(resumed.status, 0, resumed.stderr); + assert.equal(resumed.stdout, "Resumed the prior run.\nFollow-up prompt accepted.\n"); +}); + +test("task --resume-last blocks when a dead wrapper still has a live turn identity", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-current" }; + const first = run("node", [SCRIPT, "task", "initial task"], { cwd: repo, env }); + assert.equal(first.status, 0, first.stderr); + const statePath = path.join(resolveStateDir(repo), "state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.jobs.push({ id: "task-orphan-turn", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, threadId: "thr_orphan", turnId: "turn_orphan", updatedAt: "2099-01-01T00:00:00.000Z" }); + fs.writeFileSync(statePath, `${JSON.stringify(state, null, 2)}\n`, "utf8"); + const resumed = run("node", [SCRIPT, "task", "--resume-last", "follow up"], { cwd: repo, env }); + assert.notEqual(resumed.status, 0); + assert.match(resumed.stderr, /task-orphan-turn is still running/i); +}); + +test("cancel fails closed when an orphaned turn has no persisted turn id", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-turn-pending", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending", + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)} +`, "utf8"); + const env = { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }; + const result = run("node", [SCRIPT, "cancel", "task-turn-pending", "--json"], { cwd: workspace, env }); + assert.equal(result.status, 1); + assert.match(result.stderr, /turn id|safely interrupt|still running/i); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs[0].status, "running"); +}); + +test("session end preserves an orphaned turn whose worker exited", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-orphaned-turn", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending", + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)} +`, "utf8"); + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-current", cwd: workspace }) + }); + assert.equal(result.status, 0, result.stderr); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.length, 1); + assert.equal(state.jobs[0].id, "task-orphaned-turn"); +}); + +test("stop hook ignores a stale current-session worker when the review gate is disabled", () => { + const repo = makeTempDir(); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [{ id: "task-stale", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, updatedAt: "2099-01-01T00:00:00.000Z" }] }, null, 2)}\n`, "utf8"); + const result = run("node", [STOP_HOOK], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ cwd: repo }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stdout.trim(), ""); + assert.doesNotMatch(result.stderr, /task-stale is still running/i); +}); + +test("stop hook keeps an orphaned live turn active when its wrapper died", () => { + const repo = makeTempDir(); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [{ id: "task-orphan-turn", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, threadId: "thr_orphan", turnId: "turn_orphan", updatedAt: "2099-01-01T00:00:00.000Z" }] }, null, 2)}\n`, "utf8"); + const result = run("node", [STOP_HOOK], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ cwd: repo }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stdout.trim(), ""); + assert.match(result.stderr, /task-orphan-turn is still running/i); +}); + + +test("a refused cancel leaves no terminal claim behind", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-turn-pending", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending", + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)}\n`, "utf8"); + const env = { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }; + + const refused = run("node", [SCRIPT, "cancel", "task-turn-pending", "--json"], { cwd: workspace, env }); + assert.equal(refused.status, 1); + + // The terminal claim is never released, so a claim taken before the refusal + // would be adopted by the next cancel (or by SessionEnd) and reasserted into + // a cancelled record — for the turn this refusal exists to protect. + assert.equal(fs.existsSync(path.join(jobsDir, "task-turn-pending.terminal")), false); + + const again = run("node", [SCRIPT, "cancel", "task-turn-pending", "--json"], { cwd: workspace, env }); + assert.equal(again.status, 1); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs[0].status, "running"); +}); + +test("task --resume-last still works with scoped read roots", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + const env = buildEnv(binDir); + + const first = run("node", [SCRIPT, "task", "--write", "--read-root", repo, "initial task"], { cwd: repo, env }); + assert.equal(first.status, 0, first.stderr); + + // A scoped run sends a permission profile and no sandbox mode, so the mode + // the app-server reports on resume is not the one this turn asked for. + // Asserting it refused every --read-root resume outright. + const resumed = run("node", [SCRIPT, "task", "--resume-last", "--write", "--read-root", repo, "follow up"], { + cwd: repo, + env + }); + assert.equal(resumed.status, 0, resumed.stderr); + const fakeState = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.equal(fakeState.lastThreadResume.sandbox, undefined); + assert.equal(fakeState.lastThreadResume.config.default_permissions, "claude_companion_scoped"); +}); + + +test("a retained orphaned turn stays reconcilable after session end", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + // The index carries the queued record's pid: null, while the job file has the + // real (now dead) pid and the thread the worker started. Session end must read + // the file, not the snapshot: on the snapshot alone there is no pid to judge, + // so the job would be recorded cancelled while its turn may still run. + fs.writeFileSync(path.join(jobsDir, "task-retained.json"), `${JSON.stringify({ + id: "task-retained", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending" + }, null, 2)}\n`, "utf8"); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-retained", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: null, threadId: null, + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)}\n`, "utf8"); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-current", cwd: workspace }) + }); + assert.equal(result.status, 0, result.stderr); + + const retained = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0]; + assert.equal(retained.status, "running"); + assert.equal(retained.threadId, "thr_pending"); + assert.equal(retained.phase, "worker-exited-turn-unknown"); + // The verdict is persisted, not the dead pid: the record has to stay + // reconcilable without handing the dead-worker reaper something to fail. + assert.equal(retained.pid, null); + assert.equal(retained.workerExited, true); + + // Another session ending must not reap it: its turn may still be running, + // and failing the record would also stop it pinning the shared broker. + const other = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-other" }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-other", cwd: workspace }) + }); + assert.equal(other.status, 0, other.stderr); + const afterOther = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0]; + assert.equal(afterOther.status, "running"); +}); + + +test("the session start hook appends to CLAUDE_ENV_FILE without rewriting it", () => { + const repo = makeTempDir(); + const envFile = path.join(makeTempDir(), "claude-env.sh"); + const foreignExport = "export SOME_OTHER_PLUGIN_VAR='kept'\n"; + fs.writeFileSync(envFile, foreignExport, "utf8"); + fs.chmodSync(envFile, 0o600); + const pluginDataDir = makeTempDir(); + const transcriptPath = path.join(repo, "session.jsonl"); + const env = { + ...process.env, + CLAUDE_ENV_FILE: envFile, + CLAUDE_PLUGIN_DATA: pluginDataDir + }; + const input = JSON.stringify({ + hook_event_name: "SessionStart", + session_id: "sess-current", + transcript_path: transcriptPath, + cwd: repo + }); + + assert.equal(run("node", [SESSION_HOOK, "SessionStart"], { cwd: repo, env, input }).status, 0); + assert.equal(run("node", [SESSION_HOOK, "SessionStart"], { cwd: repo, env, input }).status, 0); + + const contents = fs.readFileSync(envFile, "utf8"); + // The file is shared with every other plugin's SessionStart hook: a rewrite + // would drop whatever another hook appended, and replacing the file discards + // its mode with it. + assert.match(contents, /export SOME_OTHER_PLUGIN_VAR='kept'/); + assert.equal(fs.statSync(envFile).mode & 0o777, 0o600); + // Re-exporting the same value must not grow the file either. + assert.equal(contents.split("\n").filter((line) => line.startsWith("export CODEX_COMPANION_SESSION_ID=")).length, 1); + + // A changed value is appended; the shell takes the last export for a key. + assert.equal( + run("node", [SESSION_HOOK, "SessionStart"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionStart", + session_id: "sess-next", + transcript_path: transcriptPath, + cwd: repo + }) + }).status, + 0 + ); + const sessionExports = fs + .readFileSync(envFile, "utf8") + .split("\n") + .filter((line) => line.startsWith("export CODEX_COMPANION_SESSION_ID=")); + assert.equal(sessionExports.at(-1), "export CODEX_COMPANION_SESSION_ID='sess-next'"); +}); + + +function seedRetainedOrphan(workspace, updatedAt) { + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify({ + version: 1, + config: { stopReviewGate: false }, + jobs: [{ + id: "task-orphan", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-owner", pid: null, threadId: "thr_pending", workerExited: true, updatedAt + }] + }, null, 2)}\n`, + "utf8" + ); + return stateDir; +} + +function endSessionFor(workspace, env) { + return run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-other", ...env }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-other", cwd: workspace }) + }); +} + +test("a retained orphan expires even when the broker idle timer is disabled", () => { + const workspace = makeTempDir(); + const hoursAgo = (hours) => new Date(Date.now() - hours * 60 * 60 * 1000).toISOString(); + + // Two hours old, idle shutdown off: still protected — nothing has proved the + // turn is over. + let stateDir = seedRetainedOrphan(workspace, hoursAgo(2)); + assert.equal(endSessionFor(workspace, { CODEX_BROKER_IDLE_SHUTDOWN_MS: "0" }).status, 0); + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "running"); + + // Past the generic staleness bound, still with the idle timer off: reaped, + // rather than pinning the broker for good. + stateDir = seedRetainedOrphan(workspace, hoursAgo(25)); + assert.equal(endSessionFor(workspace, { CODEX_BROKER_IDLE_SHUTDOWN_MS: "0" }).status, 0); + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "failed"); + + // With the timer at ten minutes, the same two-hour-old record is over. + stateDir = seedRetainedOrphan(workspace, hoursAgo(2)); + assert.equal(endSessionFor(workspace, { CODEX_BROKER_IDLE_SHUTDOWN_MS: "600000" }).status, 0); + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "failed"); +}); + +test("a retained orphan with an unreadable timestamp does not pin the broker forever", () => { + const workspace = makeTempDir(); + const stateDir = seedRetainedOrphan(workspace, "not-a-timestamp"); + + assert.equal(endSessionFor(workspace, {}).status, 0); + + // This is the one record that stops a teardown, so an unreadable timestamp + // has to count as expired: "cannot tell" must not mean "protected forever". + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "failed"); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 8b1a3903f..0097b069a 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -8,8 +8,12 @@ import { spawn } from "node:child_process"; import { fileURLToPath, pathToFileURL } from "node:url"; import { makeTempDir } from "./helpers.mjs"; +import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; +import { acquireLockSync, releaseLock } from "../plugins/codex/scripts/lib/locking.mjs"; import { getConfig, + loadState, + resolveConfigFile, listJobs, resolveJobFile, resolveJobLogFile, @@ -94,6 +98,200 @@ test("review-gate config remains authoritative when CLAUDE_PLUGIN_DATA changes", } }); +// 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); @@ -395,3 +593,130 @@ test("runTrackedJob stores no errorMessage for a completed execution", async () assert.equal(indexed.status, "completed"); assert.equal(indexed.errorMessage, null); }); + + +test("the durable review-gate config is private", { skip: process.platform === "win32" }, () => { + const workspace = makeTempDir(); + const codexHome = makeTempDir(); + const previousCodexHome = process.env.CODEX_HOME; + try { + process.env.CODEX_HOME = codexHome; + setConfig(workspace, "stopReviewGate", true); + + const configFile = resolveConfigFile(workspace); + // The gate decides whether Codex reviews every turn of this workspace, so + // it gets the same treatment as every other artifact this module writes: + // nobody else on the machine reads or rewrites it. + assert.equal(fs.statSync(configFile).mode & 0o777, 0o600); + assert.equal(fs.statSync(path.dirname(configFile)).mode & 0o777, 0o700); + } finally { + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +}); + +test("a durable config write that fails mid-write leaves the previous config intact", () => { + const workspace = makeTempDir(); + const codexHome = makeTempDir(); + const previousCodexHome = process.env.CODEX_HOME; + try { + process.env.CODEX_HOME = codexHome; + setConfig(workspace, "stopReviewGate", true); + const configFile = resolveConfigFile(workspace); + const before = fs.readFileSync(configFile, "utf8"); + + // Fails inside writeJsonFileAtomic(), after it has created its temporary + // file: the replacement is interrupted exactly where a crash or a full + // disk would interrupt it. The gate decides whether Codex reviews every + // turn, so a half-written file must never end up in its place -- that + // would read back as unset and silently disable the gate. + const explodingValue = { + toJSON() { + throw new Error("serialization failed mid-write"); + } + }; + assert.throws(() => setConfig(workspace, "stopReviewGate", explodingValue), /serialization failed mid-write/); + + assert.equal(fs.readFileSync(configFile, "utf8"), before); + assert.equal(getConfig(workspace).stopReviewGate, true); + // The temporary file is cleaned up, so nothing is left to be mistaken for + // the real config. + assert.deepEqual(fs.readdirSync(path.dirname(configFile)), [path.basename(configFile)]); + } finally { + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +}); + + +function withRoots(fn) { + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + const previousCodexHome = process.env.CODEX_HOME; + const workspace = makeTempDir(); + const pluginData = makeTempDir(); + const codexHome = makeTempDir(); + process.env.CLAUDE_PLUGIN_DATA = pluginData; + process.env.CODEX_HOME = codexHome; + const primaryDir = resolveStateDir(workspace); + // The second candidate is the tmpdir fallback a CLI invocation without + // CLAUDE_PLUGIN_DATA resolves to. + const fallbackDir = path.join(os.tmpdir(), "codex-companion", path.basename(primaryDir)); + fs.mkdirSync(fallbackDir, { recursive: true }); + try { + return fn({ workspace, primaryDir, fallbackDir, codexHome }); + } finally { + fs.rmSync(fallbackDir, { recursive: true, force: true }); + if (previousPluginData == null) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +} + +test("pruning another state root waits for nobody and clobbers nobody", () => { + withRoots(({ workspace, fallbackDir }) => { + const fallbackState = path.join(fallbackDir, "state.json"); + const foreignJob = { id: "task-foreign", status: "completed", updatedAt: "2026-01-01T00:00:00.000Z" }; + fs.writeFileSync( + fallbackState, + `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [foreignJob] }, null, 2)}\n`, + "utf8" + ); + + // A process whose primary IS that root, mid-update. + const held = acquireLockSync(path.join(fallbackDir, ".state.lock")); + try { + saveState(workspace, { version: 1, config: { stopReviewGate: false }, jobs: [] }); + // Rewriting it here would have thrown away whatever the lock holder is + // about to write. + const untouched = JSON.parse(fs.readFileSync(fallbackState, "utf8")); + assert.deepEqual(untouched.jobs, [foreignJob]); + } finally { + releaseLock(held); + } + + // Once nobody holds it, the same prune goes through. + saveState(workspace, { version: 1, config: { stopReviewGate: false }, jobs: [] }); + assert.deepEqual(JSON.parse(fs.readFileSync(fallbackState, "utf8")).jobs, []); + }); +}); + +test("disabling the review gate is not outvoted by a stranded enable in another root", () => { + withRoots(({ workspace, fallbackDir, codexHome }) => { + setConfig(workspace, "stopReviewGate", true); + // A root that was written while CLAUDE_PLUGIN_DATA was unset. + fs.writeFileSync( + path.join(fallbackDir, "state.json"), + `${JSON.stringify({ version: 1, config: { stopReviewGate: true }, jobs: [] }, null, 2)}\n`, + "utf8" + ); + + setConfig(workspace, "stopReviewGate", false); + + // With the durable config gone, getConfig() falls back to merging the + // roots, where booleans are ORed. A stranded true would outvote this + // disable forever unless the write reached that root too. + fs.rmSync(path.join(codexHome, "plugin-cc"), { recursive: true, force: true }); + assert.equal(getConfig(workspace).stopReviewGate, false); + }); +});