diff --git a/plugins/codex/agents/codex-rescue.md b/plugins/codex/agents/codex-rescue.md index 7009ec86a..e07c63a2e 100644 --- a/plugins/codex/agents/codex-rescue.md +++ b/plugins/codex/agents/codex-rescue.md @@ -19,7 +19,7 @@ Selection guidance: Forwarding rules: -- Use exactly one `Bash` call to invoke `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task ...`. +- Use exactly one `Bash` call to invoke `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task [routing flags] -- ""`. - 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. - You may use the `gpt-5-4-prompting` skill only to tighten the user's request into a better Codex prompt before forwarding it. @@ -33,6 +33,8 @@ Forwarding rules: - 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 `--resume` and `--fresh` as routing controls and do not include them in the task text you pass through. +- Place task routing flags (`--write`, `--resume-last`, `--model`, `--effort`, `--cwd`, etc.) before a `--` separator, then pass the natural-language prompt after `--`. Never put prompt text before `--`. +- `--background` and `--wait` are Claude-side execution control only. Strip them before calling `task` and never place them among the flags before `--`. - `--resume` means add `--resume-last`. - `--fresh` means do not add `--resume-last`. - If the user is clearly asking to continue prior Codex work in this repository, such as "continue", "keep going", "resume", "apply the top fix", or "dig deeper", add `--resume-last` unless `--fresh` is present. diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..c75a826bd 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -763,6 +763,7 @@ async function handleTask(argv) { const { options, positionals } = parseCommandInput(argv, { valueOptions: ["model", "effort", "cwd", "prompt-file"], booleanOptions: ["json", "write", "resume-last", "resume", "fresh", "background"], + stopAtFirstPositional: true, aliasMap: { m: "model" } diff --git a/plugins/codex/scripts/lib/args.mjs b/plugins/codex/scripts/lib/args.mjs index 6b1518502..2334d77fa 100644 --- a/plugins/codex/scripts/lib/args.mjs +++ b/plugins/codex/scripts/lib/args.mjs @@ -5,11 +5,12 @@ export function parseArgs(argv, config = {}) { const options = {}; const positionals = []; let passthrough = false; + let stopOptions = false; for (let index = 0; index < argv.length; index += 1) { const token = argv[index]; - if (passthrough) { + if (passthrough || stopOptions) { positionals.push(token); continue; } @@ -21,6 +22,9 @@ export function parseArgs(argv, config = {}) { if (!token.startsWith("-") || token === "-") { positionals.push(token); + if (config.stopAtFirstPositional) { + stopOptions = true; + } continue; } @@ -46,6 +50,9 @@ export function parseArgs(argv, config = {}) { } positionals.push(token); + if (config.stopAtFirstPositional) { + stopOptions = true; + } continue; } @@ -68,6 +75,9 @@ export function parseArgs(argv, config = {}) { } positionals.push(token); + if (config.stopAtFirstPositional) { + stopOptions = true; + } } return { options, positionals }; diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index fead00cc4..9f3931bb2 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -535,8 +535,20 @@ function applyTurnNotification(state, message) { } break; case "error": + if (message.params?.willRetry) { + emitProgress( + state.onProgress, + `Codex error (retrying): ${message.params.error?.message ?? "unknown error"}`, + resolveErrorProgressPhase(message.params) + ); + break; + } state.error = message.params.error; - emitProgress(state.onProgress, `Codex error: ${message.params.error.message}`, "failed"); + emitProgress( + state.onProgress, + `Codex error: ${message.params.error.message}`, + resolveErrorProgressPhase(message.params) + ); break; case "turn/completed": if ((message.params.threadId ?? null) !== state.threadId) { @@ -550,6 +562,9 @@ function applyTurnNotification(state, message) { "finalizing" ); completeTurn(state, message.params.turn); + if (message.params.turn.status === "completed") { + state.error = null; + } break; default: break; @@ -751,7 +766,18 @@ async function resumeThread(client, threadId, cwd, options = {}) { return client.request("thread/resume", buildResumeParams(threadId, cwd, options)); } -function buildResultStatus(turnState) { +export function shouldStoreTurnError(params = {}) { + return Boolean(params.error) && params.willRetry !== true; +} + +export function resolveErrorProgressPhase(params = {}) { + return params.willRetry === true ? "retrying" : "failed"; +} + +export function buildResultStatus(turnState) { + if (turnState.error) { + return 1; + } return turnState.finalTurn?.status === "completed" ? 0 : 1; } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..13da41b6c 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,4 +1,5 @@ import { spawnSync } from "node:child_process"; +import fs from "node:fs"; import process from "node:process"; export function runCommand(command, args = [], options = {}) { @@ -36,7 +37,12 @@ export function runCommandChecked(command, args = [], options = {}) { } export function binaryAvailable(command, versionArgs = ["--version"], options = {}) { - const result = runCommand(command, versionArgs, options); + const probeOptions = { ...options }; + if (probeOptions.cwd && !fs.existsSync(probeOptions.cwd)) { + delete probeOptions.cwd; + } + + const result = runCommand(command, versionArgs, probeOptions); if (result.error && /** @type {NodeJS.ErrnoException} */ (result.error).code === "ENOENT") { return { available: false, detail: "not found" }; } diff --git a/plugins/codex/skills/codex-cli-runtime/SKILL.md b/plugins/codex/skills/codex-cli-runtime/SKILL.md index 0e91bfb50..036778efd 100644 --- a/plugins/codex/skills/codex-cli-runtime/SKILL.md +++ b/plugins/codex/skills/codex-cli-runtime/SKILL.md @@ -9,7 +9,7 @@ user-invocable: false Use this skill only inside the `codex:codex-rescue` subagent. Primary helper: -- `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task ""` +- `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task [routing flags] -- ""` Execution rules: - The rescue subagent is a forwarder, not an orchestrator. Its only job is to invoke `task` once and return that stdout unchanged. @@ -38,6 +38,8 @@ Command selection: Safety rules: - Default to write-capable Codex work in `codex:codex-rescue` unless the user explicitly asks for read-only behavior. - Preserve the user's task text as-is apart from stripping routing flags. +- Always insert `--` immediately before the natural-language prompt. Put task routing flags (`--write`, `--resume-last`, `--model`, `--effort`, `--cwd`, etc.) before `--` and never include prompt text before `--`. +- Never place `--background` or `--wait` before `--`; those are Claude-side only and must already have been stripped. - 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. - If the Bash call fails or Codex cannot be invoked, return nothing. diff --git a/tests/args.test.mjs b/tests/args.test.mjs new file mode 100644 index 000000000..36acac2d5 --- /dev/null +++ b/tests/args.test.mjs @@ -0,0 +1,84 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { parseArgs } from "../plugins/codex/scripts/lib/args.mjs"; + +const TASK_CONFIG = { + valueOptions: ["model", "effort", "cwd", "prompt-file"], + booleanOptions: ["json", "write", "resume-last", "resume", "fresh", "background"], + aliasMap: { + m: "model", + C: "cwd" + } +}; + +test("parseArgs with stopAtFirstPositional keeps prompt fragments out of options", () => { + const result = parseArgs(["--write", "review", "-m", "pytest"], { + ...TASK_CONFIG, + stopAtFirstPositional: true + }); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, undefined); + assert.equal(result.positionals.join(" "), "review -m pytest"); +}); + +test("parseArgs without stopAtFirstPositional still consumes -m as model", () => { + const result = parseArgs(["--write", "review", "-m", "pytest"], TASK_CONFIG); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, "pytest"); + assert.equal(result.positionals.join(" "), "review"); +}); + +test("parseArgs honors -- passthrough before stopAtFirstPositional matters", () => { + const result = parseArgs(["--write", "--", "-m", "pytest"], { + ...TASK_CONFIG, + stopAtFirstPositional: true + }); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, undefined); + assert.equal(result.positionals.join(" "), "-m pytest"); +}); + +test("stopAtFirstPositional stops after unrecognized long option becomes positional", () => { + const result = parseArgs(["--write", "--coverage", "run", "-m", "pytest"], { + ...TASK_CONFIG, + stopAtFirstPositional: true + }); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, undefined); + assert.equal(result.positionals.join(" "), "--coverage run -m pytest"); +}); + +test("without stopAtFirstPositional unrecognized long then -m still sets model", () => { + const result = parseArgs(["--write", "--coverage", "run", "-m", "pytest"], TASK_CONFIG); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, "pytest"); + assert.equal(result.positionals.join(" "), "--coverage run"); +}); + +test("stopAtFirstPositional stops after unrecognized short option becomes positional", () => { + const result = parseArgs(["--write", "-z", "-m", "pytest"], { + ...TASK_CONFIG, + stopAtFirstPositional: true + }); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, undefined); + assert.equal(result.positionals.join(" "), "-z -m pytest"); +}); + +test("stopAtFirstPositional still parses recognized flags before first positional", () => { + const result = parseArgs(["--write", "--model", "spark", "fix it"], { + ...TASK_CONFIG, + stopAtFirstPositional: true + }); + + assert.equal(result.options.write, true); + assert.equal(result.options.model, "spark"); + assert.equal(result.positionals.join(" "), "fix it"); +}); diff --git a/tests/codex-turn-status.test.mjs b/tests/codex-turn-status.test.mjs new file mode 100644 index 000000000..362a82485 --- /dev/null +++ b/tests/codex-turn-status.test.mjs @@ -0,0 +1,72 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + buildResultStatus, + resolveErrorProgressPhase, + shouldStoreTurnError +} from "../plugins/codex/scripts/lib/codex.mjs"; + +test("resolveErrorProgressPhase uses retrying for willRetry errors", () => { + assert.equal( + resolveErrorProgressPhase({ + willRetry: true, + error: { message: "x" } + }), + "retrying" + ); + assert.equal(resolveErrorProgressPhase({ error: { message: "x" } }), "failed"); + assert.equal(resolveErrorProgressPhase({}), "failed"); +}); + +test("shouldStoreTurnError ignores retriable app-server errors", () => { + assert.equal( + shouldStoreTurnError({ + error: { message: "transient" }, + willRetry: true + }), + false + ); + assert.equal( + shouldStoreTurnError({ + error: { message: "terminal" } + }), + true + ); +}); + +test("buildResultStatus fails when turnState.error is set despite completed finalTurn", () => { + const status = buildResultStatus({ + error: { message: "boom" }, + finalTurn: { id: "turn_1", status: "completed" } + }); + + assert.equal(status, 1); +}); + +test("buildResultStatus succeeds for completed turn without error", () => { + const status = buildResultStatus({ + error: null, + finalTurn: { id: "turn_1", status: "completed" } + }); + + assert.equal(status, 0); +}); + +test("buildResultStatus fails for non-completed finalTurn", () => { + const status = buildResultStatus({ + error: null, + finalTurn: { id: "turn_1", status: "failed" } + }); + + assert.equal(status, 1); +}); + +test("buildResultStatus fails when only error is present", () => { + const status = buildResultStatus({ + error: { message: "x" }, + finalTurn: null + }); + + assert.equal(status, 1); +}); diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index c34b06059..42d0b74f2 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -130,6 +130,14 @@ test("rescue command absorbs continue semantics", () => { 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, /Use exactly one `Bash` call/i); + assert.match(agent, /task \[routing flags\] -- ""/); + assert.match(agent, /Never put prompt text before `--`/i); + assert.match(agent, /`--background` and `--wait` are Claude-side execution control only/i); + assert.match(agent, /never place them among the flags before `--`/i); + assert.doesNotMatch( + agent, + /Place (?:task )?routing flags \(`--write`[^)]*`--background`/ + ); 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); assert.match(agent, /Leave `--effort` unset unless the user explicitly requests a specific reasoning effort/i); @@ -192,7 +200,9 @@ test("internal docs use task terminology for rescue runs", () => { const promptingSkill = read("skills/gpt-5-4-prompting/SKILL.md"); const promptRecipes = read("skills/gpt-5-4-prompting/references/codex-prompt-recipes.md"); - assert.match(runtimeSkill, /codex-companion\.mjs" task ""/); + assert.match(runtimeSkill, /codex-companion\.mjs" task \[routing flags\] -- ""/); + assert.match(runtimeSkill, /Never include prompt text before `--`/i); + assert.match(runtimeSkill, /Never place `--background` or `--wait` before `--`/i); assert.match(runtimeSkill, /Use `task` for every rescue request/i); assert.match(runtimeSkill, /task --resume-last/i); assert.match(promptingSkill, /Use `task` when the task is diagnosis/i); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..d51956100 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,7 +1,25 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import process from "node:process"; + +import { binaryAvailable, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; + +test("binaryAvailable ignores a missing cwd for version probes", () => { + const status = binaryAvailable(process.execPath, ["--version"], { + cwd: "/does/not/exist" + }); + + assert.equal(status.available, true); +}); + +test("binaryAvailable still reports a missing binary when cwd is invalid", () => { + const status = binaryAvailable("definitely-not-a-real-binary-xyz", ["--version"], { + cwd: "/does/not/exist" + }); + + assert.equal(status.available, false); +}); test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..0d0502429 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -763,6 +763,29 @@ test("task --fresh is treated as routing control and does not leak into the prom assert.equal(fakeState.lastTurnStart.prompt, "diagnose the flaky test"); }); +test("task does not treat prompt fragments as --model when passed as one raw argument", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const prompt = + '--write review the conflict_scan.cli module and run -m pytest to verify'; + const result = run("node", [SCRIPT, "task", prompt], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + const fakeState = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.notEqual(fakeState.lastTurnStart.model, "pytest"); + assert.match(fakeState.lastTurnStart.prompt, /-m pytest/); +}); + test("task forwards model selection and reasoning effort to app-server turn/start", () => { const repo = makeTempDir(); const binDir = makeTempDir();