From b2a0ac3b795c5327b43d4be61b06ce676ffb07ce Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 22 May 2026 13:36:14 -0700 Subject: [PATCH] feat: auto-review PRs on opened / ready-for-review (#1325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: auto-review PRs on opened / ready-for-review Trigger Open SWE Review on `pull_request` actions `opened` and `ready_for_review` against the canonical reviewer thread (no need to request open-swe[bot] as a reviewer). `converted_to_draft` now also flips watch=False on the existing reviewer thread. Draft PRs are gated by a tri-state user setting on the profile: inherit team default, always on, or always off. The team-wide `review_draft_prs` setting is the org-wide default; each user can override it in My Settings. External contributors with no Open SWE profile fall back to the team default. * fix: PR review comments — auth source + draft-aware watch toggle - `process_github_pr_ready` now dispatches with `source="github"` so the auth resolver finds the bot token persisted on the thread. The previous `source="github_auto"` fell through to the email-based path in non bot-token-only deployments and failed with a missing-user-email error. - `converted_to_draft` no longer unconditionally clears `watch`. When the PR author's effective `review_draft_prs` setting is on, watch stays on so subsequent pushes still trigger re-reviews while the PR is in draft. * feat(reviewer): skip "no issues found" comment on empty re-reviews A re-review run with no new findings to surface no longer posts another "Open SWE Review: No issues found" comment on the PR. The "no issues" summary now only appears on the first review of a PR — matching Devin's behavior, where subsequent reviews are silent unless there's something new to flag. Resolved-thread reconciliation and ``last_reviewed_sha`` persistence still happen on the skipped path, so findings the user just fixed still get their GitHub threads marked resolved, and the next push event sees an up-to-date dedup SHA. --------- Co-authored-by: open-swe[bot] --- agent/dashboard/profiles.py | 2 + agent/tools/publish_review.py | 29 ++- agent/webapp.py | 111 ++++++++++-- tests/test_pr_ready_auto_review.py | 273 +++++++++++++++++++++++++++++ tests/test_reviewer_publish.py | 65 +++++++ ui/src/lib/api.ts | 2 + ui/src/lib/profile.ts | 1 + ui/src/routes/my-settings.tsx | 81 ++++++++- ui/src/routes/review.tsx | 2 +- 9 files changed, 551 insertions(+), 15 deletions(-) create mode 100644 tests/test_pr_ready_auto_review.py diff --git a/agent/dashboard/profiles.py b/agent/dashboard/profiles.py index edf81d26..21509dfc 100644 --- a/agent/dashboard/profiles.py +++ b/agent/dashboard/profiles.py @@ -40,6 +40,7 @@ class ProfileUpdate(BaseModel): branch_prefix: str | None = None auto_fix_ci: bool = True create_prs: bool = True + review_draft_prs: bool | None = None @field_validator("default_model") @classmethod @@ -95,6 +96,7 @@ async def upsert_profile(login: str, email: str, update: ProfileUpdate) -> dict[ "branch_prefix": update.branch_prefix, "auto_fix_ci": update.auto_fix_ci, "create_prs": update.create_prs, + "review_draft_prs": update.review_draft_prs, "updated_at": datetime.now(UTC).isoformat(), } for stale_field in ( diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 6eef1e80..e31dd9f7 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -51,7 +51,11 @@ def publish_review( 3. POST a single GitHub PR Review with the eligible findings as inline comments. ``finding.suggestion`` becomes a ```suggestion``` block (the "Commit suggestion" UX). The review body is a fixed, - host-formatted summary line — you do not write it. + host-formatted summary line — you do not write it. On a re-review + run with no new findings to surface, the GitHub Review post is + skipped entirely (resolved threads and ``last_reviewed_sha`` are + still updated). The "no issues found" summary only posts on the + first review of a PR. 4. Store the returned per-comment IDs back on each finding so a future re-review can resolve those threads on GitHub when the issues are fixed. 5. For findings whose status moved ``open`` → ``resolved`` since the last @@ -209,6 +213,29 @@ async def _publish_review_async( inline_comments.append(payload) eligible_with_payload.append((dict(finding), payload)) + # On re-review with nothing new to surface, skip the "no issues found" + # comment — the user already saw the previous findings, and posting + # another summary on every push is noise. Still resolve threads for + # findings that just moved to resolved, and advance last_reviewed_sha so + # subsequent pushes don't redo the same diff. + if is_re_review and not inline_comments: + resolved_thread_count = await _resolve_threads_for_resolved_findings( + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + findings=findings, + ) + await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) + return { + "success": True, + "review_id": None, + "surfaced_count": 0, + "hidden_count": max(len(open_unpublished), 0), + "resolved_thread_count": resolved_thread_count, + "skipped_empty_re_review": True, + } + review_body = render_review_body( pr_number=pr_number, surfaced_count=len(inline_comments), diff --git a/agent/webapp.py b/agent/webapp.py index a0c6ca85..31928145 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -23,6 +23,8 @@ from .dashboard.agent_overrides import ( resolve_login_from_email, ) from .dashboard.enabled_repos import is_review_repo_enabled +from .dashboard.profiles import get_profile +from .dashboard.team_settings import get_team_settings from .reviewer_findings import ( REVIEWER_THREAD_KIND, ReviewerPRMeta, @@ -1335,7 +1337,18 @@ _SUPPORTED_GH_EVENTS = frozenset( ] ) _SUPPORTED_GH_ISSUE_ACTIONS = frozenset(["edited", "opened", "reopened"]) -_SUPPORTED_GH_PULL_REQUEST_ACTIONS = frozenset(["review_requested", "closed", "reopened"]) +_SUPPORTED_GH_PULL_REQUEST_ACTIONS = frozenset( + [ + "review_requested", + "opened", + "ready_for_review", + "converted_to_draft", + "closed", + "reopened", + ] +) +_GH_PR_WATCH_TOGGLE_ACTIONS = frozenset(["closed", "reopened", "converted_to_draft"]) +_GH_PR_FIRST_REVIEW_ACTIONS = frozenset(["opened", "ready_for_review"]) _SUPPORTED_GH_COMMENT_ACTIONS = { "issue_comment": frozenset(["created", "edited"]), "pull_request_review_comment": frozenset(["created", "edited"]), @@ -1630,8 +1643,25 @@ def _build_reviewer_configurable( return configurable -async def process_github_pr_review_request(payload: dict[str, Any]) -> None: - """Trigger the reviewer agent when the Open SWE bot is requested on a PR.""" +async def _draft_review_enabled_for_author(author_login: str) -> bool: + """Return whether draft PRs by ``author_login`` should auto-review. + + Tri-state: the PR author's profile ``review_draft_prs`` wins when set to + True/False; ``None`` (or no profile, e.g. external contributors) falls + back to the team-wide default. + """ + if author_login: + profile = await get_profile(author_login) + if isinstance(profile, dict): + override = profile.get("review_draft_prs") + if isinstance(override, bool): + return override + team = await get_team_settings() + return bool(team.get("review_draft_prs")) + + +async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, source: str) -> None: + """Trigger a first-review run on the canonical reviewer thread for a PR.""" repo = payload.get("repository", {}) pull_request = payload.get("pull_request", {}) repo_config = { @@ -1649,7 +1679,7 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: github_user_id = payload.get("sender", {}).get("id") if not pr_number or not pr_url or not base_sha or not head_sha: - logger.warning("Missing PR review request context, skipping reviewer run") + logger.warning("Missing PR context for reviewer dispatch, skipping run") return thread_id = generate_reviewer_thread_id( @@ -1658,7 +1688,7 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: 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 PR reviewer request") + logger.warning("No GitHub App token available for reviewer dispatch") return langgraph_client = get_client(url=LANGGRAPH_URL) @@ -1684,7 +1714,7 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: prompt = build_github_pr_review_prompt(repo_config, pr_number, pr_url, base_sha, head_sha) configurable = _build_reviewer_configurable( - source="github", + source=source, github_login=github_login, github_user_id=github_user_id, repo_config=repo_config, @@ -1697,11 +1727,11 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: thread_active = await is_thread_active(thread_id) if thread_active: - logger.info("Reviewer thread %s is busy, queuing PR review request", thread_id) + logger.info("Reviewer thread %s is busy, queuing PR review (source=%s)", thread_id, source) await queue_message_for_thread(thread_id, prompt) return - logger.info("Creating reviewer run for thread %s from GitHub PR review request", thread_id) + logger.info("Creating reviewer run for thread %s (source=%s)", thread_id, source) await langgraph_client.runs.create( thread_id, "reviewer", @@ -1709,7 +1739,35 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: config={"configurable": configurable, "metadata": _AGENT_VERSION_METADATA}, if_not_exists="create", ) - logger.info("Reviewer run created for thread %s from GitHub PR review request", thread_id) + logger.info("Reviewer run created for thread %s (source=%s)", thread_id, source) + + +async def process_github_pr_review_request(payload: dict[str, Any]) -> None: + """Trigger the reviewer agent when the Open SWE bot is requested on a PR.""" + await _dispatch_first_review_from_pr_payload(payload, source="github") + + +async def process_github_pr_ready(payload: dict[str, Any]) -> None: + """Auto-review a PR that has just been opened or marked ready-for-review. + + Drafts are gated by the PR author's ``review_draft_prs`` profile flag + (with the team-wide setting as a fallback). + """ + pull_request = payload.get("pull_request", {}) + is_draft = bool(pull_request.get("draft")) + if is_draft: + author = pull_request.get("user") or {} + author_login = author.get("login", "") if isinstance(author, dict) else "" + if not await _draft_review_enabled_for_author(author_login): + logger.info( + "Skipping auto-review of draft PR by %s: review_draft_prs is disabled", + author_login or "", + ) + return + # Use source="github" so the auth resolver finds the bot token persisted on + # the thread; "github_auto" would fall through to the email-based path, + # which has no user_email to route on for webhook-triggered runs. + await _dispatch_first_review_from_pr_payload(payload, source="github") async def process_github_pr_review_command( @@ -1826,7 +1884,13 @@ async def _get_thread_metadata_safe(thread_id: str) -> dict[str, Any] | None: async def process_github_pr_close(payload: dict[str, Any]) -> None: - """Disable watch on the canonical reviewer thread when the PR closes/reopens.""" + """Toggle watch on the canonical reviewer thread on close/reopen/draft transitions. + + ``reopened`` re-enables watch; ``closed`` always disables it. + ``converted_to_draft`` disables watch only when the PR author's effective + draft-review setting is off — if drafts should be reviewed, watch stays on + so subsequent pushes still trigger re-reviews while the PR is in draft. + """ repo = payload.get("repository", {}) pull_request = payload.get("pull_request", {}) repo_config = { @@ -1853,7 +1917,21 @@ async def process_github_pr_close(payload: dict[str, Any]) -> None: ) return action = payload.get("action", "") - desired_watch = action == "reopened" + if action == "converted_to_draft": + author = pull_request.get("user") or {} + author_login = author.get("login", "") if isinstance(author, dict) else "" + if await _draft_review_enabled_for_author(author_login): + logger.info( + "PR %s/%s#%s converted to draft but author %s has draft reviews enabled; keeping watch", + repo_config.get("owner"), + repo_config.get("name"), + pr_number, + author_login or "", + ) + return + desired_watch = False + else: + desired_watch = action == "reopened" if metadata.get("watch") == desired_watch: return await set_reviewer_thread_metadata(thread_id, watch=desired_watch) @@ -2346,12 +2424,21 @@ async def github_webhook(request: Request, background_tasks: BackgroundTasks) -> "status": "ignored", "reason": f"Unsupported GitHub pull_request action: {action}", } - if action in {"closed", "reopened"}: + if action in _GH_PR_WATCH_TOGGLE_ACTIONS: if not await _is_repo_enabled_for_review(webhook_repo_config): return {"status": "ignored", "reason": "Repository not enabled for review"} logger.info("Accepted GitHub PR %s webhook, scheduling reviewer watch update", action) background_tasks.add_task(process_github_pr_close, payload) return {"status": "accepted", "message": f"Processing PR {action} for reviewer watch"} + if action in _GH_PR_FIRST_REVIEW_ACTIONS: + if not await _is_repo_enabled_for_review(webhook_repo_config): + return {"status": "ignored", "reason": "Repository not enabled for review"} + gate_rejection = await _enforce_public_repo_org_gate(payload, "pull_request") + if gate_rejection is not None: + return gate_rejection + logger.info("Accepted GitHub PR %s webhook, scheduling auto-review task", action) + background_tasks.add_task(process_github_pr_ready, payload) + return {"status": "accepted", "message": f"Processing PR {action} for auto-review"} if not _is_open_swe_reviewer_request(payload): logger.info("Ignoring PR review request for a different reviewer") return {"status": "ignored", "reason": "Review request is not for open-swe bot"} diff --git a/tests/test_pr_ready_auto_review.py b/tests/test_pr_ready_auto_review.py new file mode 100644 index 00000000..75fbe12b --- /dev/null +++ b/tests/test_pr_ready_auto_review.py @@ -0,0 +1,273 @@ +"""Tests for the opened / ready_for_review auto-review webhook handlers.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from agent import webapp + + +def _pr_payload(*, action: str, draft: bool, author: str = "alice") -> dict[str, Any]: + return { + "action": action, + "repository": {"owner": {"login": "lc"}, "name": "repo"}, + "pull_request": { + "number": 7, + "html_url": "https://github.com/lc/repo/pull/7", + "title": "T", + "draft": draft, + "user": {"login": author}, + "head": {"sha": "headsha", "ref": "feat-x"}, + "base": {"sha": "basesha", "ref": "main"}, + }, + "sender": {"login": author, "id": 1}, + } + + +def _patch_dispatch_deps(monkeypatch: pytest.MonkeyPatch, fake_client: Any) -> None: + monkeypatch.setattr( + webapp, + "get_github_app_installation_token_with_expiry", + AsyncMock(return_value=("token", None)), + ) + monkeypatch.setattr(webapp, "_ensure_thread_exists_for_metadata", AsyncMock(return_value=True)) + monkeypatch.setattr(webapp, "persist_encrypted_github_token", AsyncMock(return_value="enc")) + monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", AsyncMock()) + monkeypatch.setattr(webapp, "is_thread_active", AsyncMock(return_value=False)) + monkeypatch.setattr(webapp, "get_client", lambda url: fake_client) + + +@pytest.mark.asyncio +async def test_pr_ready_non_draft_triggers_run(monkeypatch: pytest.MonkeyPatch) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, 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="opened", draft=False)) + + fake_client.runs.create.assert_awaited_once() + _, kwargs = fake_client.runs.create.await_args + assert kwargs["config"]["configurable"]["source"] == "github" + assert kwargs["config"]["configurable"]["pr_number"] == 7 + + +@pytest.mark.asyncio +async def test_pr_ready_for_review_triggers_run(monkeypatch: pytest.MonkeyPatch) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, 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_awaited_once() + + +@pytest.mark.asyncio +async def test_pr_ready_draft_user_override_off_wins_over_team_on( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, fake_client) + monkeypatch.setattr( + webapp, + "get_profile", + AsyncMock(return_value={"login": "alice", "review_draft_prs": False}), + ) + monkeypatch.setattr( + webapp, "get_team_settings", AsyncMock(return_value={"review_draft_prs": True}) + ) + + await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=True)) + + fake_client.runs.create.assert_not_called() + + +@pytest.mark.asyncio +async def test_pr_ready_draft_user_override_on_wins_over_team_off( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, fake_client) + monkeypatch.setattr( + webapp, + "get_profile", + AsyncMock(return_value={"login": "alice", "review_draft_prs": True}), + ) + monkeypatch.setattr( + webapp, + "get_team_settings", + AsyncMock(return_value={"review_draft_prs": False}), + ) + + await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=True)) + + fake_client.runs.create.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_pr_ready_draft_user_default_falls_back_to_team_on( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, fake_client) + # User profile exists but review_draft_prs is None — inherit team default. + monkeypatch.setattr( + webapp, + "get_profile", + AsyncMock(return_value={"login": "alice", "review_draft_prs": None}), + ) + monkeypatch.setattr( + webapp, "get_team_settings", AsyncMock(return_value={"review_draft_prs": True}) + ) + + await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=True)) + + fake_client.runs.create.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_pr_ready_draft_no_profile_falls_back_to_team_off( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, fake_client) + # External contributor — inherit team default (off). + monkeypatch.setattr(webapp, "get_profile", AsyncMock(return_value=None)) + monkeypatch.setattr( + webapp, + "get_team_settings", + AsyncMock(return_value={"review_draft_prs": False}), + ) + + await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=True)) + + fake_client.runs.create.assert_not_called() + + +@pytest.mark.asyncio +async def test_pr_ready_draft_no_profile_falls_back_to_team_on( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + _patch_dispatch_deps(monkeypatch, fake_client) + monkeypatch.setattr(webapp, "get_profile", AsyncMock(return_value=None)) + monkeypatch.setattr( + webapp, "get_team_settings", AsyncMock(return_value={"review_draft_prs": True}) + ) + + await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=True)) + + fake_client.runs.create.assert_awaited_once() + + +def _converted_to_draft_payload(author: str = "alice") -> dict[str, Any]: + return { + "action": "converted_to_draft", + "repository": {"owner": {"login": "lc"}, "name": "repo"}, + "pull_request": { + "number": 7, + "head": {"ref": "feat-x"}, + "user": {"login": author}, + }, + } + + +@pytest.mark.asyncio +async def test_converted_to_draft_disables_watch_when_drafts_off( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: list[Any] = [] + + async def fake_set(thread_id: str, **kwargs: Any) -> None: + captured.append((thread_id, kwargs)) + + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "reviewer", "watch": True}, + ), + patch( + "agent.webapp.get_profile", + new_callable=AsyncMock, + return_value={"login": "alice", "review_draft_prs": False}, + ), + patch( + "agent.webapp.get_team_settings", + new_callable=AsyncMock, + return_value={"review_draft_prs": False}, + ), + patch("agent.webapp.set_reviewer_thread_metadata", side_effect=fake_set), + ): + await webapp.process_github_pr_close(_converted_to_draft_payload()) + assert captured and captured[0][1]["watch"] is False + + +@pytest.mark.asyncio +async def test_converted_to_draft_keeps_watch_when_author_drafts_on( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_set = AsyncMock() + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "reviewer", "watch": True}, + ), + patch( + "agent.webapp.get_profile", + new_callable=AsyncMock, + return_value={"login": "alice", "review_draft_prs": True}, + ), + patch( + "agent.webapp.get_team_settings", + new_callable=AsyncMock, + return_value={"review_draft_prs": False}, + ), + patch("agent.webapp.set_reviewer_thread_metadata", new=fake_set), + ): + await webapp.process_github_pr_close(_converted_to_draft_payload()) + fake_set.assert_not_called() + + +@pytest.mark.asyncio +async def test_converted_to_draft_keeps_watch_when_team_default_drafts_on( + monkeypatch: pytest.MonkeyPatch, +) -> None: + fake_set = AsyncMock() + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "reviewer", "watch": True}, + ), + # Author inherits team default — team has drafts on. + patch( + "agent.webapp.get_profile", + new_callable=AsyncMock, + return_value={"login": "alice", "review_draft_prs": None}, + ), + patch( + "agent.webapp.get_team_settings", + new_callable=AsyncMock, + return_value={"review_draft_prs": True}, + ), + patch("agent.webapp.set_reviewer_thread_metadata", new=fake_set), + ): + await webapp.process_github_pr_close(_converted_to_draft_payload()) + fake_set.assert_not_called() diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 182a830c..9f19291c 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -243,6 +243,71 @@ async def test_publish_review_skips_findings_already_published() -> None: assert paths == {"b.py"} +@pytest.mark.asyncio +async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> None: + """Re-review with nothing new to surface must not spam another comment.""" + from agent.tools.publish_review import _publish_review_async + + # All findings already have github_review_comment_id from the prior publish + # (so none are "unpublished"), plus one previously-resolved finding whose + # thread still needs to be resolved on GitHub. + findings = [ + { + "id": "f_old", + "severity": "high", + "category": "correctness", + "file": "a.py", + "start_line": 1, + "end_line": 1, + "side": "RIGHT", + "description": "x", + "suggestion": None, + "status": "resolved", + "first_seen_sha": "s", + "last_confirmed_sha": "s", + "github_review_comment_id": 100, + }, + ] + list_async = AsyncMock(return_value=findings) + post_review = AsyncMock() + set_metadata = AsyncMock() + resolve_threads = AsyncMock(return_value=1) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", list_async), + patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + resolve_threads, + ), + 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", + repo="r", + pr_number=7, + head_sha="newsha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=True, + ) + + post_review.assert_not_called() + resolve_threads.assert_awaited_once() + set_metadata.assert_awaited_once() + assert result["success"] is True + assert result["review_id"] is None + assert result["surfaced_count"] == 0 + assert result["resolved_thread_count"] == 1 + assert result["skipped_empty_re_review"] is True + + @pytest.mark.asyncio async def test_publish_review_posts_summary_when_no_findings() -> None: """An empty findings list must still post a review so the user sees feedback.""" diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index d28e1924..743c000b 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -71,6 +71,7 @@ export interface Profile { branch_prefix?: string | null; auto_fix_ci?: boolean; create_prs?: boolean; + review_draft_prs?: boolean | null; updated_at?: string; } @@ -82,6 +83,7 @@ export interface ProfileUpdate { branch_prefix?: string | null; auto_fix_ci?: boolean; create_prs?: boolean; + review_draft_prs?: boolean | null; } export type TriggerMode = "every_push" | "once_per_pr" | "manual"; diff --git a/ui/src/lib/profile.ts b/ui/src/lib/profile.ts index d4a27734..0278dd14 100644 --- a/ui/src/lib/profile.ts +++ b/ui/src/lib/profile.ts @@ -68,6 +68,7 @@ export function buildProfileUpdate( branch_prefix: current?.branch_prefix ?? null, auto_fix_ci: current?.auto_fix_ci ?? true, create_prs: current?.create_prs ?? true, + review_draft_prs: current?.review_draft_prs ?? null, ...patch, }; } diff --git a/ui/src/routes/my-settings.tsx b/ui/src/routes/my-settings.tsx index 800f47eb..c8b3b1ff 100644 --- a/ui/src/routes/my-settings.tsx +++ b/ui/src/routes/my-settings.tsx @@ -1,18 +1,50 @@ import { Navigate, createFileRoute, useNavigate } from "@tanstack/react-router"; -import { useQueryClient } from "@tanstack/react-query"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; import { AppShell, SettingsRow, SettingsSection } from "@/components/AppShell"; import { Button } from "@/components/ui/button"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "@/components/ui/select"; import { Skeleton } from "@/components/ui/skeleton"; import { api } from "@/lib/api"; +import { buildProfileUpdate, useOptions, useProfile, useSaveProfile } from "@/lib/profile"; import { useSession } from "@/lib/session"; export const Route = createFileRoute("/my-settings")({ component: MySettingsPage }); +type DraftReviewChoice = "team_default" | "always_on" | "always_off"; + +function toChoice(value: boolean | null | undefined): DraftReviewChoice { + if (value === true) return "always_on"; + if (value === false) return "always_off"; + return "team_default"; +} + +function fromChoice(choice: DraftReviewChoice): boolean | null { + if (choice === "always_on") return true; + if (choice === "always_off") return false; + return null; +} + function MySettingsPage() { const session = useSession(); const qc = useQueryClient(); const navigate = useNavigate(); + const profile = useProfile(); + const options = useOptions(); + const save = useSaveProfile(); + const teamSettings = useQuery({ + queryKey: ["teamSettings"], + queryFn: api.getTeamSettings, + enabled: !!session.data, + }); + const [error, setError] = useState(null); if (session.isLoading) { return ( @@ -29,6 +61,28 @@ function MySettingsPage() { void navigate({ to: "/login" }); }; + const firstModel = options.data?.models[0]; + const fallbackModel = firstModel?.id ?? ""; + const fallbackEffort = firstModel?.default_effort ?? ""; + + const draftChoice = toChoice(profile.data?.review_draft_prs); + const teamDefaultOn = teamSettings.data?.review_draft_prs ?? false; + const teamDefaultLabel = `Use team default (currently: ${teamDefaultOn ? "On" : "Off"})`; + + const handleDraftChoiceChange = (next: DraftReviewChoice) => { + setError(null); + save + .mutateAsync( + buildProfileUpdate( + profile.data, + { review_draft_prs: fromChoice(next) }, + fallbackModel, + fallbackEffort, + ), + ) + .catch((e: Error) => setError(e.message)); + }; + return ( @@ -42,6 +96,29 @@ function MySettingsPage() { /> + + handleDraftChoiceChange(v as DraftReviewChoice)} + disabled={profile.isLoading || save.isPending} + > + + + + + {teamDefaultLabel} + Always review my drafts + Never review my drafts + + + } + /> + + + + {error &&

{error}

} ); } diff --git a/ui/src/routes/review.tsx b/ui/src/routes/review.tsx index 27b877e9..706ecf6e 100644 --- a/ui/src/routes/review.tsx +++ b/ui/src/routes/review.tsx @@ -152,7 +152,7 @@ function ReviewPage() { />