diff --git a/agent/dashboard/auto_verdict_repos.py b/agent/dashboard/auto_verdict_repos.py new file mode 100644 index 00000000..c08c83b5 --- /dev/null +++ b/agent/dashboard/auto_verdict_repos.py @@ -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) diff --git a/agent/dashboard/routes.py b/agent/dashboard/routes.py index 448ef0ff..69f36b3f 100644 --- a/agent/dashboard/routes.py +++ b/agent/dashboard/routes.py @@ -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, diff --git a/agent/dashboard/team_settings.py b/agent/dashboard/team_settings.py index a484af47..a90f8dca 100644 --- a/agent/dashboard/team_settings.py +++ b/agent/dashboard/team_settings.py @@ -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() diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 3be23a9d..2f90256a 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -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( diff --git a/agent/webhooks/common.py b/agent/webhooks/common.py index c47a2bb5..5e427573 100644 --- a/agent/webhooks/common.py +++ b/agent/webhooks/common.py @@ -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: diff --git a/agent/webhooks/github.py b/agent/webhooks/github.py index 36074959..89330a52 100644 --- a/agent/webhooks/github.py +++ b/agent/webhooks/github.py @@ -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( { diff --git a/tests/dashboard/test_auto_verdict_settings.py b/tests/dashboard/test_auto_verdict_settings.py new file mode 100644 index 00000000..90a25881 --- /dev/null +++ b/tests/dashboard/test_auto_verdict_settings.py @@ -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) diff --git a/tests/github/test_github_issue_webhook.py b/tests/github/test_github_issue_webhook.py index 6b8073f2..1e9313ca 100644 --- a/tests/github/test_github_issue_webhook.py +++ b/tests/github/test_github_issue_webhook.py @@ -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 "\nApprove if it meets the merge bar." in prompt diff --git a/tests/reviewer/test_pr_ready_auto_review.py b/tests/reviewer/test_pr_ready_auto_review.py index 0b9a4312..bb9a231b 100644 --- a/tests/reviewer/test_pr_ready_auto_review.py +++ b/tests/reviewer/test_pr_ready_auto_review.py @@ -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 diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index 9735b280..3202a307 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -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: diff --git a/tests/reviewer/test_reviewer_watch.py b/tests/reviewer/test_reviewer_watch.py index 65e767a5..3b879de5 100644 --- a/tests/reviewer/test_reviewer_watch.py +++ b/tests/reviewer/test_reviewer_watch.py @@ -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 = [ diff --git a/tests/webhooks/test_verdict_authorization.py b/tests/webhooks/test_verdict_authorization.py new file mode 100644 index 00000000..314b9d76 --- /dev/null +++ b/tests/webhooks/test_verdict_authorization.py @@ -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 diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index f93fdd24..9d455d8e 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -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 } +export interface RepoPolicyPayload { + repos: Array +} + export type ReviewStyleStatus = "idle" | "running" | "completed" | "failed" export interface ReviewStyle { @@ -730,12 +735,18 @@ export const api = { method: "DELETE", }), listAutoReviewRepos: () => - request<{ repos: Array }>("/enabled-review-repos"), + request("/enabled-review-repos"), setAutoReviewRepo: (full_name: string, runAutomatically: boolean) => - request<{ repos: Array }>("/enabled-review-repos", { + request("/enabled-review-repos", { method: "PUT", body: JSON.stringify({ full_name, enabled: runAutomatically }), }), + listAutoVerdictRepos: () => request("/auto-verdict-repos"), + setAutoVerdictRepo: (full_name: string, enabled: boolean) => + request("/auto-verdict-repos", { + method: "PUT", + body: JSON.stringify({ full_name, enabled }), + }), usageLeaderboard: (period: UsageLeaderboardPeriod = "30d", limit = 10) => request( `/agent-usage-leaderboard?period=${encodeURIComponent(period)}&limit=${limit}` diff --git a/ui/src/routes/review.tsx b/ui/src/routes/review.tsx index 1f1a75fc..8b36acbc 100644 --- a/ui/src/routes/review.tsx +++ b/ui/src/routes/review.tsx @@ -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() {
+ persist({ auto_verdict: v })} + disabled={!canEdit} + /> + } + /> @@ -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 ( - {autoReviewCount}/{ownerRepos.length} run automatically + {autoReviewCount}/{ownerRepos.length} run automatically · {autoVerdictCount} verdict enabled
@@ -119,6 +136,7 @@ function RepositoriesOwnerPage() {
    {pageRepos.map((r) => { const runsAutomatically = autoReviewSet.has(r.full_name); + const verdictEnabled = autoVerdictSet.has(r.full_name); return (
  • private )}
-
- Run automatically - - - toggleAutoReview.mutate({ full_name: r.full_name, on: v }) +
+
+ Run automatically + - + className={!canEdit ? "cursor-not-allowed" : undefined} + > + + toggleAutoReview.mutate({ full_name: r.full_name, on: v }) + } + /> + +
+
+ Automatic verdicts + + + toggleAutoVerdict.mutate({ full_name: r.full_name, on: v }) + } + /> + +
);