From 8c098a744518ba6a0d39847b184f57eebe783d83 Mon Sep 17 00:00:00 2001 From: ProtocolWarden Date: Fri, 17 Jul 2026 16:12:19 -0400 Subject: [PATCH] =?UTF-8?q?feat(reviewer):=20D1=20=E2=80=94=20run=20ordina?= =?UTF-8?q?ry=20reviews=20on=20codex=20when=20claude=20is=20cooled=20(back?= =?UTF-8?q?end-agnostic=20fallback)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The self-review sweep's defer branch already flagged this as "a validated follow-up": until now, any non-claude backend selection meant claude — the only review backend — was unavailable, so the ordinary review PARKED. This gives the ordinary single-reviewer the controller's full claude→codex ladder instead: - claude runnable → review on claude/haiku (unchanged path). - claude cooled, codex up → DIVERT the ordinary review to codex_cli/codex, feeding its verdict into the SAME downstream pipeline (verdict parse → self-heal ladder → LGTM-only green-CI merge). Charges codex's budget. - whole ladder exhausted → PARK (defer+return, no burn); #446 auto-resume. backend→model reuses the validated council seat pairing (verdict._COUNCIL_PANEL, codex_cli→codex) via a small _review_model_for_backend helper — no new registry. The claude path keeps routing through _run_direct_review (the name the test suite patches; back-compat intact); the codex path branches to the already backend-agnostic _run_member_review. Downstream _dispatch_verdict_outcome was already backend-agnostic — no claude-specific assumption on the ordinary path. GUARDRAIL PRs are untouched: they fork to the K=3 cross-family council BEFORE this gate, so a cooled claude still genuinely PARKS the council (F14) rather than falling back to a single codex reviewer — pinned by a new test. Codex was validated as the ordinary reviewer by the 2026-07-15 spike (schema verdict.json, caught a planted bug with the exact span, did not author the overall verdict — INJ boundary held) and runs live as council seat C3. Tests: 3 root integration (codex-runs / ladder-exhausted-parks / guardrail-not-single-reviewed) + 3 unit under tests/unit/reviewer. 169 reviewer + 8536 unit green; ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .console/log.md | 24 ++++ .../entrypoints/pr_review_watcher/main.py | 91 ++++++++++++--- tests/test_pr_review_watcher.py | 104 ++++++++++++++++++ tests/unit/reviewer/test_d1_codex_fallback.py | 39 +++++++ 4 files changed, 243 insertions(+), 15 deletions(-) create mode 100644 tests/unit/reviewer/test_d1_codex_fallback.py diff --git a/.console/log.md b/.console/log.md index 05306f9b0..5a8c7f676 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,27 @@ +## 2026-07-17 — feat(reviewer): D1 — run ordinary reviews on codex when claude is cooled + +Built the validated follow-up the code itself flagged (self-review sweep defer +branch): give the ORDINARY single-reviewer the controller's full claude→codex +LADDER instead of parking whenever claude is unavailable. At the sweep's +backend-selection gate: claude runnable → review on claude/haiku (unchanged); +claude cooled but codex runnable → DIVERT this review to codex_cli/codex (charges +codex's budget, not claude's) and feed its verdict into the SAME downstream +pipeline (verdict parse → self-heal ladder → LGTM-only green-CI merge); whole +ladder exhausted (no runnable backend) → PARK (defer+return, no burn) preserving +#446 auto-resume. backend→model reuses the validated council seat pairing +(`verdict._COUNCIL_PANEL`, codex_cli→codex) via a tiny `_review_model_for_backend` +helper — no new registry. The claude path still routes through `_run_direct_review` +(the name the suite patches, back-compat intact); the codex path branches to the +already-backend-agnostic `_run_member_review`. GUARDRAIL PRs are untouched — they +fork to the K=3 council BEFORE this gate, so they still genuinely PARK when a +family is cooled (F14), never single-reviewed on codex (pinned by a new test). +Downstream `_dispatch_verdict_outcome` was already backend-agnostic (plain +{result, failing_checks, summary} shared with the council) — no claude-specific +assumption found on the ordinary path. Tests: 3 root integration (codex-runs / +ladder-exhausted-parks / guardrail-not-single-reviewed) + 3 unit +(tests/unit/reviewer/test_d1_codex_fallback.py: model pairing, unknown→None, +back-compat alias). Full: 169 reviewer + 8536 unit green; ruff clean. + ## 2026-07-15 — feat(reviewer): ACTIVATE the council — populate guardrail_paths (§G1) The council's go-live. C1/C2/C3 all merged; `reviewer.council.guardrail_paths` diff --git a/src/operations_center/entrypoints/pr_review_watcher/main.py b/src/operations_center/entrypoints/pr_review_watcher/main.py index f4ba2252c..b76abd241 100644 --- a/src/operations_center/entrypoints/pr_review_watcher/main.py +++ b/src/operations_center/entrypoints/pr_review_watcher/main.py @@ -687,6 +687,23 @@ def _run_direct_review( return _run_member_review(oc_root, goal_text, state_key, backend="claude_code", model="haiku") +def _review_model_for_backend(backend: str) -> str | None: + """Model to pair with a fallback review backend for the ordinary review (D1). + + Sourced from the validated council panel (``verdict._COUNCIL_PANEL``) so the + ordinary-review claude→codex fallback reuses the SAME ``(backend, model)`` + pairing already proven safe as a live council seat (``codex_cli`` → ``codex``) + — no new, unvalidated pairing is introduced. Returns ``None`` when the + backend has no known review pairing (caller then parks rather than guess a + model). Not consulted for ``claude_code`` (that path keeps its own + ``_run_direct_review`` haiku default). + """ + for member_backend, model, _lens in _COUNCIL_PANEL: + if member_backend == backend: + return model + return None + + def _member_on_cooldown(usage_store, backend: str, model: str, *, now: datetime) -> bool: """True when this council member's ``(backend, model)`` is currently cooled. @@ -3455,28 +3472,70 @@ def _phase1( state["self_review_loops"], ) - # D1: consult the shared fleet ladder BEFORE spawning a claude review. If - # claude is on cooldown or over the 25% budget reserve, don't burn it — - # defer this sweep and retry when it returns (degrade-never-halt applied to - # review; the budget window drains within ~5h). No claude spawn, no budget - # charge, no needs-human escalation for a transient wait. (Codex-fallback - # for the review itself is a validated follow-up; until then any non-claude - # selection means claude — the only review backend — is unavailable.) + # D1: consult the shared fleet ladder BEFORE spawning the review. The + # reviewer is part of the fleet, so it runs the controller's claude→codex + # LADDER instead of burning claude unconditionally: + # * claude runnable → review on claude/haiku (today's path); + # * claude cooled, fallback → DIVERT this ordinary review to the fallback + # backend (e.g. codex_cli/codex) so review keeps flowing — charges the + # fallback's budget, not claude's (the codex reviewer was validated safe + # as the ordinary single reviewer by the 2026-07-15 spike, and runs live + # as council seat C3); + # * whole ladder exhausted → PARK (defer+return, no burn); the #446 + # 1h auto-resume retries when a backend returns. + # Selection failure (``None``) proceeds on claude — today's fail-open behavior. + # This is the ORDINARY single-reviewer path only; guardrail PRs already + # forked to _run_council above and never reach here. + _review_backend = "claude_code" + _review_model = "haiku" _selection = _select_review_backend(settings) if _selection is not None and _selection.selected_backend != "claude_code": _resets = [r for r in _selection.cooldowns.values() if r is not None] _reset_at = max(_resets) if _resets else None + _fallback_model = ( + _review_model_for_backend(_selection.selected_backend) + if _selection.selected_backend is not None + else None + ) + if _fallback_model is None: + # Whole ladder exhausted (no runnable backend), or the runnable + # fallback has no known review pairing — PARK rather than burn or + # guess a model. Not budget-charged; retries via #446 auto-resume. + logger.info( + "pr_review_watcher: PR #%d review DEFERRED — no runnable review " + "backend (selected=%s, reset≈%s); not burning budget, will retry " + "when a backend returns.", + pr_number, + _selection.selected_backend, + _reset_at.isoformat() if _reset_at else "unknown", + ) + return + # claude cooled but a validated fallback is runnable — divert the review. + _review_backend = _selection.selected_backend + _review_model = _fallback_model logger.info( - "pr_review_watcher: PR #%d review DEFERRED — claude unavailable " - "(selected=%s, reset≈%s); not burning budget, will retry when it returns.", + "pr_review_watcher: PR #%d claude cooled/over-reserve — running the " + "ordinary review on fallback backend %s/%s (D1 ladder; charges %s " + "budget, not claude).", pr_number, - _selection.selected_backend, - _reset_at.isoformat() if _reset_at else "unknown", + _review_backend, + _review_model, + _review_backend, ) - return try: - verdict = _run_direct_review(oc_root, goal_text, state_key) + if _review_backend == "claude_code": + # Claude path stays on _run_direct_review (the name the test suite + # patches) so its back-compat contract is untouched. + verdict = _run_direct_review(oc_root, goal_text, state_key) + else: + verdict = _run_member_review( + oc_root, + goal_text, + state_key, + backend=_review_backend, + model=_review_model, + ) except OCSourceTreeUncleanError as exc: # The reviewer's own source tree is broken — this would crash for EVERY # PR, so it is not charged against this PR's review budget. Skip the @@ -3624,8 +3683,10 @@ def _phase1( state["no_verdict_passes"] = 0 # a verdict was produced state["no_verdict_backoff_level"] = 0 # Reset backoff when verdict is produced - # INJ Phase 1 (D-INJ-1): `result` was COMPUTED BY CODE in _run_direct_review - # from the model's typed per-check statuses — it is not a model-authored field. + # INJ Phase 1 (D-INJ-1): `result` was COMPUTED BY CODE in the review member + # (_run_member_review, on whichever backend ran — claude or the D1 codex + # fallback) from the model's typed per-check statuses — not a model-authored + # field. The trust boundary is identical regardless of which backend reviewed. # Fail-safe: a missing/malformed verdict computed to CONCERNS upstream. result = (verdict.get("result") or CONCERNS).upper() failing_checks = verdict.get("failing_checks") or [] diff --git a/tests/test_pr_review_watcher.py b/tests/test_pr_review_watcher.py index c29820b78..050b6cecb 100644 --- a/tests/test_pr_review_watcher.py +++ b/tests/test_pr_review_watcher.py @@ -3533,6 +3533,110 @@ def test_select_review_backend_respects_dynamic_disabled(monkeypatch, tmp_path): assert sel is not None and sel.selected_backend == "claude_code" +# ── D1: ordinary-review claude→codex fallback ───────────────────────────────── + + +def _fake_selection(selected_backend): + """A WorkerBackendSelection with a chosen backend, for forcing the D1 branch + deterministically without manipulating a real usage store.""" + from operations_center.backends.worker_backend_selector import WorkerBackendSelection + + return WorkerBackendSelection( + preferred_backend="claude_code", + selected_backend=selected_backend, + cooldowns={"claude_code": None}, + ) + + +def test_phase1_cooled_claude_runs_ordinary_review_on_codex(tmp_path: Path) -> None: + """D1: claude cooled but codex runnable → the ORDINARY review RUNS on + codex_cli/codex (not deferred), and its verdict flows into the same + downstream pipeline (LGTM → merge).""" + state, sp = _make_state(tmp_path, phase="self_review") + gh = _make_gh() + + with ( + patch.object( + watcher, "_select_review_backend", return_value=_fake_selection("codex_cli") + ), + patch.object( + watcher, "_run_member_review", return_value={"result": "LGTM", "summary": "ok"} + ) as mock_member, + patch.object(watcher, "_run_direct_review") as mock_direct, + patch.object(watcher, "_plane_client") as mock_pc, + ): + mock_pc.return_value = MagicMock() + mock_pc.return_value.close = MagicMock() + watcher._phase1( + state, sp, _pr_data(), gh, "owner", "repo", tmp_path, tmp_path / "cfg.yaml", SETTINGS + ) + + # Ran on the codex fallback, NOT the claude alias, and did NOT defer. + mock_direct.assert_not_called() + mock_member.assert_called_once() + assert mock_member.call_args.kwargs["backend"] == "codex_cli" + assert mock_member.call_args.kwargs["model"] == "codex" + # The codex verdict flowed into the SAME pipeline → merged. + gh.merge_pr.assert_called_once() + + +def test_phase1_ladder_exhausted_parks_no_burn(tmp_path: Path) -> None: + """D1: whole ladder exhausted (no runnable backend) → PARK (defer+return). + No review runner is invoked, nothing merges, and no budget is charged + (self_review_loops unchanged) — today's #446 auto-resume behavior.""" + state, sp = _make_state(tmp_path, phase="self_review") + gh = _make_gh() + + with ( + patch.object(watcher, "_select_review_backend", return_value=_fake_selection(None)), + patch.object(watcher, "_run_member_review") as mock_member, + patch.object(watcher, "_run_direct_review") as mock_direct, + ): + watcher._phase1( + state, sp, _pr_data(), gh, "owner", "repo", tmp_path, tmp_path / "cfg.yaml", SETTINGS + ) + + mock_member.assert_not_called() + mock_direct.assert_not_called() + gh.merge_pr.assert_not_called() + assert watcher._load_state(sp).get("self_review_loops", 0) == 0 + + +def test_phase1_guardrail_pr_cooled_claude_does_not_single_review_on_codex( + tmp_path: Path, +) -> None: + """D1 boundary: a guardrail PR with claude cooled still forks to the K=3 + council — it must NOT be silently single-reviewed on codex. The codex + fallback is for the ORDINARY single-reviewer path ONLY.""" + state, sp = _make_state(tmp_path, phase="self_review") + gh = _guardrail_gh() + settings = _council_settings() + + with ( + patch.object( + watcher, "_select_review_backend", return_value=_fake_selection("codex_cli") + ), + patch.object(watcher, "_run_council") as mock_council, + patch.object(watcher, "_run_member_review") as mock_member, + patch.object(watcher, "_run_direct_review") as mock_direct, + ): + watcher._phase1( + state, + sp, + _contributor_pr_data(), + gh, + "owner", + "repo", + tmp_path, + tmp_path / "cfg.yaml", + settings, + ) + + mock_council.assert_called_once() + mock_member.assert_not_called() # NOT single-reviewed on codex + mock_direct.assert_not_called() + + # ── C1: reviewer council mode (cross-family panel) ──────────────────────────── _GUARDRAIL_FILE = "src/operations_center/entrypoints/pr_review_watcher/main.py" diff --git a/tests/unit/reviewer/test_d1_codex_fallback.py b/tests/unit/reviewer/test_d1_codex_fallback.py new file mode 100644 index 000000000..4fc7644f3 --- /dev/null +++ b/tests/unit/reviewer/test_d1_codex_fallback.py @@ -0,0 +1,39 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 ProtocolWarden +"""Unit tests for audit D1 — the ordinary reviewer's claude→codex fallback. + +These cover the pure selection/dispatch helpers that CI (``pytest tests/unit``) +runs. The full ``_phase1`` dispatch/park/guardrail integration lives in the +root ``tests/test_pr_review_watcher.py`` (which CI does not run — validate +locally). +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +from operations_center.entrypoints.pr_review_watcher import main as watcher + + +def test_review_model_for_backend_codex_matches_council_pairing() -> None: + # The fallback reuses the validated council seat pairing (codex_cli→codex), + # not a fresh, unvalidated model choice. + assert watcher._review_model_for_backend("codex_cli") == "codex" + + +def test_review_model_for_backend_unknown_returns_none() -> None: + # An unknown/uncovered fallback backend has no known review pairing → None, + # so the caller PARKS rather than guessing a model. + assert watcher._review_model_for_backend("mystery_backend") is None + + +def test_run_direct_review_back_compat_runs_claude_haiku() -> None: + # Back-compat contract the test suite depends on: the no-arg alias still + # dispatches to claude_code/haiku via _run_member_review. + with patch.object(watcher, "_run_member_review", return_value=None) as mock_member: + watcher._run_direct_review(Path("/nonexistent"), "goal text", "MyRepo-42") + + mock_member.assert_called_once() + assert mock_member.call_args.kwargs["backend"] == "claude_code" + assert mock_member.call_args.kwargs["model"] == "haiku"