From a2a169249342b2744db61d8f825a512d12e6b36f Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 28 May 2026 14:29:33 -0700 Subject: [PATCH] Remove reviewer env allowlist (#1353) --- agent/webapp.py | 33 ++---------- tests/conftest.py | 4 +- tests/test_github_issue_webhook.py | 82 +++++++++++------------------- tests/test_pr_ready_auto_review.py | 3 -- tests/test_public_repo_org_gate.py | 2 - tests/test_reviewer_watch.py | 36 +++++++++---- 6 files changed, 61 insertions(+), 99 deletions(-) diff --git a/agent/webapp.py b/agent/webapp.py index f88d3648..47469d1d 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -146,16 +146,6 @@ ALLOWED_GITHUB_ORGS: frozenset[str] = frozenset( for org in os.environ.get("ALLOWED_GITHUB_ORGS", "").split(",") if org.strip() ) -ALLOWED_REVIEWER_GITHUB_ORGS: frozenset[str] = frozenset( - org.strip().lower() - for org in os.environ.get("ALLOWED_REVIEWER_GITHUB_ORGS", "").split(",") - if org.strip() -) -ALLOWED_REVIEWER_GITHUB_REPOS: frozenset[str] = frozenset( - repo.strip().lower() - for repo in os.environ.get("ALLOWED_REVIEWER_GITHUB_REPOS", "").split(",") - if repo.strip() -) # Org whose members are allowed to tag @open-swe on public repos. When empty, # the public-repo gate is disabled (back-compat). PUBLIC_REPO_ORG_GATE: str = os.environ.get("PUBLIC_REPO_ORG_GATE", "").strip() @@ -405,29 +395,12 @@ def _is_repo_allowed(repo_config: dict[str, str]) -> bool: return False -def _is_repo_allowed_for_reviewer(repo_config: dict[str, str]) -> bool: - """Check if a repo is allowed for reviewer-agent webhook entrypoints.""" - owner = repo_config.get("owner", "").lower() - name = repo_config.get("name", "").lower() - full_name = f"{owner}/{name}" if owner and name else "" - - if ALLOWED_REVIEWER_GITHUB_REPOS: - return full_name in ALLOWED_REVIEWER_GITHUB_REPOS - - if not ALLOWED_REVIEWER_GITHUB_ORGS: - return True - return owner in ALLOWED_REVIEWER_GITHUB_ORGS - - async def _is_repo_enabled_for_review(repo_config: dict[str, str]) -> bool: - """Combined gate: operator allowlist + team opt-in list from the dashboard. + """Check the dashboard opt-in list for reviewer-agent entrypoints. - Both checks must pass. The opt-in list is empty by default, so repos - are off until an admin enables them in the dashboard's Open SWE Review - tab. + The opt-in list is empty by default, so repos are off until an admin + enables them in the dashboard's Open SWE Review tab. """ - if not _is_repo_allowed_for_reviewer(repo_config): - return False return await is_review_repo_enabled(repo_config.get("owner", ""), repo_config.get("name", "")) diff --git a/tests/conftest.py b/tests/conftest.py index 77d2133a..94241b6e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,9 +12,7 @@ def _default_enable_review_repos(monkeypatch: pytest.MonkeyPatch) -> None: """Treat every repo as enabled for review by default. The dashboard's opt-in list (loaded by :func:`agent.dashboard.enabled_repos.is_review_repo_enabled`) - is empty in the test environment because there is no live LangGraph Store. Tests that - exercise the env-based allowlists (``ALLOWED_REVIEWER_GITHUB_*``) would otherwise be - blocked by the opt-in gate that lives on top of them. + is empty in the test environment because there is no live LangGraph Store. Tests targeting the opt-in gate itself should override this fixture or set ``monkeypatch.setattr(webapp, "is_review_repo_enabled", ...)`` to a stricter stub. diff --git a/tests/test_github_issue_webhook.py b/tests/test_github_issue_webhook.py index 0f4ac75d..538d1c54 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -94,39 +94,31 @@ def test_build_github_issue_followup_prompt_only_includes_comment() -> None: assert "## Title" not in prompt -def test_reviewer_repo_allowlist_allows_matching_repo(monkeypatch) -> None: - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset()) - monkeypatch.setattr( - webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) - ) +def test_reviewer_enablement_uses_dashboard_opt_in(monkeypatch) -> None: + seen: dict[str, str] = {} + + async def fake_is_review_repo_enabled(owner: str, name: str) -> bool: + seen["owner"] = owner + seen["name"] = name + return owner == "langchain-ai" and name == "open-swe-app" + + monkeypatch.setattr(webapp, "is_review_repo_enabled", fake_is_review_repo_enabled) assert ( - webapp._is_repo_allowed_for_reviewer({"owner": "langchain-ai", "name": "open-swe"}) is True + asyncio.run( + webapp._is_repo_enabled_for_review({"owner": "langchain-ai", "name": "open-swe-app"}) + ) + is True ) - - -def test_reviewer_repo_allowlist_blocks_non_matching_repo(monkeypatch) -> None: - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset({"langchain-ai"})) - monkeypatch.setattr( - webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) - ) - + assert seen == {"owner": "langchain-ai", "name": "open-swe-app"} assert ( - webapp._is_repo_allowed_for_reviewer({"owner": "langchain-ai", "name": "public-demo"}) + asyncio.run( + webapp._is_repo_enabled_for_review({"owner": "langchain-ai", "name": "open-swe"}) + ) is False ) -def test_reviewer_org_allowlist_allows_all_repos_in_org(monkeypatch) -> None: - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset({"langchain-ai"})) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset()) - - assert ( - webapp._is_repo_allowed_for_reviewer({"owner": "langchain-ai", "name": "any-repo"}) is True - ) - assert webapp._is_repo_allowed_for_reviewer({"owner": "other-org", "name": "any-repo"}) is False - - def test_github_webhook_accepts_issue_events(monkeypatch) -> None: called: dict[str, object] = {} @@ -501,7 +493,7 @@ def test_github_webhook_ignores_unsupported_comment_action(monkeypatch) -> None: } -def test_github_webhook_blocks_reviewer_repo_not_in_reviewer_repo_allowlist(monkeypatch) -> None: +def test_github_webhook_blocks_reviewer_repo_not_enabled_in_dashboard(monkeypatch) -> None: called = False async def fake_process_github_pr_review_request(payload: dict[str, object]) -> None: @@ -512,10 +504,11 @@ def test_github_webhook_blocks_reviewer_repo_not_in_reviewer_repo_allowlist(monk webapp, "process_github_pr_review_request", fake_process_github_pr_review_request ) monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset({"langchain-ai"})) - monkeypatch.setattr( - webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) - ) + + async def fake_is_review_repo_enabled(_owner: str, _name: str) -> bool: + return False + + monkeypatch.setattr(webapp, "is_review_repo_enabled", fake_is_review_repo_enabled) client = TestClient(webapp.app) response = _post_github_webhook( @@ -550,9 +543,6 @@ def test_github_webhook_accepts_open_swe_review_requested(monkeypatch) -> None: webapp, "process_github_pr_review_request", fake_process_github_pr_review_request ) monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) - monkeypatch.setattr( - webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) - ) client = TestClient(webapp.app) response = _post_github_webhook( @@ -1031,8 +1021,6 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: monkeypatch.setattr(webapp, "is_thread_active", fake_is_thread_active) monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata) monkeypatch.setattr(webapp, "get_client", lambda url: _FakeLangGraphClient()) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset()) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset()) result = asyncio.run( webapp.trigger_pr_review_from_ref( @@ -1071,7 +1059,7 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: } -def test_trigger_pr_review_from_ref_respects_reviewer_allowlist(monkeypatch) -> None: +def test_trigger_pr_review_from_ref_respects_dashboard_opt_in(monkeypatch) -> None: called = False async def fake_get_github_app_installation_token() -> str | None: @@ -1082,10 +1070,11 @@ def test_trigger_pr_review_from_ref_respects_reviewer_allowlist(monkeypatch) -> monkeypatch.setattr( webapp, "get_github_app_installation_token", fake_get_github_app_installation_token ) - monkeypatch.setattr( - webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) - ) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset()) + + async def fake_is_review_repo_enabled(_owner: str, _name: str) -> bool: + return False + + monkeypatch.setattr(webapp, "is_review_repo_enabled", fake_is_review_repo_enabled) result = asyncio.run( webapp.trigger_pr_review_from_ref( @@ -1458,27 +1447,16 @@ def test_github_webhook_routes_pr_comment_review_to_agent(monkeypatch) -> None: assert captured["event_type"] == "issue_comment" -def test_github_webhook_routes_pr_review_request_outside_reviewer_allowlist_to_agent( - monkeypatch, -) -> None: +def test_github_webhook_routes_pr_review_request_comment_to_agent(monkeypatch) -> None: captured: dict[str, object] = {} async def fake_process_pr_comment(payload: dict[str, object], event_type: str) -> None: captured["payload"] = payload captured["event_type"] = event_type - async def fake_process_review_command( - payload: dict[str, object], event_type: str, pr_url_override: str | None - ) -> None: - raise AssertionError("review allowlist is enforced by request_pr_review") - monkeypatch.setattr(webapp, "process_github_pr_comment", fake_process_pr_comment) - monkeypatch.setattr(webapp, "process_github_pr_review_command", fake_process_review_command) monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) monkeypatch.setattr(webapp, "ALLOWED_GITHUB_ORGS", frozenset({"langchain-ai"})) - monkeypatch.setattr( - webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) - ) client = TestClient(webapp.app) response = _post_github_webhook( diff --git a/tests/test_pr_ready_auto_review.py b/tests/test_pr_ready_auto_review.py index e84b1e74..849034af 100644 --- a/tests/test_pr_ready_auto_review.py +++ b/tests/test_pr_ready_auto_review.py @@ -260,7 +260,6 @@ async def test_converted_to_draft_disables_watch_when_drafts_off( 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, @@ -288,7 +287,6 @@ async def test_converted_to_draft_keeps_watch_when_author_drafts_on( ) -> 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, @@ -316,7 +314,6 @@ async def test_converted_to_draft_keeps_watch_when_team_default_drafts_on( ) -> 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, diff --git a/tests/test_public_repo_org_gate.py b/tests/test_public_repo_org_gate.py index 0df971d1..fa843e7a 100644 --- a/tests/test_public_repo_org_gate.py +++ b/tests/test_public_repo_org_gate.py @@ -48,8 +48,6 @@ def _common_setup(monkeypatch, *, gate: str = "langchain-ai") -> None: monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) monkeypatch.setattr(webapp, "PUBLIC_REPO_ORG_GATE", gate) monkeypatch.setattr(webapp, "ALLOWED_GITHUB_ORGS", frozenset()) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset()) - monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset()) def test_gate_blocks_non_member_on_public_pr_comment(monkeypatch) -> None: diff --git a/tests/test_reviewer_watch.py b/tests/test_reviewer_watch.py index a077e36f..2c4d9b22 100644 --- a/tests/test_reviewer_watch.py +++ b/tests/test_reviewer_watch.py @@ -32,7 +32,9 @@ async def test_push_event_skips_branch_deletion() -> None: payload = _push_payload( ref="refs/heads/feat-x", after="0000000000000000000000000000000000000000" ) - with patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True): + with patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ): await webapp.process_github_push_event(payload) # If we got here without crashing and with no other patches needed, the # function returned early on the deletion check. @@ -52,7 +54,9 @@ async def test_push_event_skips_when_thread_not_watching() -> None: fake_client.runs.create = AsyncMock() with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp.get_github_app_installation_token", new_callable=AsyncMock, @@ -89,7 +93,9 @@ async def test_push_event_skips_when_pr_diff_unchanged_since_last_review() -> No set_metadata = AsyncMock() with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, @@ -141,7 +147,9 @@ async def test_push_event_queues_when_thread_active_even_if_pr_diff_unchanged() queue_message = AsyncMock() with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, @@ -199,7 +207,9 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None: fake_client.runs.create = AsyncMock() with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp.get_github_app_installation_token", new_callable=AsyncMock, @@ -271,7 +281,9 @@ async def test_push_event_idempotent_when_head_unchanged() -> None: fake_client.runs.create = AsyncMock() with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp.get_github_app_installation_token", new_callable=AsyncMock, @@ -305,7 +317,9 @@ async def test_pr_close_disables_watch() -> None: captured.append((thread_id, kwargs)) with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp._get_thread_metadata_safe", new_callable=AsyncMock, @@ -325,7 +339,9 @@ async def test_pr_reopened_re_enables_watch() -> None: captured.append((thread_id, kwargs)) with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp._get_thread_metadata_safe", new_callable=AsyncMock, @@ -341,7 +357,9 @@ async def test_pr_reopened_re_enables_watch() -> None: async def test_pr_close_skips_non_reviewer_threads() -> None: fake_set = AsyncMock() with ( - patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._is_repo_enabled_for_review", new_callable=AsyncMock, return_value=True + ), patch( "agent.webapp._get_thread_metadata_safe", new_callable=AsyncMock,