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