diff --git a/.console/log.md b/.console/log.md index 83bdb1ab7..850152425 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,18 @@ +## 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 +`gh pr merge` (operator/admin) bypassed an unresolved CONCERNS verdict — and fast +manual merges raced past the review loop entirely (see #330/#328 today). Made the +verdict first-class: `GitHubPRClient.set_commit_status` + `_publish_reviewer_verdict` +publish a `reviewer-verdict` commit status on the PR head — `success` on LGTM (and +re-blessed inside `_merge_and_done` so the fleet's own merge + non-LGTM merge paths +clear the gate), `failure` on CONCERNS. Before any review there is no status → +fail-closed, merge blocked. DEPLOY ORDER (critical): merge + restart fleet so it +runs the publishing code BEFORE adding `reviewer-verdict` to OC main required +checks (else PRs deadlock waiting for a status the old fleet never posts). Enforce +on admins too (else my own admin merges bypass it). Fleet-outage recovery: lift +branch protection to merge manually. + ## 2026-06-18 — feat: complete coverage trend enrichment + alert routing Wired the last 5 unbaselined coverage methods into `_record_coverage_trend`: diff --git a/src/operations_center/adapters/github_pr.py b/src/operations_center/adapters/github_pr.py index 8176dd7a5..9738fd5b8 100644 --- a/src/operations_center/adapters/github_pr.py +++ b/src/operations_center/adapters/github_pr.py @@ -201,6 +201,39 @@ def update_comment(self, owner: str, repo: str, comment_id: int, body: str) -> d resp.raise_for_status() return resp.json() + def set_commit_status( + self, + owner: str, + repo: str, + sha: str, + *, + state: str, + context: str, + description: str = "", + target_url: str | None = None, + ) -> dict: + """Publish a commit status (``success``/``failure``/``pending``/``error``). + + Used to surface the reviewer's verdict as a first-class status check on + the PR head SHA, so it can be made a *required* status check — closing + the gap where a manual ``gh pr merge`` bypasses the (comment-only) + review verdict. + """ + payload: dict[str, Any] = { + "state": state, + "context": context, + "description": description[:140], + } + if target_url: + payload["target_url"] = target_url + resp = self._request( + "POST", + f"{self._API}/repos/{owner}/{repo}/statuses/{sha}", + json=payload, + ) + resp.raise_for_status() + return resp.json() + def get_check_runs(self, owner: str, repo: str, ref: str) -> list[dict]: """Return all check-runs for a given commit SHA or ref.""" resp = self._request( diff --git a/src/operations_center/entrypoints/pr_review_watcher/main.py b/src/operations_center/entrypoints/pr_review_watcher/main.py index 1cb48dfd0..f845494e9 100644 --- a/src/operations_center/entrypoints/pr_review_watcher/main.py +++ b/src/operations_center/entrypoints/pr_review_watcher/main.py @@ -817,6 +817,48 @@ def _git(*args: str) -> subprocess.CompletedProcess: return "error" +_REVIEWER_VERDICT_STATUS_CONTEXT = "reviewer-verdict" + + +def _publish_reviewer_verdict( + gh_client, + owner: str, + repo: str, + sha: str | None, + *, + result: str, + description: str, +) -> None: + """Publish the reviewer's verdict as a commit status on the PR head SHA. + + This makes the (otherwise comment-only) verdict a first-class status check + so it can be marked *required* in branch protection — closing the gap where + a manual ``gh pr merge`` bypasses an unresolved CONCERNS verdict. Until the + reviewer posts ``success`` (LGTM), the context is ``failure``/absent and the + merge is blocked, for the fleet and humans alike. + + Best-effort: a status-post failure must never crash the review loop. + """ + if not sha: + return + try: + gh_client.set_commit_status( + owner, + repo, + sha, + state=result, + context=_REVIEWER_VERDICT_STATUS_CONTEXT, + description=description, + ) + except Exception as exc: # noqa: BLE001 — status publishing is best-effort + logger.warning( + "pr_review_watcher: failed to publish %s status on %s — %s", + _REVIEWER_VERDICT_STATUS_CONTEXT, + sha[:8], + exc, + ) + + def _merge_and_done( state: dict, state_path: Path, @@ -838,6 +880,19 @@ def _merge_and_done( _auto_rebase_or_escalate(state, state_path, gh_client, owner, repo, settings, reason) return state["rebase_attempts"] = 0 # mergeable — clear any rebase bookkeeping + # Bless this head with reviewer-verdict=success BEFORE merging, so the + # required status check is satisfied for the fleet's own merge — and so the + # non-LGTM merge paths (e.g. ci_validated_after_retraction) also clear the + # gate. GitHub records the status synchronously; a brief propagation lag at + # most causes one retry on the next poll. + _publish_reviewer_verdict( + gh_client, + owner, + repo, + _pr_head_sha(_pr_data), + result="success", + description=f"reviewer approved ({reason})", + ) try: gh_client.merge_pr(owner, repo, pr_number, merge_method="squash") logger.info( @@ -2189,6 +2244,19 @@ def _phase1( logger.info("pr_review_watcher: PR #%d self-review verdict=%s", pr_number, result) + # Surface the verdict as a required status check on the reviewed head, so an + # unresolved CONCERNS verdict blocks merge for humans (manual gh pr merge) + # too, not just the fleet's own verdict-gated path. _merge_and_done re-blesses + # success right before merging, which also covers the non-LGTM merge paths. + _publish_reviewer_verdict( + gh_client, + owner, + repo, + current_head_sha, + result="success" if result == "LGTM" else "failure", + description="reviewer LGTM" if result == "LGTM" else "reviewer concerns — auto-fixing", + ) + if result == "LGTM": # The ONLY merge path on the self-review track — verdict-gated. record_decision_outcome( diff --git a/tests/test_pr_review_watcher.py b/tests/test_pr_review_watcher.py index 9218436be..04298ce83 100644 --- a/tests/test_pr_review_watcher.py +++ b/tests/test_pr_review_watcher.py @@ -2530,3 +2530,38 @@ def _boom(*args, **kwargs): monkeypatch.setattr(watcher.subprocess, "run", _boom) # Must not raise, and returns None (silent no-op). assert watcher._capture_human_intervention("sig", "ctx") is None + + +# --------------------------------------------------------------------------- +# reviewer-verdict status publishing (Part B: make the verdict a required check) +# --------------------------------------------------------------------------- +def test_publish_reviewer_verdict_posts_success(): + gh = MagicMock() + watcher._publish_reviewer_verdict( + gh, "o", "r", "sha123", result="success", description="reviewer LGTM" + ) + gh.set_commit_status.assert_called_once_with( + "o", + "r", + "sha123", + state="success", + context=watcher._REVIEWER_VERDICT_STATUS_CONTEXT, + description="reviewer LGTM", + ) + + +def test_publish_reviewer_verdict_noop_on_empty_sha(): + gh = MagicMock() + watcher._publish_reviewer_verdict(gh, "o", "r", "", result="failure", description="x") + gh.set_commit_status.assert_not_called() + + +def test_publish_reviewer_verdict_swallows_errors(): + gh = MagicMock() + gh.set_commit_status.side_effect = RuntimeError("boom") + # Best-effort: a status-post failure must never crash the review loop. + out = watcher._publish_reviewer_verdict( + gh, "o", "r", "sha", result="failure", description="x" + ) + assert out is None + gh.set_commit_status.assert_called_once() # attempted despite the raise diff --git a/tests/unit/adapters/test_github_pr_cov.py b/tests/unit/adapters/test_github_pr_cov.py index df7deb9c9..075c09696 100644 --- a/tests/unit/adapters/test_github_pr_cov.py +++ b/tests/unit/adapters/test_github_pr_cov.py @@ -289,6 +289,41 @@ def test_post_comment(client): assert req.call_args[1]["json"] == {"body": "hello"} +# --------------------------------------------------------------------------- +# set_commit_status +# --------------------------------------------------------------------------- +def test_set_commit_status_posts_to_statuses_endpoint(client): + resp = _make_response(status_code=200, json_data={"state": "success"}) + with mock.patch.object(client, "_request", return_value=resp) as req: + out = client.set_commit_status( + "o", "r", "abc123", + state="success", context="reviewer-verdict", description="reviewer LGTM", + ) + assert out == {"state": "success"} + method, url = req.call_args[0] + assert method == "POST" + assert url.endswith("/repos/o/r/statuses/abc123") + body = req.call_args[1]["json"] + assert body == { + "state": "success", + "context": "reviewer-verdict", + "description": "reviewer LGTM", + } + + +def test_set_commit_status_truncates_long_description_and_adds_target_url(client): + resp = _make_response(status_code=200, json_data={}) + with mock.patch.object(client, "_request", return_value=resp) as req: + client.set_commit_status( + "o", "r", "sha", + state="failure", context="reviewer-verdict", + description="x" * 200, target_url="https://example/run", + ) + body = req.call_args[1]["json"] + assert len(body["description"]) == 140 # GitHub's status description cap + assert body["target_url"] == "https://example/run" + + # --------------------------------------------------------------------------- # get_check_runs # ---------------------------------------------------------------------------