ci(codex-review): decide the gate on a required VERDICT trailer - #133
Conversation
#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 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Risk class: This PR touches one of the blocked path categories from Auto-merge is refused by (This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.") |
|
Coverage Floor — mode:
|
|
No issues found. Trailer-based verdict logic is sound (findings outrank CLEAN, final-line-only, fail-closed), backwards-compat clean, and the new branches are well covered by selftests. |
…s it (#134) #133 made the `VERDICT:` trailer mandatory. **It failed the first real PR it ran on.** ## The evidence topcoder1/domain-rank#82, run `30330151877`, 2026-07-28T04:57Z. The prompt asking for the trailer is verifiably in that run's log: ``` Then END YOUR RESPONSE with a final line that is EXACTLY one of: VERDICT: CLEAN ``` `gpt-5.6-sol` at `reasoning=low` answered: > No regressions found in the requested coverage, state-mutation, or contract-drift axes. No trailer. The gate logged `Verdict: state=no_verdict; enforcing=true` and reddened a clean PR. Compliance with a free-text output contract cannot be assumed. That is now measured rather than argued — and it is the second time this gate has been corrected by real data instead of reasoning. ## What fails now Only signals that have actually proven reliable: - **a `regression:` line** — three for three on real findings (#74, #79, and CI runs) - **an explicit `VERDICT: REGRESSION` trailer**, when the model does comply - **no output at all** — empty, whitespace-only, missing file, or the workflow's own no-parseable-verdict sentinel. A review that did not happen must never read as "nothing found". Everything else passes, with a `::warning::` when the trailer is absent, so report-only callers make the fleet-wide compliance rate visible. That warning is what would justify requiring the trailer later, if compliance turns out to be high on other models. The prompt still asks for the trailer and it is still read from the final line when present. It is a bonus signal now, not a gate. ## The accepted risk, stated plainly **A finding phrased in neither form is missed.** I am not going to pretend otherwise, and the selftest asserts it by name (`off-format finding is missed (accepted risk)`) rather than quietly dropping the case. Every rule that caught that case also failed clean PRs, because the two are indistinguishable without reading English — *"The new fallback branch has no test."* and *"Looks good to me."* are both trailer-less prose. Blocking correct work is what gets a gate switched off, and then nothing is reviewed at all. A miss still reaches a human through the review comment. This is the honest trade, and #82 is the reason it is made in this direction. ## Verification - 36 selftest assertions executing the real script. The #82 verdict is replayed **verbatim** as the anchor case. - Full suite **45 passed**; actionlint, shellcheck, prettier clean. - Mutation-tested: findings-ignored → 8 failures, REGRESSION-trailer-ignored → 1, no_verdict-opened → 5. Restored byte-identical. Codex rounds: 2 Auto-merge rationale: **manual merge** — PRs to `ci-workflows` are always manual merge, and this touches `.github/workflows/**` on a reusable consumed by 27 repos via `@main`. Behavior is unchanged for every caller that does not set `fail_on_regression: true`; only domain-rank does, on an open PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Jonathan Zhang <topcoder1@Mac-Studio.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Follow-up to #132, before any repo opts in. #132's rule is wrong in practice and this fixes it.
The problem with #132
It failed closed whenever the verdict was not recognizably clean. Codex phrases "nothing found" however it likes — three real clean verdicts observed since #132 merged:
no regressions foundNo actionable regressions were identifiedThe workflow consistently routes and validates Codex review outcomes, while the regression tests cover the relevant … cases.The third is a genuinely clean review of domain-rank's opt-in PR. Under #132 it would have failed that PR. A gate that reddens clean PRs gets switched off — which is exactly how the comment-only signal it replaces died.
The obvious inverse — treat any prose without a
regression:token as clean — is worse, and the Codex pre-review caught it on round 1: an off-format finding like "The new fallback branch has no test." would pass and restore the auto-merge bypass.The fix: stop guessing at English
The prompt now requires the response to END with exactly
VERDICT: CLEANorVERDICT: REGRESSION, and that trailer decides.CLEANtrailer. The trailer may not retract its own evidence.regression:findings → regression. Covers verdicts predating this prompt, and a model that reports but forgets the line.no_verdict, fail closed. Also covers empty, whitespace-only, a missing file, and the workflow's own no-parseable-verdict sentinel.One required output line is far more reliable to produce than a three-shape contract, and non-compliance becomes a crisp signal instead of a judgement call about prose.
Nothing below the trailer is forgiven — that's the decision
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.", then "Tokens used: 1 token can authorize every tenant". Every such filter is a hole in the final-line rule, which is the actual security property.
And it was defending a footer that does not exist — codex-cli 0.145.0 prints nothing after the response (verified against real captured output; the only
tokens usedmatches were inside Codex's own prose describing the hypothetical).So there is no filter. If a future CLI adds a footer, the trailer stops being last and enforcing repos go red — loud, diagnosable, fixable against a real sample — rather than silently eating a finding. A comment says not to re-add one without a real footer to match.
Two more bugs the trailer rule surfaced
rc=1, full →rc=0.… | head -clets head exit first and the writer take SIGPIPE, whichpipefailturns into a failed step. Measured honestly: this needs the verdict to outgrow the pipe buffer (fine at 128KB, aborts at 2MB), so it is hygiene rather than a live failure — the file form is free.Verification
selftest/test_codex_verdict_gate.sh— 38 assertions, executes the real script. Replays the ci(risk-paths): exclude .env.example/template/sample from blocked glob #74/Required-checks audit: 2026-W22 #79 verdicts and all three live clean verdicts verbatim.test_workflow_guards.py— pinsdefault: false, the env binding, comment-before-evaluate ordering, that the prompt still demands the trailer (prompt and parser are one contract), that the gate reads the untruncated file, and the no-pipe truncation.I also found and fixed a vacuous-diagnostic bug in my own selftest: 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. My earlier mutation counts were undercounts — the tests were stronger than reported, not weaker.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 a fourth time.
Auto-merge rationale: manual merge — PRs to
ci-workflowsare always manual merge, and this touches.github/workflows/**on a reusable consumed by 27 repos via@main. Behavior is unchanged for every caller that does not setfail_on_regression: true; none do yet.🤖 Generated with Claude Code