mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 09:13:14 +00:00
feat: auto-review PRs on opened / ready-for-review (#1325)
* 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] <open-swe@users.noreply.github.com>
This commit is contained in:
parent
e87085139b
commit
b2a0ac3b79
9 changed files with 551 additions and 15 deletions
|
|
@ -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 (
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
111
agent/webapp.py
111
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 "<unknown>",
|
||||
)
|
||||
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 "<unknown>",
|
||||
)
|
||||
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"}
|
||||
|
|
|
|||
273
tests/test_pr_ready_auto_review.py
Normal file
273
tests/test_pr_ready_auto_review.py
Normal file
|
|
@ -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()
|
||||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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";
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string | null>(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 (
|
||||
<AppShell user={session.data} title="My Settings">
|
||||
<SettingsSection title="Profile">
|
||||
|
|
@ -42,6 +96,29 @@ function MySettingsPage() {
|
|||
/>
|
||||
</SettingsSection>
|
||||
|
||||
<SettingsSection title="Open SWE Review">
|
||||
<SettingsRow
|
||||
label="Review my draft PRs"
|
||||
description="Whether Open SWE Review runs on pull requests you open in draft. When set to the team default, your admin's org-wide setting applies."
|
||||
control={
|
||||
<Select
|
||||
value={draftChoice}
|
||||
onValueChange={(v) => handleDraftChoiceChange(v as DraftReviewChoice)}
|
||||
disabled={profile.isLoading || save.isPending}
|
||||
>
|
||||
<SelectTrigger className="w-56">
|
||||
<SelectValue />
|
||||
</SelectTrigger>
|
||||
<SelectContent>
|
||||
<SelectItem value="team_default">{teamDefaultLabel}</SelectItem>
|
||||
<SelectItem value="always_on">Always review my drafts</SelectItem>
|
||||
<SelectItem value="always_off">Never review my drafts</SelectItem>
|
||||
</SelectContent>
|
||||
</Select>
|
||||
}
|
||||
/>
|
||||
</SettingsSection>
|
||||
|
||||
<SettingsSection title="Account">
|
||||
<SettingsRow
|
||||
label="Sign out"
|
||||
|
|
@ -53,6 +130,8 @@ function MySettingsPage() {
|
|||
}
|
||||
/>
|
||||
</SettingsSection>
|
||||
|
||||
{error && <p className="text-xs text-destructive">{error}</p>}
|
||||
</AppShell>
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -152,7 +152,7 @@ function ReviewPage() {
|
|||
/>
|
||||
<SettingsRow
|
||||
label="Review Draft PRs"
|
||||
description="Allow Open SWE Review to automatically review draft pull requests"
|
||||
description="Org-wide default for whether Open SWE Review runs on draft PRs. Each user can override this in My Settings."
|
||||
control={
|
||||
<Switch
|
||||
checked={current.review_draft_prs}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue