fix(stop-review-gate): fail open on infra errors instead of blocking

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,14 +184,30 @@ 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
     });
     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);
 }
 

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

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