From e8770e1004cefafdb5c2e9f7a88f1f7166546336 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Wed, 10 Jun 2026 11:53:48 -0700 Subject: [PATCH] feat: report Open SWE Review as a PR check run (#1484) * feat: report Open SWE Review as a PR check run Auto-review dispatch now creates an in-progress 'Open SWE Review' check run on the PR head SHA; publish_review completes it (neutral with findings, success when clean). An after-agent hook fails the check if the run dies before publishing. Requires the GitHub App's Checks: Read & write permission; all calls are best-effort so a missing permission never breaks reviews. * fix: address review feedback on check-run settling Keep review_check_run_id when the completion PATCH fails so a later publish or the after-agent hook can retry instead of hanging the check; count out-of-diff findings toward the check conclusion. * fix: retry failed check completion with the real publish conclusion A transient PATCH failure after a successful publish previously left the check id for the after-agent hook, which settled it as 'failure'. Persist the intended result as review_check_pending_result and have the hook prefer it over the generic failure fallback. --------- Co-authored-by: open-swe[bot] --- INSTALLATION.md | 1 + agent/middleware/__init__.py | 2 + agent/middleware/settle_review_check.py | 84 +++++++++ agent/reviewer.py | 2 + agent/reviewer_publish.py | 53 ++++++ agent/tools/publish_review.py | 24 +++ agent/utils/github_checks.py | 136 ++++++++++++++ agent/webapp.py | 12 ++ tests/test_github_checks.py | 236 ++++++++++++++++++++++++ 9 files changed, 550 insertions(+) create mode 100644 agent/middleware/settle_review_check.py create mode 100644 agent/utils/github_checks.py create mode 100644 tests/test_github_checks.py diff --git a/INSTALLATION.md b/INSTALLATION.md index e4c35bc4..5abf687c 100644 --- a/INSTALLATION.md +++ b/INSTALLATION.md @@ -76,6 +76,7 @@ Write this down. You'll use it in the callback URL below and again in step 4 whe - Contents: Read & write - Pull requests: Read & write - Issues: Read & write + - Checks: Read & write — reports an "Open SWE Review" check run on PRs while an auto-review runs. Without it, check-run creation fails (logged, best-effort) but reviews still work. - Metadata: Read-only - **Organization permissions** (required only if you plan to set `ALLOWED_GITHUB_ORGS` — see step 5 / Security): - Members: Read-only — used to verify org membership for the dashboard-login gate via `GET /orgs/{org}/memberships/{username}`. Without this permission that call returns 403, the check fails closed, and **every** dashboard login is rejected. diff --git a/agent/middleware/__init__.py b/agent/middleware/__init__.py index bec5f345..63bb54d0 100644 --- a/agent/middleware/__init__.py +++ b/agent/middleware/__init__.py @@ -7,6 +7,7 @@ from .refresh_slack_status import SlackAssistantStatusMiddleware from .sandbox_circuit_breaker import SandboxCircuitBreakerMiddleware from .sanitize_thinking_blocks import SanitizeThinkingBlocksMiddleware from .sanitize_tool_inputs import SanitizeToolInputsMiddleware +from .settle_review_check import settle_review_check_on_exit from .tool_error_handler import ToolErrorMiddleware __all__ = [ @@ -20,4 +21,5 @@ __all__ = [ "check_message_queue_before_model", "ensure_no_empty_msg", "notify_step_limit_reached", + "settle_review_check_on_exit", ] diff --git a/agent/middleware/settle_review_check.py b/agent/middleware/settle_review_check.py new file mode 100644 index 00000000..8f08b1c7 --- /dev/null +++ b/agent/middleware/settle_review_check.py @@ -0,0 +1,84 @@ +"""After-agent middleware that closes a still-open review check run. + +``publish_review`` normally completes the ``Open SWE Review`` check run and +clears ``review_check_run_id`` from reviewer thread metadata. If the run ends +without ever publishing (crash, model-call limit, sandbox failure), the check +would hang "in progress" on the PR forever. This hook closes it as a failure +so the PR's checks section reflects reality. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from langchain.agents.middleware import AgentState, after_agent +from langgraph.config import get_config +from langgraph.runtime import Runtime + +from ..reviewer_findings import get_thread_metadata +from ..reviewer_publish import settle_review_check_run +from ..utils.github_token import get_github_token + +logger = logging.getLogger(__name__) + + +@after_agent +async def settle_review_check_on_exit( + state: AgentState, + runtime: Runtime, +) -> dict[str, Any] | None: + """Fail the tracked review check run if the run ended without publishing.""" + config = get_config() + configurable = config.get("configurable", {}) + if not isinstance(configurable, dict): + return None + thread_id = configurable.get("thread_id") + repo_config = configurable.get("repo") + if not isinstance(thread_id, str) or not thread_id or not isinstance(repo_config, dict): + return None + owner = repo_config.get("owner") + repo = repo_config.get("name") + if not isinstance(owner, str) or not owner or not isinstance(repo, str) or not repo: + return None + + try: + metadata = await get_thread_metadata(thread_id) + if not isinstance(metadata.get("review_check_run_id"), int): + return None + token = get_github_token() + if not token: + logger.warning("No GitHub token to settle stale review check on thread %s", thread_id) + return None + # A pending result means publish_review DID finish but its completion + # PATCH failed transiently — retry with the real conclusion instead of + # misreporting a published review as failed. + pending = metadata.get("review_check_pending_result") + if isinstance(pending, dict) and pending.get("conclusion") in { + "success", + "neutral", + "failure", + }: + conclusion = pending["conclusion"] + title = str(pending.get("title") or "Review completed") + summary = str(pending.get("summary") or "") + else: + conclusion = "failure" + title = "Review did not complete" + summary = ( + "The Open SWE review run ended without publishing a review. " + "Re-trigger the review by pushing a commit or re-requesting it." + ) + await settle_review_check_run( + thread_id=thread_id, + owner=owner, + repo=repo, + token=token, + conclusion=conclusion, + title=title, + summary=summary, + ) + logger.info("Settled stale review check run for thread %s", thread_id) + except Exception: + logger.exception("Failed to settle stale review check run for thread %s", thread_id) + return None diff --git a/agent/reviewer.py b/agent/reviewer.py index e56dd589..982d5a26 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -38,6 +38,7 @@ from .middleware import ( SlackAssistantStatusMiddleware, ToolErrorMiddleware, check_message_queue_before_model, + settle_review_check_on_exit, ) from .reviewer_diff import compute_diff_line_set, fetch_pr_diff, fetch_pr_metadata from .reviewer_findings import ( @@ -996,5 +997,6 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: check_message_queue_before_model, SlackAssistantStatusMiddleware(), SanitizeThinkingBlocksMiddleware(), + settle_review_check_on_exit, ], ).with_config(config) diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 16f5ca46..6b5c2f67 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -33,6 +33,7 @@ from .reviewer_findings import ( set_reviewer_thread_metadata, ) from .utils.dashboard_links import dashboard_thread_url +from .utils.github_checks import CheckConclusion, complete_review_check_run from .utils.github_token import GitHubAuthError logger = logging.getLogger(__name__) @@ -425,6 +426,58 @@ async def clear_review_started_comment( await set_reviewer_thread_metadata(thread_id, extra={"status_comment_id": None}) +async def settle_review_check_run( + *, + thread_id: str, + owner: str, + repo: str, + token: str, + conclusion: CheckConclusion, + title: str, + summary: str, +) -> None: + """Complete the tracked ``Open SWE Review`` check run, if one is open. + + The dispatching webhook stores ``review_check_run_id`` in reviewer thread + metadata when it creates the check. No-op when none is tracked. The id is + only cleared after a successful PATCH so a transient failure (timeout, + 5xx, rate limit) leaves it in place for the after-agent hook or a later + publish to retry — otherwise the check would hang in-progress forever. + On failure the intended result is persisted as + ``review_check_pending_result`` so the retry reports this conclusion, not + a generic failure that would misreport a published review. + """ + metadata = await get_thread_metadata(thread_id) + check_run_id = metadata.get("review_check_run_id") + if not isinstance(check_run_id, int): + return + ok = await complete_review_check_run( + owner=owner, + repo=repo, + check_run_id=check_run_id, + token=token, + conclusion=conclusion, + title=title, + summary=summary, + ) + if ok: + await set_reviewer_thread_metadata( + thread_id, + extra={"review_check_run_id": None, "review_check_pending_result": None}, + ) + else: + await set_reviewer_thread_metadata( + thread_id, + extra={ + "review_check_pending_result": { + "conclusion": conclusion, + "title": title, + "summary": summary, + } + }, + ) + + async def open_swe_review_exists( *, owner: str, diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 3db28f3d..e6de1ebf 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -40,9 +40,11 @@ from ..reviewer_publish import ( render_review_body, reply_to_review_comment, resolve_review_thread, + settle_review_check_run, ) from ..reviewer_reconcile import reconcile_findings_with_review_threads from ..utils.dashboard_links import dashboard_thread_url +from ..utils.github_checks import review_check_conclusion from ..utils.github_token import ( GitHubAuthError, get_github_token, @@ -322,6 +324,16 @@ async def _publish_review_async( ) await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token) + conclusion, check_title, check_summary = review_check_conclusion(0) + await settle_review_check_run( + thread_id=thread_id, + owner=owner, + repo=repo, + token=token, + conclusion=conclusion, + title=check_title, + summary=check_summary, + ) return { "success": True, "review_id": None, @@ -498,6 +510,18 @@ async def _publish_review_async( await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token) + conclusion, check_title, check_summary = review_check_conclusion( + len(inline_comments) + len(eligible_out_of_diff) + ) + await settle_review_check_run( + thread_id=thread_id, + owner=owner, + repo=repo, + token=token, + conclusion=conclusion, + title=check_title, + summary=check_summary, + ) result: dict[str, Any] = { "success": True, diff --git a/agent/utils/github_checks.py b/agent/utils/github_checks.py new file mode 100644 index 00000000..46a291dd --- /dev/null +++ b/agent/utils/github_checks.py @@ -0,0 +1,136 @@ +"""GitHub Checks API helpers for the reviewer's PR check run. + +A check run named ``Open SWE Review`` is created on the PR head SHA when an +auto-review is dispatched, so the PR's checks section shows the review as +in-progress. ``publish_review`` (or the after-agent fallback) completes it. + +All calls are best-effort: check runs require the GitHub App's +``Checks: Read & write`` permission, and a missing permission must never +break review dispatch or publish. +""" + +from __future__ import annotations + +import logging +from datetime import UTC, datetime +from typing import Literal + +import httpx + +logger = logging.getLogger(__name__) + +REVIEW_CHECK_RUN_NAME = "Open SWE Review" + +_GITHUB_API_BASE = "https://api.github.com" + +CheckConclusion = Literal["success", "neutral", "failure"] + + +def _github_headers(token: str) -> dict[str, str]: + return { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + + +def _utc_now_iso() -> str: + return datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ") + + +async def create_review_check_run( + *, + owner: str, + repo: str, + head_sha: str, + token: str, + details_url: str | None = None, +) -> int | None: + """Create an in-progress ``Open SWE Review`` check run on ``head_sha``. + + Returns the check run id, or ``None`` on any failure (most commonly the + App lacking the Checks permission). + """ + payload: dict[str, object] = { + "name": REVIEW_CHECK_RUN_NAME, + "head_sha": head_sha, + "status": "in_progress", + "started_at": _utc_now_iso(), + "output": { + "title": "Review in progress", + "summary": "Open SWE is reviewing this pull request…", + }, + } + if details_url: + payload["details_url"] = details_url + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/check-runs" + try: + async with httpx.AsyncClient() as client: + response = await client.post( + url, headers=_github_headers(token), json=payload, timeout=30 + ) + response.raise_for_status() + except httpx.HTTPError: + logger.exception( + "Failed to create review check run for %s/%s@%s " + "(does the GitHub App have Checks: Read & write?)", + owner, + repo, + head_sha, + ) + return None + data = response.json() + check_run_id = data.get("id") if isinstance(data, dict) else None + return check_run_id if isinstance(check_run_id, int) else None + + +async def complete_review_check_run( + *, + owner: str, + repo: str, + check_run_id: int, + token: str, + conclusion: CheckConclusion, + title: str, + summary: str, +) -> bool: + """Mark a review check run as completed. Returns True on success.""" + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/check-runs/{check_run_id}" + payload = { + "status": "completed", + "conclusion": conclusion, + "completed_at": _utc_now_iso(), + "output": {"title": title, "summary": summary}, + } + try: + async with httpx.AsyncClient() as client: + response = await client.patch( + url, headers=_github_headers(token), json=payload, timeout=30 + ) + response.raise_for_status() + except httpx.HTTPError: + logger.exception( + "Failed to complete review check run %s on %s/%s", check_run_id, owner, repo + ) + return False + return True + + +def review_check_conclusion(surfaced_count: int) -> tuple[CheckConclusion, str, str]: + """Map a publish result to (conclusion, title, summary). + + Findings surfaced → ``neutral`` so the check never blocks merges; a clean + review → ``success``. + """ + if surfaced_count > 0: + issue_word = "issue" if surfaced_count == 1 else "issues" + return ( + "neutral", + f"Found {surfaced_count} potential {issue_word}", + f"Open SWE surfaced {surfaced_count} potential {issue_word} on this pull request.", + ) + return ( + "success", + "No issues found", + "Open SWE reviewed this pull request and found no issues.", + ) diff --git a/agent/webapp.py b/agent/webapp.py index 98fa9404..10c3eac7 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -55,10 +55,12 @@ from .utils.auth import ( resolve_github_token_from_email, ) from .utils.comments import get_recent_comments +from .utils.dashboard_links import dashboard_thread_url from .utils.github_app import ( get_github_app_installation_token, get_github_app_installation_token_with_expiry, ) +from .utils.github_checks import create_review_check_run from .utils.github_comments import ( OPEN_SWE_TAGS, GitHubAuthError, @@ -2043,6 +2045,16 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha) + check_run_id = await create_review_check_run( + owner=repo_config.get("owner", ""), + repo=repo_config.get("name", ""), + head_sha=head_sha, + token=app_token, + details_url=dashboard_thread_url(thread_id), + ) + if check_run_id is not None: + await set_reviewer_thread_metadata(thread_id, extra={"review_check_run_id": check_run_id}) + is_re_review = bool(last_reviewed_sha) if is_re_review: prompt = ( diff --git a/tests/test_github_checks.py b/tests/test_github_checks.py new file mode 100644 index 00000000..93a00e03 --- /dev/null +++ b/tests/test_github_checks.py @@ -0,0 +1,236 @@ +from __future__ import annotations + +from typing import Any + +import httpx +import pytest + +from agent import reviewer_publish +from agent.utils import github_checks + + +class _FakeResponse: + def __init__(self, payload: dict[str, Any] | None = None, error: bool = False) -> None: + self._payload = payload or {} + self._error = error + + def raise_for_status(self) -> None: + if self._error: + raise httpx.HTTPStatusError("forbidden", request=None, response=None) # type: ignore[arg-type] + + def json(self) -> dict[str, Any]: + return self._payload + + +class _FakeAsyncClient: + last_post: dict[str, Any] | None = None + last_patch: dict[str, Any] | None = None + post_response: _FakeResponse = _FakeResponse({"id": 42}) + patch_response: _FakeResponse = _FakeResponse({}) + + async def __aenter__(self) -> _FakeAsyncClient: + return self + + async def __aexit__(self, exc_type: object, exc: object, tb: object) -> None: + return None + + async def post(self, url: str, **kwargs: Any) -> _FakeResponse: + type(self).last_post = {"url": url, **kwargs} + return type(self).post_response + + async def patch(self, url: str, **kwargs: Any) -> _FakeResponse: + type(self).last_patch = {"url": url, **kwargs} + return type(self).patch_response + + +@pytest.fixture(autouse=True) +def _reset_fake_client() -> None: + _FakeAsyncClient.last_post = None + _FakeAsyncClient.last_patch = None + _FakeAsyncClient.post_response = _FakeResponse({"id": 42}) + _FakeAsyncClient.patch_response = _FakeResponse({}) + + +async def test_create_review_check_run_posts_in_progress( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(github_checks.httpx, "AsyncClient", _FakeAsyncClient) + + check_run_id = await github_checks.create_review_check_run( + owner="acme", + repo="widgets", + head_sha="abc123", + token="tok", + details_url="https://example.com/thread", + ) + + assert check_run_id == 42 + assert _FakeAsyncClient.last_post is not None + assert _FakeAsyncClient.last_post["url"].endswith("/repos/acme/widgets/check-runs") + body = _FakeAsyncClient.last_post["json"] + assert body["name"] == github_checks.REVIEW_CHECK_RUN_NAME + assert body["head_sha"] == "abc123" + assert body["status"] == "in_progress" + assert body["details_url"] == "https://example.com/thread" + + +async def test_create_review_check_run_returns_none_on_http_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(github_checks.httpx, "AsyncClient", _FakeAsyncClient) + _FakeAsyncClient.post_response = _FakeResponse(error=True) + + check_run_id = await github_checks.create_review_check_run( + owner="acme", repo="widgets", head_sha="abc123", token="tok" + ) + + assert check_run_id is None + + +async def test_complete_review_check_run_patches_completed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(github_checks.httpx, "AsyncClient", _FakeAsyncClient) + + ok = await github_checks.complete_review_check_run( + owner="acme", + repo="widgets", + check_run_id=42, + token="tok", + conclusion="neutral", + title="Found 2 potential issues", + summary="…", + ) + + assert ok is True + assert _FakeAsyncClient.last_patch is not None + assert _FakeAsyncClient.last_patch["url"].endswith("/repos/acme/widgets/check-runs/42") + body = _FakeAsyncClient.last_patch["json"] + assert body["status"] == "completed" + assert body["conclusion"] == "neutral" + assert body["output"]["title"] == "Found 2 potential issues" + + +def test_review_check_conclusion_mapping() -> None: + conclusion, title, _ = github_checks.review_check_conclusion(0) + assert conclusion == "success" + assert title == "No issues found" + + conclusion, title, _ = github_checks.review_check_conclusion(1) + assert conclusion == "neutral" + assert "1 potential issue" in title + + conclusion, title, _ = github_checks.review_check_conclusion(3) + assert conclusion == "neutral" + assert "3 potential issues" in title + + +async def test_settle_review_check_run_noop_without_tracked_id( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_get_thread_metadata(thread_id: str) -> dict[str, Any]: + return {} + + completed: list[dict[str, Any]] = [] + + async def fake_complete(**kwargs: Any) -> bool: + completed.append(kwargs) + return True + + monkeypatch.setattr(reviewer_publish, "get_thread_metadata", fake_get_thread_metadata) + monkeypatch.setattr(reviewer_publish, "complete_review_check_run", fake_complete) + + await reviewer_publish.settle_review_check_run( + thread_id="t1", + owner="acme", + repo="widgets", + token="tok", + conclusion="success", + title="t", + summary="s", + ) + + assert completed == [] + + +async def test_settle_review_check_run_completes_and_clears( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_get_thread_metadata(thread_id: str) -> dict[str, Any]: + return {"review_check_run_id": 42} + + completed: list[dict[str, Any]] = [] + metadata_writes: list[dict[str, Any]] = [] + + async def fake_complete(**kwargs: Any) -> bool: + completed.append(kwargs) + return True + + async def fake_set_metadata(thread_id: str, **kwargs: Any) -> None: + metadata_writes.append({"thread_id": thread_id, **kwargs}) + + monkeypatch.setattr(reviewer_publish, "get_thread_metadata", fake_get_thread_metadata) + monkeypatch.setattr(reviewer_publish, "complete_review_check_run", fake_complete) + monkeypatch.setattr(reviewer_publish, "set_reviewer_thread_metadata", fake_set_metadata) + + await reviewer_publish.settle_review_check_run( + thread_id="t1", + owner="acme", + repo="widgets", + token="tok", + conclusion="neutral", + title="t", + summary="s", + ) + + assert len(completed) == 1 + assert completed[0]["check_run_id"] == 42 + assert completed[0]["conclusion"] == "neutral" + assert metadata_writes == [ + { + "thread_id": "t1", + "extra": {"review_check_run_id": None, "review_check_pending_result": None}, + } + ] + + +async def test_settle_review_check_run_keeps_id_on_patch_failure( + monkeypatch: pytest.MonkeyPatch, +) -> None: + async def fake_get_thread_metadata(thread_id: str) -> dict[str, Any]: + return {"review_check_run_id": 42} + + metadata_writes: list[dict[str, Any]] = [] + + async def fake_complete(**kwargs: Any) -> bool: + return False + + async def fake_set_metadata(thread_id: str, **kwargs: Any) -> None: + metadata_writes.append({"thread_id": thread_id, **kwargs}) + + monkeypatch.setattr(reviewer_publish, "get_thread_metadata", fake_get_thread_metadata) + monkeypatch.setattr(reviewer_publish, "complete_review_check_run", fake_complete) + monkeypatch.setattr(reviewer_publish, "set_reviewer_thread_metadata", fake_set_metadata) + + await reviewer_publish.settle_review_check_run( + thread_id="t1", + owner="acme", + repo="widgets", + token="tok", + conclusion="success", + title="t", + summary="s", + ) + + assert metadata_writes == [ + { + "thread_id": "t1", + "extra": { + "review_check_pending_result": { + "conclusion": "success", + "title": "t", + "summary": "s", + } + }, + } + ]