Skip to content

codex review gate: a clean review and a P3-only review can never PASS (overshoot of the #2496/#2524 fix) #2769

Closed

Description

@LimpingWolf

codex/sections/review-mode.md (v1.77.0.0) has a five-check fail-closed gate
in which PASS is only reachable through check 5, and check 5 requires at
least one severity tag. But codex review emits severity tags only inside
finding titles — so a review that finds nothing is legitimately untagged,
falls through to check 4, and fails.

Net effect: a clean diff cannot pass the gate. You need at least one [P2] to
be told you passed.

The two checks, quoted from v1.77.0.0

4. The output contains NO `[P0]`, `[P1]`, or `[P2]` tag (nor native `P0:`/`P1:`/
   `P2:` labels) anywhere → **GATE: FAIL** (fail-closed: untagged output — the
   severity markers this gate greps for are absent, so "no critical findings"
   cannot be verified mechanically; a human must read the verbatim output above
   and judge).
5. Severity tags are present and none is P0/P1 (only P2/advisory) →
   **GATE: PASS**.

followed by: "There is no default branch: PASS is only reachable through
check 5."

Evidence that untagged clean output is normal, not anomalous

Native JSON verdict from a completed run, verbatim:

{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"No actionable correctness issues were found in the changed classifier or its tests. The new handling preserves fall-through behavior for non-determinative Aard values while returning valid property type values.","overall_confidence_score":0.88}

Zero tags. Prose verdict from another completed run (exit 0, 181s):

"The bounded token matching fixes the reported false positives while
preserving intended cadastral classifications. The focused unit tests and
doctests pass, and no breaking regression was identified."

Zero tags. Both are correct, complete reviews of clean diffs. Both are
GATE: FAIL (fail-closed: untagged output) today.

Second defect: P3 is enumerated in neither check

[P3] appears in neither check 4's tag list nor check 5's — yet the same file,
at line 176, gives as a model recommendation:

Recommendation: Ship as-is because all 3 Codex findings are P3 cosmetic and the gate passed; addressing them would block the release without changing user-visible behavior.

The file describes a P3-only pass that its own gate cannot produce. P3 is not
hypothetical: [P3] appears in 428 of my local sessions (plus 304 native P3:
labels), and in 7 of those it is the only severity present — exactly the
P3-only body the example celebrates, and exactly the body check 4 fails.

Lineage — this is an overshoot, not a fresh bug

The fail-open → fail-closed swing was right. It just needs the middle case
back.

Not caused by the recent CLI upgrade — and not rare

The untagged clean-verdict shape is not new and not an edge case. Searching my
local Codex session history for "overall_correctness":"patch is correct":

month (2026) sessions with a clean verdict
May 26
June 44
July 29
August 12
September (1st) 2

113 sessions, earliest 2026-05-13 — months before 3e7251a2 and across
several CLI versions. Every one of them is untagged, so every one of them is a
GATE: FAIL under the current rules. The output format didn't change; the gate
did. Filing this separately from the timeout regression in #2608, which is
genuinely new (0.151.0's model swap).

Adjacent but distinct

#2669 ("every clean run reports a spurious failure") is a different bug — zsh
PIPESTATUS in challenge/consult mode; it explicitly states the review path is
unaffected. Not a duplicate.

Fix

#2448 ("tiered outside-voice review — cheap loop + frontier gate") replaces
free-form output parsing with a structured --findings-out contract, which
would obsolete this substring gate entirely. If that's the direction, this
issue is probably best closed by it rather than patched — treat the below as a
stopgap, not a proposal competing with it.

What I run locally: split check 4 in two, and add [P3] to both enumerations.

4. The output contains NO [P0]/[P1]/[P2]/[P3] tag (nor native P0:–P3: labels)
   anywhere. Codex's native review carries severity tags only inside finding
   titles, so a review that found nothing is legitimately untagged — absence of
   tags alone does not distinguish "clean" from "unverifiable". Split on whether
   the body makes an AFFIRMATIVE no-findings claim:
   a. The output affirmatively states it found nothing — native JSON with
      "findings": [], or an explicit sentence → GATE: PASS (clean — no
      findings). Quote the verbatim clean-verdict sentence or the "findings": []
      literal as the evidence.
   b. Neither severity tags NOR an affirmative clean verdict → GATE: FAIL
      (fail-closed: unverifiable output — a human must read the verbatim output
      above and judge).
5. Severity tags are present and none is P0/P1 (only P2/P3 advisory) →
   GATE: PASS.

This keeps the fail-closed property for the case #2496 actually cared about —
output that describes findings without tagging them, and output that says
nothing intelligible — while letting a review that explicitly reports nothing
report nothing. Happy to open a PR if the stopgap is wanted ahead of #2448.

Activity

  1. LimpingWolf commented on Sep 2, 2026

    @LimpingWolf
    Author

    Some lineage that may help route this, found while checking whether the defect
    had been reported before.

    This gate defect looks like one instance of a recurring class rather than a
    one-off. Every few CLI releases, something in the shell-and-grep integration
    stops holding:

    What moved How it surfaced
    #972 (Apr, closed Jul) codex CLI 0.120.0+ stdin deadlock in non-TTY contexts — hung every outside-voice skill
    #1196 (Apr, closed May) codex CLI 0.124.0-alpha.2 [PROMPT] and --base became mutually exclusive → exit 2 on every review
    #1036 (Apr, still open) nothing — chronic ~50% of /codex runs stall or truncate
    #2608 + this issue codex CLI 0.151.0 default model swap blew the 330s budget; gate can't pass a clean review

    Worth saying clearly: each of those was fixed properly at the time. < /dev/null
    landed for #972, the flag shape was corrected for #1196, stderr is captured to
    $TMPERR now instead of 2>/dev/null (codex/sections/challenge-mode.md:36
    still carries the "was: 2>/dev/null" note), and the timeout wrapper turns a
    stall into a diagnosable exit 124. None of that is the problem.

    The problem is that each fix patches a seam in the same coupling — invoke
    through a shell, parse free-form model prose with substring greps — and each new
    CLI release finds a different seam. In my own case nothing in gstack broke at
    all: the 330s budget was correct when it was written, and a model swap upstream
    silently invalidated it. Same for this gate. codex review didn't change its
    output contract; it never had one, so the greps were always going to be
    load-bearing guesses.

    That is the argument for #2448's
    structured --findings-out contract, and I think it subsumes both this issue and
    #1036's stall detection. A gate reading {"findings": []} doesn't need to
    distinguish "clean" from "unverifiable" by looking for [P2] in prose, and a
    caller with a structured result doesn't need to guess whether truncated output
    means success.

    Not asking to widen this issue's scope — the 4a/4b split still fixes the
    immediate breakage cheaply if a stopgap is wanted. Recording the pattern in case
    it affects how you'd rather sequence #2448 against point fixes.

  2. Richman711 commented on Sep 18, 2026

    @Richman711

    The same class persists in 1.87.4.0 (a6b3a57), in the shared validator this issue predates: lib/outside-review-result.ts, structured branch. It is used by /review Step 5.7 and /ship.

    const clear = /\bNO_FINDINGS\b|\bno (?:actionable |significant |new |concrete )?(?:bugs|issues|findings|problems)\b|\b(?:did not|didn't) (?:find|identify) any (?:actionable |new |concrete )?(?:bugs|issues|findings|problems)\b/i.test(text);

    The noun list omits regression(s). Measured 2026-09-19 (codex-cli 0.153.4, codex review --base <branch>, 479-line diff), the review completed with exit 0 in 183 s, ran the tests, and concluded:

    No actionable regressions were found relative to the specified base.

    The validator returned missing severity or explicit no-findings conclusion, so the skill recorded GATE: MISSING COVERAGE for a complete, clean review. The second verdict quoted above ("no breaking regression was identified") fails the same regex. This is the clean-diff-cannot-pass problem again, moved from the prompt-level gate into the shared parser.

    Adding regressions? to the noun list fixes these two phrasings. A sturdier option: codex review can emit its native JSON verdict ({"findings":[], "overall_correctness": ...}, shown above), and an empty findings array is a machine-readable "clean" that needs no prose matching.

  3. capy-ai commented on Oct 2, 2026

    @capy-ai
    Contributor

    Fixed in v1.91.13.0 (#3013, merged as f30b7b7): the Codex review gate is PASS / FAIL / UNVERIFIED: untagged clean output is UNVERIFIED instead of FAIL, and P2/P3-only output passes. Run /gstack-upgrade, then start a new Claude Code session. Thanks for the report. Reopen if you still see it after upgrading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions