fix(stop-review-gate): fail open on infra errors instead of blocking#422
fix(stop-review-gate): fail open on infra errors instead of blocking#422fangkangmi wants to merge 2 commits into
Conversation
…penai#248, openai#306) The Stop review gate treated every non-ALLOW outcome as a review BLOCK: genuine `BLOCK:` verdicts AND transient infrastructure failures (timeout, non-zero exit / rate limit, invalid JSON, empty or garbled output) all returned `{ok:false}`, and `main()` mapped every `!ok` to `decision:"block"`. A transient Codex failure therefore blocked the Stop event, Claude re-woke, the review re-ran, hit the same failure, and looped indefinitely — burning tokens with no useful review until the user disabled the gate (openai#306 is the same root cause via the rate-limit path). Split the review result into three outcomes — allow / block / infra_error. Only a real `BLOCK:` verdict emits a Stop `block`. Infrastructure failures now fail OPEN: warn on stderr and let the session end, so the rewake loop can't form. Genuine ALLOW/BLOCK behavior is unchanged. Complementary to the valid-JSON (openai#352) and opt-in round-cap (openai#396) PRs. Adds a `stop-gate-infra` fake-codex behavior (thread/start raises a 429) and a regression test asserting the hook fails open on an infra error.
…infra reasons Two hardenings from review of the fail-open change: 1. The companion prints its JSON payload before setting a non-zero exit code, so a review turn that fails AFTER streaming its final verdict still carries "BLOCK: ..." in rawOutput. runStopReview now parses the payload on the non-zero-exit path and honors a present BLOCK verdict instead of discarding it as an infra error. True infra failures (429, auth, crash, timeout) produce no verdict and still fail open, so the openai#248/openai#306 loop protection is unchanged. 2. Infra reason fragments are spliced into the stderr warning sentence; period-terminated error details produced "..". Trim and strip the trailing period before splicing. Adds a `stop-gate-block-then-error` fixture behavior (verdict streamed, then the turn ends "failed") plus a regression test that the salvaged verdict blocks, and a no-double-punctuation assertion on the infra test.
rajpratham1
left a comment
rajpratham1
left a comment
There was a problem hiding this comment.
This is a solid reliability fix. It changes the stop-review gate to fail open on infrastructure failures while still honoring genuine BLOCK verdicts, preventing infinite stop-hook loops.
Strengths
Correctly distinguishes review outcomes
ALLOW
BLOCK
INFRA_ERROR
This is much clearer than the previous boolean ok/not ok behavior.
Prevents infinite retry loops
Timeouts
429 rate limits
authentication failures
invalid JSON
empty responses
now allow the stop instead of repeatedly re-running the review.
Doesn't weaken real review protection
The important logic is:
const salvaged = tryParseVerdict(result.stdout);
if (salvaged?.outcome === REVIEW_BLOCK) {
return salvaged;
}
If the review already emitted a BLOCK before crashing, the session is still blocked.
That preserves safety while only failing open for genuine infrastructure failures.
User-facing messages are cleaner
strips trailing periods
avoids ".. Allowing..."
Test coverage
The new tests cover the important regressions:
✅ infrastructure failure → allow stop
✅ streamed BLOCK then process failure → still block
✅ 429/rate-limit behavior
✅ message formatting regression
These directly validate the new behavior.
Minor suggestion
Instead of several repeated constructions like
{
outcome: REVIEW_INFRA_ERROR,
reason: ...
}
a small helper (e.g. infraFailure(reason)) could reduce duplication, but this is only a maintainability improvement.
2 participants
Fixes #248
Fixes #306
Problem
The Stop review gate maps every non-
ALLOWoutcome to a blocking decision: a genuineBLOCK:verdict, but also timeouts, non-zero companion exits (e.g. rate limits), invalid JSON, and empty/unexpected output all returned{ok:false}, andmain()emitted{"decision":"block"}for any!ok.When Codex has a transient infrastructure failure, this loops: the hook blocks → Claude re-wakes → the stop-time review re-runs → hits the same failure → blocks again — burning tokens indefinitely with no useful review until the user disables the gate. #306 is the same root cause reached via the rate-limit path.
Fix
Split the review result into three outcomes —
allow/block/infra_error:BLOCK:verdict emits a Stopblock(reason text unchanged).ALLOW:behavior is unchanged.Two hardenings on top:
BLOCK: …inrawOutput. The non-zero-exit path now parses the payload and honors a presentBLOCKverdict instead of discarding it. True infra failures produce no verdict and still fail open, so the loop protection is unaffected."..").Complementary to #352 (always-valid stdout JSON) and #396 (opt-in round cap) — neither addresses the infra-vs-verdict conflation itself.
Testing
stop-gate-infra(thread/start raises a 429) andstop-gate-block-then-error(verdict streamed, then the turn ends"failed").".."), and still blocks when a failed turn carried a streamedBLOCKverdict.CODEX_HOME→ empty dir): pre-fix hook emitted{"decision":"block", …}(the loop trigger); this branch fails open with the stderr warning.ALLOW:→ no block; deliberately broken work → model returnedBLOCK:(it independently found the planted regression) → block emitted, as before.