From e2abaf37879196557078378ae8121095a9b683e0 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Tue, 26 May 2026 18:16:11 -0700 Subject: [PATCH] fix: treat ready-for-review as reviewer re-review (#1336) Avoid reposting empty initial review summaries when a previously reviewed PR leaves draft without changing its reviewed head. --- agent/webapp.py | 48 +++++++++++++++++----- tests/test_pr_ready_auto_review.py | 66 ++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 10 deletions(-) diff --git a/agent/webapp.py b/agent/webapp.py index bb8c998d..73043112 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -1694,6 +1694,33 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou repo_config.get("owner", ""), repo_config.get("name", ""), pr_number ) + pr_meta: ReviewerPRMeta = { + "owner": repo_config.get("owner", ""), + "name": repo_config.get("name", ""), + "number": pr_number, + "url": pr_url, + "title": pr_title, + "head_ref": branch_name, + "base_ref": base_ref, + } + last_reviewed_sha = "" + if payload.get("action") == "ready_for_review": + metadata = await _get_thread_metadata_safe(thread_id) + if metadata is not None and metadata.get("kind") == REVIEWER_THREAD_KIND: + existing_last_reviewed_sha = metadata.get("last_reviewed_sha") + if isinstance(existing_last_reviewed_sha, str) and existing_last_reviewed_sha: + if existing_last_reviewed_sha == head_sha: + await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) + logger.info( + "Skipping ready_for_review auto-review for %s/%s#%s: " + "head_sha unchanged from last_reviewed_sha", + repo_config.get("owner"), + repo_config.get("name"), + pr_number, + ) + return + last_reviewed_sha = existing_last_reviewed_sha + app_token, app_token_expires_at = await get_github_app_installation_token_with_expiry() if not app_token: logger.warning("No GitHub App token available for reviewer dispatch") @@ -1709,18 +1736,17 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou logger.warning("Could not persist bot token for reviewer thread %s", thread_id) return - pr_meta: ReviewerPRMeta = { - "owner": repo_config.get("owner", ""), - "name": repo_config.get("name", ""), - "number": pr_number, - "url": pr_url, - "title": pr_title, - "head_ref": branch_name, - "base_ref": base_ref, - } await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) - prompt = build_github_pr_review_prompt(repo_config, pr_number, pr_url, base_sha, head_sha) + is_re_review = bool(last_reviewed_sha) + if is_re_review: + prompt = ( + f"PR #{pr_number} has been marked ready for review. The new HEAD is " + f"{head_sha}. Reconcile existing findings against the new diff, add any " + f"net-new findings, and call `publish_review` once you're done." + ) + else: + prompt = build_github_pr_review_prompt(repo_config, pr_number, pr_url, base_sha, head_sha) configurable = _build_reviewer_configurable( source=source, github_login=github_login, @@ -1731,6 +1757,8 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou base_sha=base_sha, head_sha=head_sha, branch_name=branch_name, + re_review=is_re_review, + last_reviewed_sha=last_reviewed_sha, ) thread_active = await is_thread_active(thread_id) diff --git a/tests/test_pr_ready_auto_review.py b/tests/test_pr_ready_auto_review.py index 75fbe12b..e84b1e74 100644 --- a/tests/test_pr_ready_auto_review.py +++ b/tests/test_pr_ready_auto_review.py @@ -61,6 +61,7 @@ async def test_pr_ready_for_review_triggers_run(monkeypatch: pytest.MonkeyPatch) fake_client = MagicMock() fake_client.runs.create = AsyncMock() _patch_dispatch_deps(monkeypatch, fake_client) + monkeypatch.setattr(webapp, "_get_thread_metadata_safe", AsyncMock(return_value=None)) monkeypatch.setattr(webapp, "get_profile", AsyncMock(return_value=None)) monkeypatch.setattr(webapp, "get_team_settings", AsyncMock(return_value={})) @@ -69,6 +70,71 @@ async def test_pr_ready_for_review_triggers_run(monkeypatch: pytest.MonkeyPatch) fake_client.runs.create.assert_awaited_once() +@pytest.mark.asyncio +async def test_pr_ready_for_review_skips_when_head_already_reviewed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + set_metadata = AsyncMock() + get_token = AsyncMock(return_value=("token", None)) + monkeypatch.setattr(webapp, "get_github_app_installation_token_with_expiry", get_token) + monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", set_metadata) + monkeypatch.setattr( + webapp, + "_get_thread_metadata_safe", + AsyncMock( + return_value={ + "kind": "reviewer", + "watch": False, + "last_reviewed_sha": "headsha", + } + ), + ) + monkeypatch.setattr(webapp, "get_client", lambda url: fake_client) + monkeypatch.setattr(webapp, "get_profile", AsyncMock(return_value=None)) + monkeypatch.setattr(webapp, "get_team_settings", AsyncMock(return_value={})) + + await webapp.process_github_pr_ready(_pr_payload(action="ready_for_review", draft=False)) + + fake_client.runs.create.assert_not_called() + get_token.assert_not_awaited() + set_metadata.assert_awaited_once() + assert set_metadata.await_args.kwargs["watch"] is True + + +@pytest.mark.asyncio +async def test_pr_ready_for_review_uses_re_review_after_previous_review( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, fake_client) + monkeypatch.setattr( + webapp, + "_get_thread_metadata_safe", + AsyncMock( + return_value={ + "kind": "reviewer", + "watch": False, + "last_reviewed_sha": "oldsha", + } + ), + ) + monkeypatch.setattr(webapp, "get_profile", AsyncMock(return_value=None)) + monkeypatch.setattr(webapp, "get_team_settings", AsyncMock(return_value={})) + + await webapp.process_github_pr_ready(_pr_payload(action="ready_for_review", draft=False)) + + fake_client.runs.create.assert_awaited_once() + _, kwargs = fake_client.runs.create.await_args + configurable = kwargs["config"]["configurable"] + assert configurable["re_review"] is True + assert configurable["last_reviewed_sha"] == "oldsha" + assert configurable["head_sha"] == "headsha" + assert "marked ready for review" in kwargs["input"]["messages"][0]["content"] + + @pytest.mark.asyncio async def test_pr_ready_draft_user_override_off_wins_over_team_on( monkeypatch: pytest.MonkeyPatch,