diff --git a/.github/scripts/codex-verdict.mjs b/.github/scripts/codex-verdict.mjs new file mode 100644 index 0000000..4d9da95 --- /dev/null +++ b/.github/scripts/codex-verdict.mjs @@ -0,0 +1,121 @@ +// Codex verdict gate. Classifies the verdict text produced by +// codex-review.yml and, when the caller opts in, FAILS the job on a +// regression instead of merely commenting it. +// +// Why this exists: codex-review.yml has always posted Codex's verdict as a +// PR comment and exited 0 regardless of what the verdict 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 topcoder1/domain-rank#74 and #79 — both on +// 2026-07-27 — Codex reported a missing-test regression on each PR and both +// merged with the finding unaddressed. +// +// Opt-in by design: callers pass `fail_on_regression: true`. The default is +// false, so the reusable's existing consumers keep today's advisory +// behavior. Many Codex findings are advisories that get correctly declined, +// so blocking-by-default would be wrong. +// +// Secondary effect worth knowing when you opt in: claude-author-automerge.yml +// treats a SUCCESS conclusion on the configured `codex_check_name` as a +// bypass of the risk-tier manual-merge gate (Option B). It requires an +// explicit success, so a job that fails here also withdraws that bypass — +// a regression can no longer read as "second-model reviewed". +// +// 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_STEP_SUMMARY (optional) — append markdown summary +// +// Exit code: 1 only when FAIL_ON_REGRESSION=true AND the verdict is not +// clean. Report-only mode always exits 0. + +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. +const raw = existsSync(verdictFile) ? readFileSync(verdictFile, 'utf8') : ''; + +// Normalize for matching only; `raw` is still what gets reported. Markdown +// emphasis is stripped because Codex sometimes bolds its verdict line, which +// would otherwise break the `regression:` anchor. +const text = raw + .replace(/[`*_]/g, ' ') + .replace(/\s+/g, ' ') + .trim() + .toLowerCase(); + +// Codex is instructed to answer in one of three shapes: +// no regressions found +// regression: - +// regression: - ; regression: - +// Multi-finding verdicts are semicolon-separated, so split findings on `;`. +const NON_FINDING = /^(none|nothing|n\/a|na|no)\b/; +const findings = [...text.matchAll(/\bregression\s*:\s*([^;]*)/g)] + .map((m) => m[1].trim()) + .filter(Boolean) + // "regression: none" is Codex answering in the finding shape but reporting + // 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/; + +// 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/; + +// 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) { + state = 'regression'; +} else if (cleanMatch && !HEDGE.test(text.slice(cleanMatch.index + cleanMatch[0].length))) { + state = 'clean'; +} else { + state = 'unparseable'; +} + +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.' +}[state]; + +if (process.env.GITHUB_OUTPUT) { + appendFileSync(process.env.GITHUB_OUTPUT, `verdict_state=${state}\n`); +} +if (process.env.GITHUB_STEP_SUMMARY) { + appendFileSync( + process.env.GITHUB_STEP_SUMMARY, + `### Codex verdict gate\n\n- State: \`${state}\`\n- Enforcing: \`${enforce}\`\n- ${summary}\n` + ); +} + +console.log(`Verdict: state=${state}; enforcing=${enforce}; ${summary}`); +for (const f of findings) console.log(` - regression: ${f}`); + +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.'; + 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 b18c99f..84515a7 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -15,6 +15,25 @@ name: Codex Review (reusable) on: workflow_call: + inputs: + fail_on_regression: + description: >- + Fail this job when Codex's verdict is not clean, instead of only + 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. + + Opting in also withdraws the auto-merge bypass on a bad review: + 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 — so a failed job no longer + lets a regression read as "second-model reviewed". + required: false + type: boolean + default: false secrets: OPENAI_API_KEY: required: true @@ -43,13 +62,18 @@ jobs: # in the caller's risk-paths.yml lists bug-attractor files that bypass # the gate (they ALWAYS get reviewed regardless of size). See # .github/scripts/codex-gate.mjs for the decision rules. - - name: Fetch cost-gate script + - name: Fetch gate scripts env: GH_TOKEN: ${{ github.token }} run: | + set -euo pipefail mkdir -p .github/scripts - gh api repos/topcoder1/ci-workflows/contents/.github/scripts/codex-gate.mjs \ - --jq '.content' | base64 -d > .github/scripts/codex-gate.mjs + # Both scripts live in this reusable's repo, not the caller's + # checkout, so they have to be pulled at run time. + for s in codex-gate.mjs codex-verdict.mjs; do + gh api "repos/topcoder1/ci-workflows/contents/.github/scripts/$s" \ + --jq '.content' | base64 -d > ".github/scripts/$s" + done - name: Install gate deps run: npm install --no-save yaml@2 minimatch@10 @@ -211,3 +235,18 @@ jobs: echo "Model: \`$(cat /tmp/codex.model 2>/dev/null || echo unknown)\` · CLI: \`$(cat /tmp/codex.cliversion 2>/dev/null || echo unknown)\` · reasoning: \`low\`" } > /tmp/comment.md gh pr comment "$PR" --body-file /tmp/comment.md + + # Runs on every caller, enforcing only where `fail_on_regression: true`. + # Report-only mode is deliberate: it exercises the classifier against + # real verdicts fleet-wide and records the state in the step summary, + # so the false-positive rate is observable BEFORE any repo lets it + # block a merge. + # + # Ordered after the comment step so the finding is always visible on + # the PR even when this step then fails the job. + - name: Evaluate Codex verdict + if: steps.gate.outputs.should_run == 'true' + env: + VERDICT_FILE: /tmp/codex.verdict + 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 3041a58..645b3e6 100644 --- a/selftest/README.md +++ b/selftest/README.md @@ -30,6 +30,14 @@ arbitrary helper scripts; that's a different kind of repo. it would demote an unmatched path from the strict `standard` fallback into an auto-merge-eligible class (a PR adding `Tests/release.py` under `safe_test: ['tests/**']`). +- `test_codex_verdict_gate.sh` — `codex-verdict.mjs` classification and its + opt-in enforcement. Codex's verdict used to be a comment only: the job + exited 0 whatever it said, so domain-rank#74 and #79 (both 2026-07-27) each + 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. - `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 new file mode 100644 index 0000000..c45c805 --- /dev/null +++ b/selftest/test_codex_verdict_gate.sh @@ -0,0 +1,146 @@ +#!/usr/bin/env bash +# Guards codex-verdict.mjs — the gate that decides whether a Codex review +# verdict fails the job or merely gets commented. +# +# Background: codex-review.yml posted Codex's verdict as a PR comment and +# exited 0 no matter what it said. On topcoder1/domain-rank#74 and #79 (both +# 2026-07-27) Codex reported a missing-test regression on each PR; the check +# went green, nobody re-read the comment, and both merged with the finding +# unaddressed. The verdicts those two PRs actually produced are replayed +# verbatim below, so this test pins the real-world case and not just a +# synthetic one. +# +# Two properties matter and both are asserted: +# 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. +# +# Run from the repo root: +# bash selftest/test_codex_verdict_gate.sh +set -euo pipefail + +script="$PWD/.github/scripts/codex-verdict.mjs" +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT + +failed=0 + +# Run the gate over a verdict. Usage: run +# Echoes " ". Captures rc without tripping `set -e`. +run() { + local enforce="$1" verdict="$2" rc state out + printf '%s' "$verdict" > "$tmp/verdict" + : > "$tmp/ghout" + set +e + out=$(VERDICT_FILE="$tmp/verdict" FAIL_ON_REGRESSION="$enforce" \ + GITHUB_OUTPUT="$tmp/ghout" node "$script" 2>&1) + rc=$? + 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" +} + +check() { + local label="$1" want="$2" got="$3" + if [ "$got" = "$want" ]; then + echo "✓ $label" + else + echo "✗ $label — expected '$want', got '$got'" + sed 's/^/ /' "$tmp/last_out" 2>/dev/null || true + failed=1 + fi +} + +# --- The two verdicts that motivated this gate ------------------------------ +# Replayed verbatim (including the duplicated line the extractor produces). +# shellcheck disable=SC2016 # the backticks are literal verdict text, not substitutions +d74='regression: deploy/redeploy-code.sh:303 - No test forces either fallback `mv` failure and asserts that the new `failed=1` mutation produces an incomplete-restore status. +regression: deploy/redeploy-code.sh:303 - No test forces either fallback `mv` failure and asserts that the new `failed=1` mutation produces an incomplete-restore status.' +check "domain-rank#74 verdict fails under enforcement" "1 regression" "$(run true "$d74")" +check "domain-rank#74 verdict is report-only by default" "0 regression" "$(run false "$d74")" + +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. +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.')" +# 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')" + +# --- Multi-finding format --------------------------------------------------- +check "semicolon-separated findings fail" "1 regression" \ + "$(run true 'regression: a.py:1 - one; regression: b.py:2 - two')" + +# --- Fail closed ------------------------------------------------------------ +# The workflow writes this literal sentinel when awk extracts nothing. +check "workflow's no-verdict sentinel fails closed" "1 unparseable" \ + "$(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.')" +# ...but never in report-only mode. +check "unparseable verdict is report-only by default" "0 unparseable" \ + "$(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 +# review step never wrote one. Same fail-closed treatment. +set +e +VERDICT_FILE="$tmp/does-not-exist" FAIL_ON_REGRESSION=true node "$script" >/dev/null 2>&1 +rc=$? +set -e +check "missing verdict file fails closed" "1" "$rc" + +# --- Mixed verdict: findings win ------------------------------------------- +# If Codex hedges with both phrasings, the finding must decide. Otherwise a +# stray "no regressions found" in the reasoning tail would clear a real one. +check "clean phrase alongside a finding still fails" "1 regression" \ + "$(run true 'no regressions found in the tests, but regression: src/x.py:9 - state mutation unasserted')" + +# --- Enforcement is strictly opt-in ---------------------------------------- +# Anything other than the exact string "true" must leave the gate advisory; +# a caller that never sets the input gets an empty env var. +for val in "" "false" "FALSE" "0" "yes" "1"; do + got="$(run "$val" "$d79")" + if [ "${got%% *}" = "0" ]; then + echo "✓ FAIL_ON_REGRESSION='$val' does not enforce" + else + echo "✗ FAIL_ON_REGRESSION='$val' enforced — only the literal 'true' may" + failed=1 + fi +done +# ...and the boolean GitHub renders for `fail_on_regression: true` does. +check "FAIL_ON_REGRESSION='true' enforces" "1 regression" "$(run "true" "$d79")" +check "FAIL_ON_REGRESSION=' TRUE ' enforces" "1 regression" "$(run " TRUE " "$d79")" + +exit "$failed" diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index 0037749..e0f04f4 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -27,6 +27,7 @@ "selftest/test_automerge_riskfile_gate.sh", "selftest/test_classify_bracket_guard.sh", "selftest/test_classify_nocase.sh", + "selftest/test_codex_verdict_gate.sh", "selftest/test_pr_files_listing.sh", "selftest/test_prettier_scope_failsafe.sh", "selftest/test_prettier_symlink_filter.sh", @@ -40,6 +41,57 @@ def test_shell_selftest(script): assert proc.returncode == 0, f"{script} failed:\n{proc.stdout}\n{proc.stderr}" +def test_codex_verdict_gate_is_wired_and_opt_in(): + """codex-review.yml must enforce Codex's verdict only when asked to. + + The gate's logic is executed by selftest/test_codex_verdict_gate.sh; + this pins the workflow wiring around it, which that test cannot see. + + Three properties, each with a real failure mode: + + 1. `default: false`. This reusable has 27 consumers on `@main`. A + default-on gate would start failing their PRs on Codex advisories + the moment this merges. + 2. The input actually reaches the script. If the env binding is dropped + the script silently runs report-only forever — the opted-in repo + believes it is gated and is not. That is worse than no gate. + 3. The evaluation runs AFTER the comment is posted. Failing first would + leave the PR with a red X and no visible explanation of what Codex + found. + """ + text = (WORKFLOWS_DIR / "codex-review.yml").read_text() + + assert "fail_on_regression:" in text, ( + "codex-review.yml must declare the fail_on_regression input" + ) + # The default lives in the input block; check it is the false literal and + # not merely mentioned in the description prose. + m = re.search( + r"fail_on_regression:.*?^ default:\s*(\S+)", text, flags=re.S | re.M + ) + assert m, "could not read fail_on_regression's default" + assert m.group(1) == "false", ( + f"fail_on_regression must default to false, got {m.group(1)!r} — 27 " + "repos consume this reusable via @main" + ) + + assert "FAIL_ON_REGRESSION: ${{ inputs.fail_on_regression }}" in text, ( + "the input must be bound to the script's FAIL_ON_REGRESSION env var — " + "without it an opted-in repo runs report-only and believes it is gated" + ) + assert "codex-verdict.mjs" in text, ( + "codex-review.yml must fetch and run codex-verdict.mjs" + ) + + 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 + assert comment_at < evaluate_at, ( + "the verdict evaluation must run after the review comment is posted, " + "so a failing job still shows the reader what Codex found" + ) + + def test_codex_review_covers_every_automergeable_class(): """Codex must review every risk class that can merge unread.