diff --git a/.console/log.md b/.console/log.md index 850152425..2d1df0477 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,17 @@ +## 2026-06-18 — fix: reviewer escalation budget no longer reset by own fix-push + +#334 exposed a non-convergence bug: a CONCERNS PR whose concerns are unsatisfiable +in-diff (a doc summarizing out-of-diff facts — CI runs, secrets, sibling PRs) looped +forever — 7 self-pushes, `fix_attempts` stuck at 1, piling on VERIFICATION_*.md / +RESOLUTION_SUMMARY.md cruft, never escalating. Root cause: `_phase1`'s "head changed +after concerns → reset fix state" fired on EVERY head move, including the fleet's OWN +fix-push, so the budget never accumulated to `max_fix_attempts`. Fix: record the head +each fix pass produces (`last_fix_push_sha`) and only reset on an EXTERNAL push (head +≠ our last fix-push). Now self-pushes accumulate → the PR terminates (close+requeue at +max) instead of looping. Surfaced because Part B made `reviewer-verdict` required, so +the loop became a hard merge-blocker rather than advisory churn. Tests: self-push keeps +budget (→2), external push resets (→1). Reviewer suite 118 pass. + ## 2026-06-18 — feat: reviewer verdict as a required status check (Part B) The reviewer's verdict was a bot *comment*, not a status check, so a manual diff --git a/src/operations_center/entrypoints/pr_review_watcher/main.py b/src/operations_center/entrypoints/pr_review_watcher/main.py index f845494e9..6b558f292 100644 --- a/src/operations_center/entrypoints/pr_review_watcher/main.py +++ b/src/operations_center/entrypoints/pr_review_watcher/main.py @@ -1719,11 +1719,20 @@ def _phase1( current_head_sha = _pr_head_sha(pr_data) previous_concerns_head_sha = str(state.get("last_concerns_head_sha") or "").strip() + # Only reset the fix/escalation budget when the head moved because of an + # EXTERNAL push (a human, or another host) — that is genuinely new work to + # review fresh. When the head moved because OUR OWN fix pass pushed it, the + # budget must keep accumulating, otherwise every self-pushed fix resets the + # counter and the PR loops forever instead of escalating to a human (the + # #334 non-convergence: 7 self-pushes, fix_attempts stuck at 1, piling on + # evidence files). last_fix_push_sha is the head our last fix pass produced. + last_fix_push_sha = str(state.get("last_fix_push_sha") or "").strip() if ( state.get("concerns_comment_id") and current_head_sha and previous_concerns_head_sha and current_head_sha != previous_concerns_head_sha + and current_head_sha != last_fix_push_sha ): _retract_flag( state, gh_client, owner, repo, resolution="superseded by new push — re-review resumed" @@ -1732,9 +1741,11 @@ def _phase1( state.pop("last_concerns_summary", None) state.pop("last_concerns_head_sha", None) state.pop("last_fix_pass_pushed", None) + state.pop("last_fix_push_sha", None) state.pop("fix_strategy_level", None) # new code → start back at L0 logger.info( - "pr_review_watcher: PR #%d head changed after concerns; resetting fix state", + "pr_review_watcher: PR #%d head changed after concerns (external push); " + "resetting fix state", pr_number, ) _save_state(state_path, state) @@ -2505,6 +2516,21 @@ def _phase1( ) state["last_concerns_summary"] = normalized_summary state["last_fix_pass_pushed"] = pushed + if pushed: + # Record the head our own fix pass just produced, so the next poll does + # NOT mistake it for an external push and reset the escalation budget. + # Without this, fix_attempts never accumulates and the PR loops forever. + try: + state["last_fix_push_sha"] = _pr_head_sha( + gh_client.get_pr(owner, repo, pr_number) + ) + except Exception as exc: # noqa: BLE001 — best-effort; reset-guard degrades safe + logger.warning( + "pr_review_watcher: could not record fix-push head for PR #%d — %s", + pr_number, + exc, + ) + state.pop("last_fix_push_sha", None) if not pushed: logger.warning( "pr_review_watcher: fix pass for PR #%d pushed no changes (attempt %d/%d)", diff --git a/tests/test_pr_review_watcher.py b/tests/test_pr_review_watcher.py index 04298ce83..a7b9d5a43 100644 --- a/tests/test_pr_review_watcher.py +++ b/tests/test_pr_review_watcher.py @@ -2565,3 +2565,76 @@ def test_publish_reviewer_verdict_swallows_errors(): ) assert out is None gh.set_commit_status.assert_called_once() # attempted despite the raise + + +# --------------------------------------------------------------------------- +# Escalation-budget reset guard: the fleet's OWN fix-push must not reset the +# fix_attempts budget (else a non-converging PR loops forever instead of +# terminating). Only an EXTERNAL push resets. (The #334 non-convergence bug.) +# --------------------------------------------------------------------------- +def test_phase1_self_pushed_fix_does_not_reset_budget(tmp_path: Path) -> None: + # Head moved to H1 because our own previous fix pass pushed it + # (last_fix_push_sha == H1) — the budget must keep accumulating. + state, sp = _make_state( + tmp_path, + phase="self_review", + self_review_loops=1, + fix_attempts=1, + concerns_comment_id=123, + last_concerns_head_sha="H0", + last_fix_push_sha="H1", + last_concerns_summary="same issues", + ) + gh = _make_gh() + gh.get_pr.return_value = _pr_data(head_sha="H2") # our next fix pushes H2 + + with ( + patch.object( + watcher, "_run_direct_review", + return_value={"result": "CONCERNS", "summary": "still unverifiable"}, + ), + patch.object(watcher, "_run_fix_pass", return_value=True), + ): + watcher._phase1( + state, sp, _pr_data(head_sha="H1"), gh, "owner", "repo", + tmp_path, tmp_path / "cfg.yaml", SETTINGS, + ) + + loaded = watcher._load_state(sp) + # NOT reset to 0-then-1: preserved 1, dispatch incremented to 2. + assert loaded["fix_attempts"] == 2 + # The new self-push head is recorded for the next poll's guard. + assert loaded["last_fix_push_sha"] == "H2" + + +def test_phase1_external_push_resets_budget(tmp_path: Path) -> None: + # Head moved to HX by an EXTERNAL push (!= last_fix_push_sha) — genuinely new + # work; the budget resets so the human's fix is reviewed fresh. + state, sp = _make_state( + tmp_path, + phase="self_review", + self_review_loops=1, + fix_attempts=1, + concerns_comment_id=123, + last_concerns_head_sha="H0", + last_fix_push_sha="H1", + last_concerns_summary="same issues", + ) + gh = _make_gh() + gh.get_pr.return_value = _pr_data(head_sha="HX2") + + with ( + patch.object( + watcher, "_run_direct_review", + return_value={"result": "CONCERNS", "summary": "new concern on human fix"}, + ), + patch.object(watcher, "_run_fix_pass", return_value=True), + ): + watcher._phase1( + state, sp, _pr_data(head_sha="HX"), gh, "owner", "repo", + tmp_path, tmp_path / "cfg.yaml", SETTINGS, + ) + + loaded = watcher._load_state(sp) + # Reset to 0, then dispatch incremented to 1 (fresh start). + assert loaded["fix_attempts"] == 1