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,