diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index 2346bdcf..4ea7caa7 100644 --- a/plugins/codex/scripts/stop-review-gate-hook.mjs +++ b/plugins/codex/scripts/stop-review-gate-hook.mjs @@ -18,6 +18,15 @@ const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); const ROOT_DIR = path.resolve(SCRIPT_DIR, ".."); const STOP_REVIEW_TASK_MARKER = "Run a stop-gate review of the previous Claude turn."; +// Stop-review outcomes. Only a genuine BLOCK verdict should block the Stop +// event. Infrastructure failures (timeout, crash, rate limit, auth blip, empty +// or garbled output) must fail OPEN, otherwise the Stop hook re-triggers, the +// review re-runs, hits the same failure, and loops indefinitely — burning +// tokens with no useful review (#248, #306). +const REVIEW_ALLOW = "allow"; +const REVIEW_BLOCK = "block"; +const REVIEW_INFRA_ERROR = "infra_error"; + function readHookInput() { const raw = fs.readFileSync(0, "utf8").trim(); if (!raw) { @@ -70,31 +79,38 @@ function parseStopReviewOutput(rawOutput) { const text = String(rawOutput ?? "").trim(); if (!text) { return { - ok: false, - reason: - "The stop-time Codex review task returned no final output. Run /codex:review --wait manually or bypass the gate." + outcome: REVIEW_INFRA_ERROR, + reason: "the review task returned no final output" }; } const firstLine = text.split(/\r?\n/, 1)[0].trim(); if (firstLine.startsWith("ALLOW:")) { - return { ok: true, reason: null }; + return { outcome: REVIEW_ALLOW, reason: null }; } if (firstLine.startsWith("BLOCK:")) { const reason = firstLine.slice("BLOCK:".length).trim() || text; return { - ok: false, + outcome: REVIEW_BLOCK, reason: `Codex stop-time review found issues that still need fixes before ending the session: ${reason}` }; } return { - ok: false, - reason: - "The stop-time Codex review task returned an unexpected answer. Run /codex:review --wait manually or bypass the gate." + outcome: REVIEW_INFRA_ERROR, + reason: "the review task returned an unexpected answer (no ALLOW:/BLOCK: verdict)" }; } +function tryParseVerdict(stdout) { + try { + const payload = JSON.parse(String(stdout ?? "")); + return parseStopReviewOutput(payload?.rawOutput); + } catch { + return null; + } +} + function runStopReview(cwd, input = {}) { const scriptPath = path.join(SCRIPT_DIR, "codex-companion.mjs"); const prompt = buildStopReviewPrompt(input); @@ -111,19 +127,24 @@ function runStopReview(cwd, input = {}) { if (result.error?.code === "ETIMEDOUT") { return { - ok: false, - reason: - "The stop-time Codex review task timed out after 15 minutes. Run /codex:review --wait manually or bypass the gate." + outcome: REVIEW_INFRA_ERROR, + reason: "the review task timed out after 15 minutes" }; } if (result.status !== 0) { + // The companion prints its JSON payload before setting a non-zero exit + // code, so a turn that failed AFTER streaming a final verdict still + // carries it in rawOutput. Never discard a BLOCK we actually received; + // fail open only when no verdict is present. + const salvaged = tryParseVerdict(result.stdout); + if (salvaged?.outcome === REVIEW_BLOCK) { + return salvaged; + } const detail = String(result.stderr || result.stdout || "").trim(); return { - ok: false, - reason: detail - ? `The stop-time Codex review task failed: ${detail}` - : "The stop-time Codex review task failed. Run /codex:review --wait manually or bypass the gate." + outcome: REVIEW_INFRA_ERROR, + reason: detail ? `the review task failed: ${detail}` : "the review task failed" }; } @@ -132,9 +153,8 @@ function runStopReview(cwd, input = {}) { return parseStopReviewOutput(payload?.rawOutput); } catch { return { - ok: false, - reason: - "The stop-time Codex review task returned invalid JSON. Run /codex:review --wait manually or bypass the gate." + outcome: REVIEW_INFRA_ERROR, + reason: "the review task returned invalid JSON" }; } } @@ -164,7 +184,8 @@ function main() { } const review = runStopReview(cwd, input); - if (!review.ok) { + + if (review.outcome === REVIEW_BLOCK) { emitDecision({ decision: "block", reason: runningTaskNote ? `${runningTaskNote} ${review.reason}` : review.reason @@ -172,6 +193,21 @@ function main() { return; } + if (review.outcome === REVIEW_INFRA_ERROR) { + // Fail open on infrastructure failures so a transient Codex problem cannot + // turn into an unbounded Stop-hook rewake loop (#248, #306). We warn on + // stderr but do NOT block, letting the session end normally. + // Embedded error details often end with a period; strip it so splicing + // into the sentence below does not produce "..". + const reason = String(review.reason ?? "").trim().replace(/\.$/, ""); + logNote( + `Codex stop-time review could not complete — ${reason}. Allowing the stop instead of blocking; ` + + "run /codex:review --wait manually or bypass the gate." + ); + logNote(runningTaskNote); + return; + } + logNote(runningTaskNote); } diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index f83c96a0..a0ab1622 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -309,6 +309,9 @@ rl.on("line", (line) => { if (BEHAVIOR === "auth-run-fails") { throw new Error("authentication expired; run codex login"); } + if (BEHAVIOR === "stop-gate-infra") { + throw new Error("stream error: 429 Too Many Requests (rate limit reached)."); + } if (requiresExperimental("persistExtendedHistory", message, state) || requiresExperimental("persistFullHistory", message, state)) { throw new Error("thread/start.persistFullHistory requires experimentalApi capability"); } @@ -602,6 +605,16 @@ rl.on("line", (line) => { interruptibleTurns.set(turnId, { threadId: thread.id, timer }); } else if (BEHAVIOR === "slow-task") { emitTurnCompletedLater(thread.id, turnId, items, 400); + } else if (BEHAVIOR === "stop-gate-block-then-error") { + // Stream the final verdict, then end the turn with a non-completed + // status so the companion exits non-zero with rawOutput populated. + send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(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, "failed") } }); } else { emitTurnCompleted(thread.id, turnId, items); } diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835..132e605a 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -2116,6 +2116,72 @@ test("stop hook runs the actual task when auth status looks stale", () => { assert.match(payload.reason, /Missing empty-state guard/i); }); +test("stop hook fails open (does not block) when the stop-time review hits an infrastructure error", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "stop-gate-infra"); + 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 setup = run("node", [SCRIPT, "setup", "--enable-review-gate", "--json"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(setup.status, 0, setup.stderr); + + const allowed = run("node", [STOP_HOOK], { + cwd: repo, + env: buildEnv(binDir), + input: JSON.stringify({ cwd: repo, session_id: "sess-stop-infra" }) + }); + + // A transient Codex/infra failure must fail OPEN: no block decision on stdout, + // just a stderr warning. Blocking here would loop the Stop hook forever (#248, #306). + assert.equal(allowed.status, 0, allowed.stderr); + assert.equal(allowed.stdout.trim(), ""); + assert.match(allowed.stderr, /could not complete/i); + assert.match(allowed.stderr, /Allowing the stop instead of blocking/i); + assert.match(allowed.stderr, /rate limit|429/i); + // Period-terminated error details must not produce ".." when spliced into the warning. + assert.doesNotMatch(allowed.stderr, /\.\. Allowing/); +}); + +test("stop hook still blocks when the review turn fails after emitting a BLOCK verdict", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "stop-gate-block-then-error"); + 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 setup = run("node", [SCRIPT, "setup", "--enable-review-gate", "--json"], { + cwd: repo, + env: buildEnv(binDir) + }); + assert.equal(setup.status, 0, setup.stderr); + + const blocked = run("node", [STOP_HOOK], { + cwd: repo, + env: buildEnv(binDir), + input: JSON.stringify({ + cwd: repo, + session_id: "sess-stop-salvage", + last_assistant_message: "I completed the refactor and updated the retry logic." + }) + }); + + // The companion exits non-zero (turn ended "failed") but its payload still + // carries the BLOCK verdict that was streamed before the failure. A verdict + // we actually received must be honored, not discarded as an infra error. + assert.equal(blocked.status, 0, blocked.stderr); + const payload = JSON.parse(blocked.stdout); + assert.equal(payload.decision, "block"); + assert.match(payload.reason, /Missing empty-state guard/i); +}); + test("commands lazily start and reuse one shared app-server after first use", async () => { const repo = makeTempDir(); const binDir = makeTempDir();