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
15 changes: 15 additions & 0 deletions .console/log.md
Original file line number Diff line number Diff line change
@@ -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`:
Expand Down
33 changes: 33 additions & 0 deletions src/operations_center/adapters/github_pr.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
68 changes: 68 additions & 0 deletions src/operations_center/entrypoints/pr_review_watcher/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
35 changes: 35 additions & 0 deletions tests/test_pr_review_watcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
35 changes: 35 additions & 0 deletions tests/unit/adapters/test_github_pr_cov.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
# ---------------------------------------------------------------------------
Expand Down
Loading