From 3e34219ac33b7fe5168fe079dcf000f91e67c362 Mon Sep 17 00:00:00 2001 From: Jonathan Zhang Date: Mon, 27 Jul 2026 21:29:51 -0700 Subject: [PATCH] ci(codex-review): decide the gate on a required VERDICT trailer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #132 failed closed whenever the verdict was not recognizably clean, and live verdicts show that reddens clean PRs — Codex phrases "nothing found" however it likes. Three real clean verdicts observed since: - "no regressions found" (canonical) - "No actionable regressions were identified" (paraphrase) - "The workflow consistently routes and validates ... while the regression tests cover the relevant ... cases." (no negative-finding phrase at all) The third is a genuinely clean review of domain-rank's opt-in PR; under #132's rule it would have failed that PR. A gate that reddens clean PRs gets switched off, which is how the comment-only signal it replaces died. The obvious inverse — treat any prose without a `regression:` token as clean — is worse, and Codex round 1 caught it: an off-format finding like "The new fallback branch has no test." would pass AND restore the auto-merge bypass. Both rules guess at English, so stop guessing. The prompt now requires the response to END with exactly `VERDICT: CLEAN` or `VERDICT: REGRESSION`, and that trailer decides: - trailer present -> it decides, except that listed findings outrank a CLEAN trailer; the trailer may not retract its own evidence. - no trailer but `regression:` findings -> regression. Covers verdicts predating this prompt, and a model that reports but forgets the line. - neither -> no_verdict, fail closed. Also covers empty, whitespace-only, a missing file, and the workflow's no-parseable-verdict sentinel. The trailer must be the FINAL line, or trailing prose slips past it (round 2). Nothing below it is forgiven: rounds 4-6 each broke a successive attempt to whitelist trailing CLI telemetry, by writing a real finding in the same shape ("Usage: the token refresh path has no test.", "Tokens used: 1 token can authorize every tenant"). Every such filter is a hole in the final-line rule, and it was defending a footer that does not exist — codex-cli 0.145.0 prints nothing after the response, verified against real output. So there is no filter. A future footer makes the gate go RED, which is loud and fixable against a real sample, rather than silently eating a finding. Two more fixes the trailer rule surfaced: - The gate now reads an UNtruncated verdict file. The 4KB cap is a prefix cut for the PR comment, so it dropped the trailer and failed long-but- clean reviews closed (round 3; reproduced: capped -> rc=1, full -> rc=0). - That cap truncates from a file rather than a pipe. `... | head -c` lets head exit first and the writer take SIGPIPE, which pipefail turns into a failed step; measured, that needs the verdict to outgrow the pipe buffer (fine at 128KB, aborts at 2MB), so it is hygiene, not a live failure. Also fixes a selftest diagnostic bug: the failure detail was written with no trailing newline, gluing each following result line onto it, so a line-anchored grep saw only the first failure and undercounted. Codex rounds: 8 (findings in 1-6; 7 and 8 clean). Past the usual cap of 6 deliberately: rounds 4-6 were the same finding against successive versions of the telemetry filter, and the resolution was deleting that filter rather than patching it again. Refs: topcoder1/domain-rank#80 Co-Authored-By: Claude Opus 5 --- .github/scripts/codex-verdict.mjs | 112 ++++++++++++++++------ .github/workflows/codex-review.yml | 46 +++++++-- selftest/README.md | 8 +- selftest/test_codex_verdict_gate.sh | 140 ++++++++++++++++++++-------- selftest/test_workflow_guards.py | 32 +++++++ 5 files changed, 258 insertions(+), 80 deletions(-) diff --git a/.github/scripts/codex-verdict.mjs b/.github/scripts/codex-verdict.mjs index 4d9da95..90c81bf 100644 --- a/.github/scripts/codex-verdict.mjs +++ b/.github/scripts/codex-verdict.mjs @@ -23,7 +23,7 @@ // Inputs (from env): // VERDICT_FILE path to the extracted verdict (default /tmp/codex.verdict) // FAIL_ON_REGRESSION "true" to enforce; anything else reports only -// GITHUB_OUTPUT (optional) — verdict_state=clean|regression|unparseable +// GITHUB_OUTPUT (optional) — verdict_state=clean|regression|no_verdict // GITHUB_STEP_SUMMARY (optional) — append markdown summary // // Exit code: 1 only when FAIL_ON_REGRESSION=true AND the verdict is not @@ -34,9 +34,9 @@ import { readFileSync, existsSync, appendFileSync } from 'node:fs'; const verdictFile = process.env.VERDICT_FILE || '/tmp/codex.verdict'; const enforce = (process.env.FAIL_ON_REGRESSION || '').trim().toLowerCase() === 'true'; -// A missing file means the review step did not produce a verdict. Treat it -// like an unparseable one (fail closed under enforcement) rather than -// silently passing — "no verdict" is exactly the state we cannot vouch for. +// A missing file means the review step never wrote a verdict. It reads as +// the empty string, which lands in `no_verdict` below — fail closed under +// enforcement rather than silently passing a review that did not happen. const raw = existsSync(verdictFile) ? readFileSync(verdictFile, 'utf8') : ''; // Normalize for matching only; `raw` is still what gets reported. Markdown @@ -61,41 +61,93 @@ const findings = [...text.matchAll(/\bregression\s*:\s*([^;]*)/g)] // nothing. Counting it would block a clean PR. .filter((d) => !NON_FINDING.test(d)); -// Codex frequently paraphrases the clean verdict rather than emitting the -// canonical string — the pre-review of this very change answered "No -// actionable regressions were identified", which an exact-match on -// "no regressions found" would have failed under enforcement. Match the -// negative-finding family instead: "no * * ". -// Widening this is safe in the direction that matters, because a verdict -// using the `regression:` form is already decided above. -const CLEAN = - /\bno\s+(?:\w+\s+){0,3}(?:regressions?|issues?|problems?|concerns?|findings?)\s+(?:\w+\s+){0,2}(?:found|identified|detected|observed|spotted|noted)\b/; +// THE TRAILER IS THE VERDICT. codex-review.yml's prompt requires the response +// to end with exactly `VERDICT: CLEAN` or `VERDICT: REGRESSION`, and that line +// is what decides here. Everything else in the response is prose for a human. +// +// This exists because BOTH prose-reading rules fail, and each was tried: +// +// - Requiring a recognizable clean phrase false-fails clean reviews. Codex +// phrases "nothing found" freely: "no regressions found", "No actionable +// regressions were identified", and one live verdict that just described +// the diff approvingly with no negative-finding phrase at all. A gate +// that reddens clean PRs gets switched off. +// - Treating any prose without a `regression:` token as clean lets an +// off-format finding through — "The new fallback branch has no test." +// reads clean and, worse, restores the auto-merge bypass. (Codex review +// round 1 caught exactly this on the previous revision.) +// +// One required output line is far more reliable to produce than a three-shape +// contract, and non-compliance is now a single crisp signal instead of a +// judgement call about English. +// It must be the FINAL non-empty line, because that is what the prompt asks +// for. Accepting the marker anywhere would let trailing prose slip past it: +// "VERDICT: CLEAN" followed by "Also, the new branch has no test." would read +// clean, which is the same off-format-finding hole in a new costume. A marker +// that is not last means the response did not follow the contract, so it +// falls through to no_verdict below and fails closed. (Codex review round 2.) +// +// Scanned against markdown-stripped text that still has its line structure — +// `text` above collapsed newlines for the finding scan, which would defeat a +// line anchor. +// DELIBERATELY NOT trimming trailing CLI telemetry here. codex-review.yml's +// awk extraction keeps everything after the last `codex` marker, so anything +// the CLI printed below the model's answer would become the final line, and +// three successive attempts to filter that out were each shown to swallow a +// real finding instead — "Usage: the token refresh path has no test.", +// "Tokens used: 1 token can authorize every tenant". Every such filter is a +// hole in the strict final-line rule, which is the actual security property. +// +// It was defending a footer that does not exist: codex-cli 0.145.0 prints +// nothing after the response (verified against real output 2026-07-27). +// +// If a future CLI does add one, the trailer stops being last, the gate reports +// no_verdict, and enforcing repos go RED. That is loud, immediately +// diagnosable, and fixed by one narrow filter written against the actual +// footer format. The alternative — a speculative filter that silently eats a +// finding — fails in the direction this gate exists to prevent. Do not re-add +// one without a real footer sample to match against. +const contentLines = raw + .replace(/[`*_]/g, ' ') + .split('\n') + .map((l) => l.trim()) + .filter((l) => l !== ''); +const lastLine = contentLines.length ? contentLines[contentLines.length - 1] : ''; +const trailerMatch = lastLine.match(/^verdict:\s*(clean|regression)\s*\.?$/i); +const trailer = trailerMatch ? trailerMatch[1].toLowerCase() : null; -// A hedge AFTER the clean phrase means Codex cleared the diff and then -// qualified it ("no regressions found, but the new branch is untested"). -// That is not a clean verdict this gate can vouch for, and because the -// qualifier skips the `regression:` form it would otherwise pass. Scoped to -// the text following the clean phrase so an incidental "but" earlier in the -// reasoning does not demote an otherwise clean answer. -const HEDGE = /\b(?:but|however|although|though|except|caveat)\b/; +// codex-review.yml writes this exact sentinel when its awk extraction comes +// up empty. +const NO_VERDICT_SENTINEL = /codex produced no parseable verdict/; -// Order matters: a verdict that lists findings is a regression even if the -// surrounding prose also contains a clean phrase somewhere. let state; -const cleanMatch = text.match(CLEAN); -if (findings.length > 0) { +if (trailer) { + // Authoritative. A response that lists findings and then claims CLEAN is + // still a regression — the findings are the evidence, the trailer is not + // allowed to retract them. + state = trailer === 'regression' || findings.length > 0 ? 'regression' : 'clean'; +} else if (findings.length > 0) { + // No trailer, but it reported in the finding form. Believe the findings. state = 'regression'; -} else if (cleanMatch && !HEDGE.test(text.slice(cleanMatch.index + cleanMatch[0].length))) { - state = 'clean'; } else { - state = 'unparseable'; + // No trailer and no findings: the response did not answer the contract, so + // there is nothing to vouch for. Fail closed. This also covers an empty or + // missing verdict file and the workflow's own no-parseable-verdict + // sentinel — a review that did not happen must never read as "clean". + state = 'no_verdict'; + if (text !== '' && !NO_VERDICT_SENTINEL.test(text)) { + console.log( + '::warning::Codex answered without the required `VERDICT:` trailer. ' + + 'Treating as no verdict.' + ); + } } const summary = { clean: 'Codex reported no regressions.', regression: `Codex reported ${findings.length} regression(s).`, - unparseable: - 'Codex produced no verdict this gate could classify — treated as unresolved.' + no_verdict: + 'Codex produced no verdict at all — the review did not happen, so nothing is vouched for.' }[state]; if (process.env.GITHUB_OUTPUT) { @@ -115,7 +167,7 @@ if (enforce && state !== 'clean') { const hint = state === 'regression' ? 'Address the finding, or reply on the PR explaining why it is declined and re-run this check.' - : 'Re-run the Codex review; if it keeps producing no parseable verdict, check the workflow logs.'; + : 'The review produced no verdict at all — re-run it, and check the workflow logs if it keeps coming back empty.'; console.log(`::error::Codex review did not come back clean (${state}). ${hint}`); process.exit(1); } diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml index 84515a7..760c749 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -22,9 +22,13 @@ on: posting it as a comment. Default false, which is the historical behavior every existing caller relies on. Opt in per repo — many Codex findings are advisories that are correctly declined, so - blocking by default would be wrong. When true, an unparseable - verdict also fails (fail closed): "no verdict" is precisely the - state this gate cannot vouch for. + blocking by default would be wrong. + + The verdict is read from the `VERDICT: CLEAN` / `VERDICT: + REGRESSION` trailer the prompt requires, not from prose. When true, + a response carrying neither that trailer nor a `regression:` finding + also fails (fail closed): a review that did not answer the contract + is precisely the state this gate cannot vouch for. Opting in also withdraws the auto-merge bypass on a bad review: claude-author-automerge.yml's Option B treats a SUCCESS conclusion @@ -163,10 +167,16 @@ jobs: JSDoc / comment block above it. If the diff is now inconsistent with the comment, flag it. - Output format - exactly one of: - no regressions found + Output format. If and only if you are reporting regressions, list + them first, one per line: regression: - - regression: - ; regression: - + + Then END YOUR RESPONSE with a final line that is EXACTLY one of: + VERDICT: CLEAN + VERDICT: REGRESSION + + That final line is machine-read. Emit it even when you have + nothing to report. Be brief. Do not perform general bug-hunting; the first pass already did." @@ -195,11 +205,28 @@ jobs: # token `codex` on its own line followed by the verdict. Grab the # LAST such block. verdict=$(awk '/^codex$/{flag=1; v=""; next} flag{v = (v=="" ? $0 : v ORS $0)} END{print v}' /tmp/codex.out) - # Truncate to 4KB for safety if [ -z "$verdict" ]; then verdict="(Codex produced no parseable verdict — see workflow logs)" fi - echo "$verdict" | head -c 4096 > /tmp/codex.verdict + + # TWO files, and the split is load-bearing. + # + # codex.verdict.full is what the gate reads. It is never truncated, + # because the machine-read `VERDICT:` trailer is the LAST line and a + # prefix cut would drop it — classifying an otherwise clean review + # as no_verdict and failing an enforced PR closed. (Codex review + # round 3.) + # + # codex.verdict is the 4KB-capped body for the PR comment, which is + # the only thing the cap was ever protecting. + printf '%s\n' "$verdict" > /tmp/codex.verdict.full + # head reads from a FILE here rather than a pipe. In the `... | head + # -c` form head exits at its byte limit and the writer takes SIGPIPE, + # which `set -o pipefail` above turns into a failed step. Measured: + # that needs the verdict to outgrow the OS pipe buffer (fine at + # 128KB, aborts at 2MB), so it is hygiene rather than a live failure + # at realistic verdict sizes — but reading the file costs nothing. + head -c 4096 /tmp/codex.verdict.full > /tmp/codex.verdict # Record which Codex model + CLI version actually ran. `npm install # -g @openai/codex@latest` (above) floats the CLI — and thus its @@ -247,6 +274,7 @@ jobs: - name: Evaluate Codex verdict if: steps.gate.outputs.should_run == 'true' env: - VERDICT_FILE: /tmp/codex.verdict + # The UNtruncated verdict — see the two-file note in the review step. + VERDICT_FILE: /tmp/codex.verdict.full FAIL_ON_REGRESSION: ${{ inputs.fail_on_regression }} run: node .github/scripts/codex-verdict.mjs diff --git a/selftest/README.md b/selftest/README.md index 645b3e6..86e089e 100644 --- a/selftest/README.md +++ b/selftest/README.md @@ -36,8 +36,12 @@ arbitrary helper scripts; that's a different kind of repo. merged with a reported missing-test regression unaddressed. Replays both verdicts verbatim. Pins the two properties that keep the gate safe for the other 26 consumers — enforcement happens only on the literal - `FAIL_ON_REGRESSION=true`, and under enforcement an unparseable or missing - verdict fails CLOSED. + `FAIL_ON_REGRESSION=true`, and under enforcement the `VERDICT:` trailer + the prompt requires is what decides — a response answering neither that + trailer nor the `regression:` form fails CLOSED. Both prose-reading rules + were tried first and both failed: requiring a recognizable clean phrase + false-failed a live clean review, and treating any prose without + `regression:` as clean let an off-format finding through. - `test_pr_files_listing.sh` — no reusable may fetch changed files via `gh pr diff` (HTTP 406 past 20k diff lines); pins the paginated files-API idiom instead. diff --git a/selftest/test_codex_verdict_gate.sh b/selftest/test_codex_verdict_gate.sh index c45c805..16aadaa 100644 --- a/selftest/test_codex_verdict_gate.sh +++ b/selftest/test_codex_verdict_gate.sh @@ -14,9 +14,13 @@ # 1. Enforcement is OPT-IN. Without FAIL_ON_REGRESSION=true the script # must always exit 0 — 26 other repos consume this reusable and a # default-on gate would start blocking their merges. -# 2. Under enforcement it fails CLOSED: an unparseable or missing verdict -# is a failure, because "no verdict" is exactly the state the gate -# cannot vouch for. +# 2. Under enforcement the `VERDICT:` trailer decides, and a response +# answering neither the trailer nor the `regression:` form fails +# CLOSED. Both prose-reading rules were tried first and both failed: +# requiring a recognizable clean phrase false-failed a live clean +# review, and treating any prose without `regression:` as clean let an +# off-format finding through. One required output line replaces a +# judgement call about English. # # Run from the repo root: # bash selftest/test_codex_verdict_gate.sh @@ -41,8 +45,11 @@ run() { set -e state=$(sed -n 's/^verdict_state=//p' "$tmp/ghout" | head -1) printf '%s %s' "$rc" "${state:-}" - # Surface the script's own output when a case fails. - printf '%s' "$out" > "$tmp/last_out" + # Surface the script's own output when a case fails. The trailing newline + # matters: without it the next result line is glued onto the end of this + # diagnostic, which both mangles the output and hides every failure after + # the first from a line-anchored `grep '^✗'`. + printf '%s\n' "$out" > "$tmp/last_out" } check() { @@ -67,50 +74,105 @@ check "domain-rank#74 verdict is report-only by default" "0 regression" "$(run f d79='regression: deploy/redeploy-code.sh:171 - no test exercises the new failure path when the resolved compose config cannot be read from the box' check "domain-rank#79 verdict fails under enforcement" "1 regression" "$(run true "$d79")" -# --- Clean verdicts must never fail ---------------------------------------- -# A false positive here blocks a good PR, so cover the spelling variants -# Codex actually emits rather than only the canonical string. -check "canonical clean verdict passes" "0 clean" "$(run true 'no regressions found')" -check "clean verdict with capitals and period passes" "0 clean" "$(run true 'No regressions found.')" -check "singular clean verdict passes" "0 clean" "$(run true 'no regression found')" -check "clean verdict with 'were' passes" "0 clean" "$(run true 'No regressions were found')" -check "bolded clean verdict passes" "0 clean" "$(run true '**no regressions found**')" -# Codex paraphrases far more often than it emits the canonical string. This -# is the verbatim verdict from the pre-review OF THIS CHANGE — an exact-match -# gate called it unparseable and would have failed a clean PR. +# --- The trailer decides ---------------------------------------------------- +# The prompt requires the response to END with `VERDICT: CLEAN` or +# `VERDICT: REGRESSION`. Prose above it is for humans. +check "CLEAN trailer passes" "0 clean" \ + "$(run true 'Reviewed the diff. +VERDICT: CLEAN')" +check "REGRESSION trailer fails" "1 regression" \ + "$(run true 'regression: a.py:1 - no test covers the new branch +VERDICT: REGRESSION')" +check "lowercase trailer passes" "0 clean" "$(run true 'ok +verdict: clean')" +check "bolded trailer passes" "0 clean" "$(run true 'ok +**VERDICT: CLEAN**')" +check "trailing whitespace on the trailer is tolerated" "0 clean" \ + "$(run true 'ok +VERDICT: CLEAN ')" + +# Prose Codex actually emits, all genuinely clean, each carrying the trailer. +# The middle one has no negative-finding phrase at all — an earlier revision +# that required a recognizable clean phrase failed its PR. check "paraphrased clean verdict passes" "0 clean" \ - "$(run true 'The opt-in verdict gate is correctly wired, fails closed under enforcement, and preserves advisory behavior by default. No actionable regressions were identified.')" -check "'no issues detected' passes" "0 clean" "$(run true 'No issues detected.')" -check "'no problems were observed' passes" "0 clean" "$(run true 'no problems were observed')" - -# A hedge after the clean phrase is not clean: Codex cleared the diff and -# then qualified it, and the qualifier never uses the `regression:` form, so -# nothing else would catch it. -check "hedged clean verdict fails closed" "1 unparseable" \ - "$(run true 'No regressions found, but the new fallback branch has no test.')" -check "'however' hedge fails closed" "1 unparseable" \ - "$(run true 'No issues found. However, the state mutation on line 40 is unasserted.')" -# ...while a hedge BEFORE the clean phrase is just reasoning prose. -check "hedge before the clean phrase stays clean" "0 clean" \ - "$(run true 'I considered the error path, but it is covered. No regressions found.')" + "$(run true 'The opt-in verdict gate is correctly wired. No actionable regressions were identified. +VERDICT: CLEAN')" +check "purely descriptive clean verdict passes" "0 clean" \ + "$(run true 'The workflow consistently routes and validates Codex review outcomes, while the regression tests cover the relevant success, failure, skipped, cancelled, draft, and unknown-class cases. +VERDICT: CLEAN')" + +# The trailer may not RETRACT findings. A response that lists regressions and +# then claims CLEAN is a regression — the findings are the evidence. +check "findings outrank a CLEAN trailer" "1 regression" \ + "$(run true 'regression: a.py:1 - unasserted state mutation +VERDICT: CLEAN')" +# Only the FINAL non-empty line counts, so a quoted example earlier in the +# prose cannot decide the verdict. +check "only the final line decides" "1 regression" \ + "$(run true 'The format asks me to answer VERDICT: CLEAN when nothing is wrong. +VERDICT: REGRESSION')" +# ...and prose AFTER the trailer means the contract was not followed. Reading +# the earlier marker would be the off-format-finding hole in a new costume. +check "prose after a CLEAN trailer fails closed" "1 no_verdict" \ + "$(run true 'VERDICT: CLEAN +Also, the new fallback branch has no test.')" +check "trailer with a trailing period passes" "0 clean" "$(run true 'ok +VERDICT: CLEAN.')" + +# NOTHING below the trailer is forgiven, and that is the decision, not an +# oversight. Three attempts to whitelist trailing CLI telemetry were each +# shown to swallow a real finding written in the same shape, so the gate now +# trusts only the last line. codex-cli 0.145.0 prints nothing after the +# response (verified against real output), and if a future CLI adds a footer +# these cases go no_verdict — RED and diagnosable — rather than silently +# promoting an earlier CLEAN marker. +check "a footer below the trailer fails closed, not open" "1 no_verdict" \ + "$(run true 'Reviewed. +VERDICT: CLEAN +tokens used: 12345')" +# The three real findings that killed each successive filter. All must fail. +check "prose below the trailer fails closed" "1 no_verdict" \ + "$(run true 'VERDICT: CLEAN +The token refresh path has no test.')" +check "a finding opening with 'Usage:' fails closed" "1 no_verdict" \ + "$(run true 'VERDICT: CLEAN +Usage: the token refresh path has no test.')" +check "a finding shaped like a token count fails closed" "1 no_verdict" \ + "$(run true 'VERDICT: CLEAN +Tokens used: 1 token can authorize every tenant.')" # Codex answering in the finding shape but reporting nothing must not block. check "'regression: none' is clean, not a finding" "0 clean" \ - "$(run true 'no regressions found -regression: none')" + "$(run true 'regression: none +VERDICT: CLEAN')" # --- Multi-finding format --------------------------------------------------- check "semicolon-separated findings fail" "1 regression" \ - "$(run true 'regression: a.py:1 - one; regression: b.py:2 - two')" + "$(run true 'regression: a.py:1 - one; regression: b.py:2 - two +VERDICT: REGRESSION')" + +# --- Fail closed when the contract is not answered -------------------------- +# Both live findings predate the trailer and carry none, so they also pin the +# fallback: a `regression:` line is believed even without a trailer. +check "findings without a trailer still fail" "1 regression" "$(run true "$d79")" + +# An off-format FINDING — no trailer, no `regression:` token. Reading this as +# clean is the hole Codex round 1 caught in the previous revision: it would +# have passed AND restored the auto-merge bypass. +check "off-format finding fails closed" "1 no_verdict" \ + "$(run true 'The new fallback branch has no test.')" +check "terse approval without a trailer fails closed" "1 no_verdict" \ + "$(run true 'Looks good to me.')" +check "clean phrasing without a trailer fails closed" "1 no_verdict" \ + "$(run true 'no regressions found')" -# --- Fail closed ------------------------------------------------------------ # The workflow writes this literal sentinel when awk extracts nothing. -check "workflow's no-verdict sentinel fails closed" "1 unparseable" \ +check "workflow's no-verdict sentinel fails closed" "1 no_verdict" \ "$(run true '(Codex produced no parseable verdict — see workflow logs)')" -check "empty verdict fails closed" "1 unparseable" "$(run true '')" -check "chatty off-format verdict fails closed" "1 unparseable" \ - "$(run true 'I reviewed the diff and it looks fine to me overall.')" +check "empty verdict fails closed" "1 no_verdict" "$(run true '')" +check "whitespace-only verdict fails closed" "1 no_verdict" "$(run true ' + ')" # ...but never in report-only mode. -check "unparseable verdict is report-only by default" "0 unparseable" \ +check "no_verdict is report-only by default" "0 no_verdict" \ "$(run false '(Codex produced no parseable verdict — see workflow logs)')" # A missing verdict file is not the same as an empty one — it means the diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index e0f04f4..90baf50 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -83,6 +83,38 @@ def test_codex_verdict_gate_is_wired_and_opt_in(): "codex-review.yml must fetch and run codex-verdict.mjs" ) + # The prompt and the parser are one contract. codex-verdict.mjs reads the + # `VERDICT:` trailer and fails closed without it, so a prompt edit that + # drops the instruction would fail every enforced repo's PRs — and it + # would look like Codex breaking, not like an edit here. + assert "VERDICT: CLEAN" in text and "VERDICT: REGRESSION" in text, ( + "the review prompt must require the VERDICT: trailer — codex-verdict.mjs " + "reads it and fails closed when it is absent" + ) + + # The gate must read the UNtruncated verdict. The `VERDICT:` trailer is the + # last line, and the 4KB comment cap is a prefix cut — pointing the gate at + # the capped file would classify any long-but-clean review as no_verdict and + # fail an enforced PR closed. (Codex review round 3.) + assert "VERDICT_FILE: /tmp/codex.verdict.full" in text, ( + "the verdict gate must read the untruncated verdict file — the 4KB cap " + "is for the PR comment and would drop the trailer the gate reads" + ) + # And the cap must truncate from a file, not a pipe. In the `... | head -c` + # form head exits at its limit and the writer takes SIGPIPE, which pipefail + # turns into a failed step. Measured, that needs the verdict to outgrow the + # OS pipe buffer (fine at 128KB, aborts at 2MB), so this is hygiene rather + # than a live failure at realistic verdict sizes — kept because the file + # form is free and this shell trap has bitten the fleet before. Comments are + # stripped so naming the anti-pattern in prose does not trip the guard. + code = "\n".join( + line for line in text.splitlines() if not line.lstrip().startswith("#") + ) + assert not re.search(r"\|\s*head -c", code), ( + "truncate from a FILE (`head -c N file`), never `... | head -c N` — " + "the early pipe close SIGPIPEs the writer and pipefail fails the step" + ) + comment_at = text.find("gh pr comment") evaluate_at = text.find("node .github/scripts/codex-verdict.mjs") assert comment_at != -1 and evaluate_at != -1