Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .console/log.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
28 changes: 27 additions & 1 deletion src/operations_center/entrypoints/pr_review_watcher/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand Down Expand Up @@ -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)",
Expand Down
73 changes: 73 additions & 0 deletions tests/test_pr_review_watcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading