diff --git a/agent/reviewer.py b/agent/reviewer.py index 9b0b6509..46f0a80e 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -63,9 +63,8 @@ from .tools import ( web_search, ) from .utils.agents_md import fetch_agents_md -from .utils.auth import resolve_github_token from .utils.github_app import get_github_app_installation_token_with_expiry -from .utils.github_token import cache_github_token_for_thread, get_github_token_from_thread +from .utils.github_token import cache_github_token_for_thread from .utils.model import DEFAULT_LLM_REASONING, make_model, provider_model_kwargs from .utils.sandbox_paths import aresolve_sandbox_work_dir @@ -626,20 +625,20 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: repo_config = config["configurable"].get("repo") or {} github_token: str | None = None if config["configurable"].get("source"): - cached_token, _cached_expires_at = await get_github_token_from_thread(thread_id) - if cached_token: - github_token = cached_token - else: - try: - github_token, _expires_at = await resolve_github_token(config, thread_id) - except RuntimeError: - github_token, expires_at = await get_github_app_installation_token_with_expiry( - repositories=[str(repo_config.get("name"))] if repo_config.get("name") else None - ) - if github_token: - cache_github_token_for_thread(thread_id, github_token, expires_at=expires_at) - else: - raise + # Reviewer runs always act as the GitHub App (open-swe[bot]). Resolve the + # installation token in this process at run start rather than relying on a + # token cached by the webhook handler, which runs in a separate process. The + # App token also bypasses org SAML enforcement that blocks user OAuth tokens. + repo_name = str(repo_config.get("name") or "") + github_token, expires_at = await get_github_app_installation_token_with_expiry( + repositories=[repo_name] if repo_name else None + ) + if not github_token: + raise RuntimeError( + f"GitHub App installation token unavailable for reviewer thread {thread_id}" + ) + # Cache in-process so reviewer tools and the sandbox proxy can read it this run. + cache_github_token_for_thread(thread_id, github_token, expires_at=expires_at) repo_private = config["configurable"].get("repo_private") github_proxy_token = github_token if repo_private is False else None diff --git a/agent/utils/auth.py b/agent/utils/auth.py index d35d2643..6e3958bb 100644 --- a/agent/utils/auth.py +++ b/agent/utils/auth.py @@ -278,7 +278,7 @@ async def leave_failure_comment( f"connect your Slack account in {link}, then tag me again.", ) return - if source == "github": + if source in ("github", "github_push"): logger.warning( "Auth failure for GitHub-triggered run (no token to post comment): %s", message ) diff --git a/agent/webapp.py b/agent/webapp.py index a0aa63e9..247668e7 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -1808,8 +1808,6 @@ async def trigger_pr_review_from_ref( if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client): return {"success": False, "error": "Could not create reviewer thread"} - cache_github_token_for_thread(thread_id, app_token, expires_at=app_token_expires_at) - pr_meta: ReviewerPRMeta = { "owner": pr_ref.owner, "name": pr_ref.repo, @@ -1998,8 +1996,6 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client): return - cache_github_token_for_thread(thread_id, app_token, expires_at=app_token_expires_at) - await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha) is_re_review = bool(last_reviewed_sha) @@ -2342,7 +2338,6 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: langgraph_client = get_client(url=LANGGRAPH_URL) if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client): return - cache_github_token_for_thread(thread_id, app_token, expires_at=app_token_expires_at) try: threads = await fetch_pr_review_threads( owner=repo_config["owner"], @@ -2651,7 +2646,6 @@ async def process_github_review_finding_reply(payload: dict[str, Any]) -> None: ) if not app_token: return - cache_github_token_for_thread(thread_id, app_token, expires_at=app_token_expires_at) threads = await fetch_pr_review_threads( owner=repo_config["owner"], diff --git a/tests/test_github_issue_webhook.py b/tests/test_github_issue_webhook.py index 867c9adf..e4e7466f 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -797,8 +797,6 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None: "thread_id": captured["thread_id"], "if_exists": "do_nothing", } - assert captured["cache_token"] == "app-token" - assert captured["cache_thread_id"] == captured["thread_id"] assert "https://github.com/langchain-ai/open-swe/pull/1244" in prompt assert "Base SHA: base-sha" in prompt assert "Head SHA: head-sha" in prompt @@ -894,7 +892,6 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: "if_exists": "do_nothing", } assert captured["metadata_token"] == "app-token" - assert captured["cache_token"] == "app-token" assert "Base SHA: base-sha" in prompt assert "Head SHA: head-sha" in prompt assert config["source"] == "slack" diff --git a/tests/test_pr_ready_auto_review.py b/tests/test_pr_ready_auto_review.py index 2cb3be96..39701c15 100644 --- a/tests/test_pr_ready_auto_review.py +++ b/tests/test_pr_ready_auto_review.py @@ -85,8 +85,6 @@ async def test_pr_ready_public_repo_uses_scoped_reviewer_token( await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=False, private=False)) get_token.assert_awaited_once_with(repository_ids=[123]) - cache_token.assert_called_once() - assert cache_token.call_args.args[1] == "scoped-token" _, kwargs = fake_client.runs.create.await_args assert kwargs["config"]["configurable"]["repo_private"] is False diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index ed617510..b714e4a7 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -59,11 +59,12 @@ class _DummyAgent: @pytest.mark.asyncio -async def test_reviewer_uses_cached_thread_token_for_slack_review_request() -> None: +async def test_reviewer_resolves_app_installation_token_at_run_start() -> None: config: RunnableConfig = { "configurable": { "__is_for_execution__": True, "thread_id": "reviewer-thread-id", + "repo": {"owner": "acme", "name": "repo"}, "source": "slack", "review_requested": True, }, @@ -73,11 +74,11 @@ async def test_reviewer_uses_cached_thread_token_for_slack_review_request() -> N with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("app-token", None), - ) as mock_get_thread_token, - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock) as mock_resolve_token, + ) as mock_app_token, + patch("agent.reviewer.cache_github_token_for_thread") as mock_cache_token, patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -96,12 +97,45 @@ async def test_reviewer_uses_cached_thread_token_for_slack_review_request() -> N metadata = config["metadata"] assert isinstance(metadata, dict) assert "github_token_encrypted" not in metadata - mock_get_thread_token.assert_awaited_once_with("reviewer-thread-id") - mock_resolve_token.assert_not_called() + # Token is resolved in this process at run start (scoped to the repo), not read + # from a cache the webhook handler populated in a different process. + mock_app_token.assert_awaited_once_with(repositories=["repo"]) + mock_cache_token.assert_called_once_with("reviewer-thread-id", "app-token", expires_at=None) middleware = create_agent.call_args.kwargs["middleware"] assert reviewer.check_message_queue_before_model in middleware +@pytest.mark.asyncio +async def test_reviewer_raises_when_app_installation_token_unavailable() -> None: + config: RunnableConfig = { + "configurable": { + "__is_for_execution__": True, + "thread_id": "reviewer-thread-id", + "repo": {"owner": "acme", "name": "repo"}, + "source": "github_push", + }, + "metadata": {}, + } + + with ( + patch( + "agent.reviewer.get_github_app_installation_token_with_expiry", + new_callable=AsyncMock, + return_value=(None, None), + ), + patch( + "agent.reviewer.ensure_sandbox_for_thread", + new_callable=AsyncMock, + return_value=MagicMock(), + ) as mock_sandbox, + patch("agent.reviewer.create_deep_agent", return_value=_DummyAgent()), + ): + with pytest.raises(RuntimeError, match="installation token unavailable"): + await reviewer.get_reviewer_agent(config) + + mock_sandbox.assert_not_awaited() + + @pytest.mark.asyncio async def test_reviewer_applies_eval_model_and_effort_overrides() -> None: config: RunnableConfig = { @@ -288,11 +322,10 @@ async def test_reviewer_inlines_agents_md_into_system_prompt() -> None: with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -648,11 +681,10 @@ async def test_reviewer_injects_pr_review_threads_into_first_review_context() -> with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -720,11 +752,10 @@ async def test_reviewer_injects_pr_review_threads_into_re_review_context() -> No with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -784,11 +815,10 @@ async def test_reviewer_omits_threads_block_when_fetch_returns_empty() -> None: with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -844,11 +874,10 @@ async def test_reviewer_continues_when_thread_fetch_raises() -> None: with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -912,11 +941,10 @@ async def test_reviewer_populates_diff_line_set_from_github_api() -> None: with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -978,11 +1006,10 @@ async def test_reviewer_leaves_validation_disabled_when_diff_fetch_fails() -> No with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, @@ -1040,11 +1067,10 @@ async def test_reviewer_injects_pr_title_and_body_into_context() -> None: with ( patch( - "agent.reviewer.get_github_token_from_thread", + "agent.reviewer.get_github_app_installation_token_with_expiry", new_callable=AsyncMock, return_value=("gh-token", None), ), - patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock), patch( "agent.reviewer.ensure_sandbox_for_thread", new_callable=AsyncMock, diff --git a/tests/test_reviewer_watch.py b/tests/test_reviewer_watch.py index 014369cc..23d74acf 100644 --- a/tests/test_reviewer_watch.py +++ b/tests/test_reviewer_watch.py @@ -404,7 +404,6 @@ async def test_push_event_public_repo_uses_scoped_token() -> None: await webapp.process_github_push_event(payload) get_token.assert_awaited_once_with(repository_ids=[123]) - assert cache_token.call_args.args[1] == "scoped-token" _, kwargs = fake_client.runs.create.await_args assert kwargs["config"]["configurable"]["repo_private"] is False @@ -450,7 +449,6 @@ async def test_push_event_rescopes_token_when_pr_metadata_reveals_public() -> No await webapp.process_github_push_event(payload) assert get_token.await_args_list == [call(), call(repository_ids=[456])] - assert cache_token.call_args.args[1] == "scoped-token" _, kwargs = fake_client.runs.create.await_args assert kwargs["config"]["configurable"]["repo_private"] is False