garrytan / garrytan/gstack

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

Open
#2,769 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
133k
Forks
19.9k
Avg merge
18h 46m
Merged PRs (30d)
26

Description

`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:

```json
{"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

- #2496 ("…review mode runs unsandboxed and its pass/fail gate cannot fail")
and #2524 ("Two stated guarantees in the codex skill don't hold (v1.60.1.0)")
reported the *predecessor* defect: the gate was fail-**open**, so untagged or
errored output passed.
- Commit `3e7251a2` (2026-08-14, "fix(codex): sandbox the review path, fail the
gate closed, order timeouts wrapper-first") closed both by introducing this
five-check fail-closed gate. `review-mode.md` line 40 still cites
"(#2496, #2524)".
- #2496's own proposed fix specifically warned against the overshoot. Quoting
it: *"if the output carries no `[P1]` or `[P2]` tag at all **but does
describe findings**, report `GATE: UNKNOWN` rather than PASS."* The shipped
fix dropped that qualifier and made **all** untagged output FAIL.

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.

Contributor guide

Open the contributing guide

Research direction

Start with codex/sections/review-mode.md, especially the five-check gate and the P3 example around line 176. Compare the current checks with the quoted clean JSON, prose verdict, and P3-only cases, then consider #2448 before changing anything. Done means clean affirmative verdicts and P3-only reviews can pass while untagged findings remain fail-closed.

Written by the indexing model from the issue text.

Assessment

Tech stack
markdown
Domain
cli, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.