mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 05:43:14 +00:00
feat(reviewer): add dispatch verdict authorization
This commit is contained in:
Adam Moussa
parent
54497290dc
commit
1b8f8d480d
15 changed files with 500 additions and 39 deletions
60
agent/dashboard/auto_verdict_repos.py
Normal file
60
agent/dashboard/auto_verdict_repos.py
Normal file
|
|
@ -0,0 +1,60 @@
|
|||
"""Per-repository opt-ins for automatic reviewer verdicts."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from datetime import UTC, datetime
|
||||
|
||||
from langgraph_sdk import get_client
|
||||
|
||||
from .review_styles import normalize_repo_full_name
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
AUTO_VERDICT_REPOS_NAMESPACE: list[str] = ["auto_verdict_repos"]
|
||||
AUTO_VERDICT_REPOS_KEY = "default"
|
||||
|
||||
|
||||
def _client():
|
||||
return get_client()
|
||||
|
||||
|
||||
async def list_auto_verdict_repos() -> list[str]:
|
||||
try:
|
||||
item = await _client().store.get_item(AUTO_VERDICT_REPOS_NAMESPACE, AUTO_VERDICT_REPOS_KEY)
|
||||
except Exception as e:
|
||||
logger.debug("auto-verdict repos lookup failed: %s", e)
|
||||
return []
|
||||
if item is None:
|
||||
return []
|
||||
value = item.get("value") if isinstance(item, dict) else getattr(item, "value", None)
|
||||
if not isinstance(value, dict):
|
||||
return []
|
||||
repos = value.get("repos")
|
||||
if not isinstance(repos, list):
|
||||
return []
|
||||
return [repo for repo in repos if isinstance(repo, str)]
|
||||
|
||||
|
||||
async def set_auto_verdict_repo_enabled(full_name: str, enabled: bool) -> list[str]:
|
||||
full_name = normalize_repo_full_name(full_name)
|
||||
current = set(await list_auto_verdict_repos())
|
||||
if enabled:
|
||||
current.add(full_name)
|
||||
else:
|
||||
current.discard(full_name)
|
||||
repos = sorted(current)
|
||||
await _client().store.put_item(
|
||||
AUTO_VERDICT_REPOS_NAMESPACE,
|
||||
AUTO_VERDICT_REPOS_KEY,
|
||||
{"repos": repos, "updated_at": datetime.now(UTC).isoformat()},
|
||||
)
|
||||
return repos
|
||||
|
||||
|
||||
async def is_auto_verdict_repo_enabled(owner: str, name: str) -> bool:
|
||||
if not owner or not name:
|
||||
return False
|
||||
full_name = f"{owner.lower()}/{name.lower()}"
|
||||
enabled = await list_auto_verdict_repos()
|
||||
return any(repo.lower() == full_name for repo in enabled)
|
||||
|
|
@ -29,6 +29,10 @@ from .agent_usage import (
|
|||
refresh_usage_leaderboard_cache,
|
||||
)
|
||||
from .analyzer_cron import remove_continual_cron
|
||||
from .auto_verdict_repos import (
|
||||
list_auto_verdict_repos,
|
||||
set_auto_verdict_repo_enabled,
|
||||
)
|
||||
from .enabled_repos import (
|
||||
list_enabled_review_repos,
|
||||
set_review_repo_enabled,
|
||||
|
|
@ -735,6 +739,11 @@ class EnabledReviewRepoUpdate(BaseModel):
|
|||
enabled: bool
|
||||
|
||||
|
||||
class AutoVerdictRepoUpdate(BaseModel):
|
||||
full_name: str
|
||||
enabled: bool
|
||||
|
||||
|
||||
@router.get("/enabled-review-repos")
|
||||
async def api_list_enabled_review_repos(
|
||||
_session: dict[str, Any] = _SESSION_DEP,
|
||||
|
|
@ -751,6 +760,22 @@ async def api_set_enabled_review_repo(
|
|||
return {"repos": repos}
|
||||
|
||||
|
||||
@router.get("/auto-verdict-repos")
|
||||
async def api_list_auto_verdict_repos(
|
||||
_session: dict[str, Any] = _SESSION_DEP,
|
||||
) -> dict[str, list[str]]:
|
||||
return {"repos": await list_auto_verdict_repos()}
|
||||
|
||||
|
||||
@router.put("/auto-verdict-repos")
|
||||
async def api_set_auto_verdict_repo(
|
||||
update: AutoVerdictRepoUpdate,
|
||||
_admin: dict[str, Any] = _ADMIN_DEP,
|
||||
) -> dict[str, list[str]]:
|
||||
repos = await set_auto_verdict_repo_enabled(update.full_name, update.enabled)
|
||||
return {"repos": repos}
|
||||
|
||||
|
||||
@router.get("/repo-snapshots")
|
||||
async def api_list_repo_snapshots(
|
||||
_admin: dict[str, Any] = _ADMIN_DEP,
|
||||
|
|
|
|||
|
|
@ -52,6 +52,7 @@ Only file a finding that anchors to a changed line and names a concrete failure
|
|||
|
||||
|
||||
class TeamSettingsUpdate(BaseModel):
|
||||
auto_verdict: bool = False
|
||||
review_draft_prs: bool = False
|
||||
pr_summaries: bool = True
|
||||
review_trace_links: bool = True
|
||||
|
|
@ -270,6 +271,7 @@ def _parse_repo(value: object) -> dict[str, str] | None:
|
|||
def _default_settings() -> dict[str, Any]:
|
||||
fallback_model, fallback_effort = default_model_pair()
|
||||
return {
|
||||
"auto_verdict": False,
|
||||
"review_draft_prs": False,
|
||||
"pr_summaries": True,
|
||||
"review_trace_links": True,
|
||||
|
|
@ -326,6 +328,7 @@ async def get_team_settings() -> dict[str, Any]:
|
|||
|
||||
async def upsert_team_settings(update: TeamSettingsUpdate) -> dict[str, Any]:
|
||||
value: dict[str, Any] = {
|
||||
"auto_verdict": update.auto_verdict,
|
||||
"review_draft_prs": update.review_draft_prs,
|
||||
"pr_summaries": update.pr_summaries,
|
||||
"review_trace_links": update.review_trace_links,
|
||||
|
|
@ -451,6 +454,13 @@ async def get_team_review_trace_links_enabled() -> bool:
|
|||
return bool(settings.get("review_trace_links", True))
|
||||
|
||||
|
||||
async def get_team_auto_verdict_enabled() -> bool:
|
||||
"""Return whether automatic reviewer verdicts are enabled team-wide."""
|
||||
settings = await get_team_settings()
|
||||
value = settings.get("auto_verdict")
|
||||
return bool(value) if isinstance(value, bool) else False
|
||||
|
||||
|
||||
async def get_team_gateway_enabled() -> bool | None:
|
||||
"""Return the stored LLM Gateway toggle (``None`` means inherit the env default)."""
|
||||
settings = await get_team_settings()
|
||||
|
|
|
|||
|
|
@ -182,17 +182,14 @@ async def publish_review(
|
|||
if not token:
|
||||
return {"success": False, "error": "No GitHub token available"}
|
||||
|
||||
# Verdict authorization is enforced here in code, not in the prompt: only
|
||||
# a run whose dispatching webhook set verdict_requested (the explicit
|
||||
# mention path) may submit a blocking review state. Anything else —
|
||||
# including a model that hallucinates authorization — publishes as a plain
|
||||
# comment. Exception: an unsolicited "approve" is allowed through, and
|
||||
# _publish_review_async honors it only when the review has zero open
|
||||
# findings (clean-review auto-approve).
|
||||
# Verdict authorization is enforced here in code, not in the prompt.
|
||||
# Explicit requests may submit either verdict. Automatic approvals require
|
||||
# the dispatch-set verdict_authorized flag and still pass the clean-review
|
||||
# gate in _publish_review_async.
|
||||
verdict_not_requested = False
|
||||
unsolicited_approve = False
|
||||
if verdict is not None and configurable.get("verdict_requested") is not True:
|
||||
if verdict == "approve":
|
||||
if verdict == "approve" and configurable.get("verdict_authorized") is True:
|
||||
unsolicited_approve = True
|
||||
else:
|
||||
logger.info(
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ from ..dashboard.agent_overrides import (
|
|||
resolve_agent_model_id, # noqa: F401
|
||||
resolve_login_from_email_async,
|
||||
)
|
||||
from ..dashboard.auto_verdict_repos import is_auto_verdict_repo_enabled
|
||||
from ..dashboard.enabled_repos import is_review_repo_enabled
|
||||
from ..dashboard.oauth import build_settings_url
|
||||
from ..dashboard.options import default_vision_model_pair, model_supports_images # noqa: F401
|
||||
|
|
@ -30,6 +31,7 @@ from ..dashboard.profiles import ( # noqa: F401
|
|||
has_access_token_record,
|
||||
)
|
||||
from ..dashboard.team_settings import (
|
||||
get_team_auto_verdict_enabled,
|
||||
get_team_default_repo,
|
||||
get_team_settings,
|
||||
)
|
||||
|
|
@ -185,12 +187,14 @@ __all__ = [
|
|||
"_is_pr_diff_unchanged_since_last_review",
|
||||
"_is_repo_allowed",
|
||||
"_is_repo_auto_review_enabled",
|
||||
"_is_repo_auto_verdict_enabled",
|
||||
"_post_account_link_prompt",
|
||||
"_refresh_thread_github_token_after_401",
|
||||
"_repo_id_from_payload",
|
||||
"_repo_id_from_pr_metadata",
|
||||
"_repo_private_from_payload",
|
||||
"_repo_private_from_pr_metadata",
|
||||
"_resolve_verdict_authorization",
|
||||
"_review_comment_reply_parent_id",
|
||||
"_reviewer_token_for_repo",
|
||||
"_run_id_for_logging",
|
||||
|
|
@ -768,6 +772,43 @@ async def _is_repo_auto_review_enabled(repo_config: dict[str, str]) -> bool:
|
|||
return await is_review_repo_enabled(repo_config.get("owner", ""), repo_config.get("name", ""))
|
||||
|
||||
|
||||
async def _is_repo_auto_verdict_enabled(repo_config: dict[str, str]) -> bool:
|
||||
"""Return the effective team or repository automatic-verdict policy."""
|
||||
return await get_team_auto_verdict_enabled() or await is_auto_verdict_repo_enabled(
|
||||
repo_config.get("owner", ""), repo_config.get("name", "")
|
||||
)
|
||||
|
||||
|
||||
async def _resolve_verdict_authorization(
|
||||
repo_config: dict[str, str], pr_metadata: dict[str, Any]
|
||||
) -> bool:
|
||||
"""Resolve dispatch-set automatic verdict authorization for one reviewer run."""
|
||||
owner = repo_config.get("owner", "")
|
||||
if not await _is_repo_auto_verdict_enabled(repo_config):
|
||||
return False
|
||||
|
||||
head = pr_metadata.get("head")
|
||||
base = pr_metadata.get("base")
|
||||
head_repo = head.get("repo") if isinstance(head, dict) else None
|
||||
base_repo = base.get("repo") if isinstance(base, dict) else None
|
||||
head_full_name = head_repo.get("full_name") if isinstance(head_repo, dict) else None
|
||||
base_full_name = base_repo.get("full_name") if isinstance(base_repo, dict) else None
|
||||
if (
|
||||
not isinstance(head_full_name, str)
|
||||
or not isinstance(base_full_name, str)
|
||||
or head_full_name.casefold() != base_full_name.casefold()
|
||||
):
|
||||
return False
|
||||
|
||||
author = pr_metadata.get("user")
|
||||
author_login = author.get("login") if isinstance(author, dict) else None
|
||||
if not isinstance(author_login, str) or not author_login:
|
||||
return False
|
||||
if author_login.casefold() in {login.casefold() for login in INTERNAL_BOT_LOGINS}:
|
||||
return True
|
||||
return await is_user_active_org_member(author_login, owner)
|
||||
|
||||
|
||||
_PUBLIC_REPO_GATE_REJECTION = {
|
||||
"status": "ignored",
|
||||
"reason": "Sender is not a member of the allowed organization for public-repo triggers",
|
||||
|
|
@ -1445,6 +1486,7 @@ def _build_reviewer_configurable(
|
|||
last_reviewed_sha: str = "",
|
||||
slack_channel_id: str = "",
|
||||
slack_thread_ts: str = "",
|
||||
verdict_authorized: bool = False,
|
||||
verdict_requested: bool = False,
|
||||
) -> dict[str, Any]:
|
||||
"""Assemble the runnable-config ``configurable`` dict for a reviewer run."""
|
||||
|
|
@ -1465,6 +1507,8 @@ def _build_reviewer_configurable(
|
|||
# Set ONLY by the explicit-mention path; auto-review dispatches must
|
||||
# never pass it.
|
||||
configurable["verdict_requested"] = True
|
||||
if verdict_authorized:
|
||||
configurable["verdict_authorized"] = True
|
||||
if branch_name:
|
||||
configurable["branch_name"] = branch_name
|
||||
if repo_private is not None:
|
||||
|
|
|
|||
|
|
@ -195,6 +195,7 @@ async def trigger_pr_review_from_ref(
|
|||
head_sha,
|
||||
instructions=instructions,
|
||||
)
|
||||
verdict_authorized = await common._resolve_verdict_authorization(repo_config, pr_metadata)
|
||||
configurable = common._build_reviewer_configurable(
|
||||
source=source,
|
||||
github_login=github_login,
|
||||
|
|
@ -208,6 +209,7 @@ async def trigger_pr_review_from_ref(
|
|||
repo_private=repo_private,
|
||||
slack_channel_id=slack_channel_id,
|
||||
slack_thread_ts=slack_thread_ts,
|
||||
verdict_authorized=verdict_authorized,
|
||||
verdict_requested=request_verdict,
|
||||
)
|
||||
|
||||
|
|
@ -319,6 +321,7 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou
|
|||
)
|
||||
else:
|
||||
prompt = build_github_pr_review_prompt(repo_config, pr_number, pr_url, base_sha, head_sha)
|
||||
verdict_authorized = await common._resolve_verdict_authorization(repo_config, pull_request)
|
||||
configurable = common._build_reviewer_configurable(
|
||||
source=source,
|
||||
github_login=github_login,
|
||||
|
|
@ -332,6 +335,7 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou
|
|||
repo_private=repo_private,
|
||||
re_review=is_re_review,
|
||||
last_reviewed_sha=last_reviewed_sha,
|
||||
verdict_authorized=verdict_authorized,
|
||||
)
|
||||
|
||||
common.logger.info("Dispatching reviewer run for thread %s (source=%s)", thread_id, source)
|
||||
|
|
@ -627,6 +631,7 @@ async def process_github_push_event(payload: dict[str, Any]) -> None:
|
|||
f"{head_sha}. Reconcile existing findings against the new diff, add any "
|
||||
f"net-new findings, and call `publish_review` once you're done."
|
||||
)
|
||||
verdict_authorized = await common._resolve_verdict_authorization(repo_config, pr)
|
||||
configurable = common._build_reviewer_configurable(
|
||||
source="github_push",
|
||||
github_login=payload.get("sender", {}).get("login", "") or "",
|
||||
|
|
@ -640,6 +645,7 @@ async def process_github_push_event(payload: dict[str, Any]) -> None:
|
|||
repo_private=repo_private,
|
||||
re_review=True,
|
||||
last_reviewed_sha=last_reviewed_sha if isinstance(last_reviewed_sha, str) else "",
|
||||
verdict_authorized=verdict_authorized,
|
||||
)
|
||||
|
||||
common.logger.info("Dispatching push re-review run for thread %s", thread_id)
|
||||
|
|
@ -876,6 +882,7 @@ async def process_github_review_finding_reply(payload: dict[str, Any]) -> None:
|
|||
head_sha = pull_request.get("head", {}).get("sha", "")
|
||||
pr_url = pull_request.get("html_url", "") or pull_request.get("url", "")
|
||||
branch_name = pull_request.get("head", {}).get("ref", "")
|
||||
verdict_authorized = await common._resolve_verdict_authorization(repo_config, pull_request)
|
||||
configurable = common._build_reviewer_configurable(
|
||||
source="github_review_comment",
|
||||
github_login=reply_author,
|
||||
|
|
@ -888,6 +895,7 @@ async def process_github_review_finding_reply(payload: dict[str, Any]) -> None:
|
|||
branch_name=branch_name,
|
||||
repo_private=repo_private,
|
||||
re_review=True,
|
||||
verdict_authorized=verdict_authorized,
|
||||
)
|
||||
configurable.update(
|
||||
{
|
||||
|
|
|
|||
68
tests/dashboard/test_auto_verdict_settings.py
Normal file
68
tests/dashboard/test_auto_verdict_settings.py
Normal file
|
|
@ -0,0 +1,68 @@
|
|||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.dashboard import auto_verdict_repos, routes, team_settings
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_team_auto_verdict_defaults_off(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
store = SimpleNamespace(get_item=AsyncMock(return_value=None))
|
||||
monkeypatch.setattr(team_settings, "_client", lambda: SimpleNamespace(store=store))
|
||||
|
||||
assert await team_settings.get_team_auto_verdict_enabled() is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_team_auto_verdict_reads_enabled_setting(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
store = SimpleNamespace(get_item=AsyncMock(return_value={"value": {"auto_verdict": True}}))
|
||||
monkeypatch.setattr(team_settings, "_client", lambda: SimpleNamespace(store=store))
|
||||
|
||||
assert await team_settings.get_team_auto_verdict_enabled() is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_auto_verdict_repo_list_is_default_off_and_editable(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
saved: dict[str, object] = {}
|
||||
store = SimpleNamespace(
|
||||
get_item=AsyncMock(return_value=None),
|
||||
put_item=AsyncMock(
|
||||
side_effect=lambda namespace, key, value: saved.update(
|
||||
{"namespace": namespace, "key": key, "value": value}
|
||||
)
|
||||
),
|
||||
)
|
||||
monkeypatch.setattr(auto_verdict_repos, "_client", lambda: SimpleNamespace(store=store))
|
||||
|
||||
assert await auto_verdict_repos.list_auto_verdict_repos() == []
|
||||
repos = await auto_verdict_repos.set_auto_verdict_repo_enabled("Acme/Repo", True)
|
||||
|
||||
assert repos == ["Acme/Repo"]
|
||||
assert saved["namespace"] == auto_verdict_repos.AUTO_VERDICT_REPOS_NAMESPACE
|
||||
assert saved["key"] == auto_verdict_repos.AUTO_VERDICT_REPOS_KEY
|
||||
value = saved["value"]
|
||||
assert isinstance(value, dict)
|
||||
assert value["repos"] == ["Acme/Repo"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_auto_verdict_dashboard_endpoints(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
list_repos = AsyncMock(return_value=["acme/repo"])
|
||||
set_repo = AsyncMock(return_value=["acme/repo", "acme/widgets"])
|
||||
monkeypatch.setattr(routes, "list_auto_verdict_repos", list_repos)
|
||||
monkeypatch.setattr(routes, "set_auto_verdict_repo_enabled", set_repo)
|
||||
|
||||
listed = await routes.api_list_auto_verdict_repos({})
|
||||
updated = await routes.api_set_auto_verdict_repo(
|
||||
routes.AutoVerdictRepoUpdate(full_name="acme/widgets", enabled=True),
|
||||
{},
|
||||
)
|
||||
|
||||
assert listed == {"repos": ["acme/repo"]}
|
||||
assert updated == {"repos": ["acme/repo", "acme/widgets"]}
|
||||
set_repo.assert_awaited_once_with("acme/widgets", True)
|
||||
|
|
@ -380,6 +380,12 @@ def test_process_github_review_finding_reply_uses_rereview_config(monkeypatch) -
|
|||
async def fake_store_current_run_id(_thread_id: str, _run: object) -> None:
|
||||
return None
|
||||
|
||||
async def fake_resolve_verdict_authorization(
|
||||
repo_config: dict[str, str], pr_metadata: dict[str, object]
|
||||
) -> bool:
|
||||
captured["verdict_resolution"] = (repo_config, pr_metadata)
|
||||
return True
|
||||
|
||||
class _FakeRunsClient:
|
||||
async def create(self, thread_id: str, graph: str, **kwargs) -> dict[str, str]:
|
||||
captured["thread_id"] = thread_id
|
||||
|
|
@ -400,6 +406,9 @@ def test_process_github_review_finding_reply_uses_rereview_config(monkeypatch) -
|
|||
monkeypatch.setattr(webhook_common, "list_reviewer_findings", fake_list_findings)
|
||||
monkeypatch.setattr(webhook_common, "append_finding_interaction", fake_append_interaction)
|
||||
monkeypatch.setattr(webhook_common, "_store_current_reviewer_run_id", fake_store_current_run_id)
|
||||
monkeypatch.setattr(
|
||||
webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization
|
||||
)
|
||||
monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient())
|
||||
|
||||
asyncio.run(
|
||||
|
|
@ -429,6 +438,11 @@ def test_process_github_review_finding_reply_uses_rereview_config(monkeypatch) -
|
|||
assert config["reviewer_event"] == "finding_reply"
|
||||
assert config["re_review"] is True
|
||||
assert config["finding_reply_id"] == "f_1"
|
||||
assert config["verdict_authorized"] is True
|
||||
assert captured["verdict_resolution"][0] == {
|
||||
"owner": "langchain-ai",
|
||||
"name": "open-swe",
|
||||
}
|
||||
|
||||
|
||||
def test_process_github_review_finding_reply_dispatches_sanitized_reply_body(monkeypatch) -> None:
|
||||
|
|
@ -922,6 +936,12 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None:
|
|||
captured["set_metadata_thread_id"] = thread_id
|
||||
captured["set_metadata_kwargs"] = kwargs
|
||||
|
||||
async def fake_resolve_verdict_authorization(
|
||||
repo_config: dict[str, str], pr_metadata: dict[str, object]
|
||||
) -> bool:
|
||||
captured["verdict_resolution"] = (repo_config, pr_metadata)
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(
|
||||
webhook_common,
|
||||
"get_github_app_installation_token_with_expiry",
|
||||
|
|
@ -939,6 +959,9 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None:
|
|||
monkeypatch.setattr(
|
||||
webhook_common, "post_review_started_comment", fake_post_review_started_comment
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization
|
||||
)
|
||||
monkeypatch.setattr(webhook_common, "get_client", lambda url: _FakeLangGraphClient())
|
||||
|
||||
asyncio.run(
|
||||
|
|
@ -973,8 +996,12 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None:
|
|||
assert config["repo"] == {"owner": "langchain-ai", "name": "open-swe"}
|
||||
assert config["pr_number"] == 1244
|
||||
assert config["review_requested"] is True
|
||||
# Auto-reviews are never authorized to submit verdicts.
|
||||
assert config["verdict_authorized"] is True
|
||||
assert "verdict_requested" not in config
|
||||
assert captured["verdict_resolution"][0] == {
|
||||
"owner": "langchain-ai",
|
||||
"name": "open-swe",
|
||||
}
|
||||
|
||||
|
||||
def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
||||
|
|
@ -986,6 +1013,12 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
|||
auto_review_checked = True
|
||||
return False
|
||||
|
||||
async def fake_resolve_verdict_authorization(
|
||||
repo_config: dict[str, str], pr_metadata: dict[str, object]
|
||||
) -> bool:
|
||||
captured["verdict_resolution"] = (repo_config, pr_metadata)
|
||||
return True
|
||||
|
||||
async def fake_get_github_app_installation_token() -> str | None:
|
||||
return "app-token"
|
||||
|
||||
|
|
@ -1028,6 +1061,9 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
|||
captured["set_metadata_kwargs"] = kwargs
|
||||
|
||||
monkeypatch.setattr(webhook_common, "_is_repo_auto_review_enabled", fake_auto_review_enabled)
|
||||
monkeypatch.setattr(
|
||||
webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
webhook_common, "get_github_app_installation_token", fake_get_github_app_installation_token
|
||||
)
|
||||
|
|
@ -1091,8 +1127,13 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None:
|
|||
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
|
||||
# Without an explicit request_verdict, the run is not verdict-authorized.
|
||||
assert config["verdict_authorized"] is True
|
||||
# Dispatch authorization does not imply an explicit verdict request.
|
||||
assert "verdict_requested" not in config
|
||||
assert captured["verdict_resolution"][0] == {
|
||||
"owner": "langchain-ai",
|
||||
"name": "open-swe",
|
||||
}
|
||||
|
||||
|
||||
def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None:
|
||||
|
|
@ -1110,6 +1151,12 @@ def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None
|
|||
"head": {"sha": "head-sha", "ref": "feature-branch"},
|
||||
}
|
||||
|
||||
async def fake_resolve_verdict_authorization(
|
||||
_repo_config: dict[str, str], _pr_metadata: dict[str, object]
|
||||
) -> bool:
|
||||
captured["verdict_resolved"] = True
|
||||
return False
|
||||
|
||||
class _FakeRunsClient:
|
||||
async def create(self, thread_id: str, graph: str, **kwargs) -> None:
|
||||
captured["thread_id"] = thread_id
|
||||
|
|
@ -1132,6 +1179,9 @@ def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None
|
|||
fake_get_github_app_installation_token_with_expiry,
|
||||
)
|
||||
monkeypatch.setattr(webhook_common, "fetch_github_pr_metadata", fake_fetch_github_pr_metadata)
|
||||
monkeypatch.setattr(
|
||||
webhook_common, "_resolve_verdict_authorization", fake_resolve_verdict_authorization
|
||||
)
|
||||
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None)
|
||||
monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", fake_async_noop)
|
||||
monkeypatch.setattr(webhook_common, "post_review_started_comment", fake_async_noop)
|
||||
|
|
@ -1157,6 +1207,8 @@ def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None
|
|||
config = kwargs["config"]["configurable"]
|
||||
prompt = kwargs["input"]["messages"][0]["content"]
|
||||
assert config["verdict_requested"] is True
|
||||
assert "verdict_authorized" not in config
|
||||
assert captured["verdict_resolved"] is True
|
||||
assert "## Requester instructions" in prompt
|
||||
assert "<requester_instructions>\nApprove if it meets the merge bar." in prompt
|
||||
|
||||
|
|
|
|||
|
|
@ -48,6 +48,9 @@ def _patch_dispatch_deps(monkeypatch: pytest.MonkeyPatch, fake_client: Any) -> N
|
|||
)
|
||||
monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", MagicMock())
|
||||
monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", AsyncMock())
|
||||
monkeypatch.setattr(
|
||||
webhook_common, "_resolve_verdict_authorization", AsyncMock(return_value=False)
|
||||
)
|
||||
monkeypatch.setattr(webhook_common, "get_client", lambda url: fake_client)
|
||||
|
||||
|
||||
|
|
@ -65,8 +68,9 @@ async def test_pr_ready_non_draft_triggers_run(monkeypatch: pytest.MonkeyPatch)
|
|||
_, kwargs = fake_client.runs.create.await_args
|
||||
assert kwargs["config"]["configurable"]["source"] == "github"
|
||||
assert kwargs["config"]["configurable"]["pr_number"] == 7
|
||||
# Auto-reviews must never be authorized to submit verdicts.
|
||||
# Explicit verdict requests remain absent on automatic reviews.
|
||||
assert "verdict_requested" not in kwargs["config"]["configurable"]
|
||||
assert "verdict_authorized" not in kwargs["config"]["configurable"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -2438,13 +2438,41 @@ async def test_publish_review_drops_verdict_when_not_requested() -> None:
|
|||
|
||||
|
||||
async def test_publish_review_forwards_unsolicited_approve() -> None:
|
||||
"""An approve without verdict_requested is forwarded (not dropped) with the
|
||||
unsolicited flag set, so the async layer can apply the clean-review gate."""
|
||||
"""A dispatch-authorized approve reaches the clean-review gate."""
|
||||
from agent.tools.publish_review import publish_review
|
||||
|
||||
publish_async = AsyncMock(
|
||||
return_value={"success": True, "review_id": 9, "verdict_submitted": True}
|
||||
)
|
||||
with (
|
||||
patch(
|
||||
"agent.tools.publish_review.get_config",
|
||||
return_value={
|
||||
"configurable": {
|
||||
"thread_id": "tid",
|
||||
"repo": {"owner": "o", "name": "r"},
|
||||
"pr_number": 7,
|
||||
"head_sha": "sha",
|
||||
"verdict_authorized": True,
|
||||
},
|
||||
"metadata": {},
|
||||
},
|
||||
),
|
||||
patch("agent.tools.publish_review.get_github_token", return_value="token"),
|
||||
patch("agent.tools.publish_review._publish_review_async", publish_async),
|
||||
):
|
||||
result = await publish_review(verdict="approve")
|
||||
|
||||
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
||||
assert publish_async.call_args.kwargs["unsolicited_approve"] is True
|
||||
assert result["verdict_submitted"] is True
|
||||
assert "verdict_ignored" not in result
|
||||
|
||||
|
||||
async def test_publish_review_drops_unauthorized_unsolicited_approve() -> None:
|
||||
from agent.tools.publish_review import publish_review
|
||||
|
||||
publish_async = AsyncMock(return_value={"success": True, "review_id": 9})
|
||||
with (
|
||||
patch(
|
||||
"agent.tools.publish_review.get_config",
|
||||
|
|
@ -2463,10 +2491,11 @@ async def test_publish_review_forwards_unsolicited_approve() -> None:
|
|||
):
|
||||
result = await publish_review(verdict="approve")
|
||||
|
||||
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
||||
assert publish_async.call_args.kwargs["unsolicited_approve"] is True
|
||||
assert result["verdict_submitted"] is True
|
||||
assert "verdict_ignored" not in result
|
||||
assert publish_async.call_args.kwargs["verdict"] is None
|
||||
assert publish_async.call_args.kwargs["unsolicited_approve"] is False
|
||||
assert result["verdict_ignored"] is True
|
||||
assert result["verdict_ignored_reason"] == "verdict_not_requested"
|
||||
assert result["verdict_submitted"] is False
|
||||
|
||||
|
||||
async def test_publish_review_forwards_authorized_verdict() -> None:
|
||||
|
|
|
|||
|
|
@ -220,6 +220,11 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None:
|
|||
new_callable=AsyncMock,
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"agent.webhooks.common._resolve_verdict_authorization",
|
||||
new_callable=AsyncMock,
|
||||
return_value=True,
|
||||
) as resolve_verdict,
|
||||
patch("agent.webhooks.common.cache_github_token_for_thread"),
|
||||
patch(
|
||||
"agent.webhooks.common.set_reviewer_thread_metadata",
|
||||
|
|
@ -241,6 +246,8 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None:
|
|||
assert configurable["re_review"] is True
|
||||
assert configurable["last_reviewed_sha"] == "oldsha"
|
||||
assert configurable["head_sha"] == "newsha"
|
||||
assert configurable["verdict_authorized"] is True
|
||||
resolve_verdict.assert_awaited_once_with({"owner": "lc", "name": "repo"}, pr)
|
||||
# The live head is persisted to thread metadata so a re-review queued into
|
||||
# an in-flight run can resolve it despite the run's frozen config.
|
||||
head_sha_writes = [
|
||||
|
|
|
|||
94
tests/webhooks/test_verdict_authorization.py
Normal file
94
tests/webhooks/test_verdict_authorization.py
Normal file
|
|
@ -0,0 +1,94 @@
|
|||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.webhooks import common
|
||||
|
||||
|
||||
def _pr(*, author: str = "alice", fork: bool = False) -> dict[str, object]:
|
||||
return {
|
||||
"user": {"login": author},
|
||||
"head": {"repo": {"full_name": "external/repo" if fork else "acme/repo"}},
|
||||
"base": {"repo": {"full_name": "acme/repo"}},
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
(
|
||||
"team_enabled",
|
||||
"repo_enabled",
|
||||
"pr_metadata",
|
||||
"active_member",
|
||||
"expected",
|
||||
),
|
||||
[
|
||||
(False, True, _pr(), True, True),
|
||||
(False, False, _pr(), True, False),
|
||||
(False, True, _pr(fork=True), True, False),
|
||||
(False, True, _pr(), False, False),
|
||||
(False, True, _pr(author="open-swe[bot]"), False, True),
|
||||
(True, False, _pr(), True, True),
|
||||
],
|
||||
ids=[
|
||||
"repo-enabled-trusted-nonfork",
|
||||
"disabled",
|
||||
"fork",
|
||||
"untrusted",
|
||||
"internal-bot",
|
||||
"team-enabled",
|
||||
],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_verdict_authorization_truth_table(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
team_enabled: bool,
|
||||
repo_enabled: bool,
|
||||
pr_metadata: dict[str, object],
|
||||
active_member: bool,
|
||||
expected: bool,
|
||||
) -> None:
|
||||
monkeypatch.setattr(
|
||||
common,
|
||||
"get_team_auto_verdict_enabled",
|
||||
AsyncMock(return_value=team_enabled),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
common,
|
||||
"is_auto_verdict_repo_enabled",
|
||||
AsyncMock(return_value=repo_enabled),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
common,
|
||||
"is_user_active_org_member",
|
||||
AsyncMock(return_value=active_member),
|
||||
)
|
||||
|
||||
authorized = await common._resolve_verdict_authorization(
|
||||
{"owner": "acme", "name": "repo"}, pr_metadata
|
||||
)
|
||||
|
||||
assert authorized is expected
|
||||
|
||||
|
||||
def test_build_reviewer_configurable_only_sets_true_verdict_flags() -> None:
|
||||
base = {
|
||||
"source": "github",
|
||||
"github_login": "alice",
|
||||
"github_user_id": 1,
|
||||
"repo_config": {"owner": "acme", "name": "repo"},
|
||||
"pr_number": 7,
|
||||
"pr_url": "https://github.com/acme/repo/pull/7",
|
||||
"base_sha": "base",
|
||||
"head_sha": "head",
|
||||
"branch_name": "feature",
|
||||
}
|
||||
|
||||
disabled = common._build_reviewer_configurable(**base)
|
||||
authorized = common._build_reviewer_configurable(**base, verdict_authorized=True)
|
||||
|
||||
assert "verdict_authorized" not in disabled
|
||||
assert "verdict_requested" not in disabled
|
||||
assert authorized["verdict_authorized"] is True
|
||||
assert "verdict_requested" not in authorized
|
||||
|
|
@ -157,6 +157,7 @@ export interface ProfileUpdate {
|
|||
}
|
||||
|
||||
export interface TeamSettings {
|
||||
auto_verdict: boolean
|
||||
review_draft_prs: boolean
|
||||
pr_summaries: boolean
|
||||
review_trace_links: boolean
|
||||
|
|
@ -303,6 +304,10 @@ export interface ReposPayload {
|
|||
repositories: Array<Repository>
|
||||
}
|
||||
|
||||
export interface RepoPolicyPayload {
|
||||
repos: Array<string>
|
||||
}
|
||||
|
||||
export type ReviewStyleStatus = "idle" | "running" | "completed" | "failed"
|
||||
|
||||
export interface ReviewStyle {
|
||||
|
|
@ -730,12 +735,18 @@ export const api = {
|
|||
method: "DELETE",
|
||||
}),
|
||||
listAutoReviewRepos: () =>
|
||||
request<{ repos: Array<string> }>("/enabled-review-repos"),
|
||||
request<RepoPolicyPayload>("/enabled-review-repos"),
|
||||
setAutoReviewRepo: (full_name: string, runAutomatically: boolean) =>
|
||||
request<{ repos: Array<string> }>("/enabled-review-repos", {
|
||||
request<RepoPolicyPayload>("/enabled-review-repos", {
|
||||
method: "PUT",
|
||||
body: JSON.stringify({ full_name, enabled: runAutomatically }),
|
||||
}),
|
||||
listAutoVerdictRepos: () => request<RepoPolicyPayload>("/auto-verdict-repos"),
|
||||
setAutoVerdictRepo: (full_name: string, enabled: boolean) =>
|
||||
request<RepoPolicyPayload>("/auto-verdict-repos", {
|
||||
method: "PUT",
|
||||
body: JSON.stringify({ full_name, enabled }),
|
||||
}),
|
||||
usageLeaderboard: (period: UsageLeaderboardPeriod = "30d", limit = 10) =>
|
||||
request<UsageLeaderboardPayload>(
|
||||
`/agent-usage-leaderboard?period=${encodeURIComponent(period)}&limit=${limit}`
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ import { useSession } from "@/lib/session";
|
|||
export const Route = createFileRoute("/review")({ component: ReviewPage });
|
||||
|
||||
const DEFAULT_SETTINGS: TeamSettings = {
|
||||
auto_verdict: false,
|
||||
review_draft_prs: false,
|
||||
pr_summaries: true,
|
||||
review_trace_links: true,
|
||||
|
|
@ -141,6 +142,17 @@ function ReviewPage() {
|
|||
|
||||
<SettingsSection title="Configuration">
|
||||
<div className="divide-y divide-border">
|
||||
<SettingsRow
|
||||
label="Automatic Verdicts"
|
||||
description="Allow trusted, non-fork pull requests to receive reviewer verdicts by default. Repository-specific opt-ins remain available when this is off."
|
||||
control={
|
||||
<Switch
|
||||
checked={current.auto_verdict}
|
||||
onCheckedChange={(v) => persist({ auto_verdict: v })}
|
||||
disabled={!canEdit}
|
||||
/>
|
||||
}
|
||||
/>
|
||||
<SettingsRow
|
||||
label="Review Draft PRs"
|
||||
description="Org-wide default for whether Open SWE Review runs on draft PRs. Each user can override this in Profile Settings."
|
||||
|
|
|
|||
|
|
@ -41,6 +41,11 @@ function RepositoriesOwnerPage() {
|
|||
queryFn: api.listAutoReviewRepos,
|
||||
enabled: !!session.data,
|
||||
});
|
||||
const autoVerdict = useQuery({
|
||||
queryKey: ["autoVerdictRepos"],
|
||||
queryFn: api.listAutoVerdictRepos,
|
||||
enabled: !!session.data,
|
||||
});
|
||||
|
||||
const toggleAutoReview = useMutation({
|
||||
mutationFn: ({ full_name, on }: { full_name: string; on: boolean }) =>
|
||||
|
|
@ -49,6 +54,13 @@ function RepositoriesOwnerPage() {
|
|||
qc.setQueryData(["autoReviewRepos"], data);
|
||||
},
|
||||
});
|
||||
const toggleAutoVerdict = useMutation({
|
||||
mutationFn: ({ full_name, on }: { full_name: string; on: boolean }) =>
|
||||
api.setAutoVerdictRepo(full_name, on),
|
||||
onSuccess: (data) => {
|
||||
qc.setQueryData(["autoVerdictRepos"], data);
|
||||
},
|
||||
});
|
||||
|
||||
const ownerRepos = useMemo(
|
||||
() =>
|
||||
|
|
@ -62,6 +74,10 @@ function RepositoriesOwnerPage() {
|
|||
() => new Set(autoReview.data?.repos ?? []),
|
||||
[autoReview.data?.repos],
|
||||
);
|
||||
const autoVerdictSet = useMemo(
|
||||
() => new Set(autoVerdict.data?.repos ?? []),
|
||||
[autoVerdict.data?.repos],
|
||||
);
|
||||
|
||||
const [page, setPage] = useState(0);
|
||||
useEffect(() => setPage(0), [owner]);
|
||||
|
|
@ -83,7 +99,8 @@ function RepositoriesOwnerPage() {
|
|||
|
||||
const canEdit = session.data.is_admin;
|
||||
const autoReviewCount = ownerRepos.filter((r) => autoReviewSet.has(r.full_name)).length;
|
||||
const loading = repos.isLoading || autoReview.isLoading;
|
||||
const autoVerdictCount = ownerRepos.filter((r) => autoVerdictSet.has(r.full_name)).length;
|
||||
const loading = repos.isLoading || autoReview.isLoading || autoVerdict.isLoading;
|
||||
|
||||
return (
|
||||
<AppShell
|
||||
|
|
@ -102,7 +119,7 @@ function RepositoriesOwnerPage() {
|
|||
Repositories
|
||||
</h2>
|
||||
<span className="text-xs text-muted-foreground">
|
||||
{autoReviewCount}/{ownerRepos.length} run automatically
|
||||
{autoReviewCount}/{ownerRepos.length} run automatically · {autoVerdictCount} verdict enabled
|
||||
</span>
|
||||
</div>
|
||||
<div className="rounded-lg border border-border bg-card">
|
||||
|
|
@ -119,6 +136,7 @@ function RepositoriesOwnerPage() {
|
|||
<ul className="divide-y divide-border">
|
||||
{pageRepos.map((r) => {
|
||||
const runsAutomatically = autoReviewSet.has(r.full_name);
|
||||
const verdictEnabled = autoVerdictSet.has(r.full_name);
|
||||
return (
|
||||
<li
|
||||
key={r.full_name}
|
||||
|
|
@ -135,25 +153,47 @@ function RepositoriesOwnerPage() {
|
|||
<span className="text-[10px] text-muted-foreground">private</span>
|
||||
)}
|
||||
</div>
|
||||
<div className="flex items-center gap-2">
|
||||
<span className="text-xs text-muted-foreground">Run automatically</span>
|
||||
<span
|
||||
title={
|
||||
!canEdit
|
||||
? "Only team admins can modify automatic review settings"
|
||||
: undefined
|
||||
}
|
||||
className={!canEdit ? "cursor-not-allowed" : undefined}
|
||||
>
|
||||
<Switch
|
||||
aria-label={`Run reviews automatically for ${r.full_name}`}
|
||||
checked={runsAutomatically}
|
||||
disabled={!canEdit || toggleAutoReview.isPending}
|
||||
onCheckedChange={(v) =>
|
||||
toggleAutoReview.mutate({ full_name: r.full_name, on: v })
|
||||
<div className="flex items-center gap-4">
|
||||
<div className="flex items-center gap-2">
|
||||
<span className="text-xs text-muted-foreground">Run automatically</span>
|
||||
<span
|
||||
title={
|
||||
!canEdit
|
||||
? "Only team admins can modify automatic review settings"
|
||||
: undefined
|
||||
}
|
||||
/>
|
||||
</span>
|
||||
className={!canEdit ? "cursor-not-allowed" : undefined}
|
||||
>
|
||||
<Switch
|
||||
aria-label={`Run reviews automatically for ${r.full_name}`}
|
||||
checked={runsAutomatically}
|
||||
disabled={!canEdit || toggleAutoReview.isPending}
|
||||
onCheckedChange={(v) =>
|
||||
toggleAutoReview.mutate({ full_name: r.full_name, on: v })
|
||||
}
|
||||
/>
|
||||
</span>
|
||||
</div>
|
||||
<div className="flex items-center gap-2">
|
||||
<span className="text-xs text-muted-foreground">Automatic verdicts</span>
|
||||
<span
|
||||
title={
|
||||
!canEdit
|
||||
? "Only team admins can modify automatic verdict settings"
|
||||
: undefined
|
||||
}
|
||||
className={!canEdit ? "cursor-not-allowed" : undefined}
|
||||
>
|
||||
<Switch
|
||||
aria-label={`Allow automatic verdicts for ${r.full_name}`}
|
||||
checked={verdictEnabled}
|
||||
disabled={!canEdit || toggleAutoVerdict.isPending}
|
||||
onCheckedChange={(v) =>
|
||||
toggleAutoVerdict.mutate({ full_name: r.full_name, on: v })
|
||||
}
|
||||
/>
|
||||
</span>
|
||||
</div>
|
||||
</div>
|
||||
</li>
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue