fix(stop-gate): preserve review failure details

plugins/codex/scripts/lib/stop-review-output.mjs

@@ -0,0 +1,135 @@
+const STOP_REVIEW_FALLBACK = "Run /codex:review --wait manually or bypass the gate.";
+const MAX_DIAGNOSTIC_CHARS = 4000;
+
+function diagnosticText(value) {
+  if (value == null) {
+    return "";
+  }
+  if (typeof value === "string") {
+    return value.trim();
+  }
+
+  try {
+    const serialized = JSON.stringify(value, null, 2);
+    if (serialized) {
+      return serialized.trim();
+    }
+  } catch {
+    // Fall back to String for values that cannot be serialized.
+  }
+
+  return String(value).trim();
+}
+
+function truncateDiagnostic(value) {
+  const text = diagnosticText(value);
+  if (text.length <= MAX_DIAGNOSTIC_CHARS) {
+    return text;
+  }
+
+  const omitted = text.length - MAX_DIAGNOSTIC_CHARS;
+  return `${text.slice(0, MAX_DIAGNOSTIC_CHARS)}\n[output truncated: ${omitted} characters omitted]`;
+}
+
+function firstNonEmptyDiagnostic(...values) {
+  for (const value of values) {
+    const text = diagnosticText(value);
+    if (text) {
+      return text;
+    }
+  }
+  return "";
+}
+
+function stripKnownNodeWarnings(value) {
+  return diagnosticText(value)
+    .split(/\r?\n/)
+    .filter((line) => {
+      const trimmed = line.trim();
+      return (
+        !/^\(node:\d+\) \[DEP0190\] DeprecationWarning:/.test(trimmed) &&
+        !/^\(Use `node --trace-deprecation .*` to show where the warning was created\)$/.test(trimmed)
+      );
+    })
+    .join("\n")
+    .trim();
+}
+
+export function parseStopReviewOutput(rawOutput) {
+  const text = diagnosticText(rawOutput);
+  if (!text) {
+    return {
+      ok: false,
+      reason: `The stop-time Codex review task returned no final output. ${STOP_REVIEW_FALLBACK}`
+    };
+  }
+
+  const firstLine = text.split(/\r?\n/, 1)[0].trim();
+  if (firstLine.startsWith("ALLOW:")) {
+    return { ok: true, reason: null };
+  }
+  if (firstLine.startsWith("BLOCK:")) {
+    const reason = text.slice(text.indexOf("BLOCK:") + "BLOCK:".length).trim() || text;
+    return {
+      ok: false,
+      reason: `Codex stop-time review found issues that still need fixes before ending the session:\n${truncateDiagnostic(reason)}`
+    };
+  }
+
+  return {
+    ok: false,
+    reason: `The stop-time Codex review task returned an unexpected answer:\n${truncateDiagnostic(text)}\n${STOP_REVIEW_FALLBACK}`
+  };
+}
+
+export function parseStopReviewPayload(rawStdout) {
+  const stdout = diagnosticText(rawStdout);
+  let payload;
+  try {
+    payload = JSON.parse(stdout);
+  } catch {
+    const detail = stdout ? `\nOutput:\n${truncateDiagnostic(stdout)}` : "";
+    return {
+      ok: false,
+      reason: `The stop-time Codex review task returned invalid JSON.${detail}\n${STOP_REVIEW_FALLBACK}`
+    };
+  }
+
+  const output = firstNonEmptyDiagnostic(payload?.error, payload?.rawOutput);
+  if (output) {
+    return parseStopReviewOutput(output);
+  }
+
+  const detail = firstNonEmptyDiagnostic(stdout, payload);
+  return {
+    ok: false,
+    reason: detail
+      ? `The stop-time Codex review task returned no final output.\nPayload:\n${truncateDiagnostic(detail)}\n${STOP_REVIEW_FALLBACK}`
+      : `The stop-time Codex review task returned no final output. ${STOP_REVIEW_FALLBACK}`
+  };
+}
+
+export function formatStopReviewProcessFailure(result = {}) {
+  const status = Number.isInteger(result.status)
+    ? ` (exit ${result.status})`
+    : result.signal
+      ? ` (signal ${result.signal})`
+      : "";
+  const sections = [];
+  const spawnError = diagnosticText(result.error?.message ?? result.error);
+  const stderr = stripKnownNodeWarnings(result.stderr);
+  const stdout = diagnosticText(result.stdout);
+
+  if (spawnError) {
+    sections.push(`spawn error:\n${truncateDiagnostic(spawnError)}`);
+  }
+  if (stderr) {
+    sections.push(`stderr:\n${truncateDiagnostic(stderr)}`);
+  }
+  if (stdout) {
+    sections.push(`stdout:\n${truncateDiagnostic(stdout)}`);
+  }
+
+  const detail = sections.length > 0 ? `\n${sections.join("\n\n")}\n` : " ";
+  return `The stop-time Codex review task failed${status}.${detail}${STOP_REVIEW_FALLBACK}`;
+}

plugins/codex/scripts/stop-review-gate-hook.mjs

@@ -9,6 +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 { formatStopReviewProcessFailure, parseStopReviewPayload } from "./lib/stop-review-output.mjs";
 import { sortJobsNewestFirst } from "./lib/job-control.mjs";
 import { SESSION_ID_ENV } from "./lib/tracked-jobs.mjs";
 import { resolveWorkspaceRoot } from "./lib/workspace.mjs";
@@ -66,35 +67,6 @@ function buildSetupNote(cwd) {
   return `Codex is not set up for the review gate.${detail} Run /codex:setup.`;
 }
 
-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."
-    };
-  }
-
-  const firstLine = text.split(/\r?\n/, 1)[0].trim();
-  if (firstLine.startsWith("ALLOW:")) {
-    return { ok: true, reason: null };
-  }
-  if (firstLine.startsWith("BLOCK:")) {
-    const reason = firstLine.slice("BLOCK:".length).trim() || text;
-    return {
-      ok: false,
-      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."
-  };
-}
-
 function runStopReview(cwd, input = {}) {
   const scriptPath = path.join(SCRIPT_DIR, "codex-companion.mjs");
   const prompt = buildStopReviewPrompt(input);
@@ -118,25 +90,13 @@ function runStopReview(cwd, input = {}) {
   }
 
   if (result.status !== 0) {
-    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."
+      reason: formatStopReviewProcessFailure(result)
     };
   }
 
-  try {
-    const payload = JSON.parse(result.stdout);
-    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."
-    };
-  }
+  return parseStopReviewPayload(result.stdout);
 }
 
 function main() {

tests/stop-review-output.test.mjs

@@ -0,0 +1,87 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+
+import {
+  formatStopReviewProcessFailure,
+  parseStopReviewOutput,
+  parseStopReviewPayload
+} from "../plugins/codex/scripts/lib/stop-review-output.mjs";
+
+test("parseStopReviewOutput preserves the full multiline BLOCK explanation", () => {
+  const result = parseStopReviewOutput(
+    "BLOCK: Missing empty-state guard\nsrc/app.js:4 indexes the collection before checking its length.\nAdd a regression test."
+  );
+
+  assert.equal(result.ok, false);
+  assert.match(result.reason, /Missing empty-state guard/);
+  assert.match(result.reason, /src\/app\.js:4 indexes the collection/);
+  assert.match(result.reason, /Add a regression test/);
+});
+
+test("parseStopReviewOutput includes unexpected answers with bounded diagnostics", () => {
+  const refusal = `I cannot complete this review.\n${"detail ".repeat(1000)}`;
+  const result = parseStopReviewOutput(refusal);
+
+  assert.equal(result.ok, false);
+  assert.match(result.reason, /unexpected answer/i);
+  assert.match(result.reason, /I cannot complete this review/);
+  assert.match(result.reason, /output truncated: \d+ characters omitted/);
+});
+
+test("parseStopReviewPayload treats an empty error as absent", () => {
+  const result = parseStopReviewPayload(
+    JSON.stringify({ error: "", rawOutput: "BLOCK: The actionable reason" })
+  );
+
+  assert.equal(result.ok, false);
+  assert.match(result.reason, /The actionable reason/);
+});
+
+test("parseStopReviewPayload serializes structured errors", () => {
+  const result = parseStopReviewPayload(
+    JSON.stringify({ error: { code: "rate_limit", message: "Try again later" }, rawOutput: "" })
+  );
+
+  assert.equal(result.ok, false);
+  assert.match(result.reason, /rate_limit/);
+  assert.match(result.reason, /Try again later/);
+  assert.doesNotMatch(result.reason, /\[object Object\]/);
+});
+
+test("parseStopReviewPayload falls back to the complete payload when no detail field is populated", () => {
+  const result = parseStopReviewPayload(
+    JSON.stringify({ status: 1, threadId: "thr_failed", error: "", rawOutput: "" })
+  );
+
+  assert.equal(result.ok, false);
+  assert.match(result.reason, /no final output/i);
+  assert.match(result.reason, /thr_failed/);
+  assert.match(result.reason, /"status":1/);
+});
+
+test("parseStopReviewPayload includes the offending bytes for invalid JSON", () => {
+  const result = parseStopReviewPayload("not-json: connection closed");
+
+  assert.equal(result.ok, false);
+  assert.match(result.reason, /invalid JSON/i);
+  assert.match(result.reason, /not-json: connection closed/);
+});
+
+test("formatStopReviewProcessFailure reports both streams and removes the known DEP0190 noise", () => {
+  const result = formatStopReviewProcessFailure({
+    status: 1,
+    stderr: [
+      "(node:31720) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true.",
+      "(Use `node --trace-deprecation ...` to show where the warning was created)",
+      "broker disconnected"
+    ].join("\n"),
+    stdout: JSON.stringify({ status: 1, rawOutput: "THE REAL REASON" })
+  });
+
+  assert.match(result, /exit 1/);
+  assert.match(result, /stderr:\nbroker disconnected/);
+  assert.match(result, /stdout:/);
+  assert.match(result, /THE REAL REASON/);
+  assert.doesNotMatch(result, /DEP0190/);
+  assert.doesNotMatch(result, /trace-deprecation/);
+});