diff --git a/plugins/codex/scripts/lib/stop-review-output.mjs b/plugins/codex/scripts/lib/stop-review-output.mjs new file mode 100644 index 000000000..e431e3b9e --- /dev/null +++ b/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}`; +} diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index 2346bdcf4..5e460d918 100644 --- a/plugins/codex/scripts/stop-review-gate-hook.mjs +++ b/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() { diff --git a/tests/stop-review-output.test.mjs b/tests/stop-review-output.test.mjs new file mode 100644 index 000000000..8d7ff47c8 --- /dev/null +++ b/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/); +});