ci(codex-review): let callers fail the job on a bad Codex verdict - #132
Merged
Conversation
codex-review.yml posted Codex's verdict as a PR comment and exited 0 whatever it said — its only `exit 1` is the 3000-file cost gate. A green check plus a comment nobody re-reads is not a gate. On domain-rank#74 and #79, both on 2026-07-27, Codex reported a missing-test regression on each and both merged with the finding unaddressed. Adds a `fail_on_regression` input (default false) and codex-verdict.mjs, which classifies the verdict as clean / regression / unparseable. The evaluation always runs and records its state in the step summary; only an opted-in caller turns that state into a failure. Default false is the point: 27 repos consume this reusable via @main, and many Codex findings are advisories that get correctly declined. Blocking by default would be wrong. Report-only mode also exercises the classifier against real verdicts fleet-wide, so its false-positive rate is observable before any repo lets it block a merge. Under enforcement it fails CLOSED — an unparseable or missing verdict is exactly the state the gate cannot vouch for. Secondary effect, deliberate: claude-author-automerge.yml's Option B treats a SUCCESS conclusion on codex_check_name as a substitute for the risk-tier manual-merge gate and requires an explicit success. A job that fails here therefore also withdraws that bypass, so a reported regression can no longer read as "second-model reviewed". NOT changed: a cost-gated SKIP still concludes success. Narrowing that is the separate fleet-wide decision already recorded in pr-codex-review.yml. 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. Classifier logic (findings-win ordering, NON_FINDING filter, hedge scoping, fail-closed on missing/unparseable) and opt-in enforcement wiring all check out; findings can't be misclassified clean and enforcement is strictly gated on the literal |
topcoder1
added a commit
that referenced
this pull request
Jul 28, 2026
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: | verdict | #132 verdict | |---|---| | `no regressions found` | clean | | `No actionable regressions were identified` | clean | | `The workflow consistently routes and validates Codex review outcomes, while the regression tests cover the relevant … cases.` | **unparseable → FAIL** | 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: 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 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 used` matches 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 - **The gate now reads an untruncated verdict.** The 4KB cap is a *prefix* cut for the PR comment, so it dropped the trailer and failed long-but-clean reviews closed. Reproduced: same verdict, capped → `rc=1`, full → `rc=0`. - **That cap truncates from a file, not a pipe.** `… | head -c` lets head exit first and the writer take SIGPIPE, which `pipefail` turns 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 #74/#79 verdicts and all three live clean verdicts verbatim. - `test_workflow_guards.py` — pins `default: 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. - Full suite **45 passed**; actionlint, shellcheck, prettier clean. - Mutation-tested throughout. Latest: trailer-accepted-anywhere → 2 failures; telemetry-filter-restored (both loose variants) → 2 each; findings-cannot-override → 1; prompt-drops-trailer → guard fails. 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-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`; none do yet. 🤖 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
codex-review.ymlposts Codex's verdict as a PR comment and exits 0 whatever it says — the onlyexit 1in the file is the 3000-file cost gate. A green check plus a comment nobody re-reads is not a gate.It cost us twice in one day. On
topcoder1/domain-rank(2026-07-27), Codex reported a missing-test regression on each of #74 and #79, and both merged with the finding unaddressed:regression: deploy/redeploy-code.sh:303 - No test forces either fallback mv failure…regression: deploy/redeploy-code.sh:171 - no test exercises the new failure path…Tracked as topcoder1/domain-rank#80.
Change
fail_on_regressioninput on the reusable, defaultfalse..github/scripts/codex-verdict.mjs— classifies the verdict asclean/regression/unparseable.Nothing opts in in this PR. Wiring
domain-rankup is a separate change once this lands.Why default-false, and why report-only still runs
27 repos consume this reusable via
@main. Many Codex findings are advisories that get correctly declined, so blocking by default would be wrong.Running the classifier in report-only mode everywhere is deliberate: it exercises the parser against real verdicts fleet-wide, so its false-positive rate is observable before any repo lets it block a merge. That already paid for itself — see below.
Fails closed
Under enforcement an unparseable or missing verdict fails. "No verdict" is precisely the state this gate cannot vouch for.
Secondary effect (deliberate)
claude-author-automerge.yml's Option B treats a SUCCESS conclusion oncodex_check_nameas a substitute for the risk-tier manual-merge gate, and requires an explicit success. A job that fails here therefore also withdraws that bypass — a reported regression can no longer read as "second-model reviewed".Not changed
A cost-gated SKIP still concludes
success. Narrowing that is the separate fleet-wide decision already recorded inpr-codex-review.yml(lines 91-94) and is not taken as a side effect here.Verification
selftest/test_codex_verdict_gate.sh— 30 assertions, executes the real script. Replays the ci(risk-paths): exclude .env.example/template/sample from blocked glob #74 and Required-checks audit: 2026-W22 #79 verdicts verbatim.test_workflow_guards.py::test_codex_verdict_gate_is_wired_and_opt_in— pinsdefault: false, the env binding (a dropped binding would leave an opted-in repo silently ungated), and that evaluation runs after the comment is posted.actionlintclean,shellcheckclean,prettier --checkclean.A false positive the pre-review caught in itself: round 1 answered "No actionable regressions were identified" — which an exact-match on
no regressions foundclassified asunparseable. Under enforcement that would have failed a clean PR.CLEANnow matches the negative-finding family, with a hedge guard so "no regressions found, but X is untested" does not read as clean. Both behaviors are tested, and round 2's live verdict now classifiescleanthrough the real script.Codex rounds: 2
Auto-merge rationale: manual merge — PRs to
ci-workflowsare always manual merge, and this touches.github/workflows/**(production infrastructure) on a reusable consumed by 27 repos via@main.🤖 Generated with Claude Code