Codex Review Loop

Codex Review Loop

Claude develops, Codex reviews, Claude fixes — in a loop — the human reviews once at the end. The two reviewers catch different defect classes, and Codex reliably finds real bugs inside the fixes for earlier findings. 3+ rounds per change is normal, not a smell — it's the loop catching the-fix-has-a-bug class, the most expensive class to ship.

The loop

1. Build on a branch → tests green.
2. Round 0 — local pre-review of the built branch diff, when the Codex plugin is installed (see below). If the PR already exists, skip this step and continue at step 3 — Round 0 is the pre-PR round only. Otherwise: fix its real, in-scope findings — the same scope rules apply before a PR exists, and an out-of-scope one is tracked, not built (see Scope boundaries) — re-run the relevant test suite until green again — fixes invalidate step 1's green — then push the post-Round-0 HEAD and open the PR. A PR created by a non-pushing flow from the stale remote SHA omits the reviewed fixes.
3. Trigger: gh pr comment <PR> -R <owner>/<repo> --body "@codex review".
4. Pull findings from all three surfaces (see REFERENCE) — and from every reviewer bot on the PR, not just Codex (Copilot and friends post to the same surfaces; see "Other reviewer bots"). Verify each against HEAD — Codex re-posts stale + false-positive findings every round.
5. Fix the real, in-scope ones — each with a regression test, its own commit. React 👍 to real findings, 👎 to false positives (so the end-of-loop human review sees they were examined, not missed; where a surface takes no reaction, a written reply is that record). A real finding that is outside this PR's scope gets tracked, not committed — see Scope boundaries.
6. Re-trigger and repeat 3–5 until Codex says "Didn't find any major issues" against the current HEAD — or until every blocking finding (P0/P1/P2) it still returns at that HEAD is one you have already put in a terminal triage state and re-verified this round — non-blocking nits never gated the loop and do not now (see Convergence). Some findings can be re-posted for ever — a tracked out-of-scope defect is still in the tree, and a false positive stays false — so a clean verdict is not always reachable; that is the only way the loop ends without one.
7. Human reviews once, at the end. Never auto-merge a substantial PR without a nod.

Round 0 — local Codex pre-review

If the OpenAI Codex plugin for Claude Code (openai/codex-plugin-cc) is installed — check for the /codex:review command or codex:setup skill — run a local review round on the branch diff before opening the PR:

/codex:review --wait --base <default-branch> --scope branch

(/codex:adversarial-review accepts custom focus text and emits schema-validated findings.)

Round 0 runs through the plugin's slash commands only. Do not shell out to codex-companion.mjs directly from another skill: ${CLAUDE_PLUGIN_ROOT} resolves to the plugin whose own component is executing — never the Codex plugin — and even with the cache path resolved manually, a raw headless invocation has been observed to hang indefinitely (50+ minutes, no CPU) because the companion expects the slash-command session wiring.

Round 0 is an accelerator, never a blocker. If the review has not returned within ~10 minutes, or the plugin's commands are unavailable in the session, kill it and fall back to the no-Round-0 path: push the branch, open the PR (noting the skip in its body), then continue the cloud loop from step 3 — the cloud reviewer remains the convergence gate either way.

Triage its findings exactly like cloud findings: verify against the code, fix the real in-scope ones with regression tests, ignore false positives. There is no PR body to anchor scope against yet, so write the branch's goal down before triaging — see Scope boundaries.

Tracking differs in one respect during Round 0: there is nothing to reply to, so record the finding against the branch and name it in the PR body you write next, under what you deliberately did not fix — that line is the audit trail the reply would have been. Re-run the relevant test suite until it is green again — fixes invalidate the pre-Round-0 green. Then push the post-Round-0 HEAD, open the PR, and continue from step 3.

Why: the cloud bot's round-trip is minutes per round, and its early rounds are dominated by findings a local pass catches in seconds. The local and cloud reviewers share a model family, so a local pre-pass mostly de-duplicates the first cloud rounds rather than adding a new defect class — that is exactly the point: spend the cheap reviewer first.

What Round 0 is NOT:

• Not a convergence gate. It reviews local state, not the PR, and emits no authoritative clean verdict. Convergence is decided only by the cloud loop, per the Convergence section — a clean local review never justifies skipping it or merging.
• Not a substitute for the fix rules. Round-0 fixes follow the same discipline: regression test per code fix, fix-the-rule-not-the-line, own commit.

Convergence

Converged = Codex's latest review is against current HEAD, its findings have fully landed (see the race below), and every blocking finding (P0/P1/P2) has reached a terminal triage state. Do not declare convergence off a single comment surface — a PR clean on /reviews can still carry an un-triaged finding on the inline or issue surface.

There are four terminal states, not two. A finding is cleared when it is fixed, stale (already handled in an earlier round), refuted (a verified false positive, with the rationale written where the human review will see it), or tracked — real, out of this PR's scope, now recorded somewhere that outlives this checkout, with its thread saying so (see Scope boundaries). Tracked is a decision, not a deferral of one, so it clears the finding the way a fix does.

Two consequences worth stating, because both have burned rounds:

• A finding in a terminal state may come back — and that is what ends the loop when a clean verdict cannot. A tracked defect is still in the tree and a false positive stays false, so Codex can re-post either every round and never go clean. Answer it as before and move on; it never re-enters the fix loop. When every blocking finding live at HEAD is in a terminal state, the PR is converged without a clean verdict and step 6 stops there. Nits never gated the loop and do not now — tracking polish just to clear the list is the scope creep this skill spends a section on.

  This is the only exception to "Codex's clean verdict ends the loop", and it is narrow by construction: every blocking finding live at HEAD must be triaged this round — re-read at HEAD, not remembered from an earlier one — and carry its evidence. "I don't think that one matters" is not a terminal state, and neither is a finding you have not re-read.

• Tracking is not a way out of a finding you simply don't want to fix. It applies only where the scope table says it applies. A defect the diff introduced is in scope at any severity — tracking that is shipping a known bug, and the human review at the end is entitled to say so.

Codex posts its outcome on different surfaces depending on the result — poll BOTH or you will misread the loop:

A clean pass emits only an issue comment. If your poll watches pulls/N/reviews for a HEAD-matching commit_id, a clean PR reads as "never reviewed" forever — you'll re-trigger endlessly and wrongly conclude Codex is down/rate-capped. Convergence requires EITHER (a) an issues/N/comments Codex comment matching /didn.t find any major issues/i on/after your last push — the only unambiguous clean signal — or (b) a reviews entry at HEAD whose inline findings you have actually enumerated and triaged. Never gate convergence on the /reviews surface alone.

⚠️ The review-object / inline-comment race — this WILL bite you

Codex posts the review object first (state COMMENTED, body = a generic "💡 Codex Review — Here are some automated review suggestions" wrapper) and its inline review-comments land seconds-to-minutes later. A poll that fires inside that window sees a review at HEAD with zero inline comments, which looks exactly like a clean pass. It is not. Merging there ships the findings unfixed — including P0/P1s.

• A review whose body is the generic suggestions wrapper means findings exist. Go find them. An empty inline list at that moment is a race, not a verdict.
• Never conclude "0 findings" from a single poll. Either wait for the explicit clean-verdict issue comment, or re-poll ≥90 s later and require the live set to be stable across two consecutive polls — compared by the set of comment ids, not by path/line/body. Codex re-posts an identical-looking finding with a new id, so a text-only diff hides a fresh blocking finding inside a "stable" set.
• Always --paginate, on BOTH comment surfaces. pulls/N/comments (inline) pages at 30, and gh pr view --json comments silently truncates to comments(first: 100) — so its last is not the newest comment on a busy PR. Read the verdict from gh api --paginate repos/<o>/<r>/issues/<PR>/comments instead. In a multi-round review the newest blocking finding routinely lands past page 1, so an un-paginated fetch reads a converged PR that isn't one — the same false-convergence failure wearing a different disguise.
• Never filter inline comments by commit_id. Fetch all of pulls/N/comments and partition by line: line != null = live finding; line == null = stale/outdated (already handled in an earlier round). A live finding can carry a sha your filter didn't expect, and the commit filter drops it silently.
• commit_id and line are re-anchored; original_commit_id and original_line are not. GitHub moves an inline comment onto the current HEAD as the branch advances. So finding.commit_id == HEAD does not mean the finding is fresh — it may be an already-fixed comment that followed you. Use original_commit_id as the "raised at" anchor (REFERENCE §3), and treat an unchanged comment id as "not a new finding" even when its line moved.
• Ancestry proves CURRENT, never STALE. original_commit_id == HEAD ⇒ current, triage it. But an older original_commit_id only says some commit landed after — not that it touched this code, and not that it fixed the bug. An unrelated push, or a fix that missed, leaves the defect live. Auto-skipping on ancestry is how you ship the bug Codex handed you. When the anchor predates HEAD and line != null, read the code at HEAD — that is the only thing that settles it.
• A convergence check that can print nothing is broken. On a findings-only PR there is no Codex issue comment at all, so […] | last // empty empties the jq stream and the whole if/else never runs — the poll outputs silence, which reads identically to "the query is broken". Default the body (last.body // "") so the NOT-CLEAN branch always fires. Same false-convergence bug, wearing silence instead of a wrong answer.
• Poll surface (c) in the same breath as (a). A clean pass emits only an issue comment — no review object, no inline comment. If your poll watches inline findings alone, a green PR looks "still in review" forever and you never converge.
• Select the record inside jq; never tail raw body text. Codex bodies are multi-line, so --jq '…|.body' | tail -1 tails physical lines, not comments — it drops the Didn't find any major issues text and prints the trailing <details> block, so the verdict can never match. Do last inside jq and flatten newlines.
• Make sure your poll command actually runs. gh's built-in --jq accepts one jq expression, not jq CLI flags like --arg — passing it exits 1 and the check fails silently, so you read "no verdict" forever. Pipe gh's JSON into the real jq binary. A convergence check that can't fail loudly is worse than none.
• Never 2>/dev/null a convergence poll. Suppressing stderr converts the failure above — and any gh/auth/network/jq error — into false silence: an empty result that reads exactly like "no findings" and merges the bug unfixed. This is the real-world trigger of every false-convergence variant above. Let the poll's errors print and eyeball them; a convergence check must fail loud, never quiet. If you must separate streams, capture stderr and assert it's empty — don't discard it.
• The clean verdict must name the CURRENT HEAD. Codex's clean comment prints Reviewed commit: <sha>. A PR clean on commit A that then receives commit B still shows A's verdict — and B has no inline findings yet because Codex hasn't reviewed it. Pairing those two reads as "converged". Compare the verdict's SHA to HEAD; never trust the text alone.
• Corollary: never merge on a premature zero. If you have not seen either the clean-verdict text or a stable, triaged inline set, the review is still in flight.

Rules that keep it correct

• Verify vs HEAD first — by reading the code, not by arithmetic on shas. A finding raised on HEAD (original_commit_id == HEAD) is current — triage it. A finding raised on a strict ancestor of HEAD is undecided: a later commit may have fixed it, or may have been unrelated, or may have missed. Open the file at HEAD and look. Still exhibits the defect → current, fix it. Genuinely fixed → stale, do not re-fix (re-fixing churns the PR and restarts the loop). Present at HEAD but wrong → false positive — verify, 👎, leave it. Only a real, still-live finding re-enters the fix loop. (queries → REFERENCE.md §3)

• Tell stale from new by id + line. line: null or a re-anchored (unchanged) comment id = outdated/already-handled. A new comment id on the latest commit = a new finding.

• Every code fix ships a regression test — encode the failure mode so a later round can't silently re-break it. This is what stops the loop oscillating. Test where applicable: doc / copy / config-flag fixes have no unit test — don't invent a meaningless one.

• Fix the RULE, not the line — then grep to prove it. When a finding is about a claim, invariant or convention (a doc statement, a validation rule, a naming convention, a security caveat), the flagged line is one instance, not the bug. The bug is that the rule is taught in N places and you just fixed one. Before committing, grep every place that teaches the same rule and fix them all in the same commit — then re-grep and paste the empty result as your proof.

  This is the most expensive mistake in the loop, and the "no test → the next Codex pass is the check" instinct is exactly what causes it: it outsources the sweep to the reviewer, so you pay a full round per instance. Observed: a public-repos-are-free caveat was corrected in the one place Codex flagged, four rounds running — a single grep found five stale copies, including a REFERENCE line that directly contradicted a bucket added two commits earlier.

  A high round count on the same invariant is the tell. 3+ rounds finding different bugs is the loop working. 3+ rounds re-finding the same rule means you are patching pointwise — stop, sweep, and land it in one commit.

  Count findings by SOURCE, not just by rule — "different bugs" is not a clean bill of health. The test above clears you when each round finds something new, and that is the hole: findings can be genuinely distinct and still all trace to one artifact, in which case the artifact is the bug. Observed: seven rounds on a skill doc produced a mismatched sort, a NULL concatenation, a nullglob hole, and a swallowed exit status — four unrelated bugs by any normal reading, so the "different bugs = working" test said keep going. Six of the seven traced to one optional shell snippet, and all six were one class (a stage failing open). Deleting the snippet retired the class in a single commit; six rounds of patching had not. So tally each round's findings against the file, function, or block they came from — when one source keeps producing them, ask what that source is for and whether it earns its place, instead of fixing the next instance.

• Stay inside the project's constraints. Match its language/runtime version matrix, lint rules, framework, and conventions. A "fix" that breaks the CI matrix (e.g. a newer-language builtin on an older runtime) is itself a new finding — check the CI config before writing the fix.

• Surface owner decisions; don't guess. A finding whose fix is a product / design / security / API tradeoff goes to the human, not an autonomous guess. So does any fix that would widen the PR — see Scope boundaries.

• Escalate the mechanism by round 3–4, not round 7. The signal is a repeat: a second round patching the same invariant, or a new finding sharing a failure class with an earlier one (both fail open, both trust an unchecked input, both re-derive the same unsound proof). Note it the round you see it, and if the next round confirms the pattern, put the decision to the human — by round 3–4, not round 7. Co-location alone is not the signal: two unrelated bugs in one file usually just means a small PR, and the "different bugs = the loop working" test above still governs. What escalates is a repeated class or a proof that cannot hold, never a shared line range. When it is real, ask whether the proof mechanism is wrong rather than the patch: patching an unsound mechanism converges slowly or never, while replacing it converges in one commit. The redesign itself is the human's call — deleting or restructuring someone's code is the one decision the loop cannot make for itself, and noticing that a mechanism is wrong is not permission to replace it (see Scope boundaries). This applies to any change, not only the distributed-state kind below: a doc that ships a paste-able command owns that command's failure modes exactly the way code does, and one fail-open surface per pipeline stage is a mechanism problem, not a series of typos.

• Fixes get their own commit, naming the round, e.g. fix(auth): register category before abilities (Codex round-3 P1) — keeps the loop auditable.

Scope boundaries — the PR's subject is the diff

The loop's strength is also its failure mode: a reviewer asked "what is wrong here?" always answers something, and answering everything turns a three-file fix into a redesign. The scope is fixed before the first review round, and no round raises it.

The anchor is what the branch set out to do — the issue it closes, the task you were given, or two lines you write down before reviewing. Write it down before Round 0, which runs before any PR exists, and then keep it: the PR body restates that goal, it does not replace it. A body written to describe whatever the diff has become is not an anchor, it is a mirror.

Round 0 is where an unanchored scope does the most damage and is hardest to see afterwards: absorb an adjacent refactor there and it lands in the opening diff, a body written from the diff describes the expanded work as if it were always the plan, and every drift check below then compares the branch to that inflated baseline and finds nothing wrong. So when the body would have to claim more than the goal, that is the drift signal — growing the scope is the human's call, and you get a yes before the wider goal becomes the baseline.

Joining a PR that is already open — the loop's other entry point, where Round 0 is skipped and nothing was written down — means recovering the anchor before triaging anything: the linked issue or task, else the body as first opened, else one line from the human on what this PR is for. Don't reconstruct it from the opening diff — nothing tells you reliably where "opening" was — and don't adopt the current body by default, which on an already-expanded PR ratifies the drift this section exists to catch.

In scope — a defect the diff introduces, or one that makes the stated goal untrue. That is the whole list.

Out of scope by default — track it, don't build it:

Tracking is a real outcome, not a dodge: it costs a minute, keeps the finding from being lost, and leaves the PR reviewable. Two properties make it real — the record outlives this checkout (an issue or tracker item, else something pushed, else a comment on the PR itself quoting the finding; never a local note or an unpushed branch), and the finding's own thread says where it went, so the human review sees it was judged rather than dropped. Which button or endpoint that takes depends on the surface; that it is written down does not.

Keep each fix inside the blast radius of the change it repairs. The test is necessity, not membership in the opening diff: a fix may touch whatever repairing the defect actually requires — a caller the change broke, a new test file for the regression test, a doc stating the behaviour being corrected. What it may not do is carry passengers. A fix that introduces an abstraction the repair does not need, edits a file for reasons unrelated to the defect, or is substantially larger than the change it repairs, is a second PR wearing a fix's commit message — stop and put it to the human.

The tells, and what to do about them

Check these at the end of every round — they are cheap, and they catch drift while it is still one commit:

• The stated goal no longer describes the diff. The single most reliable signal. If the goal under-sells what the branch does, scope crept — revert the excess, or say so explicitly and let the human widen the goal. Never quietly rewrite the anchor to fit the diff: that erases the only evidence the drift happened.
• The diff grows every round. Fixes shrink or hold the diff as often as they grow it, so a monotonically growing one across 3+ rounds is expansion — unless each round's growth is a fix and its regression test, which is the loop working. Ask why a file joined the diff, not whether one did: "the regression test needed a new file" is an answer, "I was in there anyway" is the drift. Read the size from the PR itself (gh pr view <PR> --json changedFiles,additions,deletions) — it is already measured against the PR's base, and needs nothing of your local checkout.
• New dependencies or new configuration appear after round 1. Ask what put them there; the change did not need them at open.
• You are writing design rationale in a fix commit. If the commit needs a paragraph arguing for a new approach, it is a design decision — human's call, per the rule below.

Redesign is proposed, never performed

The "escalate the mechanism by round 3–4" rule above says when to notice that patching won't converge. It does not authorise the rewrite. When the mechanism looks wrong: stop the loop, write at most a paragraph — what keeps failing, why the current mechanism cannot hold, what you would replace it with, and what it costs — and hand it to the human. Then do what they say. Deleting or restructuring working code, and expanding the change to reach a better design, are the two decisions the loop is not allowed to make for itself.

The same boundary applies to the fix-the-rule-not-the-line sweep: it covers every place that teaches the same claim the finding is about, and is not licence for a general cleanup of the files it visits.

Rounds are for defects, not for polish

Convergence means no actionable finding at a blocking severity (P0/P1/P2) — not that the reviewer has run out of suggestions. A reviewer will keep producing nits indefinitely; a PR that only accumulates non-blocking polish across a round is done, and that polish is tracked, not built.

Severity decides when the loop may stop, never what is in scope. A defect this diff introduced is in scope at any severity, P3 included — fix it, with its regression test, exactly as you would a P1. What a low severity buys you is the right to stop looping for it: a real in-scope P3 is fixed on the round you see it, not carried; only a suggestion with no defect behind it is tracked or refuted. Shipping a known regression because the reviewer labelled it P3 is the failure this whole section is meant to prevent, wearing a severity badge. Round 0 obeys every rule in this section too, measured against the written-down goal rather than a PR body that does not exist yet — a local review before the PR exists is where an unbounded "improve it" pass is cheapest to start and most expensive to notice.

Reviewer failure modes

The reviewer is not an oracle — three failure modes will mislead the loop if you trust its latest word blindly:

• Right diagnosis, wrong prescription — verify the FIX, not just the finding. Codex is much better at spotting that something is broken than at knowing what this codebase should do instead. Its suggested remedy is a hypothesis; treat it exactly like its findings and check it against reality before you type it.

  Observed: it correctly warned that a cadence probe reading all runs would misclassify legitimate schedule workflows — a real bug — and prescribed filtering to --event push. Running that against the actual repos showed the two robot-backups it was meant to catch fire as event dynamic (default-setup code scanning), not push: the prescription would have silently deleted the only finding that was costing money. The correct fix was the inverse — exclude schedule, keep everything a push can trigger.

  So: accept the finding on evidence, then derive the fix yourself from the code. A remedy you can't reproduce a reason for is a remedy you haven't verified. Say so in the commit when you deviate — "Codex proposed X; checked against the repo, X drops the real case; did Y instead" — so the human review sees the reasoning, not a silent override.

• Codex contradicts its own earlier verdict (flip-flop). It can flag a value one round, and the next round flag the fix you just made — sometimes reversing itself outright (e.g. "change 1 → 5", then "change 5 → 1"). A reversal is not automatically correct. Re-verify against the code at HEAD, not Codex's newest claim; if the current value is what the code actually enforces, it's a false positive — 👎 with a one-line rationale and hold. Do not ping-pong the value to appease successive reviews.

• Transient errors are not verdicts. Codex Review: Something went wrong. Try again later… (and similar) means the review didn't run — it is neither "clean" nor a finding. Re-trigger with @codex review; never count it toward convergence, and don't conclude Codex is down after one. Your convergence match must require the actual clean-verdict text, so a transient message can't be mistaken for either outcome.

Other reviewer bots (Copilot etc.) — sweep them, don't gate on them

A repo often has more than one reviewer bot. Filter your polls by nothing narrower than "every bot that commented" — enumerate the distinct user.login values on the PR's comment surfaces and triage each bot's live findings. Observed: a poll filtered to codex|chatgpt silently ignored 24 live Copilot comments across a 19-round loop, including one that refuted a convergence argument the fixes relied on; several (a stderr-corrupts-JSON class, a doctrine hole) would have saved whole rounds had they been read when posted.

Division of roles:

• Codex is the only convergence gate. Its explicit clean verdict at HEAD ends the loop — as does the one documented exception in Convergence: every blocking finding still live at HEAD is in a terminal triage state, re-verified this round and carrying its evidence. Nothing else does.
• Copilot (and similar) are findings sources, never gates. They emit no clean-verdict signal — silence is indistinguishable from "hasn't reviewed" — so they cannot prove convergence. But every live finding of theirs must be triaged (fix / 👍 / 👎-with-rationale) before merge, same as a Codex finding. Add their triage to the convergence checklist, not to the convergence definition.

Design the evidence model before the code (distributed-state work)

When the change orchestrates distributed state — an external registry with no state query, suppressed webhook/event delivery, cancellations, re-runs — the review loop will grind through every hole in an improvised design one round at a time. Observed: a release-pipeline PR spent ~8 of 19 rounds retrofitting what an upfront hour would have specified. Before writing such code, write down:

• What durable artifact proves each state? ("a release exists" proved nothing; a marker written only after the irreversible step did.)
• Which evidence classes may trigger an irreversible action? Deterministic proof only; a failed command is not proof the remote didn't commit (two-generals).
• What does ambiguity do? Always preserve, never delete; sticky across retries — a later guard-failure never launders an earlier ambiguous attempt.

The "escalate the mechanism by round 3–4" rule above is at its sharpest here, because an improvised evidence model is precisely an unsound proof mechanism: every round retrofits one more hole, and the redesign that ends it is one commit.

Polling cadence

Poll the first time ~60–90 s after the trigger, not four minutes later. Codex often answers in about a minute. A fixed 4-minute wait optimises the wrong variable: it saves a little prompt cache and spends human time — the reviewer finishes, the PR sits idle, and the person watching sees the review land before you do and has to prod you. If the first poll is empty, back off (90 s → 2 min → 4 min); don't busy-poll a reviewer that is genuinely still thinking.

Measure, don't assume — but measure the moment the round becomes readable, not the moment something first appears. reviews[].submitted_at is the wrong clock for both outcomes: on a findings run it timestamps the wrapper, which lands before the inline comments (the race above), and on a clean run there is no review object at all. Calibrate the delay from your trigger comment to whichever signal actually ends the round:

• clean pass → the issue comment's created_at;
• findings → the created_at of the last inline comment in the stable set.

If a push isn't auto-re-reviewed (Codex reviews reliably on PR-open, less so on later pushes), re-trigger with a @codex review comment.

A single poll never decides the round. Because of the review-object/inline-comment race above, one poll showing "review at HEAD, no inline findings" is indistinguishable from "the findings haven't posted yet." Treat a round as read only after the clean-verdict issue comment, or after two consecutive polls (≥90 s apart) return the same live-finding set.

See REFERENCE.md for the exact gh commands — verify-vs-HEAD, the three finding surfaces, triggering, reacting — and a worked round.