Cap consecutive FAIL review rounds (CODEX_REVIEW_MAX_ROUNDS)

[martingtobias]

Problem

CODEX_REVIEW_MAX_LOOPS is a Stop-hook safety valve — it only counts when Claude tries to end its turn on a FAIL. A fix-and-amend cycle that stays inside a single turn never increments it, so a review saga that keeps surfacing new findings could run effectively unbounded, re-reviewing after every amend.

In practice that means a long tail of diminishing-return rounds on one commit, with no natural exit.

Fix

Add CODEX_REVIEW_MAX_ROUNDS (default 8): consecutive FAIL verdicts since the last PASS. The counter lives at .git/codex-review-round-count and survives amends — each amend is a new sha but the same saga, which is exactly the case MAX_LOOPS misses.

At the cap the hook stops blocking rather than hard-failing:

• findings demote to advisory context (hookSpecificOutput, not decision: block)
• state and both counters clear, so the Stop hook releases too
• the verdict is still recorded as FAIL in history
• the next natural commit starts a fresh saga

Garbage env values fall back to the default, matching how the other numeric knobs in this script are guarded.

Tests

Four new cases in tests/post-commit-review.bats, plus a round_counter_file helper:

• FAIL below the cap blocks and records the round (fix round 1 of max 8 in the reason)
• cap demotes findings to advisory, clears state and the round counter, still logs FAIL
• garbage CODEX_REVIEW_MAX_ROUNDS falls back to the default
• PASS clears the round counter

bats tests/ — 53/53 pass (1 pre-existing skip: coreutils timeout unavailable on this machine).

Note

Touches the same FAIL-state write as #1, so whichever merges second needs a one-line conflict resolution. The two changes are independent in intent and can be reviewed separately.

[andreidavid]

Thanks for this — the diagnosis is exactly right: CODEX_REVIEW_MAX_LOOPS only counts turn-end attempts, so a same-turn fix-and-amend cycle was unbounded, and since every round is a billed review, this caps cost as well as time. Demoting to advisory at the cap instead of hard-failing is the right ergonomics, and the tests match the house style closely.

I'm merging this manually with the one-line resolution you predicted in the PR body (#1 lands first, so the FAIL-state write in the blocking arm picks up the session field).

One refinement I'll push in a follow-up PR rather than bounce back to you: the round counter never goes stale — it's only cleared on PASS or at the cap, so an abandoned saga (interrupt, kill switch, manual fix outside a session) leaves the count behind, and unrelated FAILs weeks later resume from it and can hit the cap prematurely. The fix will stamp the counter with an epoch and reset counts older than the existing one-hour staleness window. I'll link it here in case you're curious.

Both PRs were a pleasure to review — much appreciated.

[andreidavid]

Follow-up as promised: #4 — stamps the round counter with the epoch of the last FAIL so an abandoned saga's count expires after an hour instead of leaking into unrelated future reviews (a future-dated stamp from a backward clock step also reads as stale), adds "capped": true to history entries for FAILs demoted at the cap, and lands the README/CHANGELOG for both of your PRs in the 1.6.0 release. Thanks again!