diff --git a/agent/middleware/settle_review_check.py b/agent/middleware/settle_review_check.py index 3c584275..721e88eb 100644 --- a/agent/middleware/settle_review_check.py +++ b/agent/middleware/settle_review_check.py @@ -28,7 +28,7 @@ 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.""" + """Neutralize the tracked review check if the run ended without publishing.""" config = get_config() configurable = config.get("configurable", {}) if not isinstance(configurable, dict): diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index c406d273..c34ee753 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -403,7 +403,23 @@ 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) + skip_result: dict[str, Any] = { + "success": True, + "review_id": None, + "surfaced_count": 0, + "hidden_count": max(len(open_unpublished), 0), + "resolved_thread_count": resolved_thread_count, + "skipped_empty_re_review": True, + "blocking_finding_count": sum( + 1 for finding in findings if _finding_blocks_verdict(finding) + ), + "verdict_authorization": verdict_authorization, + } + if verdict_ignored_reason: + skip_result["verdict_submitted"] = False + skip_result["verdict_ignored"] = True + skip_result["verdict_ignored_reason"] = verdict_ignored_reason + conclusion, check_title, check_summary = review_check_conclusion(skip_result) await settle_review_check_run( thread_id=thread_id, owner=owner, @@ -413,18 +429,6 @@ async def _publish_review_async( title=check_title, summary=check_summary, ) - skip_result: dict[str, Any] = { - "success": True, - "review_id": None, - "surfaced_count": 0, - "hidden_count": max(len(open_unpublished), 0), - "resolved_thread_count": resolved_thread_count, - "skipped_empty_re_review": True, - } - if verdict_ignored_reason: - skip_result["verdict_submitted"] = False - skip_result["verdict_ignored"] = True - skip_result["verdict_ignored_reason"] = verdict_ignored_reason return skip_result review_body = render_review_body( @@ -695,16 +699,6 @@ 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)) - 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, @@ -712,6 +706,10 @@ async def _publish_review_async( "surfaced_count": len(inline_comments), "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), "resolved_thread_count": resolved_thread_count, + "blocking_finding_count": sum( + 1 for finding in findings if _finding_blocks_verdict(finding) + ), + "verdict_authorization": verdict_authorization, } if recorded_state: result["review_state"] = recorded_state @@ -734,6 +732,16 @@ async def _publish_review_async( "Some findings had anchors not in the PR diff; " "call update_finding to fix or resolve them." ) + conclusion, check_title, check_summary = review_check_conclusion(result) + await settle_review_check_run( + thread_id=thread_id, + owner=owner, + repo=repo, + token=token, + conclusion=conclusion, + title=check_title, + summary=check_summary, + ) return result diff --git a/agent/utils/github_checks.py b/agent/utils/github_checks.py index 8ce86719..7e522bb9 100644 --- a/agent/utils/github_checks.py +++ b/agent/utils/github_checks.py @@ -12,6 +12,7 @@ break review dispatch or publish. from __future__ import annotations import logging +from collections.abc import Mapping from datetime import UTC, datetime from typing import Literal @@ -153,23 +154,75 @@ async def post_autofix_status_check( return True -def review_check_conclusion(surfaced_count: int) -> tuple[CheckConclusion, str, str]: - """Map a publish result to (conclusion, title, summary). - - Always ``success`` so the check is informational and non-blocking, and so - GitHub groups it under "successful checks" rather than a confusing - "neutral check". The finding count is surfaced in the title; the findings - themselves are posted as PR comments. - """ - if surfaced_count > 0: - issue_word = "issue" if surfaced_count == 1 else "issues" +def review_check_conclusion( + publish_outcome: Mapping[str, object], +) -> tuple[CheckConclusion, str, str]: + """Map the authoritative publish outcome to a check conclusion.""" + verdict_submitted = publish_outcome.get("verdict_submitted") is True + verdict_event = publish_outcome.get("verdict_event") + if verdict_submitted and verdict_event == "APPROVE": return ( "success", - f"Found {surfaced_count} potential {issue_word}", - f"Open SWE surfaced {surfaced_count} potential {issue_word} on this pull request.", + "Review approved", + "Open SWE recorded an approving review on this pull request.", ) - return ( - "success", - "No issues found", - "Open SWE reviewed this pull request and found no issues.", + if verdict_submitted and verdict_event == "REQUEST_CHANGES": + return ( + "failure", + "Changes requested", + "Open SWE recorded a request-changes review on this pull request.", + ) + + blocking_count_raw = publish_outcome.get("blocking_finding_count") + blocking_count = ( + blocking_count_raw + if isinstance(blocking_count_raw, int) and not isinstance(blocking_count_raw, bool) + else None + ) + ignored_reason = publish_outcome.get("verdict_ignored_reason") + if ignored_reason and ignored_reason != "self_review": + return ( + "neutral", + "Verdict withheld", + f"Open SWE published without a verdict ({str(ignored_reason).replace('_', ' ')}).", + ) + if blocking_count == 0: + return ( + "success", + "No issues found", + "Open SWE reviewed this pull request and found no blocking issues.", + ) + + verdict_authorization = publish_outcome.get("verdict_authorization") + if ( + blocking_count is not None + and blocking_count > 0 + and verdict_authorization + in { + "requested", + "consistent", + } + ): + issue_word = "issue" if blocking_count == 1 else "issues" + return ( + "failure", + f"Found {blocking_count} blocking {issue_word}", + f"Open SWE found {blocking_count} blocking {issue_word} on this pull request.", + ) + + surfaced_count_raw = publish_outcome.get("surfaced_count") + surfaced_count = ( + surfaced_count_raw + if isinstance(surfaced_count_raw, int) and not isinstance(surfaced_count_raw, bool) + else 0 + ) + if surfaced_count > 0: + issue_word = "issue" if surfaced_count == 1 else "issues" + title = f"Found {surfaced_count} potential {issue_word}" + else: + title = "Review completed without verdict" + return ( + "neutral", + title, + "Open SWE completed the review without an authoritative verdict.", ) diff --git a/agent/webhooks/github.py b/agent/webhooks/github.py index 89330a52..2cc5ff05 100644 --- a/agent/webhooks/github.py +++ b/agent/webhooks/github.py @@ -159,6 +159,7 @@ async def trigger_pr_review_from_ref( langgraph_client = common.get_client(url=common.LANGGRAPH_URL) if not await common._ensure_thread_exists_for_metadata(thread_id, langgraph_client): return {"success": False, "error": "Could not create reviewer thread"} + existing_metadata = await common._get_thread_metadata_safe(thread_id) or {} pr_meta: ReviewerPRMeta = { "owner": pr_ref.owner, @@ -179,6 +180,20 @@ async def trigger_pr_review_from_ref( await common.set_reviewer_thread_metadata( thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta, head_sha=head_sha ) + existing_check_id = existing_metadata.get("review_check_run_id") + existing_check_head = existing_metadata.get("head_sha") + if not (isinstance(existing_check_id, int) and existing_check_head == head_sha): + check_run_id = await common.create_review_check_run( + owner=pr_ref.owner, + repo=pr_ref.repo, + head_sha=head_sha, + token=app_token, + details_url=common.dashboard_thread_url(thread_id), + ) + if check_run_id is not None: + await common.set_reviewer_thread_metadata( + thread_id, extra={"review_check_run_id": check_run_id} + ) await common.post_review_started_comment( thread_id=thread_id, owner=pr_ref.owner, diff --git a/tests/github/test_github_checks.py b/tests/github/test_github_checks.py index 3d993f8d..64e10b93 100644 --- a/tests/github/test_github_checks.py +++ b/tests/github/test_github_checks.py @@ -1,10 +1,12 @@ from __future__ import annotations from typing import Any +from unittest.mock import AsyncMock, patch import httpx import pytest +from agent.middleware.settle_review_check import settle_review_check_on_exit from agent.review import publish as reviewer_publish from agent.utils import github_checks @@ -146,18 +148,86 @@ async def test_post_autofix_status_check_completes_neutral( assert body["details_url"] == "https://example.com/thread" -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 == "success" - assert "1 potential issue" in title - - conclusion, title, _ = github_checks.review_check_conclusion(3) - assert conclusion == "success" - assert "3 potential issues" in title +@pytest.mark.parametrize( + ("outcome", "expected_conclusion", "expected_title"), + [ + ( + { + "verdict_submitted": True, + "verdict_event": "APPROVE", + "blocking_finding_count": 2, + "verdict_authorization": "requested", + }, + "success", + "Review approved", + ), + ( + { + "verdict_submitted": True, + "verdict_event": "REQUEST_CHANGES", + "blocking_finding_count": 0, + "verdict_authorization": "requested", + }, + "failure", + "Changes requested", + ), + ( + {"blocking_finding_count": 0, "verdict_authorization": "none"}, + "success", + "No issues found", + ), + ( + {"blocking_finding_count": 2, "verdict_authorization": "consistent"}, + "failure", + "Found 2 blocking issues", + ), + ( + { + "blocking_finding_count": 1, + "verdict_authorization": "requested", + "verdict_ignored_reason": "self_review", + }, + "failure", + "Found 1 blocking issue", + ), + ( + { + "blocking_finding_count": 0, + "verdict_authorization": "consistent", + "verdict_ignored_reason": "self_review", + }, + "success", + "No issues found", + ), + ( + { + "blocking_finding_count": 1, + "verdict_authorization": "consistent", + "verdict_ignored_reason": "head_moved", + }, + "neutral", + "Verdict withheld", + ), + ( + { + "blocking_finding_count": 3, + "surfaced_count": 3, + "verdict_authorization": "none", + }, + "neutral", + "Found 3 potential issues", + ), + ({}, "neutral", "Review completed without verdict"), + ], +) +def test_review_check_conclusion_mapping( + outcome: dict[str, object], + expected_conclusion: str, + expected_title: str, +) -> None: + conclusion, title, _ = github_checks.review_check_conclusion(outcome) + assert conclusion == expected_conclusion + assert title == expected_title async def test_settle_review_check_run_noop_without_tracked_id( @@ -269,3 +339,66 @@ async def test_settle_review_check_run_keeps_id_on_patch_failure( }, } ] + + +async def test_settle_review_check_on_exit_without_publish_is_neutral() -> None: + settle = AsyncMock() + with ( + patch( + "agent.middleware.settle_review_check.get_config", + return_value={ + "configurable": { + "thread_id": "t1", + "repo": {"owner": "acme", "name": "widgets"}, + } + }, + ), + patch( + "agent.middleware.settle_review_check.get_thread_metadata", + AsyncMock(return_value={"review_check_run_id": 42}), + ), + patch("agent.middleware.settle_review_check.get_github_token", return_value="tok"), + patch("agent.middleware.settle_review_check.settle_review_check_run", settle), + ): + await settle_review_check_on_exit.aafter_agent({}, None) + + settle.assert_awaited_once() + assert settle.await_args.kwargs["conclusion"] == "neutral" + assert settle.await_args.kwargs["title"] == "Review did not complete" + + +@pytest.mark.parametrize("conclusion", ["success", "neutral", "failure"]) +async def test_settle_review_check_on_exit_preserves_pending_conclusion( + conclusion: str, +) -> None: + settle = AsyncMock() + with ( + patch( + "agent.middleware.settle_review_check.get_config", + return_value={ + "configurable": { + "thread_id": "t1", + "repo": {"owner": "acme", "name": "widgets"}, + } + }, + ), + patch( + "agent.middleware.settle_review_check.get_thread_metadata", + AsyncMock( + return_value={ + "review_check_run_id": 42, + "review_check_pending_result": { + "conclusion": conclusion, + "title": "Published result", + "summary": "Authoritative outcome", + }, + } + ), + ), + patch("agent.middleware.settle_review_check.get_github_token", return_value="tok"), + patch("agent.middleware.settle_review_check.settle_review_check_run", settle), + ): + await settle_review_check_on_exit.aafter_agent({}, None) + + assert settle.await_args.kwargs["conclusion"] == conclusion + assert settle.await_args.kwargs["title"] == "Published result" diff --git a/tests/github/test_github_issue_webhook.py b/tests/github/test_github_issue_webhook.py index 1e9313ca..95d8af66 100644 --- a/tests/github/test_github_issue_webhook.py +++ b/tests/github/test_github_issue_webhook.py @@ -6,6 +6,7 @@ import hmac import importlib import json import logging +from unittest.mock import AsyncMock from fastapi.testclient import TestClient @@ -1006,6 +1007,7 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None: def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: captured: dict[str, object] = {} + metadata_writes: list[dict[str, object]] = [] auto_review_checked = False async def fake_auto_review_enabled(_repo_config: dict[str, str]) -> bool: @@ -1058,7 +1060,14 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: async def fake_set_reviewer_thread_metadata(thread_id: str, **kwargs: object) -> None: captured["set_metadata_thread_id"] = thread_id - captured["set_metadata_kwargs"] = kwargs + metadata_writes.append(kwargs) + + async def fake_get_thread_metadata_safe(_thread_id: str) -> dict[str, object]: + return {} + + async def fake_create_review_check_run(**kwargs: object) -> int: + captured["check_run_kwargs"] = kwargs + return 77 monkeypatch.setattr(webhook_common, "_is_repo_auto_review_enabled", fake_auto_review_enabled) monkeypatch.setattr( @@ -1079,9 +1088,11 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: monkeypatch.setattr(webhook_common, "fetch_github_pr_metadata", fake_fetch_github_pr_metadata) monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", fake_cache_github_token) + monkeypatch.setattr(webhook_common, "_get_thread_metadata_safe", fake_get_thread_metadata_safe) monkeypatch.setattr( webhook_common, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata ) + monkeypatch.setattr(webhook_common, "create_review_check_run", fake_create_review_check_run) monkeypatch.setattr( webhook_common, "post_review_started_comment", fake_post_review_started_comment ) @@ -1124,7 +1135,15 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: } # The live head must be persisted to metadata so resolve_review_head_sha # doesn't return a stale head left by a prior push/ready dispatch. - assert captured["set_metadata_kwargs"]["head_sha"] == "head-sha" + assert any(write.get("head_sha") == "head-sha" for write in metadata_writes) + assert captured["check_run_kwargs"] == { + "owner": "langchain-ai", + "repo": "open-swe", + "head_sha": "head-sha", + "token": "app-token", + "details_url": webhook_common.dashboard_thread_url(str(captured["thread_id"])), + } + assert any(write.get("extra") == {"review_check_run_id": 77} for write in metadata_writes) # A live status comment is posted on dispatch so the PR shows "reviewing". assert captured["status_comment_kwargs"]["pr_number"] == 1244 assert config["verdict_authorized"] is True @@ -1136,6 +1155,65 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: } +def test_trigger_pr_review_from_ref_reuses_open_check_on_same_head(monkeypatch) -> None: + created_check = AsyncMock(return_value=88) + + async def fake_token() -> tuple[str | None, str | None]: + return "app-token", None + + async def fake_metadata(pr_ref: GitHubPrRef, *, token: str) -> dict[str, object]: + return { + "html_url": pr_ref.url, + "base": {"sha": "base-sha"}, + "head": {"sha": "head-sha", "ref": "feature-branch"}, + } + + class _FakeRunsClient: + async def create(self, *_args: object, **_kwargs: object) -> dict[str, str]: + return {"run_id": "run-1"} + + class _FakeThreadsClient: + async def create(self, **_kwargs: object) -> None: + return None + + class _FakeLangGraphClient: + runs = _FakeRunsClient() + threads = _FakeThreadsClient() + + monkeypatch.setattr(webhook_common, "get_github_app_installation_token_with_expiry", fake_token) + monkeypatch.setattr(webhook_common, "fetch_github_pr_metadata", fake_metadata) + monkeypatch.setattr( + webhook_common, + "_get_thread_metadata_safe", + AsyncMock(return_value={"review_check_run_id": 77, "head_sha": "head-sha"}), + ) + monkeypatch.setattr( + webhook_common, "_resolve_verdict_authorization", AsyncMock(return_value=True) + ) + monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None) + monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", AsyncMock()) + monkeypatch.setattr(webhook_common, "create_review_check_run", created_check) + monkeypatch.setattr(webhook_common, "post_review_started_comment", AsyncMock(return_value=1)) + monkeypatch.setattr(webhook_common, "_store_current_reviewer_run_id", AsyncMock()) + monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient()) + + result = asyncio.run( + github_webhooks.trigger_pr_review_from_ref( + GitHubPrRef( + owner="langchain-ai", + repo="open-swe", + number=1244, + url="https://github.com/langchain-ai/open-swe/pull/1244", + ), + source="dashboard", + request_verdict=True, + ) + ) + + assert result["success"] is True + created_check.assert_not_awaited() + + def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None: captured: dict[str, object] = {} @@ -1183,7 +1261,9 @@ def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization ) monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None) + monkeypatch.setattr(webhook_common, "_get_thread_metadata_safe", AsyncMock(return_value={})) monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", fake_async_noop) + monkeypatch.setattr(webhook_common, "create_review_check_run", fake_async_noop) monkeypatch.setattr(webhook_common, "post_review_started_comment", fake_async_noop) monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient()) diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index e7d672df..611c1388 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -756,6 +756,7 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> post_review = AsyncMock() set_metadata = AsyncMock() resolve_threads = AsyncMock(return_value=1) + settle_check = AsyncMock() with ( patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), @@ -766,6 +767,7 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> resolve_threads, ), patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + patch("agent.tools.publish_review.settle_review_check_run", settle_check), patch( "agent.tools.publish_review._maybe_post_slack_completion_reply", new_callable=AsyncMock, @@ -790,6 +792,8 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> assert result["surfaced_count"] == 0 assert result["resolved_thread_count"] == 1 assert result["skipped_empty_re_review"] is True + assert result["blocking_finding_count"] == 0 + assert settle_check.await_args.kwargs["conclusion"] == "success" @pytest.mark.asyncio @@ -2539,6 +2543,7 @@ def _verdict_publish_patches( post_review: AsyncMock, thread_metadata: dict[str, Any] | None = _UNSET_METADATA, dismiss: AsyncMock | None = None, + settle_check: AsyncMock | None = None, ) -> list[Any]: if thread_metadata is _UNSET_METADATA: thread_metadata = {"pr": {"author": "external-contributor"}} @@ -2556,7 +2561,10 @@ def _verdict_publish_patches( ), patch("agent.tools.publish_review.set_reviewer_thread_metadata", AsyncMock()), patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()), - patch("agent.tools.publish_review.settle_review_check_run", AsyncMock()), + patch( + "agent.tools.publish_review.settle_review_check_run", + settle_check or AsyncMock(), + ), patch( "agent.tools.publish_review.get_thread_metadata", AsyncMock(return_value=thread_metadata or {}), @@ -2741,11 +2749,13 @@ async def test_publish_async_self_review_downgrades_verdict_to_comment() -> None from agent.tools.publish_review import _publish_review_async post_review = AsyncMock(return_value={"id": 1001, "state": "COMMENTED"}) + settle_check = AsyncMock() with ExitStack() as stack: for p in _verdict_publish_patches( findings=[], post_review=post_review, thread_metadata={"pr": {"author": "seahaven-openswe[bot]"}}, + settle_check=settle_check, ): stack.enter_context(p) result = await _publish_review_async( @@ -2768,6 +2778,41 @@ async def test_publish_async_self_review_downgrades_verdict_to_comment() -> None assert post_review.await_args.kwargs["event"] == "COMMENT" body = post_review.await_args.kwargs["body"] assert "Verdict withheld (self review)" in body + assert settle_check.await_args.kwargs["conclusion"] == "success" + + +async def test_publish_async_self_review_with_findings_fails_check() -> None: + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)] + post_review = AsyncMock(return_value={"id": 1002, "state": "COMMENTED"}) + settle_check = AsyncMock() + with ExitStack() as stack: + for patcher in _verdict_publish_patches( + findings=findings, + post_review=post_review, + thread_metadata={"pr": {"author": "seahaven-openswe[bot]"}}, + settle_check=settle_check, + ): + stack.enter_context(patcher) + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + verdict="request_changes", + verdict_authorization="consistent", + ) + + assert result["verdict_ignored_reason"] == "self_review" + assert post_review.await_args.kwargs["event"] == "COMMENT" + assert settle_check.await_args.kwargs["conclusion"] == "failure" async def test_publish_async_dismisses_stale_approval_on_new_findings() -> None: