From 0a4f02b4fa1321016944f7ea21b6a724df22c5f7 Mon Sep 17 00:00:00 2001 From: igerber Date: Sat, 8 Aug 2026 12:55:25 -0400 Subject: [PATCH] chore(plan-review): raise codex ceiling 1200s -> 2400s The 1200s cap was calibrated to the plan-review runtimes Campaign 1 observed (up to ~430s), whose plans were far smaller than production ones. A dense ~500-line plan whose reviewer must verify claims against 15+ repo files runs 3-4x that: codex timed out on 6 of 9 rounds of a single v4 plan, every failure a genuine codex exec timed out after 1200.0s (model=gpt-5.6-sol, effort=xhigh) never an auth or availability error. Retrying is the wrong remedy - a failed attempt burns the full ceiling before the retry starts (one round retried once and timed out twice, 40 minutes for no review). This does not touch the graded engine. The 'do not change without re-validation' note binds CODEX_MODEL and CODEX_EFFORT (what the campaign graded); the ceiling is an operational knob, and the campaign itself ran unattended at 3600s, so 2400 is still below the value the graded configuration used. SKILL.md states the ceiling to the reader ("caps at Ns and exits 3"), so it is updated in the same commit - otherwise the skill's own instructions describe behavior the code no longer has. The existing contract test asserted only the code constant and could not catch that, so it now DERIVES both values and compares them instead of hard-coding the number twice; verified by injecting the old value and confirming the test fails. Trade-off worth stating: 2400s is a 40-minute interactive ceiling. It is a cap, not a wait - fast reviews still return fast - but a genuinely hung codex now takes twice as long to reach the loud single-Claude fallback. Accepted because the observed failures were clean timeouts on real verification work, not hangs. --- .claude/skills/plan-review/SKILL.md | 2 +- .claude/skills/plan-review/codex_review.py | 22 ++++++++++++++++++---- tests/test_plan_review_skill.py | 16 ++++++++++++++-- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/.claude/skills/plan-review/SKILL.md b/.claude/skills/plan-review/SKILL.md index e9318e3c..57bd442a 100644 --- a/.claude/skills/plan-review/SKILL.md +++ b/.claude/skills/plan-review/SKILL.md @@ -163,7 +163,7 @@ python3 ".claude/skills/plan-review/codex_review.py" \ - **Exit 0** → codex review is in `/review_b.md`; go to step 5. - **Non-zero (2 = codex absent, 3 = timeout/error)** → codex is unavailable; take the **Loud fallback** below and skip steps 5-6. A hung codex cannot - wedge the gate — `codex_review.py` caps at 1200s and exits 3. + wedge the gate — `codex_review.py` caps at 2400s and exits 3. ### 5. Merge + verify — Claude @ Opus diff --git a/.claude/skills/plan-review/codex_review.py b/.claude/skills/plan-review/codex_review.py index f79a70e0..2a0b11cd 100644 --- a/.claude/skills/plan-review/codex_review.py +++ b/.claude/skills/plan-review/codex_review.py @@ -28,10 +28,24 @@ CODEX_EFFORT = "xhigh" # The timeout is NOT the campaign value (the campaign ran unattended at # CODEX_TIMEOUT_S=3600). It is an interactive-gate ceiling: high enough to clear -# the plan-review runtimes observed in the campaign (up to ~430s) with room to -# spare, low enough to bound the interactive wait. A timeout is treated as codex -# being unavailable (exit 3 → LOUD single-Claude fallback), so err generous. -CODEX_TIMEOUT_S = 1200.0 +# real plan-review runtimes with room to spare, low enough to bound the +# interactive wait. A timeout is treated as codex being unavailable (exit 3 → +# LOUD single-Claude fallback), so err generous. +# +# Raised 1200 → 2400 on 2026-08-08. The old value was calibrated to the campaign's +# observed runtimes (up to ~430s), whose plans were far smaller than production +# ones: a dense ~500-line plan whose reviewer must verify claims against 15+ repo +# files ran 3-4x that, and codex timed out on 6 of 9 rounds of a single v4 plan +# (every failure a genuine `codex exec timed out after 1200.0s`, never an auth or +# availability error). Retrying is the wrong remedy — a failed attempt burns the +# full ceiling before the retry starts, and a round that retried once timed out +# twice for 40 minutes total. Plan review is simply the slow surface here: its +# reviewer reads the plan AND cross-checks every claim against the live tree. +# +# Changing this does NOT require engine re-validation: the "do not change without +# re-validation" note above binds CODEX_MODEL and CODEX_EFFORT (what Campaign 1 +# graded). The ceiling is an operational knob and is graded by nothing. +CODEX_TIMEOUT_S = 2400.0 def _load_openai_review(repo_root: str): diff --git a/tests/test_plan_review_skill.py b/tests/test_plan_review_skill.py index 5c5eaa57..305a9635 100644 --- a/tests/test_plan_review_skill.py +++ b/tests/test_plan_review_skill.py @@ -153,11 +153,23 @@ def test_reviewer_invocations_are_pinned(): codex = (_SKILL / "codex_review.py").read_text() assert 'CODEX_MODEL = "gpt-5.6-sol"' in codex assert 'CODEX_EFFORT = "xhigh"' in codex - assert "CODEX_TIMEOUT_S = 1200" in codex + assert "CODEX_TIMEOUT_S = 2400" in codex # and codex_review.py actually passes them to call_codex assert "model=CODEX_MODEL" in codex and "effort=CODEX_EFFORT" in codex assert "timeout_s=CODEX_TIMEOUT_S" in codex + # SKILL.md STATES the ceiling to the reader ("caps at Ns and exits 3"), so + # bumping CODEX_TIMEOUT_S without editing the prose leaves the skill's own + # instructions lying about its behavior. Derive both and compare rather than + # hard-coding the number twice, so this cannot drift on the next bump. + code_cap = re.search(r"^CODEX_TIMEOUT_S\s*=\s*([\d.]+)", codex, re.M) + assert code_cap, "CODEX_TIMEOUT_S assignment not found in codex_review.py" + doc_cap = re.search(r"caps at (\d+)s and exits 3", (_SKILL / "SKILL.md").read_text()) + assert doc_cap, "SKILL.md no longer states the codex cap - keep it or drop this pin" + assert float(doc_cap.group(1)) == float(code_cap.group(1)), ( + f"SKILL.md says {doc_cap.group(1)}s but codex_review.py caps at " f"{code_cap.group(1)}s" + ) + skill = (_SKILL / "SKILL.md").read_text() # BOTH Claude subagents (reviewer 1 AND merge) select model=opus — assert # each section separately, not just one match anywhere (round-4: a single @@ -321,7 +333,7 @@ def test_codex_review_forwards_pins_and_prints_sensitive_notice(tmp_path, monkey assert call["prompt"] == "RENDERED PROMPT" assert call["model"] == cr.CODEX_MODEL == "gpt-5.6-sol" assert call["effort"] == cr.CODEX_EFFORT == "xhigh" - assert call["timeout_s"] == cr.CODEX_TIMEOUT_S == 1200.0 + assert call["timeout_s"] == cr.CODEX_TIMEOUT_S == 2400.0 def test_codex_review_absent_returns_2_and_writes_nothing(tmp_path, monkeypatch):