From 33d334be915f03066b24dd279bfa6f73e11daf5d Mon Sep 17 00:00:00 2001 From: Dremig <1466140007@qq.com> Date: Wed, 4 Nov 2026 12:57:55 +0800 Subject: [PATCH 1/2] feat: let users pick Codex model, effort, and sandbox for every run - task: add --sandbox ; --write stays as shorthand for workspace-write. Background jobs carry the sandbox through the stored request, with a fallback for requests queued by older versions. - review / adversarial-review: accept --effort (and normalize --model). Native review has no per-turn effort field, so it is pinned via the thread config override model_reasoning_effort. - accept the `max` reasoning effort; the protocol now types effort as a plain string and current Codex configs default to it. - rescue: collect every open choice (thread, model, effort, sandbox) in a single AskUserQuestion before routing. The codex-rescue subagent only has Bash, so its description now tells the main thread to ask before spawning it, which covers plain-language delegation as well. - never pick danger-full-access implicitly or mark it as recommended. Co-Authored-By: Claude Opus 5.5 --- README.md | 13 +- plugins/codex/agents/codex-rescue.md | 8 +- plugins/codex/commands/adversarial-review.md | 3 +- plugins/codex/commands/rescue.md | 60 ++++--- plugins/codex/commands/review.md | 3 +- plugins/codex/scripts/codex-companion.mjs | 63 +++++-- plugins/codex/scripts/lib/codex.mjs | 20 ++- .../codex/skills/codex-cli-runtime/SKILL.md | 8 +- tests/commands.test.mjs | 27 ++- tests/fake-codex-fixture.mjs | 15 ++ tests/runtime.test.mjs | 157 ++++++++++++++++++ 11 files changed, 330 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 937a3037b..0ee1f5d7d 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ Use it when you want: - a review of your current uncommitted changes - a review of your branch compared to a base branch like `main` -Use `--base ` for branch review. It also supports `--wait` and `--background`. It is not steerable and does not take custom focus text. Use [`/codex:adversarial-review`](#codexadversarial-review) when you want to challenge a specific decision or risk area. +Use `--base ` for branch review. It also supports `--wait`, `--background`, `--model `, and `--effort `. It is not steerable and does not take custom focus text. Use [`/codex:adversarial-review`](#codexadversarial-review) when you want to challenge a specific decision or risk area. Examples: @@ -94,6 +94,7 @@ Examples: /codex:review /codex:review --base main /codex:review --background +/codex:review --effort high ``` This command is read-only and will not perform any changes. When run in the background you can use [`/codex:status`](#codexstatus) to check on the progress and [`/codex:cancel`](#codexcancel) to cancel the ongoing task. @@ -105,7 +106,7 @@ Runs a **steerable** review that questions the chosen implementation and design. It can be used to pressure-test assumptions, tradeoffs, failure modes, and whether a different approach would have been safer or simpler. It uses the same review target selection as `/codex:review`, including `--base ` for branch review. -It also supports `--wait` and `--background`. Unlike `/codex:review`, it can take extra focus text after the flags. +It also supports `--wait`, `--background`, `--model`, and `--effort`. Unlike `/codex:review`, it can take extra focus text after the flags. Use it when you want: @@ -137,7 +138,9 @@ Use it when you want Codex to: > [!NOTE] > Depending on the task and the model you choose these tasks might take a long time and it's generally recommended to force the task to be in the background or move the agent to the background. -It supports `--background`, `--wait`, `--resume`, and `--fresh`. If you omit `--resume` and `--fresh`, the plugin can offer to continue the latest rescue thread for this repo. +It supports `--background`, `--wait`, `--resume`, `--fresh`, `--model`, `--effort`, and `--sandbox`. If you omit `--resume` and `--fresh`, the plugin can offer to continue the latest rescue thread for this repo. + +Before handing off, Claude asks once (in a single prompt) for anything you did not already specify: whether to continue the previous thread, the Codex model, the reasoning effort, and the sandbox mode. This also applies when you just ask Claude in plain words to delegate to Codex. Say "use the defaults" to skip the model, effort, and sandbox questions. Examples: @@ -147,6 +150,8 @@ Examples: /codex:rescue --resume apply the top fix from the last run /codex:rescue --model gpt-5.4-mini --effort medium investigate the flaky integration test /codex:rescue --model spark fix the issue quickly +/codex:rescue --sandbox read-only explain why the cache is never invalidated +/codex:rescue --sandbox danger-full-access install the missing toolchain and rerun the build /codex:rescue --background investigate the regression ``` @@ -160,6 +165,8 @@ Ask Codex to redesign the database connection to be more resilient. - if you do not pass `--model` or `--effort`, Codex chooses its own defaults. - if you say `spark`, the plugin maps that to `gpt-5.3-codex-spark` +- `--sandbox` accepts `read-only`, `workspace-write`, or `danger-full-access`. Rescue runs default to `workspace-write` unless the request is read-only in nature. Codex never asks for approval inside the plugin, so the sandbox is the only guard rail: `danger-full-access` lets Codex touch files outside the repo and use the network without any confirmation, so only choose it for tasks you trust. +- the sandbox mode is set by the plugin on every run, so `sandbox_mode` in `config.toml` does not apply to plugin runs; model and reasoning effort still fall back to your config - follow-up rescue requests can continue the latest Codex task in the repo ### `/codex:transfer` diff --git a/plugins/codex/agents/codex-rescue.md b/plugins/codex/agents/codex-rescue.md index 7009ec86a..172f63858 100644 --- a/plugins/codex/agents/codex-rescue.md +++ b/plugins/codex/agents/codex-rescue.md @@ -1,6 +1,6 @@ --- name: codex-rescue -description: Proactively use when Claude Code is stuck, wants a second implementation or diagnosis pass, needs a deeper root-cause investigation, or should hand a substantial coding task to Codex through the shared runtime +description: Proactively use when Claude Code is stuck, wants a second implementation or diagnosis pass, needs a deeper root-cause investigation, or should hand a substantial coding task to Codex through the shared runtime. This subagent cannot ask the user questions, so before spawning it, unless the request already settles them or the user said to use defaults, ask the user once with AskUserQuestion for the Codex model (Codex default / gpt-5.4 / gpt-5.4-mini / spark), reasoning effort (Codex default / medium / high / xhigh), and sandbox (workspace-write / read-only / danger-full-access), then put the answers in the prompt as --model / --effort / --sandbox flags; "Codex default" means omit that flag. model: sonnet tools: Bash skills: @@ -30,8 +30,10 @@ Forwarding rules: - Leave model unset by default. Only add `--model` when the user explicitly asks for a specific model. - If the user asks for `spark`, map that to `--model gpt-5.3-codex-spark`. - If the user asks for a concrete model name such as `gpt-5.4-mini`, pass it through with `--model`. -- Treat `--effort ` and `--model ` as runtime controls and do not include them in the task text you pass through. -- Default to a write-capable Codex run by adding `--write` unless the user explicitly asks for read-only behavior or only wants review, diagnosis, or research without edits. +- Treat `--effort `, `--model `, and `--sandbox ` as runtime controls and do not include them in the task text you pass through. +- If the forwarded request includes `--sandbox `, pass it through unchanged and do not also add `--write`. +- Otherwise, default to a write-capable Codex run by adding `--write` unless the user explicitly asks for read-only behavior or only wants review, diagnosis, or research without edits. +- Never choose `--sandbox danger-full-access` yourself. Only forward it when the request already contains it or the user explicitly asked for full access without a sandbox. - Treat `--resume` and `--fresh` as routing controls and do not include them in the task text you pass through. - `--resume` means add `--resume-last`. - `--fresh` means do not add `--resume-last`. diff --git a/plugins/codex/commands/adversarial-review.md b/plugins/codex/commands/adversarial-review.md index da440ab4d..49f6116fc 100644 --- a/plugins/codex/commands/adversarial-review.md +++ b/plugins/codex/commands/adversarial-review.md @@ -1,6 +1,6 @@ --- description: Run a Codex review that challenges the implementation approach and design choices -argument-hint: '[--wait|--background] [--base ] [--scope auto|working-tree|branch] [focus ...]' +argument-hint: '[--wait|--background] [--base ] [--scope auto|working-tree|branch] [--model ] [--effort ] [focus ...]' disable-model-invocation: true allowed-tools: Read, Glob, Grep, Bash(node:*), Bash(git:*), AskUserQuestion --- @@ -37,6 +37,7 @@ Execution mode rules: Argument handling: - Preserve the user's arguments exactly. - Do not strip `--wait` or `--background` yourself. +- `--model` and `--effort` select the Codex model and reasoning effort for the review; pass them through untouched. When omitted, the user's Codex config decides. - Do not weaken the adversarial framing or rewrite the user's focus text. - The companion script parses `--wait` and `--background`, but Claude Code's `Bash(..., run_in_background: true)` is what actually detaches the run. - `/codex:adversarial-review` uses the same review target selection as `/codex:review`. diff --git a/plugins/codex/commands/rescue.md b/plugins/codex/commands/rescue.md index 56de9555d..afe8fe4b9 100644 --- a/plugins/codex/commands/rescue.md +++ b/plugins/codex/commands/rescue.md @@ -1,6 +1,6 @@ --- description: Delegate investigation, an explicit fix request, or follow-up rescue work to the Codex rescue subagent -argument-hint: "[--background|--wait] [--resume|--fresh] [--model ] [--effort ] [what Codex should investigate, solve, or continue]" +argument-hint: "[--background|--wait] [--resume|--fresh] [--model ] [--effort ] [--sandbox ] [what Codex should investigate, solve, or continue]" allowed-tools: Bash(node:*), AskUserQuestion, Agent --- @@ -17,24 +17,41 @@ Execution mode: - If the request includes `--wait`, run the `codex:codex-rescue` subagent in the foreground. - If neither flag is present, default to foreground. - `--background` and `--wait` are execution flags for Claude Code. Do not forward them to `task`, and do not treat them as part of the natural-language task text. -- `--model` and `--effort` are runtime-selection flags. Preserve them for the forwarded `task` call, but do not treat them as part of the natural-language task text. -- If the request includes `--resume`, do not ask whether to continue. The user already chose. -- If the request includes `--fresh`, do not ask whether to continue. The user already chose. -- Otherwise, before starting Codex, check for a resumable rescue thread from this Claude session by running: - -```bash -node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task-resume-candidate --json -``` - -- If that helper reports `available: true`, use `AskUserQuestion` exactly once to ask whether to continue the current Codex thread or start a new one. -- The two choices must be: - - `Continue current Codex thread` - - `Start a new Codex thread` -- If the user is clearly giving a follow-up instruction such as "continue", "keep going", "resume", "apply the top fix", or "dig deeper", put `Continue current Codex thread (Recommended)` first. -- Otherwise put `Start a new Codex thread (Recommended)` first. -- If the user chooses continue, add `--resume` before routing to the subagent. -- If the user chooses a new thread, add `--fresh` before routing to the subagent. -- If the helper reports `available: false`, do not ask. Route normally. +- `--model`, `--effort`, and `--sandbox` are runtime-selection flags. Preserve them for the forwarded `task` call, but do not treat them as part of the natural-language task text. + +Pre-flight questions (ask at most once): + +The subagent cannot ask the user anything, so collect every open choice here and ask them together in a single `AskUserQuestion` call with one question per open choice. Skip the call entirely when nothing is open. + +1. Thread choice: + - If the request includes `--resume`, do not ask whether to continue. The user already chose. + - If the request includes `--fresh`, do not ask whether to continue. The user already chose. + - Otherwise, check for a resumable rescue thread from this Claude session by running: + + ```bash + node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task-resume-candidate --json + ``` + + - If that helper reports `available: true`, include a thread question with exactly these two choices: + - `Continue current Codex thread` + - `Start a new Codex thread` + - If the user is clearly giving a follow-up instruction such as "continue", "keep going", "resume", "apply the top fix", or "dig deeper", put `Continue current Codex thread (Recommended)` first. + - Otherwise put `Start a new Codex thread (Recommended)` first. + - If the helper reports `available: false`, do not include the thread question. +2. Model: unless the request already names a model (`--model` or plain words such as "use spark" or "use gpt-5.4-mini"), include a model question with the choices `Codex default (Recommended)`, `gpt-5.4`, `gpt-5.4-mini`, and `spark`. +3. Reasoning effort: unless the request already names a concrete level (`--effort` or plain words such as "high effort"), include an effort question with the choices `Codex default (Recommended)`, `medium`, `high`, and `xhigh`. Other levels (`none`, `minimal`, `low`, `max`) stay reachable through the free-text answer. +4. Sandbox: unless the request already includes `--sandbox`, `--write`, or an explicit read-only / full-access instruction, include a sandbox question with these choices: + - `workspace-write`: Codex may edit files inside the workspace; no network by default. + - `read-only`: Codex can inspect but not change anything. + - `danger-full-access`: no sandbox at all, including files outside the workspace and network. Only for trusted tasks. + - Put `read-only (Recommended)` first when the request only asks for diagnosis, research, review, or a plan without edits. Otherwise put `workspace-write (Recommended)` first. Never mark `danger-full-access` as recommended. + +- Skip the model, effort, and sandbox questions when the user says to use the defaults or not to ask. +- Applying the answers: + - If the user chooses continue, add `--resume` before routing to the subagent. + - If the user chooses a new thread, add `--fresh` before routing to the subagent. + - `Codex default` means leave that flag unset so the user's Codex config decides. + - Otherwise add `--model `, `--effort `, or `--sandbox ` to the forwarded request. Map `spark` to `gpt-5.3-codex-spark`. Operating rules: @@ -42,8 +59,9 @@ Operating rules: - Return the Codex companion stdout verbatim to the user. - Do not paraphrase, summarize, rewrite, or add commentary before or after it. - Do not ask the subagent to inspect files, monitor progress, poll `/codex:status`, fetch `/codex:result`, call `/codex:cancel`, summarize output, or do follow-up work of its own. -- Leave `--effort` unset unless the user explicitly asks for a specific reasoning effort. -- Leave the model unset unless the user explicitly asks for one. If they ask for `spark`, map it to `gpt-5.3-codex-spark`. +- Leave `--effort` unset unless the user explicitly asks for a specific reasoning effort or picks one in the pre-flight question. +- Leave the model unset unless the user explicitly asks for one or picks one in the pre-flight question. If they ask for `spark`, map it to `gpt-5.3-codex-spark`. +- Never add `--sandbox danger-full-access` unless the user explicitly asked for it or picked it in the pre-flight question. - Leave `--resume` and `--fresh` in the forwarded request. The subagent handles that routing when it builds the `task` command. - If the helper reports that Codex is missing or unauthenticated, stop and tell the user to run `/codex:setup`. - If the user did not supply a request, ask what Codex should investigate or fix. diff --git a/plugins/codex/commands/review.md b/plugins/codex/commands/review.md index fb70a4876..52fca8718 100644 --- a/plugins/codex/commands/review.md +++ b/plugins/codex/commands/review.md @@ -1,6 +1,6 @@ --- description: Run a Codex code review against local git state -argument-hint: '[--wait|--background] [--base ] [--scope auto|working-tree|branch]' +argument-hint: '[--wait|--background] [--base ] [--scope auto|working-tree|branch] [--model ] [--effort ]' disable-model-invocation: true allowed-tools: Read, Glob, Grep, Bash(node:*), Bash(git:*), AskUserQuestion --- @@ -34,6 +34,7 @@ Execution mode rules: Argument handling: - Preserve the user's arguments exactly. - Do not strip `--wait` or `--background` yourself. +- `--model` and `--effort` select the Codex model and reasoning effort for the review; pass them through untouched. When omitted, the user's Codex config decides. - Do not add extra review instructions or rewrite the user's intent. - The companion script parses `--wait` and `--background`, but Claude Code's `Bash(..., run_in_background: true)` is what actually detaches the run. - `/codex:review` is native-review only. It does not support staged-only review, unstaged-only review, or extra focus text. diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..49ad2445a 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -68,7 +68,8 @@ const ROOT_DIR = path.resolve(fileURLToPath(new URL("..", import.meta.url))); const REVIEW_SCHEMA = path.join(ROOT_DIR, "schemas", "review-output.schema.json"); const DEFAULT_STATUS_WAIT_TIMEOUT_MS = 240000; const DEFAULT_STATUS_POLL_INTERVAL_MS = 2000; -const VALID_REASONING_EFFORTS = new Set(["none", "minimal", "low", "medium", "high", "xhigh"]); +const VALID_REASONING_EFFORTS = new Set(["none", "minimal", "low", "medium", "high", "xhigh", "max"]); +const VALID_SANDBOX_MODES = new Set(["read-only", "workspace-write", "danger-full-access"]); const MODEL_ALIASES = new Map([["spark", "gpt-5.3-codex-spark"]]); const STOP_REVIEW_TASK_MARKER = "Run a stop-gate review of the previous Claude turn."; @@ -77,9 +78,9 @@ function printUsage() { [ "Usage:", " node scripts/codex-companion.mjs setup [--enable-review-gate|--disable-review-gate] [--json]", - " node scripts/codex-companion.mjs review [--wait|--background] [--base ] [--scope ]", - " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base ] [--scope ] [focus text]", - " node scripts/codex-companion.mjs task [--background] [--write] [--resume-last|--resume|--fresh] [--model ] [--effort ] [prompt]", + " node scripts/codex-companion.mjs review [--wait|--background] [--base ] [--scope ] [--model ] [--effort ]", + " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base ] [--scope ] [--model ] [--effort ] [focus text]", + " node scripts/codex-companion.mjs task [--background] [--write|--sandbox ] [--resume-last|--resume|--fresh] [--model ] [--effort ] [prompt]", " node scripts/codex-companion.mjs transfer [--source ] [--json]", " node scripts/codex-companion.mjs status [job-id] [--all] [--json]", " node scripts/codex-companion.mjs result [job-id] [--json]", @@ -121,12 +122,42 @@ function normalizeReasoningEffort(effort) { } if (!VALID_REASONING_EFFORTS.has(normalized)) { throw new Error( - `Unsupported reasoning effort "${effort}". Use one of: none, minimal, low, medium, high, xhigh.` + `Unsupported reasoning effort "${effort}". Use one of: none, minimal, low, medium, high, xhigh, max.` ); } return normalized; } +function normalizeSandboxMode(sandbox) { + if (sandbox == null) { + return null; + } + const normalized = String(sandbox).trim().toLowerCase(); + if (!normalized) { + return null; + } + if (!VALID_SANDBOX_MODES.has(normalized)) { + throw new Error( + `Unsupported sandbox mode "${sandbox}". Use one of: read-only, workspace-write, danger-full-access.` + ); + } + return normalized; +} + +// `--write` is shorthand for `--sandbox workspace-write`; an explicit `--sandbox` +// wins, except that combining `--write` with a read-only sandbox is contradictory. +function resolveTaskSandbox({ sandbox, write }) { + const requested = normalizeSandboxMode(sandbox); + if (requested === "read-only" && write) { + throw new Error("Choose either --write or --sandbox read-only."); + } + return requested ?? (write ? "workspace-write" : "read-only"); +} + +function sandboxAllowsWrites(sandbox) { + return sandbox !== "read-only"; +} + function normalizeArgv(argv) { if (argv.length === 1) { const [raw] = argv; @@ -370,6 +401,7 @@ async function executeReviewRun(request) { const result = await runAppServerReview(request.cwd, { target: reviewTarget, model: request.model, + effort: request.effort, onProgress: request.onProgress }); const payload = { @@ -411,6 +443,7 @@ async function executeReviewRun(request) { const result = await runAppServerTurn(context.repoRoot, { prompt, model: request.model, + effort: request.effort, sandbox: "read-only", outputSchema: readOutputSchema(REVIEW_SCHEMA), onProgress: request.onProgress @@ -488,7 +521,8 @@ async function executeTaskRun(request) { defaultPrompt: resumeThreadId ? DEFAULT_CONTINUE_PROMPT : "", model: request.model, effort: request.effort, - sandbox: request.write ? "workspace-write" : "read-only", + // Requests queued by older plugin versions only carry `write`. + sandbox: request.sandbox ?? (request.write ? "workspace-write" : "read-only"), onProgress: request.onProgress, persistThread: true, threadName: resumeThreadId ? null : buildPersistentTaskThreadName(request.prompt || DEFAULT_CONTINUE_PROMPT) @@ -601,11 +635,12 @@ function buildTaskJob(workspaceRoot, taskMetadata, write) { }); } -function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId }) { +function buildTaskRequest({ cwd, model, effort, sandbox, prompt, write, resumeLast, jobId }) { return { cwd, model, effort, + sandbox, prompt, write, resumeLast, @@ -711,7 +746,7 @@ function enqueueBackgroundTask(cwd, job, request) { async function handleReviewCommand(argv, config) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["base", "scope", "model", "cwd"], + valueOptions: ["base", "scope", "model", "effort", "cwd"], booleanOptions: ["json", "background", "wait"], aliasMap: { m: "model" @@ -720,6 +755,8 @@ async function handleReviewCommand(argv, config) { const cwd = resolveCommandCwd(options); const workspaceRoot = resolveCommandWorkspace(options); + const model = normalizeRequestedModel(options.model); + const effort = normalizeReasoningEffort(options.effort); const focusText = positionals.join(" ").trim(); const target = resolveReviewTarget(cwd, { base: options.base, @@ -743,7 +780,8 @@ async function handleReviewCommand(argv, config) { cwd, base: options.base, scope: options.scope, - model: options.model, + model, + effort, focusText, reviewName: config.reviewName, onProgress: progress @@ -761,7 +799,7 @@ async function handleReview(argv) { async function handleTask(argv) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["model", "effort", "cwd", "prompt-file"], + valueOptions: ["model", "effort", "sandbox", "cwd", "prompt-file"], booleanOptions: ["json", "write", "resume-last", "resume", "fresh", "background"], aliasMap: { m: "model" @@ -779,7 +817,8 @@ async function handleTask(argv) { if (resumeLast && fresh) { throw new Error("Choose either --resume/--resume-last or --fresh."); } - const write = Boolean(options.write); + const sandbox = resolveTaskSandbox({ sandbox: options.sandbox, write: Boolean(options.write) }); + const write = sandboxAllowsWrites(sandbox); const taskMetadata = buildTaskRunMetadata({ prompt, resumeLast @@ -794,6 +833,7 @@ async function handleTask(argv) { cwd, model, effort, + sandbox, prompt, write, resumeLast, @@ -812,6 +852,7 @@ async function handleTask(argv) { cwd, model, effort, + sandbox, prompt, write, resumeLast, diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index fead00cc4..57372d29a 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -59,6 +59,20 @@ function cleanCodexStderr(stderr) { .join("\n"); } +/** + * Thread-level config overrides. Only emitted when something is set so request + * shapes stay unchanged for callers that rely on the user's config.toml. + * @returns {{ config?: { [key: string]: string } }} + */ +function buildConfigOverrides(options = {}) { + /** @type {{ [key: string]: string }} */ + const config = {}; + if (options.effort) { + config.model_reasoning_effort = options.effort; + } + return Object.keys(config).length > 0 ? { config } : {}; +} + /** @returns {ThreadStartParams} */ function buildThreadParams(cwd, options = {}) { return { @@ -66,6 +80,7 @@ function buildThreadParams(cwd, options = {}) { model: options.model ?? null, approvalPolicy: options.approvalPolicy ?? "never", sandbox: options.sandbox ?? "read-only", + ...buildConfigOverrides(options), serviceName: SERVICE_NAME, ephemeral: options.ephemeral ?? true }; @@ -78,7 +93,8 @@ function buildResumeParams(threadId, cwd, options = {}) { cwd, model: options.model ?? null, approvalPolicy: options.approvalPolicy ?? "never", - sandbox: options.sandbox ?? "read-only" + sandbox: options.sandbox ?? "read-only", + ...buildConfigOverrides(options) }; } @@ -1009,6 +1025,8 @@ export async function runAppServerReview(cwd, options = {}) { emitProgress(options.onProgress, "Starting Codex review thread.", "starting"); const thread = await startThread(client, cwd, { model: options.model, + // review/start has no per-turn effort field, so pin it on the thread config. + effort: options.effort, sandbox: "read-only", ephemeral: true, threadName: options.threadName diff --git a/plugins/codex/skills/codex-cli-runtime/SKILL.md b/plugins/codex/skills/codex-cli-runtime/SKILL.md index 0e91bfb50..47ab385ec 100644 --- a/plugins/codex/skills/codex-cli-runtime/SKILL.md +++ b/plugins/codex/skills/codex-cli-runtime/SKILL.md @@ -21,22 +21,26 @@ Execution rules: - Leave `--effort` unset unless the user explicitly requests a specific effort. - Leave model unset by default. Add `--model` only when the user explicitly asks for one. - Map `spark` to `--model gpt-5.3-codex-spark`. -- Default to a write-capable Codex run by adding `--write` unless the user explicitly asks for read-only behavior or only wants review, diagnosis, or research without edits. +- If the forwarded request includes `--sandbox `, pass it through and do not add `--write`. +- Otherwise, default to a write-capable Codex run by adding `--write` unless the user explicitly asks for read-only behavior or only wants review, diagnosis, or research without edits. Command selection: - Use exactly one `task` invocation per rescue handoff. - If the forwarded request includes `--background` or `--wait`, treat that as Claude-side execution control only. Strip it before calling `task`, and do not treat it as part of the natural-language task text. - If the forwarded request includes `--model`, normalize `spark` to `gpt-5.3-codex-spark` and pass it through to `task`. - If the forwarded request includes `--effort`, pass it through to `task`. +- If the forwarded request includes `--sandbox`, pass it through to `task`. - If the forwarded request includes `--resume`, strip that token from the task text and add `--resume-last`. - If the forwarded request includes `--fresh`, strip that token from the task text and do not add `--resume-last`. - `--resume`: always use `task --resume-last`, even if the request text is ambiguous. - `--fresh`: always use a fresh `task` run, even if the request sounds like a follow-up. -- `--effort`: accepted values are `none`, `minimal`, `low`, `medium`, `high`, `xhigh`. +- `--effort`: accepted values are `none`, `minimal`, `low`, `medium`, `high`, `xhigh`, `max`. Which levels a model actually honors depends on the model. +- `--sandbox`: accepted values are `read-only`, `workspace-write`, `danger-full-access`. `--write` is shorthand for `--sandbox workspace-write`. Approval policy is always `never`, so the sandbox is the only guard rail. - `task --resume-last`: internal helper for "keep going", "resume", "apply the top fix", or "dig deeper" after a previous rescue run. Safety rules: - Default to write-capable Codex work in `codex:codex-rescue` unless the user explicitly asks for read-only behavior. +- Never pick `--sandbox danger-full-access` on your own; only forward it when the request already contains it. - Preserve the user's task text as-is apart from stripping routing flags. - Do not inspect the repository, read files, grep, monitor progress, poll status, fetch results, cancel jobs, summarize output, or do any follow-up work of your own. - Return the stdout of the `task` command exactly as-is. diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index c34b06059..d064712f9 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -22,6 +22,8 @@ test("review command uses AskUserQuestion and background Bash while staying revi assert.match(source, /```typescript/); assert.match(source, /review "\$ARGUMENTS"/); assert.match(source, /\[--scope auto\|working-tree\|branch\]/); + assert.match(source, /\[--model \] \[--effort \]/); + assert.match(source, /`--model` and `--effort` select the Codex model and reasoning effort for the review/i); assert.match(source, /run_in_background:\s*true/); assert.match(source, /command:\s*`node "\$\{CLAUDE_PLUGIN_ROOT\}\/scripts\/codex-companion\.mjs" review "\$ARGUMENTS"`/); assert.match(source, /description:\s*"Codex review"/); @@ -49,7 +51,8 @@ test("adversarial review command uses AskUserQuestion and background Bash while assert.match(source, /```bash/); assert.match(source, /```typescript/); assert.match(source, /adversarial-review "\$ARGUMENTS"/); - assert.match(source, /\[--scope auto\|working-tree\|branch\] \[focus \.\.\.\]/); + assert.match(source, /\[--scope auto\|working-tree\|branch\] \[--model \] \[--effort \] \[focus \.\.\.\]/); + assert.match(source, /`--model` and `--effort` select the Codex model and reasoning effort for the review/i); assert.match(source, /run_in_background:\s*true/); assert.match(source, /command:\s*`node "\$\{CLAUDE_PLUGIN_ROOT\}\/scripts\/codex-companion\.mjs" adversarial-review "\$ARGUMENTS"`/); assert.match(source, /description:\s*"Codex adversarial review"/); @@ -104,7 +107,8 @@ test("rescue command absorbs continue semantics", () => { assert.match(rescue, /--background\|--wait/); assert.match(rescue, /--resume\|--fresh/); assert.match(rescue, /--model /); - assert.match(rescue, /--effort /); + assert.match(rescue, /--effort /); + assert.match(rescue, /--sandbox /); assert.match(rescue, /task-resume-candidate --json/); assert.match(rescue, /AskUserQuestion/); assert.match(rescue, /Continue current Codex thread/); @@ -112,7 +116,14 @@ test("rescue command absorbs continue semantics", () => { assert.match(rescue, /run the `codex:codex-rescue` subagent in the background/i); assert.match(rescue, /default to foreground/i); assert.match(rescue, /Do not forward them to `task`/i); - assert.match(rescue, /`--model` and `--effort` are runtime-selection flags/i); + assert.match(rescue, /`--model`, `--effort`, and `--sandbox` are runtime-selection flags/i); + // Natural-language delegation never reaches the subagent with a way to ask, + // so every open runtime choice must be collected up front in one prompt. + assert.match(rescue, /ask them together in a single `AskUserQuestion` call/i); + assert.match(rescue, /Codex default \(Recommended\)/); + assert.match(rescue, /`Codex default` means leave that flag unset/i); + assert.match(rescue, /Never mark `danger-full-access` as recommended/i); + assert.match(rescue, /Never add `--sandbox danger-full-access` unless the user explicitly asked for it or picked it/i); assert.match(rescue, /Leave `--effort` unset unless the user explicitly asks for a specific reasoning effort/i); assert.match(rescue, /If they ask for `spark`, map it to `gpt-5\.3-codex-spark`/i); assert.match(rescue, /If the request includes `--resume`, do not ask whether to continue/i); @@ -126,6 +137,10 @@ test("rescue command absorbs continue semantics", () => { assert.match(rescue, /Leave `--resume` and `--fresh` in the forwarded request/i); assert.match(agent, /--resume/); assert.match(agent, /--fresh/); + assert.match(agent, /^description:.*cannot ask the user questions.*AskUserQuestion.*--model \/ --effort \/ --sandbox/m); + assert.match(agent, /If the forwarded request includes `--sandbox `, pass it through unchanged/i); + assert.match(agent, /default to a write-capable Codex run by adding `--write`/i); + assert.match(agent, /Never choose `--sandbox danger-full-access` yourself/i); assert.match(agent, /thin forwarding wrapper/i); assert.match(agent, /prefer foreground for a small, clearly bounded rescue request/i); assert.match(agent, /If the user did not explicitly choose `--background` or `--wait` and the task looks complicated, open-ended, multi-step, or likely to keep Codex running for a long time, prefer background execution/i); @@ -150,12 +165,16 @@ test("rescue command absorbs continue semantics", () => { assert.match(runtimeSkill, /Map `spark` to `--model gpt-5\.3-codex-spark`/i); assert.match(runtimeSkill, /If the forwarded request includes `--background` or `--wait`, treat that as Claude-side execution control only/i); assert.match(runtimeSkill, /Strip it before calling `task`/i); - assert.match(runtimeSkill, /`--effort`: accepted values are `none`, `minimal`, `low`, `medium`, `high`, `xhigh`/i); + assert.match(runtimeSkill, /`--effort`: accepted values are `none`, `minimal`, `low`, `medium`, `high`, `xhigh`, `max`/i); + assert.match(runtimeSkill, /`--sandbox`: accepted values are `read-only`, `workspace-write`, `danger-full-access`/i); + assert.match(runtimeSkill, /Never pick `--sandbox danger-full-access` on your own/i); assert.match(runtimeSkill, /Do not inspect the repository, read files, grep, monitor progress, poll status, fetch results, cancel jobs, summarize output, or do any follow-up work of your own/i); assert.match(runtimeSkill, /If the Bash call fails or Codex cannot be invoked, return nothing/i); assert.match(readme, /`codex:codex-rescue` subagent/i); assert.match(readme, /if you do not pass `--model` or `--effort`, Codex chooses its own defaults/i); assert.match(readme, /--model gpt-5\.4-mini --effort medium/i); + assert.match(readme, /--sandbox danger-full-access/); + assert.match(readme, /`sandbox_mode` in `config\.toml` does not apply to plugin runs/i); assert.match(readme, /`spark`, the plugin maps that to `gpt-5\.3-codex-spark`/i); assert.match(readme, /continue a previous Codex task/i); assert.match(readme, /### `\/codex:setup`/); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index f83c96a0d..5ab3e077f 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -313,6 +313,14 @@ rl.on("line", (line) => { throw new Error("thread/start.persistFullHistory requires experimentalApi capability"); } const thread = nextThread(state, message.params.cwd, message.params.ephemeral); + state.lastThreadStart = { + threadId: thread.id, + model: message.params.model ?? null, + sandbox: message.params.sandbox ?? null, + approvalPolicy: message.params.approvalPolicy ?? null, + config: message.params.config ?? null + }; + 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: { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }, reasoningEffort: null } }); send({ method: "thread/started", params: { thread: { id: thread.id } } }); break; @@ -346,6 +354,13 @@ rl.on("line", (line) => { } const thread = ensureThread(state, message.params.threadId); thread.updatedAt = now(); + state.lastThreadResume = { + threadId: thread.id, + model: message.params.model ?? null, + sandbox: message.params.sandbox ?? null, + approvalPolicy: message.params.approvalPolicy ?? null, + config: message.params.config ?? null + }; 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: { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }, reasoningEffort: null } }); break; diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..9d03fa5c0 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -784,6 +784,163 @@ test("task forwards model selection and reasoning effort to app-server turn/star assert.equal(fakeState.lastTurnStart.effort, "low"); }); +function setupTaskRepo(behavior) { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir, behavior); + 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 readState = () => JSON.parse(fs.readFileSync(statePath, "utf8")); + return { repo, binDir, readState }; +} + +test("task defaults to a read-only sandbox and --write selects workspace-write", () => { + const { repo, binDir, readState } = setupTaskRepo(); + + const readOnly = run("node", [SCRIPT, "task", "diagnose the failing test"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(readOnly.status, 0, readOnly.stderr); + assert.equal(readState().lastThreadStart.sandbox, "read-only"); + assert.equal(readState().lastThreadStart.approvalPolicy, "never"); + assert.equal(readState().lastThreadStart.config, null); + + const writable = run("node", [SCRIPT, "task", "--write", "fix the failing test"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(writable.status, 0, writable.stderr); + assert.equal(readState().lastThreadStart.sandbox, "workspace-write"); +}); + +test("task accepts the max reasoning effort that current Codex configs use", () => { + const { repo, binDir, readState } = setupTaskRepo(); + + const result = run("node", [SCRIPT, "task", "--effort", "max", "diagnose the failing test"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(readState().lastTurnStart.effort, "max"); +}); + +test("task --sandbox danger-full-access reaches thread/start and thread/resume", () => { + const { repo, binDir, readState } = setupTaskRepo(); + + const fresh = run("node", [SCRIPT, "task", "--sandbox", "danger-full-access", "install the missing toolchain"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(fresh.status, 0, fresh.stderr); + assert.equal(readState().lastThreadStart.sandbox, "danger-full-access"); + assert.equal(readState().lastTurnStart.prompt, "install the missing toolchain"); + + const resumed = run("node", [SCRIPT, "task", "--resume-last", "--sandbox=danger-full-access", "keep going"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(resumed.status, 0, resumed.stderr); + assert.equal(readState().lastThreadResume.sandbox, "danger-full-access"); + assert.equal(readState().lastTurnStart.prompt, "keep going"); +}); + +test("task --write does not downgrade an explicit --sandbox danger-full-access", () => { + const { repo, binDir, readState } = setupTaskRepo(); + + const result = run("node", [SCRIPT, "task", "--write", "--sandbox", "danger-full-access", "fix it"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(readState().lastThreadStart.sandbox, "danger-full-access"); +}); + +test("task rejects contradictory or unknown sandbox modes before starting Codex", () => { + const { repo, binDir } = setupTaskRepo(); + + const contradictory = run("node", [SCRIPT, "task", "--write", "--sandbox", "read-only", "fix it"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.notEqual(contradictory.status, 0); + assert.match(contradictory.stderr, /Choose either --write or --sandbox read-only/); + + const unknown = run("node", [SCRIPT, "task", "--sandbox", "yolo", "fix it"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.notEqual(unknown.status, 0); + assert.match(unknown.stderr, /Unsupported sandbox mode "yolo"/); +}); + +test("task --background carries the sandbox mode through to the detached worker", () => { + const { repo, binDir, readState } = setupTaskRepo(); + + const launched = run( + "node", + [SCRIPT, "task", "--background", "--json", "--sandbox", "danger-full-access", "investigate the failing test"], + { + cwd: repo, + env: buildEnv(binDir) + } + ); + assert.equal(launched.status, 0, launched.stderr); + const { jobId } = JSON.parse(launched.stdout); + + const waited = run("node", [SCRIPT, "status", jobId, "--wait", "--timeout-ms", "15000", "--json"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(waited.status, 0, waited.stderr); + assert.equal(JSON.parse(waited.stdout).job.status, "completed"); + assert.equal(readState().lastThreadStart.sandbox, "danger-full-access"); +}); + +test("review and adversarial review forward --model and --effort", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.mkdirSync(path.join(repo, "src")); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0];\n"); + run("git", ["add", "src/app.js"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0].id;\n"); + + const review = run("node", [SCRIPT, "review", "--model", "spark", "--effort", "high"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(review.status, 0, review.stderr); + let fakeState = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.equal(fakeState.lastThreadStart.model, "gpt-5.3-codex-spark"); + assert.equal(fakeState.lastThreadStart.sandbox, "read-only"); + assert.deepEqual(fakeState.lastThreadStart.config, { model_reasoning_effort: "high" }); + + const adversarial = run("node", [SCRIPT, "adversarial-review", "--effort", "xhigh", "question the retry design"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(adversarial.status, 0, adversarial.stderr); + fakeState = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.equal(fakeState.lastThreadStart.sandbox, "read-only"); + assert.equal(fakeState.lastTurnStart.effort, "xhigh"); + assert.match(fakeState.lastTurnStart.prompt, /question the retry design/); + assert.doesNotMatch(fakeState.lastTurnStart.prompt, /--effort/); + + const invalid = run("node", [SCRIPT, "review", "--effort", "extreme"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.notEqual(invalid.status, 0); + assert.match(invalid.stderr, /Unsupported reasoning effort "extreme"/); +}); + test("task logs reasoning summaries and assistant messages to the job log", () => { const repo = makeTempDir(); const binDir = makeTempDir(); From 83f852ec41e014b271f285a6f8b8e3b55ea8e03a Mon Sep 17 00:00:00 2001 From: Dremig <1466140007@qq.com> Date: Wed, 4 Nov 2026 15:22:28 +0800 Subject: [PATCH 2/2] fix: stop Codex turns when their companion process exits Interrupt unfinished broker turns when their owning socket disconnects, keeping ownership until pending start requests and orphan interrupts settle. Record signal cancellations and reconcile jobs with dead companion PIDs before status, result, cancellation, resume, and stop checks. Gate detached workers until queued job state is saved and atomically replace job/state JSON to prevent partial reads. Run long rescue requests through Claude background execution and document the task lifetime. Validation: 109 tests pass; build and version checks pass; live Codex 0.166.0 direct EOF and broker disconnect smoke tests pass. --- README.md | 10 +- plugins/codex/agents/codex-rescue.md | 3 +- plugins/codex/commands/rescue.md | 2 +- plugins/codex/scripts/app-server-broker.mjs | 136 +++++++-- plugins/codex/scripts/codex-companion.mjs | 29 +- plugins/codex/scripts/lib/job-control.mjs | 75 ++++- plugins/codex/scripts/lib/state.mjs | 16 +- plugins/codex/scripts/lib/tracked-jobs.mjs | 58 ++++ .../codex/scripts/stop-review-gate-hook.mjs | 3 +- .../codex/skills/codex-cli-runtime/SKILL.md | 1 + tests/commands.test.mjs | 2 +- tests/fake-codex-fixture.mjs | 91 ++++-- tests/lifecycle.test.mjs | 281 ++++++++++++++++++ tests/state.test.mjs | 44 ++- 14 files changed, 677 insertions(+), 74 deletions(-) create mode 100644 tests/lifecycle.test.mjs diff --git a/README.md b/README.md index 0ee1f5d7d..f88573cf1 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ Use it when you want Codex to: - take a faster or cheaper pass with a smaller model > [!NOTE] -> Depending on the task and the model you choose these tasks might take a long time and it's generally recommended to force the task to be in the background or move the agent to the background. +> Long or open-ended rescue tasks run in the background unless you request `--wait`. See [Task lifetime](#task-lifetime) for what happens when a task is stopped. It supports `--background`, `--wait`, `--resume`, `--fresh`, `--model`, `--effort`, and `--sandbox`. If you omit `--resume` and `--fresh`, the plugin can offer to continue the latest rescue thread for this repo. @@ -292,6 +292,14 @@ Your configuration will be picked up based on: Check out the Codex docs for more [configuration options](https://developers.openai.com/codex/config-reference). +### Task lifetime + +The companion process running a task or review owns its Codex turn. If that process exits before the turn finishes, the shared broker interrupts the turn, including after `SIGKILL`. Stopping a foreground Bash task therefore also stops the Codex work it owns. + +`SIGTERM`, `SIGINT`, and `SIGHUP` record the job as cancelled. After an abrupt exit, status, result, resume checks, and the stop hook repair active job records whose process is gone: completed results are preserved, and unfinished jobs become failed so they no longer block resuming work. + +Use Claude background execution for work that may exceed a foreground Bash timeout. The runtime's internal `task --background` mode instead launches a detached worker: the worker owns the turn and continues after its launcher exits, until it finishes or is cancelled. + ### 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. diff --git a/plugins/codex/agents/codex-rescue.md b/plugins/codex/agents/codex-rescue.md index 172f63858..da8ce777e 100644 --- a/plugins/codex/agents/codex-rescue.md +++ b/plugins/codex/agents/codex-rescue.md @@ -21,7 +21,8 @@ Forwarding rules: - Use exactly one `Bash` call to invoke `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task ...`. - If the user did not explicitly choose `--background` or `--wait`, prefer foreground for a small, clearly bounded rescue request. -- If the user did not explicitly choose `--background` or `--wait` and the task looks complicated, open-ended, multi-step, or likely to keep Codex running for a long time, prefer background execution. +- If the user did not explicitly choose `--background` or `--wait` and the task looks complicated, open-ended, multi-step, or likely to keep Codex running for a long time, use background execution with `Bash(..., run_in_background: true)`. +- Foreground calls stop with their owning Bash task. See [Task lifetime](../../../README.md#task-lifetime) when choosing how to run a long task. - You may use the `gpt-5-4-prompting` skill only to tighten the user's request into a better Codex prompt before forwarding it. - Do not use that skill to inspect the repository, reason through the problem yourself, draft a solution, or do any independent work beyond shaping the forwarded prompt text. - Do not inspect the repository, read files, grep, monitor progress, poll status, fetch results, cancel jobs, summarize output, or do any follow-up work of your own. diff --git a/plugins/codex/commands/rescue.md b/plugins/codex/commands/rescue.md index afe8fe4b9..3a4badf9d 100644 --- a/plugins/codex/commands/rescue.md +++ b/plugins/codex/commands/rescue.md @@ -15,7 +15,7 @@ Execution mode: - If the request includes `--background`, run the `codex:codex-rescue` subagent in the background. - If the request includes `--wait`, run the `codex:codex-rescue` subagent in the foreground. -- If neither flag is present, default to foreground. +- If neither flag is present, default to foreground for a small, clearly bounded request. Use a background subagent for complicated, open-ended, multi-step, or long-running work. - `--background` and `--wait` are execution flags for Claude Code. Do not forward them to `task`, and do not treat them as part of the natural-language task text. - `--model`, `--effort`, and `--sandbox` are runtime-selection flags. Preserve them for the forwarded `task` call, but do not treat them as part of the natural-language task text. diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 1954274fe..1b89fdcad 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -22,6 +22,14 @@ function buildStreamThreadIds(method, params, result) { return threadIds; } +// Capture the turn returned by a streaming request. Compaction returns no turn +// here; turn/started notifications may still supply one. +function buildStreamTurn(method, params, result) { + const turnId = result?.turn?.id ?? null; + const threadId = method === "review/start" ? result?.reviewThreadId ?? params?.threadId : params?.threadId; + return turnId && threadId ? { threadId, turnId } : null; +} + function buildJsonRpcError(code, message, data) { return data === undefined ? { code, message } : { code, message, data }; } @@ -67,39 +75,80 @@ async function main() { const appClient = await CodexAppServerClient.connect(cwd, { disableBroker: true }); let activeRequestSocket = null; - let activeStreamSocket = null; - let activeStreamThreadIds = null; + let activeStream = null; + let pendingOrphanInterrupts = 0; + let shuttingDown = false; const sockets = new Set(); - function clearSocketOwnership(socket) { - if (activeRequestSocket === socket) { - activeRequestSocket = null; + // The broker outlives the companion process that started a turn. Without this, + // a killed client (e.g. its Claude subagent was stopped) leaves Codex running + // with nobody listening, so tie the turn's lifetime to the owning connection. + async function interruptOrphanedTurn(stream) { + if (!stream.turn || stream.completed || stream.interrupting || shuttingDown) { + return; + } + stream.interrupting = true; + pendingOrphanInterrupts += 1; + const turn = stream.turn; + try { + await appClient.request("turn/interrupt", turn); + process.stderr.write(`Interrupted turn ${turn.turnId} on ${turn.threadId}: its client disconnected.\n`); + } catch (error) { + // The turn may already have completed; nothing else to clean up. + process.stderr.write( + `Could not interrupt turn ${turn.turnId} on ${turn.threadId} after its client disconnected: ${error instanceof Error ? error.message : String(error)}\n` + ); + } finally { + pendingOrphanInterrupts -= 1; + if (activeStream === stream) { + activeStream = null; + } } - if (activeStreamSocket === socket) { - activeStreamSocket = null; - activeStreamThreadIds = null; + } + + function clearSocketOwnership(socket) { + // Keep an in-flight request busy until its response arrives: it may still + // create a turn after the socket closes. + if (activeStream?.socket === socket) { + activeStream.disconnected = true; + void interruptOrphanedTurn(activeStream); + if (!activeStream.pending && !activeStream.turn) { + activeStream = null; + } } } function routeNotification(message) { - const target = activeRequestSocket ?? activeStreamSocket; - if (!target) { - return; + const stream = activeStream; + const threadId = message.params?.threadId ?? null; + const turnId = message.params?.turn?.id ?? null; + if (stream && message.method === "turn/started" && threadId && turnId && stream.threadIds.has(threadId)) { + stream.turn = { threadId, turnId }; + if (stream.disconnected) { + void interruptOrphanedTurn(stream); + } } - send(target, message); - if (message.method === "turn/completed" && activeStreamSocket === target) { - const threadId = message.params?.threadId ?? null; - if (!threadId || !activeStreamThreadIds || activeStreamThreadIds.has(threadId)) { - activeStreamSocket = null; - activeStreamThreadIds = null; - if (activeRequestSocket === target) { - activeRequestSocket = null; - } + + const target = activeRequestSocket ?? stream?.socket; + if (target) { + send(target, message); + } + if (stream && message.method === "turn/completed") { + // Notifications can arrive in the same chunk as the start response, + // before the awaiting request handler has resumed. + stream.completedTurns.add(`${threadId}:${turnId}`); + if ( + (stream.turn?.threadId === threadId && stream.turn?.turnId === turnId) || + (!stream.turn && stream.method === "thread/compact/start" && stream.threadIds.has(threadId)) + ) { + stream.completed = true; + activeStream = null; } } } async function shutdown(server) { + shuttingDown = true; for (const socket of sockets) { socket.end(); } @@ -168,11 +217,12 @@ async function main() { } const allowInterruptDuringActiveStream = - isInterruptRequest(message) && activeStreamSocket && activeStreamSocket !== socket && !activeRequestSocket; + isInterruptRequest(message) && activeStream && activeStream.socket !== socket && !activeRequestSocket; if ( - ((activeRequestSocket && activeRequestSocket !== socket) || (activeStreamSocket && activeStreamSocket !== socket)) && - !allowInterruptDuringActiveStream + pendingOrphanInterrupts > 0 || + (((activeRequestSocket && activeRequestSocket !== socket) || (activeStream && activeStream.socket !== socket)) && + !allowInterruptDuringActiveStream) ) { send(socket, { id: message.id, @@ -196,13 +246,43 @@ async function main() { const isStreaming = STREAMING_METHODS.has(message.method); activeRequestSocket = socket; + const stream = isStreaming + ? { + socket, + method: message.method, + threadIds: buildStreamThreadIds(message.method, message.params ?? {}, {}), + turn: null, + pending: true, + disconnected: false, + completed: false, + interrupting: false, + completedTurns: new Set() + } + : null; + if (stream) { + activeStream = stream; + } try { const result = await appClient.request(message.method, message.params ?? {}); send(socket, { id: message.id, result }); - if (isStreaming) { - activeStreamSocket = socket; - activeStreamThreadIds = buildStreamThreadIds(message.method, message.params ?? {}, result); + if (stream) { + stream.pending = false; + stream.threadIds = buildStreamThreadIds(message.method, message.params ?? {}, result); + stream.turn = buildStreamTurn(message.method, message.params ?? {}, result) ?? stream.turn; + if (stream.turn && stream.completedTurns.has(`${stream.turn.threadId}:${stream.turn.turnId}`)) { + stream.completed = true; + } + if (stream.completed) { + if (activeStream === stream) { + activeStream = null; + } + } else if (stream.disconnected) { + void interruptOrphanedTurn(stream); + if (!stream.turn && activeStream === stream) { + activeStream = null; + } + } } if (activeRequestSocket === socket) { activeRequestSocket = null; @@ -215,8 +295,8 @@ async function main() { if (activeRequestSocket === socket) { activeRequestSocket = null; } - if (activeStreamSocket === socket && !isStreaming) { - activeStreamSocket = null; + if (stream && activeStream === stream) { + activeStream = null; } } } diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 49ad2445a..530804628 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -38,6 +38,7 @@ import { buildSingleJobSnapshot, buildStatusSnapshot, readStoredJob, + reconcileOrphanedJobs, resolveCancelableJob, resolveResultJob, sortJobsNewestFirst @@ -367,6 +368,7 @@ async function waitForSingleJobSnapshot(cwd, reference, options = {}) { async function resolveLatestTrackedTaskThread(cwd, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); const sessionId = getCurrentClaudeSessionId(); + reconcileOrphanedJobs(workspaceRoot); const jobs = sortJobsNewestFirst(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")); @@ -695,7 +697,7 @@ async function runForegroundCommand(job, runner, options = {}) { logFile: options.logFile, stderr: !options.json }); - const execution = await runTrackedJob(job, () => runner(progress), { logFile }); + const execution = await runTrackedJob(job, () => runner(progress), { logFile, stopOnSignal: true }); outputResult(options.json ? execution.payload : execution.rendered, options.json); if (execution.exitStatus !== 0) { process.exitCode = execution.exitStatus; @@ -705,11 +707,11 @@ async function runForegroundCommand(job, runner, options = {}) { function spawnDetachedTaskWorker(cwd, jobId) { const scriptPath = path.join(ROOT_DIR, "scripts", "codex-companion.mjs"); - const child = spawn(process.execPath, [scriptPath, "task-worker", "--cwd", cwd, "--job-id", jobId], { + const child = spawn(process.execPath, [scriptPath, "task-worker", "--cwd", cwd, "--job-id", jobId, "--wait-for-launch"], { cwd, env: process.env, detached: true, - stdio: "ignore", + stdio: ["pipe", "ignore", "ignore"], windowsHide: true }); child.unref(); @@ -731,6 +733,9 @@ function enqueueBackgroundTask(cwd, job, request) { }; writeJobFile(job.workspaceRoot, job.id, queuedRecord); upsertJob(job.workspaceRoot, queuedRecord); + // The worker must not read or update the job before both queued records exist. + child.stdin.on("error", () => {}); + child.stdin.end("start\n"); return { payload: { @@ -878,12 +883,22 @@ async function handleTransfer(argv) { async function handleTaskWorker(argv) { const { options } = parseCommandInput(argv, { - valueOptions: ["cwd", "job-id"] + valueOptions: ["cwd", "job-id"], + booleanOptions: ["wait-for-launch"] }); if (!options["job-id"]) { throw new Error("Missing required --job-id for task-worker."); } + if (options["wait-for-launch"]) { + let launchMessage = ""; + for await (const chunk of process.stdin) { + launchMessage += chunk; + } + if (launchMessage.trim() !== "start") { + throw new Error("Background task launcher exited before saving the job."); + } + } const cwd = resolveCommandCwd(options); const workspaceRoot = resolveCommandWorkspace(options); @@ -891,6 +906,9 @@ async function handleTaskWorker(argv) { if (!storedJob) { throw new Error(`No stored job found for ${options["job-id"]}.`); } + if (storedJob.status === "cancelled" || storedJob.status === "completed" || storedJob.status === "failed") { + return; + } const request = storedJob.request; if (!request || typeof request !== "object") { @@ -917,7 +935,7 @@ async function handleTaskWorker(argv) { ...request, onProgress: progress }), - { logFile } + { logFile, stopOnSignal: true } ); } @@ -975,6 +993,7 @@ function handleTaskResumeCandidate(argv) { const cwd = resolveCommandCwd(options); const workspaceRoot = resolveCommandWorkspace(options); const sessionId = getCurrentClaudeSessionId(); + reconcileOrphanedJobs(workspaceRoot); const jobs = filterJobsForCurrentClaudeSession(sortJobsNewestFirst(listJobs(workspaceRoot))); const candidate = findLatestResumableTaskJob(jobs); diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index ad152c157..3a7252e34 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -1,8 +1,8 @@ import fs from "node:fs"; import { getSessionRuntimeStatus } from "./codex.mjs"; -import { getConfig, listJobs, readJobFile, resolveJobFile } from "./state.mjs"; -import { SESSION_ID_ENV } from "./tracked-jobs.mjs"; +import { getConfig, listJobs, readJobFile, resolveJobFile, upsertJob, writeJobFile } from "./state.mjs"; +import { appendLogLine, nowIso, SESSION_ID_ENV } from "./tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./workspace.mjs"; export const DEFAULT_MAX_STATUS_JOBS = 8; @@ -188,6 +188,73 @@ export function readStoredJob(workspaceRoot, jobId) { return readJobFile(jobFile); } +function isProcessAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM: the process exists but belongs to someone else. + return error?.code === "EPERM"; + } +} + +/** + * A job whose companion process died without recording an outcome (for example + * SIGKILL) would otherwise stay "running" forever and block `--resume-last`. + * Settle such jobs from the stored job file, or mark them failed. + */ +export function reconcileOrphanedJobs(workspaceRoot, options = {}) { + const isAlive = options.isProcessAlive ?? isProcessAlive; + for (const job of listJobs(workspaceRoot)) { + if (job.status !== "queued" && job.status !== "running") { + continue; + } + if (!Number.isInteger(job.pid) || job.pid <= 0 || isAlive(job.pid)) { + continue; + } + + const stored = readStoredJob(workspaceRoot, job.id); + if (stored && (stored.status === "completed" || stored.status === "failed" || stored.status === "cancelled")) { + upsertJob(workspaceRoot, { + id: job.id, + status: stored.status, + phase: stored.phase ?? stored.status, + pid: null, + threadId: stored.threadId ?? job.threadId ?? null, + turnId: stored.turnId ?? job.turnId ?? null, + ...(stored.errorMessage ? { errorMessage: stored.errorMessage } : {}), + completedAt: stored.completedAt ?? nowIso() + }); + continue; + } + + const completedAt = nowIso(); + const errorMessage = `The Codex companion process (pid ${job.pid}) exited before the job finished.`; + if (stored) { + writeJobFile(workspaceRoot, job.id, { + ...stored, + status: "failed", + phase: "failed", + pid: null, + errorMessage, + completedAt + }); + } + upsertJob(workspaceRoot, { + id: job.id, + status: "failed", + phase: "failed", + pid: null, + errorMessage, + completedAt + }); + const logFile = job.logFile ?? stored?.logFile; + if (logFile && fs.existsSync(logFile)) { + appendLogLine(logFile, errorMessage); + } + } +} + function matchJobReference(jobs, reference, predicate = () => true) { const filtered = jobs.filter(predicate); if (!reference) { @@ -212,6 +279,7 @@ function matchJobReference(jobs, reference, predicate = () => true) { export function buildStatusSnapshot(cwd, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); + reconcileOrphanedJobs(workspaceRoot); const config = getConfig(workspaceRoot); const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(listJobs(workspaceRoot), options)); const maxJobs = options.maxJobs ?? DEFAULT_MAX_STATUS_JOBS; @@ -241,6 +309,7 @@ export function buildStatusSnapshot(cwd, options = {}) { export function buildSingleJobSnapshot(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); + reconcileOrphanedJobs(workspaceRoot); const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)); const selected = matchJobReference(jobs, reference); if (!selected) { @@ -255,6 +324,7 @@ export function buildSingleJobSnapshot(cwd, reference, options = {}) { export function resolveResultJob(cwd, reference) { const workspaceRoot = resolveWorkspaceRoot(cwd); + reconcileOrphanedJobs(workspaceRoot); const jobs = sortJobsNewestFirst(reference ? listJobs(workspaceRoot) : filterJobsForCurrentSession(listJobs(workspaceRoot))); const selected = matchJobReference( jobs, @@ -280,6 +350,7 @@ export function resolveResultJob(cwd, reference) { export function resolveCancelableJob(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); + reconcileOrphanedJobs(workspaceRoot); const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)); const activeJobs = jobs.filter((job) => job.status === "queued" || job.status === "running"); diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 2da23498f..f07346ab8 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -1,4 +1,4 @@ -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -89,6 +89,16 @@ function removeFileIfExists(filePath) { } } +function writeJsonAtomically(filePath, value) { + const temporaryPath = `${filePath}.${process.pid}.${randomUUID()}.tmp`; + try { + fs.writeFileSync(temporaryPath, `${JSON.stringify(value, null, 2)}\n`, "utf8"); + fs.renameSync(temporaryPath, filePath); + } finally { + removeFileIfExists(temporaryPath); + } +} + export function saveState(cwd, state) { const previousJobs = loadState(cwd).jobs; ensureStateDir(cwd); @@ -111,7 +121,7 @@ export function saveState(cwd, state) { removeFileIfExists(job.logFile); } - fs.writeFileSync(resolveStateFile(cwd), `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); + writeJsonAtomically(resolveStateFile(cwd), nextState); return nextState; } @@ -166,7 +176,7 @@ export function getConfig(cwd) { export function writeJobFile(cwd, jobId, payload) { ensureStateDir(cwd); const jobFile = resolveJobFile(cwd, jobId); - fs.writeFileSync(jobFile, `${JSON.stringify(payload, null, 2)}\n`, "utf8"); + writeJsonAtomically(jobFile, payload); return jobFile; } diff --git a/plugins/codex/scripts/lib/tracked-jobs.mjs b/plugins/codex/scripts/lib/tracked-jobs.mjs index 902869012..fae0a0503 100644 --- a/plugins/codex/scripts/lib/tracked-jobs.mjs +++ b/plugins/codex/scripts/lib/tracked-jobs.mjs @@ -1,9 +1,11 @@ import fs from "node:fs"; +import os from "node:os"; import process from "node:process"; import { readJobFile, resolveJobFile, resolveJobLogFile, upsertJob, writeJobFile } from "./state.mjs"; export const SESSION_ID_ENV = "CODEX_COMPANION_SESSION_ID"; +const TERMINATION_SIGNALS = ["SIGTERM", "SIGINT", "SIGHUP"]; export function nowIso() { return new Date().toISOString(); @@ -139,6 +141,59 @@ function readStoredJobOrNull(workspaceRoot, jobId) { return readJobFile(jobFile); } +function isTerminalJobStatus(status) { + return status === "completed" || status === "failed" || status === "cancelled"; +} + +// The companion process owns the Codex turn: when it is told to stop (its Claude +// Bash call or subagent was stopped), record that before exiting. Exiting closes +// the app-server connection, which makes the broker interrupt the turn and takes +// a directly spawned app-server down with it. +function stopJobWithProcess(job, runningRecord) { + let stopping = false; + const onSignal = (signal) => { + if (stopping) { + return; + } + stopping = true; + try { + const existing = readStoredJobOrNull(job.workspaceRoot, job.id) ?? runningRecord; + // `/codex:cancel` may already have recorded its own outcome. + if (!isTerminalJobStatus(existing.status)) { + const completedAt = nowIso(); + const errorMessage = `Interrupted: the Codex companion received ${signal}.`; + writeJobFile(job.workspaceRoot, job.id, { + ...existing, + status: "cancelled", + phase: "cancelled", + errorMessage, + pid: null, + completedAt + }); + upsertJob(job.workspaceRoot, { + id: job.id, + status: "cancelled", + phase: "cancelled", + pid: null, + errorMessage, + completedAt + }); + appendLogLine(runningRecord.logFile, errorMessage); + } + } finally { + process.exit(128 + (os.constants.signals[signal] ?? 0)); + } + }; + for (const signal of TERMINATION_SIGNALS) { + process.on(signal, onSignal); + } + return () => { + for (const signal of TERMINATION_SIGNALS) { + process.off(signal, onSignal); + } + }; +} + export async function runTrackedJob(job, runner, options = {}) { const runningRecord = { ...job, @@ -150,6 +205,7 @@ export async function runTrackedJob(job, runner, options = {}) { }; writeJobFile(job.workspaceRoot, job.id, runningRecord); upsertJob(job.workspaceRoot, runningRecord); + const releaseSignals = options.stopOnSignal ? stopJobWithProcess(job, runningRecord) : () => {}; try { const execution = await runner(); @@ -200,5 +256,7 @@ export async function runTrackedJob(job, runner, options = {}) { completedAt }); throw error; + } finally { + releaseSignals(); } } diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index 2346bdcf4..1f87f3a5d 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 { reconcileOrphanedJobs, sortJobsNewestFirst } from "./lib/job-control.mjs"; import { SESSION_ID_ENV } from "./lib/tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; @@ -145,6 +145,7 @@ function main() { const workspaceRoot = resolveWorkspaceRoot(cwd); const config = getConfig(workspaceRoot); + reconcileOrphanedJobs(workspaceRoot); const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(listJobs(workspaceRoot), input)); const runningJob = jobs.find((job) => job.status === "queued" || job.status === "running"); const runningTaskNote = runningJob diff --git a/plugins/codex/skills/codex-cli-runtime/SKILL.md b/plugins/codex/skills/codex-cli-runtime/SKILL.md index 47ab385ec..95ec95e7c 100644 --- a/plugins/codex/skills/codex-cli-runtime/SKILL.md +++ b/plugins/codex/skills/codex-cli-runtime/SKILL.md @@ -27,6 +27,7 @@ Execution rules: Command selection: - Use exactly one `task` invocation per rescue handoff. - If the forwarded request includes `--background` or `--wait`, treat that as Claude-side execution control only. Strip it before calling `task`, and do not treat it as part of the natural-language task text. +- Without an explicit `--wait`, run long or open-ended work using `Bash(..., run_in_background: true)`; see [Task lifetime](../../../../README.md#task-lifetime). - If the forwarded request includes `--model`, normalize `spark` to `gpt-5.3-codex-spark` and pass it through to `task`. - If the forwarded request includes `--effort`, pass it through to `task`. - If the forwarded request includes `--sandbox`, pass it through to `task`. diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index d064712f9..c0cb7c76d 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -143,7 +143,7 @@ test("rescue command absorbs continue semantics", () => { assert.match(agent, /Never choose `--sandbox danger-full-access` yourself/i); assert.match(agent, /thin forwarding wrapper/i); assert.match(agent, /prefer foreground for a small, clearly bounded rescue request/i); - assert.match(agent, /If the user did not explicitly choose `--background` or `--wait` and the task looks complicated, open-ended, multi-step, or likely to keep Codex running for a long time, prefer background execution/i); + assert.match(agent, /If the user did not explicitly choose `--background` or `--wait` and the task looks complicated, open-ended, multi-step, or likely to keep Codex running for a long time, use background execution with `Bash\(\.\.\., run_in_background: true\)`/i); assert.match(agent, /Use exactly one `Bash` call/i); assert.match(agent, /Do not inspect the repository, read files, grep, monitor progress, poll status, fetch results, cancel jobs, summarize output, or do any follow-up work of your own/i); assert.match(agent, /Do not call `review`, `adversarial-review`, `status`, `result`, or `cancel`/i); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 5ab3e077f..17f3cc656 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -25,7 +25,9 @@ const readline = require("node:readline"); } function saveState(state) { - fs.writeFileSync(STATE_PATH, JSON.stringify(state, null, 2)); + const temporaryPath = STATE_PATH + "." + process.pid + ".tmp"; + fs.writeFileSync(temporaryPath, JSON.stringify(state, null, 2)); + fs.renameSync(temporaryPath, STATE_PATH); } function requiresExperimental(field, message, state) { @@ -180,6 +182,28 @@ function emitTurnCompletedLater(threadId, turnId, item, delayMs) { }, delayMs); } +function whenReleased(phase, action) { + if (fs.existsSync(STATE_PATH + ".release-" + phase)) { + action(); + } else { + setTimeout(() => whenReleased(phase, action), 10); + } +} + +function startInterruptibleTurn(threadId, turnId, items = []) { + send({ method: "turn/started", params: { threadId, turn: buildTurn(turnId) } }); + const timer = setTimeout(() => { + interruptibleTurns.delete(turnId); + for (const entry of items) { + if (entry && entry.completed) { + send({ method: "item/completed", params: { threadId, turnId, item: entry.completed } }); + } + } + send({ method: "turn/completed", params: { threadId, turn: buildTurn(turnId, "completed") } }); + }, 5000); + interruptibleTurns.set(turnId, { threadId, timer }); +} + function nativeReviewText(target) { if (target.type === "baseBranch") { return "Reviewed changes against " + target.branch + ".\\nNo material issues found."; @@ -275,6 +299,7 @@ bootState.appServerStarts = (bootState.appServerStarts || 0) + 1; saveState(bootState); const rl = readline.createInterface({ input: process.stdin }); +rl.on("close", () => process.exit(0)); rl.on("line", (line) => { if (!line.trim()) { return; @@ -428,7 +453,7 @@ rl.on("line", (line) => { } const turnId = nextTurnId(state); send({ id: message.id, result: { turn: buildTurn(turnId), reviewThreadId: reviewThread.id } }); - emitTurnCompleted(reviewThread.id, turnId, [ + const items = [ { started: { type: "enteredReviewMode", id: turnId, review: "current changes" } }, @@ -447,7 +472,12 @@ rl.on("line", (line) => { { completed: { type: "exitedReviewMode", id: turnId, review: nativeReviewText(message.params.target) } } - ]); + ]; + if (BEHAVIOR === "interruptible-review") { + startInterruptibleTurn(reviewThread.id, turnId, items); + } else { + emitTurnCompleted(reviewThread.id, turnId, items); + } break; } @@ -467,6 +497,13 @@ rl.on("line", (line) => { prompt }; saveState(state); + if (BEHAVIOR === "deferred-task") { + whenReleased("start", () => { + send({ id: message.id, result: { turn: buildTurn(turnId) } }); + startInterruptibleTurn(thread.id, turnId); + }); + break; + } send({ id: message.id, result: { turn: buildTurn(turnId) } }); const payload = message.params.outputSchema && message.params.outputSchema.properties && message.params.outputSchema.properties.verdict @@ -601,20 +638,7 @@ rl.on("line", (line) => { ]; if (BEHAVIOR === "interruptible-slow-task") { - send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(turnId) } }); - const timer = setTimeout(() => { - if (!interruptibleTurns.has(turnId)) { - return; - } - interruptibleTurns.delete(turnId); - for (const entry of items) { - if (entry && entry.completed) { - send({ method: "item/completed", params: { threadId: thread.id, turnId, item: entry.completed } }); - } - } - send({ method: "turn/completed", params: { threadId: thread.id, turn: buildTurn(turnId, "completed") } }); - }, 5000); - interruptibleTurns.set(turnId, { threadId: thread.id, timer }); + startInterruptibleTurn(thread.id, turnId, items); } else if (BEHAVIOR === "slow-task") { emitTurnCompletedLater(thread.id, turnId, items, 400); } else { @@ -629,19 +653,26 @@ rl.on("line", (line) => { turnId: message.params.turnId }; saveState(state); - const pending = interruptibleTurns.get(message.params.turnId); - if (pending) { - clearTimeout(pending.timer); - interruptibleTurns.delete(message.params.turnId); - send({ - method: "turn/completed", - params: { - threadId: pending.threadId, - turn: buildTurn(message.params.turnId, "interrupted") - } - }); - } - send({ id: message.id, result: {} }); + const finishInterrupt = () => { + const pending = interruptibleTurns.get(message.params.turnId); + if (pending) { + clearTimeout(pending.timer); + interruptibleTurns.delete(message.params.turnId); + send({ + method: "turn/completed", + params: { + threadId: pending.threadId, + turn: buildTurn(message.params.turnId, "interrupted") + } + }); + } + send({ id: message.id, result: {} }); + }; + if (BEHAVIOR === "deferred-task") { + whenReleased("interrupt", finishInterrupt); + } else { + finishInterrupt(); + } break; } diff --git a/tests/lifecycle.test.mjs b/tests/lifecycle.test.mjs new file mode 100644 index 000000000..0c2b2b03b --- /dev/null +++ b/tests/lifecycle.test.mjs @@ -0,0 +1,281 @@ +import fs from "node:fs"; +import net from "node:net"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import { fileURLToPath } from "node:url"; + +import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; +import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; +import { BROKER_BUSY_RPC_CODE } from "../plugins/codex/scripts/lib/app-server.mjs"; +import { createBrokerEndpoint, parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; +import { waitForBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { reconcileOrphanedJobs } from "../plugins/codex/scripts/lib/job-control.mjs"; +import { listJobs, readJobFile, resolveJobFile, upsertJob, writeJobFile } from "../plugins/codex/scripts/lib/state.mjs"; + +const PLUGIN_ROOT = fileURLToPath(new URL("../plugins/codex/", import.meta.url)); +const SCRIPT = path.join(PLUGIN_ROOT, "scripts", "codex-companion.mjs"); +const BROKER = path.join(PLUGIN_ROOT, "scripts", "app-server-broker.mjs"); + +async function waitFor(predicate, timeoutMs = 5000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const value = predicate(); + if (value) return value; + await new Promise((resolve) => setTimeout(resolve, 10)); + } + assert.fail("Timed out waiting for lifecycle transition."); +} + +async function startBroker(t, behavior = "interruptible-slow-task") { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const sessionDir = makeTempDir("cxl-"); + initGitRepo(repo); + installFakeCodex(binDir, behavior); + const endpoint = createBrokerEndpoint(sessionDir); + const env = { ...buildEnv(binDir), CODEX_COMPANION_APP_SERVER_ENDPOINT: endpoint }; + const broker = spawn(process.execPath, [BROKER, "serve", "--cwd", repo, "--endpoint", endpoint], { + cwd: repo, env, stdio: ["ignore", "ignore", "pipe"] + }); + let stderr = ""; + broker.stderr.on("data", (chunk) => { stderr += chunk; }); + const exited = once(broker, "exit"); + const sockets = new Set(); + t.after(async () => { + for (const socket of sockets) socket.destroy(); + if (broker.exitCode === null && broker.signalCode === null) broker.kill("SIGTERM"); + await exited; + }); + assert.equal(await waitForBrokerEndpoint(endpoint, 5000), true, stderr); + const statePath = path.join(binDir, "fake-codex-state.json"); + return { + repo, env, + readState: () => JSON.parse(fs.readFileSync(statePath, "utf8")), + release: (phase) => fs.writeFileSync(`${statePath}.release-${phase}`, ""), + async connect() { + const socket = net.createConnection({ path: parseBrokerEndpoint(endpoint).path }); + sockets.add(socket); + socket.setEncoding("utf8"); + const pending = new Map(); + const notifications = []; + let nextId = 1; + let buffer = ""; + socket.on("data", (chunk) => { + buffer += chunk; + let newline; + while ((newline = buffer.indexOf("\n")) !== -1) { + const message = JSON.parse(buffer.slice(0, newline)); + buffer = buffer.slice(newline + 1); + if (message.id !== undefined) { + pending.get(message.id)?.resolve(message); + pending.delete(message.id); + } else { + notifications.push(message); + } + } + }); + socket.on("close", () => { + for (const { reject } of pending.values()) reject(new Error("Connection closed.")); + pending.clear(); + sockets.delete(socket); + }); + await once(socket, "connect"); + return { + notifications, + request(method, params = {}) { + const id = nextId++; + return new Promise((resolve, reject) => { + pending.set(id, { resolve, reject }); + socket.write(`${JSON.stringify({ id, method, params })}\n`); + }); + }, + async close() { + const closed = once(socket, "close"); + socket.destroy(); + await closed; + } + }; + } + }; +} + +async function startTurn(client, repo) { + const started = await client.request("thread/start", { cwd: repo }); + assert.ok(started.result, JSON.stringify(started)); + const threadId = started.result.thread.id; + const response = await client.request("turn/start", { threadId, input: [{ type: "text", text: "Investigate." }] }); + assert.ok(response.result, JSON.stringify(response)); + return { threadId, turnId: response.result.turn.id }; +} + +async function waitForAvailableBroker(client, repo) { + const deadline = Date.now() + 5000; + do { + const response = await client.request("thread/start", { cwd: repo }); + if (response.result) return response.result; + assert.equal(response.error.code, BROKER_BUSY_RPC_CODE); + await new Promise((resolve) => setTimeout(resolve, 10)); + } while (Date.now() < deadline); + assert.fail("Broker stayed busy after interrupting its orphaned turn."); +} + +test("broker interrupts a disconnected owner while leaving observer disconnects alone", async (t) => { + const harness = await startBroker(t); + const owner = await harness.connect(); + const turn = await startTurn(owner, harness.repo); + const observer = await harness.connect(); + await observer.request("initialize"); + await observer.close(); + const probe = await harness.connect(); + assert.equal((await probe.request("thread/start", { cwd: harness.repo })).error.code, BROKER_BUSY_RPC_CODE); + assert.equal(harness.readState().lastInterrupt, null); + await owner.close(); + assert.deepEqual(await waitFor(() => harness.readState().lastInterrupt), turn); + // A subsequent client can use the broker once the old turn is interrupted. + await waitForAvailableBroker(probe, harness.repo); +}); + +test("broker keeps a disconnected start request busy until the turn and interrupt settle", async (t) => { + const harness = await startBroker(t, "deferred-task"); + const owner = await harness.connect(); + const started = await owner.request("thread/start", { cwd: harness.repo }); + const threadId = started.result.thread.id; + const request = owner.request("turn/start", { threadId, input: [] }).catch(() => null); + const turn = await waitFor(() => harness.readState().lastTurnStart); + await owner.close(); + await request; + const probe = await harness.connect(); + assert.equal((await probe.request("thread/start", { cwd: harness.repo })).error.code, BROKER_BUSY_RPC_CODE); + harness.release("start"); + assert.deepEqual(await waitFor(() => harness.readState().lastInterrupt), { threadId, turnId: turn.turnId }); + assert.equal((await probe.request("thread/start", { cwd: harness.repo })).error.code, BROKER_BUSY_RPC_CODE); + harness.release("interrupt"); + await waitForAvailableBroker(probe, harness.repo); +}); + +test("broker does not interrupt normally completed turns when their clients close", async (t) => { + const harness = await startBroker(t, "review-ok"); + const owner = await harness.connect(); + const turn = await startTurn(owner, harness.repo); + await waitFor(() => owner.notifications.find((message) => message.method === "turn/completed")); + await owner.close(); + const probe = await harness.connect(); + assert.ok((await probe.request("thread/start", { cwd: harness.repo })).result); + assert.equal(harness.readState().lastInterrupt, null, `Completed turn ${turn.turnId} was interrupted.`); +}); + +test("broker interrupts the detached review thread owned by a disconnected client", async (t) => { + const harness = await startBroker(t, "interruptible-review"); + const owner = await harness.connect(); + const started = await owner.request("thread/start", { cwd: harness.repo }); + const review = await owner.request("review/start", { + threadId: started.result.thread.id, delivery: "detached", target: { type: "uncommittedChanges" } + }); + assert.notEqual(review.result.reviewThreadId, started.result.thread.id); + await owner.close(); + assert.deepEqual(await waitFor(() => harness.readState().lastInterrupt), { + threadId: review.result.reviewThreadId, turnId: review.result.turn.id + }); +}); + +function launchCompanion(t, harness, args = ["task", "--write", "Investigate the worker."]) { + const child = spawn(process.execPath, [SCRIPT, ...args], { cwd: harness.repo, env: harness.env, stdio: "pipe" }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => { stdout += chunk; }); + child.stderr.on("data", (chunk) => { stderr += chunk; }); + const closed = once(child, "close"); + t.after(async () => { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); + await closed; + }); + return { child, closed, stdout: () => stdout, stderr: () => stderr }; +} + +for (const signal of ["SIGTERM", "SIGINT", "SIGHUP"]) { + test(`companion records cancellation and interrupts Codex on ${signal}`, { skip: process.platform === "win32" }, async (t) => { + const harness = await startBroker(t); + const companion = launchCompanion(t, harness); + const job = await waitFor(() => listJobs(harness.repo).find((candidate) => candidate.turnId)); + companion.child.kill(signal); + const [code] = await companion.closed; + assert.equal(code, 128 + os.constants.signals[signal], companion.stderr()); + assert.deepEqual(await waitFor(() => harness.readState().lastInterrupt), { threadId: job.threadId, turnId: job.turnId }); + const stored = readJobFile(resolveJobFile(harness.repo, job.id)); + assert.equal(stored.status, "cancelled"); + assert.equal(stored.pid, null); + assert.match(stored.errorMessage, new RegExp(signal)); + assert.equal(listJobs(harness.repo)[0].status, "cancelled"); + }); +} + +test("SIGKILL interrupts the turn and status repairs the orphan for later resume", { skip: process.platform === "win32" }, async (t) => { + const harness = await startBroker(t); + const companion = launchCompanion(t, harness); + const job = await waitFor(() => listJobs(harness.repo).find((candidate) => candidate.turnId)); + companion.child.kill("SIGKILL"); + await companion.closed; + assert.deepEqual(await waitFor(() => harness.readState().lastInterrupt), { threadId: job.threadId, turnId: job.turnId }); + const status = run(process.execPath, [SCRIPT, "status", job.id, "--json"], { cwd: harness.repo, env: harness.env }); + assert.equal(status.status, 0, status.stderr); + assert.equal(JSON.parse(status.stdout).job.status, "failed"); + assert.equal(readJobFile(resolveJobFile(harness.repo, job.id)).pid, null); + const candidate = run(process.execPath, [SCRIPT, "task-resume-candidate", "--json"], { cwd: harness.repo, env: harness.env }); + assert.equal(candidate.status, 0, candidate.stderr); + assert.equal(JSON.parse(candidate.stdout).available, true); + const resumed = run(process.execPath, [SCRIPT, "task", "--resume-last", "Continue."], { cwd: harness.repo, env: harness.env }); + assert.equal(resumed.status, 0, resumed.stderr); +}); + +test("a detached worker survives its launcher and is still cancelled through the broker", async (t) => { + const harness = await startBroker(t); + const companion = launchCompanion(t, harness, ["task", "--background", "--json", "Investigate the worker."]); + const [code] = await companion.closed; + assert.equal(code, 0, companion.stderr()); + const { jobId } = JSON.parse(companion.stdout()); + const job = await waitFor(() => listJobs(harness.repo).find((candidate) => candidate.id === jobId && candidate.turnId)); + t.after(() => { try { process.kill(job.pid, "SIGKILL"); } catch {} }); + assert.equal(job.status, "running"); + assert.equal(harness.readState().lastInterrupt, null); + const cancelled = run(process.execPath, [SCRIPT, "cancel", jobId, "--json"], { cwd: harness.repo, env: harness.env }); + assert.equal(cancelled.status, 0, cancelled.stderr); + assert.equal(JSON.parse(cancelled.stdout).turnInterrupted, true); + assert.equal(listJobs(harness.repo)[0].status, "cancelled"); +}); + +test("orphan reconciliation repairs active jobs once and preserves a stored terminal result", () => { + const repo = makeTempDir(); + const logFile = path.join(repo, "job.log"); + fs.writeFileSync(logFile, ""); + const orphan = { id: "orphan", status: "running", pid: 42, threadId: "thr_orphan", logFile }; + const finished = { id: "finished", status: "queued", pid: 43 }; + const live = { id: "live", status: "running", pid: 44 }; + const queued = { id: "queued", status: "queued", pid: null }; + for (const job of [orphan, finished, live, queued]) { + upsertJob(repo, job); + writeJobFile(repo, job.id, job); + } + writeJobFile(repo, finished.id, { + ...finished, status: "completed", phase: "done", pid: null, threadId: "thr_finished", turnId: "turn_finished", + completedAt: "2026-01-01T00:00:00.000Z", rendered: "Kept the finished output." + }); + const options = { isProcessAlive: (pid) => pid === live.pid }; + reconcileOrphanedJobs(repo, options); + const jobs = Object.fromEntries(listJobs(repo).map((job) => [job.id, job])); + assert.equal(jobs.orphan.status, "failed"); + assert.equal(jobs.orphan.pid, null); + assert.equal(jobs.orphan.threadId, "thr_orphan"); + assert.equal(readJobFile(resolveJobFile(repo, orphan.id)).status, "failed"); + assert.equal(jobs.finished.status, "completed"); + assert.equal(jobs.finished.threadId, "thr_finished"); + assert.equal(jobs.finished.turnId, "turn_finished"); + assert.equal(readJobFile(resolveJobFile(repo, finished.id)).rendered, "Kept the finished output."); + assert.equal(jobs.live.status, "running"); + assert.equal(jobs.queued.status, "queued"); + reconcileOrphanedJobs(repo, options); + assert.equal(fs.readFileSync(logFile, "utf8").split("exited before").length - 1, 1); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0f8f57cea..1474ba6d4 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -3,9 +3,51 @@ import os from "node:os"; import path from "node:path"; import test from "node:test"; import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; import { makeTempDir } from "./helpers.mjs"; -import { resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs"; +import { resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState, upsertJob, writeJobFile } from "../plugins/codex/scripts/lib/state.mjs"; + +test("concurrent readers see complete state and job snapshots during progress updates", { timeout: 15000 }, async (t) => { + const workspace = makeTempDir(); + const job = { id: "progress", status: "running", details: "x".repeat(128 * 1024) }; + writeJobFile(workspace, job.id, job); + upsertJob(workspace, job); + const stateFile = resolveStateFile(workspace); + const jobFile = resolveJobFile(workspace, job.id); + const moduleUrl = new URL("../plugins/codex/scripts/lib/state.mjs", import.meta.url).href; + const writer = spawn(process.execPath, ["--input-type=module", "-e", ` + import { upsertJob, writeJobFile } from ${JSON.stringify(moduleUrl)}; + const workspace = ${JSON.stringify(workspace)}; + for (let progress = 0; progress < 16; progress++) { + const job = { id: "progress", status: "running", details: "x".repeat(128 * 1024), progress }; + writeJobFile(workspace, job.id, job); + upsertJob(workspace, job); + } + `], { stdio: ["ignore", "ignore", "pipe"] }); + let stderr = ""; + writer.stderr.on("data", (chunk) => { stderr += chunk; }); + const closed = once(writer, "close"); + t.after(async () => { + if (writer.exitCode === null && writer.signalCode === null) writer.kill("SIGKILL"); + await closed; + }); + let snapshots = 0; + while (writer.exitCode === null && writer.signalCode === null) { + const state = JSON.parse(fs.readFileSync(stateFile, "utf8")); + const stored = JSON.parse(fs.readFileSync(jobFile, "utf8")); + assert.equal(state.jobs[0].details.length, job.details.length); + assert.equal(stored.details.length, job.details.length); + snapshots++; + await new Promise((resolve) => setImmediate(resolve)); + } + const [code] = await closed; + assert.equal(code, 0, stderr); + assert.ok(snapshots > 0); + assert.equal(JSON.parse(fs.readFileSync(stateFile, "utf8")).jobs[0].progress, 15); + assert.equal(JSON.parse(fs.readFileSync(jobFile, "utf8")).progress, 15); +}); test("resolveStateDir uses a temp-backed per-workspace directory", () => { const workspace = makeTempDir();