From d2268871eb0f90899e3aebd1068baedd8518a656 Mon Sep 17 00:00:00 2001 From: Ramon Nogueira Date: Sun, 7 Jun 2026 01:17:09 -0400 Subject: [PATCH] feat: surface dashboard UI link on PR reviews [INF-0000] (#1440) * feat(reviewer): surface dashboard UI link on PR reviews [INF-0000] Post a transient "review in progress" comment (with an "Open in Web" dashboard link) when a reviewer run starts, then delete it once the review lands. The published review body now carries the same "Open in Web" link, so the link persists on the review itself. The transient comment's id is tracked in reviewer thread metadata (status_comment_id) so it can be deleted on completion. * refactor(reviewer): inline dashboard URL helper, drop redundant future import [INF-0000] --- agent/reviewer_publish.py | 138 ++++++++++++++++++++++++++++- agent/tools/publish_review.py | 7 ++ agent/utils/dashboard_links.py | 14 +++ agent/utils/slack.py | 15 +--- agent/webapp.py | 9 +- tests/test_github_issue_webhook.py | 14 +++ tests/test_reviewer_publish.py | 102 +++++++++++++++++++++ 7 files changed, 282 insertions(+), 17 deletions(-) create mode 100644 agent/utils/dashboard_links.py diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 90070f49..800a5575 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -18,8 +18,6 @@ After publish, the returned per-comment IDs get stored back on each Finding as the GraphQL ``resolveReviewThread`` mutation (REST doesn't expose this). """ -from __future__ import annotations - import json import logging import re @@ -27,7 +25,14 @@ from typing import Any, TypedDict import httpx -from .reviewer_findings import DiffSide, Finding, normalize_finding_title +from .reviewer_findings import ( + DiffSide, + Finding, + get_thread_metadata, + normalize_finding_title, + set_reviewer_thread_metadata, +) +from .utils.dashboard_links import dashboard_thread_url from .utils.github_token import GitHubAuthError logger = logging.getLogger(__name__) @@ -265,6 +270,7 @@ def render_review_body( pr_number: int, surfaced_count: int, trace_url: str | None = None, + ui_url: str | None = None, out_of_diff_findings: list[Finding] | None = None, ) -> str: """Compose the top-level review body.""" @@ -283,12 +289,136 @@ def render_review_body( parts = [headline] if out_of_diff_findings: parts.append(render_out_of_diff_section(out_of_diff_findings)) + links = [] + if ui_url: + links.append(f"[Open in Web]({ui_url})") if trace_url: - parts.append(f"[View Open SWE trace]({trace_url})") + links.append(f"[View Open SWE trace]({trace_url})") + if links: + parts.append(" • ".join(links)) parts.append(review_summary_marker(pr_number)) return "\n\n".join(parts) +def status_comment_marker(pr_number: int) -> str: + """Hidden marker stamped on the live status comment for a PR.""" + return f"" + + +def render_status_comment( + *, + pr_number: int, + thread_id: str | None = None, + trace_url: str | None = None, +) -> str: + """Compose the transient "review in progress" comment. + + Posted when a review starts so the PR shows activity and a clickable + "Open in Web" link while the run is live; deleted once ``publish_review`` + posts the review (which carries the same link). + """ + parts = ["## 🔍 Open SWE Review: in progress\n\nOpen SWE is reviewing this PR…"] + links = [] + ui_url = dashboard_thread_url(thread_id) if thread_id else None + if ui_url: + links.append(f"[Open in Web]({ui_url})") + if trace_url: + links.append(f"[View Open SWE trace]({trace_url})") + if links: + parts.append(" • ".join(links)) + parts.append(status_comment_marker(pr_number)) + return "\n\n".join(parts) + + +async def post_status_comment( + *, + owner: str, + repo: str, + pr_number: int, + body: str, + token: str, +) -> int | None: + """POST the live status comment to a PR. Returns its comment id or None.""" + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/issues/{pr_number}/comments" + async with httpx.AsyncClient() as client: + try: + response = await client.post( + url, headers=_github_headers(token), json={"body": body}, timeout=30 + ) + response.raise_for_status() + except httpx.HTTPError: + logger.exception("Failed to post status comment for %s/%s#%s", owner, repo, pr_number) + return None + data = response.json() + comment_id = data.get("id") if isinstance(data, dict) else None + return comment_id if isinstance(comment_id, int) else None + + +async def delete_status_comment( + *, + owner: str, + repo: str, + comment_id: int, + token: str, +) -> bool: + """DELETE a status comment by id. Returns True on success (or if gone).""" + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/issues/comments/{comment_id}" + async with httpx.AsyncClient() as client: + try: + response = await client.delete(url, headers=_github_headers(token), timeout=30) + if response.status_code == 404: # noqa: PLR2004 + return True + response.raise_for_status() + except httpx.HTTPError: + logger.exception("Failed to delete status comment %s on %s/%s", comment_id, owner, repo) + return False + return True + + +async def post_review_started_comment( + *, + thread_id: str, + owner: str, + repo: str, + pr_number: int, + token: str, + trace_url: str | None = None, +) -> int | None: + """Post (or refresh) the transient "review in progress" comment. + + Reuses the ``status_comment_id`` persisted in reviewer thread metadata when + one already lingers (a prior run that never settled), otherwise posts a + fresh comment and stores its id so ``clear_review_started_comment`` can + delete it once the review lands. + """ + metadata = await get_thread_metadata(thread_id) + existing_id = metadata.get("status_comment_id") + if isinstance(existing_id, int): + await delete_status_comment(owner=owner, repo=repo, comment_id=existing_id, token=token) + body = render_status_comment(pr_number=pr_number, thread_id=thread_id, trace_url=trace_url) + new_id = await post_status_comment( + owner=owner, repo=repo, pr_number=pr_number, body=body, token=token + ) + await set_reviewer_thread_metadata(thread_id, extra={"status_comment_id": new_id}) + return new_id + + +async def clear_review_started_comment( + *, + thread_id: str, + owner: str, + repo: str, + token: str, +) -> None: + """Delete the transient "review in progress" comment, if one is tracked.""" + metadata = await get_thread_metadata(thread_id) + comment_id = metadata.get("status_comment_id") + if not isinstance(comment_id, int): + return + await delete_status_comment(owner=owner, repo=repo, comment_id=comment_id, token=token) + await set_reviewer_thread_metadata(thread_id, extra={"status_comment_id": None}) + + async def open_swe_review_exists( *, owner: str, diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 852d7264..4b66a5c3 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -25,6 +25,7 @@ from ..reviewer_findings import ( list_findings as list_findings_async, ) from ..reviewer_publish import ( + clear_review_started_comment, fetch_pr_review_threads, fetch_review_comments, fetch_review_thread_id_for_comment, @@ -38,6 +39,7 @@ from ..reviewer_publish import ( resolve_review_thread, ) from ..reviewer_reconcile import reconcile_findings_with_review_threads +from ..utils.dashboard_links import dashboard_thread_url from ..utils.github_token import ( GitHubAuthError, get_github_token, @@ -219,6 +221,7 @@ async def _publish_review_async( # reviewed, not the stale one this run was created for. head_sha = await resolve_review_head_sha(thread_id, {"head_sha": head_sha}) review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override) + review_ui_url = dashboard_thread_url(thread_id) findings = await _backfill_findings_from_pr_threads( thread_id=thread_id, owner=owner, @@ -293,6 +296,7 @@ async def _publish_review_async( findings=findings, ) 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) return { "success": True, "review_id": None, @@ -306,6 +310,7 @@ async def _publish_review_async( pr_number=pr_number, surfaced_count=len(inline_comments), trace_url=review_trace_url, + ui_url=review_ui_url, out_of_diff_findings=eligible_out_of_diff, ) @@ -340,6 +345,7 @@ async def _publish_review_async( pr_number=pr_number, surfaced_count=len(retry_inline), trace_url=review_trace_url, + ui_url=review_ui_url, out_of_diff_findings=eligible_out_of_diff, ) retry_response = await post_pull_request_review( @@ -470,6 +476,7 @@ 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) result: dict[str, Any] = { "success": True, diff --git a/agent/utils/dashboard_links.py b/agent/utils/dashboard_links.py new file mode 100644 index 00000000..5639c5ae --- /dev/null +++ b/agent/utils/dashboard_links.py @@ -0,0 +1,14 @@ +"""Shared builders for dashboard ("Open in Web") URLs.""" + +import os +from urllib.parse import quote + +_DEFAULT_DASHBOARD_BASE_URL = "https://openswe.vercel.app" + + +def dashboard_thread_url(thread_id: str) -> str | None: + """Build the dashboard thread URL for a given thread id.""" + base_url = os.environ.get("DASHBOARD_BASE_URL", _DEFAULT_DASHBOARD_BASE_URL).strip().rstrip("/") + if not base_url or not thread_id: + return None + return f"{base_url}/agents/{quote(thread_id, safe='')}" diff --git a/agent/utils/slack.py b/agent/utils/slack.py index 63558b90..f53354f8 100644 --- a/agent/utils/slack.py +++ b/agent/utils/slack.py @@ -12,11 +12,12 @@ import re import time from dataclasses import dataclass from typing import Any -from urllib.parse import quote, urlparse +from urllib.parse import urlparse import httpx from langgraph_sdk.client import LangGraphClient +from agent.utils.dashboard_links import dashboard_thread_url from agent.utils.langsmith import get_langsmith_trace_url logger = logging.getLogger(__name__) @@ -684,16 +685,6 @@ TRACE_REPLY_TIPS: tuple[str, ...] = ( ) -def _get_dashboard_thread_url(thread_id: str) -> str | None: - """Build the dashboard thread URL for a given thread ID.""" - base_url = ( - os.environ.get("DASHBOARD_BASE_URL", "https://openswe.vercel.app").strip().rstrip("/") - ) - if not base_url: - return None - return f"{base_url}/agents/{quote(thread_id, safe='')}" - - def _format_trace_reply(trace_url: str | None, dashboard_url: str | None) -> str: """Format the initial trace reply with a randomly selected tip.""" tip = random.choice(TRACE_REPLY_TIPS) @@ -711,7 +702,7 @@ async def post_slack_trace_reply( ) -> str | None: """Post a trace URL reply in a Slack thread and return its Slack timestamp.""" trace_url = get_langsmith_trace_url(thread_id) - dashboard_url = _get_dashboard_thread_url(thread_id) if include_dashboard_link else None + dashboard_url = dashboard_thread_url(thread_id) if include_dashboard_link else None message_ts, _ = await post_slack_thread_reply_with_ts( channel_id, thread_ts, diff --git a/agent/webapp.py b/agent/webapp.py index 4c900625..d181f599 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -49,7 +49,7 @@ from .reviewer_findings import ( from .reviewer_findings import ( list_findings as list_reviewer_findings, ) -from .reviewer_publish import fetch_pr_review_threads +from .reviewer_publish import fetch_pr_review_threads, post_review_started_comment from .reviewer_reconcile import reconcile_findings_with_review_threads from .utils.auth import ( is_bot_token_only_mode, @@ -1854,6 +1854,13 @@ async def trigger_pr_review_from_ref( await set_reviewer_thread_metadata( thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta, head_sha=head_sha ) + await post_review_started_comment( + thread_id=thread_id, + owner=pr_ref.owner, + repo=pr_ref.repo, + pr_number=pr_ref.number, + token=app_token, + ) 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 e4e7466f..043c010c 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -767,9 +767,15 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None: "get_github_app_installation_token_with_expiry", fake_get_github_app_installation_token_with_expiry, ) + + async def fake_post_review_started_comment(**kwargs: object) -> int: + captured["status_comment_kwargs"] = kwargs + return 1 + monkeypatch.setattr(webapp, "cache_github_token_for_thread", fake_cache_github_token) monkeypatch.setattr(webapp, "is_thread_active", fake_is_thread_active) monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata) + monkeypatch.setattr(webapp, "post_review_started_comment", fake_post_review_started_comment) monkeypatch.setattr(webapp, "get_client", lambda url: _FakeLangGraphClient()) asyncio.run( @@ -862,10 +868,16 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: "get_github_app_installation_token_with_expiry", fake_get_github_app_installation_token_with_expiry, ) + + async def fake_post_review_started_comment(**kwargs: object) -> int: + captured["status_comment_kwargs"] = kwargs + return 1 + monkeypatch.setattr(webapp, "fetch_github_pr_metadata", fake_fetch_github_pr_metadata) monkeypatch.setattr(webapp, "cache_github_token_for_thread", fake_cache_github_token) monkeypatch.setattr(webapp, "is_thread_active", fake_is_thread_active) monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata) + monkeypatch.setattr(webapp, "post_review_started_comment", fake_post_review_started_comment) monkeypatch.setattr(webapp, "get_client", lambda url: _FakeLangGraphClient()) result = asyncio.run( @@ -905,6 +917,8 @@ 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" + # A live status comment is posted on dispatch so the PR shows "reviewing". + assert captured["status_comment_kwargs"]["pr_number"] == 1244 def test_trigger_pr_review_from_ref_respects_dashboard_opt_in(monkeypatch) -> None: diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 74f93655..ee91e8a4 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -10,17 +10,21 @@ import pytest from agent.reviewer_findings import Finding, new_finding from agent.reviewer_publish import ( + clear_review_started_comment, fetch_pr_review_threads, open_swe_review_exists, parse_review_comment_marker, post_pull_request_review, + post_review_started_comment, render_inline_comment_body, render_inline_comment_payload, render_resolution_comment, render_review_body, + render_status_comment, reply_to_review_comment, resolve_review_thread, review_summary_marker, + status_comment_marker, ) @@ -45,6 +49,7 @@ def _isolate_publish_review_pr_state() -> Iterator[None]: patch("agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=[])), patch("agent.tools.publish_review.replace_findings", AsyncMock()), patch("agent.tools.publish_review.open_swe_review_exists", AsyncMock(return_value=False)), + patch("agent.tools.publish_review.clear_review_started_comment", AsyncMock()), patch( "agent.tools.publish_review.resolve_review_head_sha", AsyncMock( @@ -208,6 +213,103 @@ def test_render_review_body_no_findings_message() -> None: assert "Open SWE reviewed this PR and found no potential bugs to report." in body +def test_render_status_comment_reviewing_includes_ui_link(monkeypatch: Any) -> None: + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://dash.example") + body = render_status_comment(pr_number=7, thread_id="tid-1") + assert "🔍 Open SWE Review: in progress" in body + assert "[Open in Web](https://dash.example/agents/tid-1)" in body + assert status_comment_marker(7) in body + + +def test_render_review_body_includes_ui_link() -> None: + body = render_review_body( + pr_number=7, surfaced_count=0, ui_url="https://dash.example/agents/tid-1" + ) + assert "[Open in Web](https://dash.example/agents/tid-1)" in body + + +def test_render_review_body_orders_ui_link_before_trace() -> None: + body = render_review_body( + pr_number=7, + surfaced_count=1, + ui_url="https://dash.example/agents/tid-1", + trace_url="https://trace.example/x", + ) + assert "[Open in Web](https://dash.example/agents/tid-1) • [View Open SWE trace]" in body + + +@pytest.mark.asyncio +async def test_post_review_started_comment_posts_and_persists_id() -> None: + post = AsyncMock(return_value=4242) + set_meta = AsyncMock() + with ( + patch("agent.reviewer_publish.get_thread_metadata", AsyncMock(return_value={})), + patch("agent.reviewer_publish.post_status_comment", post), + patch("agent.reviewer_publish.delete_status_comment", AsyncMock()) as delete, + patch("agent.reviewer_publish.set_reviewer_thread_metadata", set_meta), + ): + cid = await post_review_started_comment( + thread_id="tid", owner="o", repo="r", pr_number=7, token="t" + ) + assert cid == 4242 + delete.assert_not_called() + post.assert_awaited_once() + set_meta.assert_awaited_once_with("tid", extra={"status_comment_id": 4242}) + + +@pytest.mark.asyncio +async def test_post_review_started_comment_deletes_lingering_before_reposting() -> None: + post = AsyncMock(return_value=500) + delete = AsyncMock(return_value=True) + with ( + patch( + "agent.reviewer_publish.get_thread_metadata", + AsyncMock(return_value={"status_comment_id": 99}), + ), + patch("agent.reviewer_publish.post_status_comment", post), + patch("agent.reviewer_publish.delete_status_comment", delete), + patch("agent.reviewer_publish.set_reviewer_thread_metadata", AsyncMock()), + ): + cid = await post_review_started_comment( + thread_id="tid", owner="o", repo="r", pr_number=7, token="t" + ) + assert cid == 500 + delete.assert_awaited_once() + assert delete.await_args.kwargs["comment_id"] == 99 + + +@pytest.mark.asyncio +async def test_clear_review_started_comment_deletes_and_clears_metadata() -> None: + delete = AsyncMock(return_value=True) + set_meta = AsyncMock() + with ( + patch( + "agent.reviewer_publish.get_thread_metadata", + AsyncMock(return_value={"status_comment_id": 99}), + ), + patch("agent.reviewer_publish.delete_status_comment", delete), + patch("agent.reviewer_publish.set_reviewer_thread_metadata", set_meta), + ): + await clear_review_started_comment(thread_id="tid", owner="o", repo="r", token="t") + delete.assert_awaited_once() + assert delete.await_args.kwargs["comment_id"] == 99 + set_meta.assert_awaited_once_with("tid", extra={"status_comment_id": None}) + + +@pytest.mark.asyncio +async def test_clear_review_started_comment_noop_without_tracked_id() -> None: + delete = AsyncMock() + set_meta = AsyncMock() + with ( + patch("agent.reviewer_publish.get_thread_metadata", AsyncMock(return_value={})), + patch("agent.reviewer_publish.delete_status_comment", delete), + patch("agent.reviewer_publish.set_reviewer_thread_metadata", set_meta), + ): + await clear_review_started_comment(thread_id="tid", owner="o", repo="r", token="t") + delete.assert_not_called() + set_meta.assert_not_called() + + def test_render_review_body_with_only_out_of_diff_findings() -> None: body = render_review_body( pr_number=7,