diff --git a/agent/utils/github_checks.py b/agent/utils/github_checks.py index 46a291dd..e4faf81f 100644 --- a/agent/utils/github_checks.py +++ b/agent/utils/github_checks.py @@ -119,13 +119,15 @@ async def complete_review_check_run( 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``. + 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" return ( - "neutral", + "success", f"Found {surfaced_count} potential {issue_word}", f"Open SWE surfaced {surfaced_count} potential {issue_word} on this pull request.", ) diff --git a/agent/webapp.py b/agent/webapp.py index 10c3eac7..06f811b2 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -60,7 +60,7 @@ 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_checks import complete_review_check_run, create_review_check_run from .utils.github_comments import ( OPEN_SWE_TAGS, GitHubAuthError, @@ -2385,6 +2385,29 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: ) ): await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) + # The old head's check disappears once the head moves (GitHub only + # shows checks on the current head), so even though no re-review runs, + # surface a settled check on the new head. + unchanged_check_id = await create_review_check_run( + owner=repo_config["owner"], + repo=repo_config["name"], + head_sha=head_sha, + token=app_token, + details_url=dashboard_thread_url(thread_id), + ) + if unchanged_check_id is not None: + await complete_review_check_run( + owner=repo_config["owner"], + repo=repo_config["name"], + check_run_id=unchanged_check_id, + token=app_token, + conclusion="success", + title="No new changes to review", + summary=( + "The pull request diff is unchanged since the last reviewed " + f"commit {last_reviewed_sha}." + ), + ) logger.info( "Push to %s ignored: PR diff unchanged since last reviewed SHA %s", head_ref, @@ -2417,6 +2440,20 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: } await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha) + # GitHub only shows check runs on a PR's current head commit, so the check + # created on the previous head disappears after a follow-up push. Create a + # fresh in-progress check on the new head SHA so the review stays visible; + # publish (or the after-agent hook) settles this id. + check_run_id = await create_review_check_run( + owner=repo_config["owner"], + repo=repo_config["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}) + re_review_prompt = ( f"A new commit has been pushed to PR #{pr_number}. The new HEAD is " f"{head_sha}. Reconcile existing findings against the new diff, add any " diff --git a/tests/test_github_checks.py b/tests/test_github_checks.py index 93a00e03..23bd3036 100644 --- a/tests/test_github_checks.py +++ b/tests/test_github_checks.py @@ -117,11 +117,11 @@ def test_review_check_conclusion_mapping() -> None: assert title == "No issues found" conclusion, title, _ = github_checks.review_check_conclusion(1) - assert conclusion == "neutral" + assert conclusion == "success" assert "1 potential issue" in title conclusion, title, _ = github_checks.review_check_conclusion(3) - assert conclusion == "neutral" + assert conclusion == "success" assert "3 potential issues" in title diff --git a/tests/test_reviewer_watch.py b/tests/test_reviewer_watch.py index 23d74acf..b40afd83 100644 --- a/tests/test_reviewer_watch.py +++ b/tests/test_reviewer_watch.py @@ -134,6 +134,16 @@ async def test_push_event_skips_when_pr_diff_unchanged_since_last_review() -> No side_effect=["same diff", "same diff"], ), patch("agent.webapp.set_reviewer_thread_metadata", new=set_metadata), + patch( + "agent.webapp.create_review_check_run", + new_callable=AsyncMock, + return_value=42, + ) as create_check, + patch( + "agent.webapp.complete_review_check_run", + new_callable=AsyncMock, + return_value=True, + ) as complete_check, patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False), patch("agent.webapp.get_client", return_value=fake_client), ): @@ -142,6 +152,13 @@ async def test_push_event_skips_when_pr_diff_unchanged_since_last_review() -> No fake_client.runs.create.assert_not_called() set_metadata.assert_awaited_once() assert set_metadata.await_args.kwargs["last_reviewed_sha"] == "newsha" + # Even without a re-review, a settled check lands on the new head so the + # review stays visible after the head moves. + create_check.assert_awaited_once() + assert create_check.await_args.kwargs["head_sha"] == "newsha" + complete_check.assert_awaited_once() + assert complete_check.await_args.kwargs["check_run_id"] == 42 + assert complete_check.await_args.kwargs["conclusion"] == "success" @pytest.mark.asyncio @@ -258,6 +275,11 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None: "agent.webapp.set_reviewer_thread_metadata", new_callable=AsyncMock, ) as set_meta, + patch( + "agent.webapp.create_review_check_run", + new_callable=AsyncMock, + return_value=99, + ) as create_check, patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False), patch("agent.webapp.get_client", return_value=fake_client), ): @@ -278,6 +300,16 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None: if c.kwargs.get("head_sha") is not None ] assert "newsha" in head_sha_writes + # A fresh check run is created on the new head SHA (GitHub only shows + # checks on the current head), and its id is persisted for settling. + create_check.assert_awaited_once() + assert create_check.await_args.kwargs["head_sha"] == "newsha" + check_id_writes = [ + c.kwargs.get("extra", {}).get("review_check_run_id") + for c in set_meta.await_args_list + if "review_check_run_id" in (c.kwargs.get("extra") or {}) + ] + assert 99 in check_id_writes @pytest.mark.asyncio