From 892347041fb1db17ada980ab325e6f2df207cfe2 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 7 May 2026 16:11:36 -0700 Subject: [PATCH] feat(open-swe): post review summary to Slack on first review (#1258) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a Slack user kicks off a PR review with `@open-swe review `, the reviewer agent now posts a one-line summary back to the Slack thread when it finishes — either "No issues found" or "found N potential issue(s)" with a link to the GitHub review. The reviewer agent has no Slack tools by design, so the summary is sent host-side from `publish_review` after the GitHub review POST succeeds. The Slack channel/thread_ts is persisted on reviewer thread metadata at trigger time and read back on publish. Re-reviews triggered by push events stay silent in Slack to avoid noise on the original thread. --- agent/reviewer_findings.py | 23 ++++ agent/tools/publish_review.py | 50 ++++++++ agent/webapp.py | 20 ++- tests/test_github_issue_webhook.py | 6 + tests/test_reviewer_publish.py | 200 +++++++++++++++++++++++++++++ 5 files changed, 297 insertions(+), 2 deletions(-) diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index bd282547..04d9e6b1 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -71,6 +71,13 @@ class ReviewerPRMeta(TypedDict, total=False): base_ref: str +class ReviewerSlackThread(TypedDict, total=False): + """Slack thread that initiated this review — used to post a completion reply.""" + + channel_id: str + thread_ts: str + + def new_finding_id() -> str: """Return a stable, short, URL-friendly finding id (``f_``).""" return f"f_{uuid.uuid4().hex[:10]}" @@ -206,6 +213,7 @@ async def set_reviewer_thread_metadata( last_reviewed_sha: str | None = None, watch: bool | None = None, findings: list[Finding] | None = None, + slack_thread: ReviewerSlackThread | None = None, extra: dict[str, Any] | None = None, ) -> None: """Persist reviewer-thread-level metadata. @@ -224,6 +232,8 @@ async def set_reviewer_thread_metadata( metadata["watch"] = watch if findings is not None: metadata["findings"] = findings + if slack_thread is not None: + metadata["slack_thread"] = slack_thread if extra: metadata.update(extra) await client.threads.update(thread_id=thread_id, metadata=metadata) @@ -245,6 +255,19 @@ def get_thread_pr_meta(metadata: dict[str, Any]) -> ReviewerPRMeta | None: return cast(ReviewerPRMeta, pr) +def get_thread_slack_ref(metadata: dict[str, Any]) -> ReviewerSlackThread | None: + slack_thread = metadata.get("slack_thread") + if not isinstance(slack_thread, dict): + return None + channel_id = slack_thread.get("channel_id") + thread_ts = slack_thread.get("thread_ts") + if not isinstance(channel_id, str) or not isinstance(thread_ts, str): + return None + if not channel_id or not thread_ts: + return None + return cast(ReviewerSlackThread, slack_thread) + + def filter_findings_for_publish( findings: list[Finding], *, diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index b7c397e2..2c3566f3 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -11,6 +11,8 @@ from ..reviewer_findings import ( Severity, filter_findings_for_publish, get_thread_id_from_runtime, + get_thread_metadata, + get_thread_slack_ref, replace_findings, set_reviewer_thread_metadata, ) @@ -26,6 +28,7 @@ from ..reviewer_publish import ( resolve_review_thread, ) from ..utils.github_token import get_github_token +from ..utils.slack import post_slack_thread_reply def publish_review( @@ -70,6 +73,7 @@ def publish_review( repo_config = configurable.get("repo") if isinstance(configurable, dict) else None pr_number = configurable.get("pr_number") if isinstance(configurable, dict) else None head_sha = configurable.get("head_sha") if isinstance(configurable, dict) else None + is_re_review = bool(configurable.get("re_review")) if isinstance(configurable, dict) else False if ( not isinstance(repo_config, dict) @@ -95,6 +99,7 @@ def publish_review( token=token, severity_threshold=_cast_severity(severity_threshold), cap=cap, + is_re_review=is_re_review, ) ) @@ -112,6 +117,7 @@ async def _publish_review_async( token: str, severity_threshold: Severity, cap: int, + is_re_review: bool, ) -> dict[str, Any]: thread_id = get_thread_id_from_runtime() findings = await list_findings_async(thread_id) @@ -178,6 +184,16 @@ async def _publish_review_async( findings=await list_findings_async(thread_id), ) + if not is_re_review: + await _maybe_post_slack_completion_reply( + thread_id=thread_id, + owner=owner, + repo=repo, + pr_number=pr_number, + review_id=review_id, + surfaced_count=len(inline_comments), + ) + await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) return { @@ -189,6 +205,40 @@ async def _publish_review_async( } +async def _maybe_post_slack_completion_reply( + *, + thread_id: str, + owner: str, + repo: str, + pr_number: int, + review_id: int | None, + surfaced_count: int, +) -> None: + """Post a one-line completion summary to the Slack thread that started this review. + + Only fires for first reviews (gated by the caller). No-op if the reviewer + thread has no ``slack_thread`` metadata — i.e. the review wasn't started + from Slack. + """ + metadata = await get_thread_metadata(thread_id) + slack_ref = get_thread_slack_ref(metadata) + if slack_ref is None: + return + + if surfaced_count == 0: + headline = "*Open SWE Review*: No issues found." + else: + issue_word = "issue" if surfaced_count == 1 else "issues" + headline = f"*Open SWE Review* found {surfaced_count} potential {issue_word}." + + review_url = f"https://github.com/{owner}/{repo}/pull/{pr_number}" + if isinstance(review_id, int): + review_url = f"{review_url}#pullrequestreview-{review_id}" + text = f"{headline} <{review_url}|View review>" + + await post_slack_thread_reply(slack_ref["channel_id"], slack_ref["thread_ts"], text) + + async def _store_comment_ids_on_findings( *, thread_id: str, diff --git a/agent/webapp.py b/agent/webapp.py index 84d37eaf..e671348c 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -19,6 +19,7 @@ from langgraph_sdk.client import LangGraphClient from .reviewer_findings import ( REVIEWER_THREAD_KIND, ReviewerPRMeta, + ReviewerSlackThread, set_reviewer_thread_metadata, ) from .utils.auth import ( @@ -913,7 +914,12 @@ async def process_slack_mention(event_data: dict[str, Any], repo_config: dict[st async def process_slack_pr_review_request( pr_ref: GitHubPrRef, channel_id: str, thread_ts: str ) -> None: - result = await trigger_pr_review_from_ref(pr_ref, source="slack") + result = await trigger_pr_review_from_ref( + pr_ref, + source="slack", + slack_channel_id=channel_id, + slack_thread_ts=thread_ts, + ) if result.get("success"): thread_id = result.get("thread_id") if isinstance(thread_id, str) and thread_id: @@ -1383,6 +1389,8 @@ async def trigger_pr_review_from_ref( source: str, github_login: str = "", github_user_id: int | None = None, + slack_channel_id: str = "", + slack_thread_ts: str = "", ) -> dict[str, Any]: repo_config = {"owner": pr_ref.owner, "name": pr_ref.repo} if not _is_repo_allowed_for_reviewer(repo_config): @@ -1428,7 +1436,15 @@ async def trigger_pr_review_from_ref( "head_ref": branch_name, "base_ref": base_ref, } - await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) + slack_thread_meta: ReviewerSlackThread | None = None + if slack_channel_id and slack_thread_ts: + slack_thread_meta = { + "channel_id": slack_channel_id, + "thread_ts": slack_thread_ts, + } + await set_reviewer_thread_metadata( + thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta + ) prompt = build_github_pr_review_prompt(repo_config, pr_ref.number, pr_url, base_sha, head_sha) configurable = _build_reviewer_configurable( diff --git a/tests/test_github_issue_webhook.py b/tests/test_github_issue_webhook.py index 6d5f8cc9..d2553acc 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -594,9 +594,13 @@ def test_process_slack_pr_review_request_posts_trace_reply(monkeypatch) -> None: source: str, github_login: str = "", github_user_id: int | None = None, + slack_channel_id: str = "", + slack_thread_ts: str = "", ) -> dict[str, object]: captured["pr_ref"] = pr_ref captured["source"] = source + captured["slack_channel_id"] = slack_channel_id + captured["slack_thread_ts"] = slack_thread_ts return {"success": True, "thread_id": "reviewer-thread-id", "pr_url": pr_ref.url} async def fake_post_slack_trace_reply( @@ -626,6 +630,8 @@ def test_process_slack_pr_review_request_posts_trace_reply(monkeypatch) -> None: ) assert captured["source"] == "slack" + assert captured["slack_channel_id"] == "C123" + assert captured["slack_thread_ts"] == "1700000000.000100" assert captured["trace_reply"] == { "channel_id": "C123", "thread_ts": "1700000000.000100", diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 2e327bda..749073b9 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -143,6 +143,10 @@ async def test_publish_review_skips_findings_already_published() -> None: return_value=0, ), patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + patch( + "agent.tools.publish_review._maybe_post_slack_completion_reply", + new_callable=AsyncMock, + ), ): result = await _publish_review_async( owner="o", @@ -152,6 +156,7 @@ async def test_publish_review_skips_findings_already_published() -> None: token="t", severity_threshold="medium", cap=15, + is_re_review=False, ) assert result["success"] is True @@ -182,6 +187,10 @@ async def test_publish_review_posts_summary_when_no_findings() -> None: return_value=0, ), patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + patch( + "agent.tools.publish_review._maybe_post_slack_completion_reply", + new_callable=AsyncMock, + ), ): result = await _publish_review_async( owner="o", @@ -191,6 +200,7 @@ async def test_publish_review_posts_summary_when_no_findings() -> None: token="t", severity_threshold="medium", cap=15, + is_re_review=False, ) assert result["success"] is True @@ -201,3 +211,193 @@ async def test_publish_review_posts_summary_when_no_findings() -> None: posted_inline = post_review.await_args.kwargs["inline_comments"] assert posted_inline == [] assert "No issues found" in posted_body + + +@pytest.mark.asyncio +async def test_publish_review_posts_slack_reply_on_first_review_with_slack_ref() -> None: + """A first review with a slack_thread metadata ref posts a one-line summary.""" + from agent.tools.publish_review import _publish_review_async + + metadata = { + "kind": "reviewer", + "slack_thread": {"channel_id": "C1", "thread_ts": "1234.5"}, + } + slack_post = AsyncMock(return_value=True) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review.post_pull_request_review", + AsyncMock(return_value={"id": 42}), + ), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + patch( + "agent.tools.publish_review.get_thread_metadata", + new_callable=AsyncMock, + return_value=metadata, + ), + patch("agent.tools.publish_review.post_slack_thread_reply", slack_post), + ): + 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, + ) + + slack_post.assert_awaited_once() + args = slack_post.await_args.args + assert args[0] == "C1" + assert args[1] == "1234.5" + assert "No issues found" in args[2] + assert "https://github.com/o/r/pull/7#pullrequestreview-42" in args[2] + + +@pytest.mark.asyncio +async def test_publish_review_uses_plural_findings_in_slack_reply() -> None: + """Surfaced count > 1 should pluralize 'issues' in the slack summary.""" + from agent.tools.publish_review import _publish_review_async + + findings = [ + _f(id="f1", file="a.py", start_line=1, end_line=1), + _f(id="f2", file="b.py", start_line=2, end_line=2), + ] + metadata = { + "kind": "reviewer", + "slack_thread": {"channel_id": "C1", "thread_ts": "1234.5"}, + } + slack_post = AsyncMock(return_value=True) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)), + patch( + "agent.tools.publish_review.post_pull_request_review", + AsyncMock(return_value={"id": 99}), + ), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + patch( + "agent.tools.publish_review.get_thread_metadata", + new_callable=AsyncMock, + return_value=metadata, + ), + patch("agent.tools.publish_review.post_slack_thread_reply", slack_post), + ): + 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, + ) + + slack_post.assert_awaited_once() + text = slack_post.await_args.args[2] + assert "found 2 potential issues" in text + + +@pytest.mark.asyncio +async def test_publish_review_skips_slack_reply_on_re_review() -> None: + """Re-reviews must NOT post to Slack even when slack_thread metadata is set.""" + from agent.tools.publish_review import _publish_review_async + + metadata = { + "kind": "reviewer", + "slack_thread": {"channel_id": "C1", "thread_ts": "1234.5"}, + } + slack_post = AsyncMock(return_value=True) + get_metadata = AsyncMock(return_value=metadata) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review.post_pull_request_review", + AsyncMock(return_value={"id": 1}), + ), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + patch("agent.tools.publish_review.get_thread_metadata", get_metadata), + patch("agent.tools.publish_review.post_slack_thread_reply", slack_post), + ): + await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=True, + ) + + slack_post.assert_not_awaited() + # Re-review path should also avoid even fetching the slack metadata. + get_metadata.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_publish_review_skips_slack_reply_when_no_slack_ref() -> None: + """A review started from GitHub (no slack_thread metadata) must not post to Slack.""" + from agent.tools.publish_review import _publish_review_async + + slack_post = AsyncMock(return_value=True) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review.post_pull_request_review", + AsyncMock(return_value={"id": 1}), + ), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + patch( + "agent.tools.publish_review.get_thread_metadata", + new_callable=AsyncMock, + return_value={"kind": "reviewer"}, + ), + patch("agent.tools.publish_review.post_slack_thread_reply", slack_post), + ): + 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, + ) + + slack_post.assert_not_awaited()