Remove reviewer env allowlist (#1353)

This commit is contained in:
Johannes du Plessis 2026-05-28 14:29:33 -07:00 • committed by GitHub
parent d8d3794649
commit a2a1692493
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 61 additions and 99 deletions

View file

@ -146,16 +146,6 @@ ALLOWED_GITHUB_ORGS: frozenset[str] = frozenset(
for org in os.environ.get("ALLOWED_GITHUB_ORGS", "").split(",") for org in os.environ.get("ALLOWED_GITHUB_ORGS", "").split(",")
if org.strip() 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, # Org whose members are allowed to tag @open-swe on public repos. When empty,
# the public-repo gate is disabled (back-compat). # the public-repo gate is disabled (back-compat).
PUBLIC_REPO_ORG_GATE: str = os.environ.get("PUBLIC_REPO_ORG_GATE", "").strip() 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 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: 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 The opt-in list is empty by default, so repos are off until an admin
are off until an admin enables them in the dashboard's Open SWE Review enables them in the dashboard's Open SWE Review tab.
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", "")) return await is_review_repo_enabled(repo_config.get("owner", ""), repo_config.get("name", ""))

View file

@ -12,9 +12,7 @@ def _default_enable_review_repos(monkeypatch: pytest.MonkeyPatch) -> None:
"""Treat every repo as enabled for review by default. """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`) 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 is empty in the test environment because there is no live LangGraph Store.
exercise the env-based allowlists (``ALLOWED_REVIEWER_GITHUB_*``) would otherwise be
blocked by the opt-in gate that lives on top of them.
Tests targeting the opt-in gate itself should override this fixture or set Tests targeting the opt-in gate itself should override this fixture or set
``monkeypatch.setattr(webapp, "is_review_repo_enabled", ...)`` to a stricter stub. ``monkeypatch.setattr(webapp, "is_review_repo_enabled", ...)`` to a stricter stub.

View file

@ -94,39 +94,31 @@ def test_build_github_issue_followup_prompt_only_includes_comment() -> None:
assert "## Title" not in prompt assert "## Title" not in prompt
def test_reviewer_repo_allowlist_allows_matching_repo(monkeypatch) -> None: def test_reviewer_enablement_uses_dashboard_opt_in(monkeypatch) -> None:
monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset()) seen: dict[str, str] = {}
monkeypatch.setattr(
webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) 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 ( 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
) )
assert seen == {"owner": "langchain-ai", "name": "open-swe-app"}
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 ( 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 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: def test_github_webhook_accepts_issue_events(monkeypatch) -> None:
called: dict[str, object] = {} 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 called = False
async def fake_process_github_pr_review_request(payload: dict[str, object]) -> None: 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 webapp, "process_github_pr_review_request", fake_process_github_pr_review_request
) )
monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET)
monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset({"langchain-ai"}))
monkeypatch.setattr( async def fake_is_review_repo_enabled(_owner: str, _name: str) -> bool:
webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset({"langchain-ai/open-swe"}) return False
)
monkeypatch.setattr(webapp, "is_review_repo_enabled", fake_is_review_repo_enabled)
client = TestClient(webapp.app) client = TestClient(webapp.app)
response = _post_github_webhook( 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 webapp, "process_github_pr_review_request", fake_process_github_pr_review_request
) )
monkeypatch.setattr(webapp, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET) 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) client = TestClient(webapp.app)
response = _post_github_webhook( 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, "is_thread_active", fake_is_thread_active)
monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata) monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata)
monkeypatch.setattr(webapp, "get_client", lambda url: _FakeLangGraphClient()) 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( result = asyncio.run(
webapp.trigger_pr_review_from_ref( 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 called = False
async def fake_get_github_app_installation_token() -> str | None: 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( monkeypatch.setattr(
webapp, "get_github_app_installation_token", fake_get_github_app_installation_token webapp, "get_github_app_installation_token", fake_get_github_app_installation_token
) )
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, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset())
monkeypatch.setattr(webapp, "is_review_repo_enabled", fake_is_review_repo_enabled)
result = asyncio.run( result = asyncio.run(
webapp.trigger_pr_review_from_ref( 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" assert captured["event_type"] == "issue_comment"
def test_github_webhook_routes_pr_review_request_outside_reviewer_allowlist_to_agent( def test_github_webhook_routes_pr_review_request_comment_to_agent(monkeypatch) -> None:
monkeypatch,
) -> None:
captured: dict[str, object] = {} captured: dict[str, object] = {}
async def fake_process_pr_comment(payload: dict[str, object], event_type: str) -> None: async def fake_process_pr_comment(payload: dict[str, object], event_type: str) -> None:
captured["payload"] = payload captured["payload"] = payload
captured["event_type"] = event_type 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_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, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET)
monkeypatch.setattr(webapp, "ALLOWED_GITHUB_ORGS", frozenset({"langchain-ai"})) 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) client = TestClient(webapp.app)
response = _post_github_webhook( response = _post_github_webhook(

View file

@ -260,7 +260,6 @@ async def test_converted_to_draft_disables_watch_when_drafts_off(
captured.append((thread_id, kwargs)) captured.append((thread_id, kwargs))
with ( with (
patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True),
patch( patch(
"agent.webapp._get_thread_metadata_safe", "agent.webapp._get_thread_metadata_safe",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -288,7 +287,6 @@ async def test_converted_to_draft_keeps_watch_when_author_drafts_on(
) -> None: ) -> None:
fake_set = AsyncMock() fake_set = AsyncMock()
with ( with (
patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True),
patch( patch(
"agent.webapp._get_thread_metadata_safe", "agent.webapp._get_thread_metadata_safe",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -316,7 +314,6 @@ async def test_converted_to_draft_keeps_watch_when_team_default_drafts_on(
) -> None: ) -> None:
fake_set = AsyncMock() fake_set = AsyncMock()
with ( with (
patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True),
patch( patch(
"agent.webapp._get_thread_metadata_safe", "agent.webapp._get_thread_metadata_safe",
new_callable=AsyncMock, new_callable=AsyncMock,

View file

@ -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, "GITHUB_WEBHOOK_SECRET", _TEST_WEBHOOK_SECRET)
monkeypatch.setattr(webapp, "PUBLIC_REPO_ORG_GATE", gate) monkeypatch.setattr(webapp, "PUBLIC_REPO_ORG_GATE", gate)
monkeypatch.setattr(webapp, "ALLOWED_GITHUB_ORGS", frozenset()) 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: def test_gate_blocks_non_member_on_public_pr_comment(monkeypatch) -> None:

View file

@ -32,7 +32,9 @@ async def test_push_event_skips_branch_deletion() -> None:
payload = _push_payload( payload = _push_payload(
ref="refs/heads/feat-x", after="0000000000000000000000000000000000000000" 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) await webapp.process_github_push_event(payload)
# If we got here without crashing and with no other patches needed, the # If we got here without crashing and with no other patches needed, the
# function returned early on the deletion check. # 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() fake_client.runs.create = AsyncMock()
with ( 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( patch(
"agent.webapp.get_github_app_installation_token", "agent.webapp.get_github_app_installation_token",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -89,7 +93,9 @@ async def test_push_event_skips_when_pr_diff_unchanged_since_last_review() -> No
set_metadata = AsyncMock() set_metadata = AsyncMock()
with ( 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( patch(
"agent.webapp.get_github_app_installation_token_with_expiry", "agent.webapp.get_github_app_installation_token_with_expiry",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -141,7 +147,9 @@ async def test_push_event_queues_when_thread_active_even_if_pr_diff_unchanged()
queue_message = AsyncMock() queue_message = AsyncMock()
with ( 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( patch(
"agent.webapp.get_github_app_installation_token_with_expiry", "agent.webapp.get_github_app_installation_token_with_expiry",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -199,7 +207,9 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None:
fake_client.runs.create = AsyncMock() fake_client.runs.create = AsyncMock()
with ( 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( patch(
"agent.webapp.get_github_app_installation_token", "agent.webapp.get_github_app_installation_token",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -271,7 +281,9 @@ async def test_push_event_idempotent_when_head_unchanged() -> None:
fake_client.runs.create = AsyncMock() fake_client.runs.create = AsyncMock()
with ( 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( patch(
"agent.webapp.get_github_app_installation_token", "agent.webapp.get_github_app_installation_token",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -305,7 +317,9 @@ async def test_pr_close_disables_watch() -> None:
captured.append((thread_id, kwargs)) captured.append((thread_id, kwargs))
with ( 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( patch(
"agent.webapp._get_thread_metadata_safe", "agent.webapp._get_thread_metadata_safe",
new_callable=AsyncMock, new_callable=AsyncMock,
@ -325,7 +339,9 @@ async def test_pr_reopened_re_enables_watch() -> None:
captured.append((thread_id, kwargs)) captured.append((thread_id, kwargs))
with ( 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( patch(
"agent.webapp._get_thread_metadata_safe", "agent.webapp._get_thread_metadata_safe",
new_callable=AsyncMock, 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: async def test_pr_close_skips_non_reviewer_threads() -> None:
fake_set = AsyncMock() fake_set = AsyncMock()
with ( 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( patch(
"agent.webapp._get_thread_metadata_safe", "agent.webapp._get_thread_metadata_safe",
new_callable=AsyncMock, new_callable=AsyncMock,