mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 08:03:15 +00:00
fix: reviewer resolves GitHub App token at run start, not via cross-process cache (#1409)
Reviewer/push runs execute in a worker process, but the GitHub token was cached by the webhook handler in the API server process — a different process — so the worker's in-process cache was always cold. resolve_github_token then failed (User not authenticated / Unknown source: github_push) and only the app-token fallback kept reviews working, noisily. The reviewer always acts as the GitHub App (open-swe[bot]), so resolve the installation token directly at run start, scoped to the repo. This also bypasses org SAML enforcement that blocks user OAuth tokens. Drop the now-dead cross-process cache writes in the webhook reviewer-dispatch handlers, and stop leave_failure_comment raising on the github_push source.
This commit is contained in:
parent
3716380cab
commit
60426e8b10
7 changed files with 64 additions and 52 deletions
|
|
@ -63,9 +63,8 @@ from .tools import (
|
||||||
web_search,
|
web_search,
|
||||||
)
|
)
|
||||||
from .utils.agents_md import fetch_agents_md
|
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_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.model import DEFAULT_LLM_REASONING, make_model, provider_model_kwargs
|
||||||
from .utils.sandbox_paths import aresolve_sandbox_work_dir
|
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 {}
|
repo_config = config["configurable"].get("repo") or {}
|
||||||
github_token: str | None = None
|
github_token: str | None = None
|
||||||
if config["configurable"].get("source"):
|
if config["configurable"].get("source"):
|
||||||
cached_token, _cached_expires_at = await get_github_token_from_thread(thread_id)
|
# Reviewer runs always act as the GitHub App (open-swe[bot]). Resolve the
|
||||||
if cached_token:
|
# installation token in this process at run start rather than relying on a
|
||||||
github_token = cached_token
|
# token cached by the webhook handler, which runs in a separate process. The
|
||||||
else:
|
# App token also bypasses org SAML enforcement that blocks user OAuth tokens.
|
||||||
try:
|
repo_name = str(repo_config.get("name") or "")
|
||||||
github_token, _expires_at = await resolve_github_token(config, thread_id)
|
github_token, expires_at = await get_github_app_installation_token_with_expiry(
|
||||||
except RuntimeError:
|
repositories=[repo_name] if repo_name else None
|
||||||
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 not github_token:
|
||||||
)
|
raise RuntimeError(
|
||||||
if github_token:
|
f"GitHub App installation token unavailable for reviewer thread {thread_id}"
|
||||||
cache_github_token_for_thread(thread_id, github_token, expires_at=expires_at)
|
)
|
||||||
else:
|
# Cache in-process so reviewer tools and the sandbox proxy can read it this run.
|
||||||
raise
|
cache_github_token_for_thread(thread_id, github_token, expires_at=expires_at)
|
||||||
|
|
||||||
repo_private = config["configurable"].get("repo_private")
|
repo_private = config["configurable"].get("repo_private")
|
||||||
github_proxy_token = github_token if repo_private is False else None
|
github_proxy_token = github_token if repo_private is False else None
|
||||||
|
|
|
||||||
|
|
@ -278,7 +278,7 @@ async def leave_failure_comment(
|
||||||
f"connect your Slack account in {link}, then tag me again.",
|
f"connect your Slack account in {link}, then tag me again.",
|
||||||
)
|
)
|
||||||
return
|
return
|
||||||
if source == "github":
|
if source in ("github", "github_push"):
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"Auth failure for GitHub-triggered run (no token to post comment): %s", message
|
"Auth failure for GitHub-triggered run (no token to post comment): %s", message
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -1808,8 +1808,6 @@ async def trigger_pr_review_from_ref(
|
||||||
if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
||||||
return {"success": False, "error": "Could not create reviewer thread"}
|
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 = {
|
pr_meta: ReviewerPRMeta = {
|
||||||
"owner": pr_ref.owner,
|
"owner": pr_ref.owner,
|
||||||
"name": pr_ref.repo,
|
"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):
|
if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
||||||
return
|
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)
|
await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha)
|
||||||
|
|
||||||
is_re_review = bool(last_reviewed_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)
|
langgraph_client = get_client(url=LANGGRAPH_URL)
|
||||||
if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client):
|
||||||
return
|
return
|
||||||
cache_github_token_for_thread(thread_id, app_token, expires_at=app_token_expires_at)
|
|
||||||
try:
|
try:
|
||||||
threads = await fetch_pr_review_threads(
|
threads = await fetch_pr_review_threads(
|
||||||
owner=repo_config["owner"],
|
owner=repo_config["owner"],
|
||||||
|
|
@ -2651,7 +2646,6 @@ async def process_github_review_finding_reply(payload: dict[str, Any]) -> None:
|
||||||
)
|
)
|
||||||
if not app_token:
|
if not app_token:
|
||||||
return
|
return
|
||||||
cache_github_token_for_thread(thread_id, app_token, expires_at=app_token_expires_at)
|
|
||||||
|
|
||||||
threads = await fetch_pr_review_threads(
|
threads = await fetch_pr_review_threads(
|
||||||
owner=repo_config["owner"],
|
owner=repo_config["owner"],
|
||||||
|
|
|
||||||
|
|
@ -797,8 +797,6 @@ def test_process_github_pr_ready_creates_reviewer_run(monkeypatch) -> None:
|
||||||
"thread_id": captured["thread_id"],
|
"thread_id": captured["thread_id"],
|
||||||
"if_exists": "do_nothing",
|
"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 "https://github.com/langchain-ai/open-swe/pull/1244" in prompt
|
||||||
assert "Base SHA: base-sha" in prompt
|
assert "Base SHA: base-sha" in prompt
|
||||||
assert "Head SHA: head-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",
|
"if_exists": "do_nothing",
|
||||||
}
|
}
|
||||||
assert captured["metadata_token"] == "app-token"
|
assert captured["metadata_token"] == "app-token"
|
||||||
assert captured["cache_token"] == "app-token"
|
|
||||||
assert "Base SHA: base-sha" in prompt
|
assert "Base SHA: base-sha" in prompt
|
||||||
assert "Head SHA: head-sha" in prompt
|
assert "Head SHA: head-sha" in prompt
|
||||||
assert config["source"] == "slack"
|
assert config["source"] == "slack"
|
||||||
|
|
|
||||||
|
|
@ -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))
|
await webapp.process_github_pr_ready(_pr_payload(action="opened", draft=False, private=False))
|
||||||
|
|
||||||
get_token.assert_awaited_once_with(repository_ids=[123])
|
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
|
_, kwargs = fake_client.runs.create.await_args
|
||||||
assert kwargs["config"]["configurable"]["repo_private"] is False
|
assert kwargs["config"]["configurable"]["repo_private"] is False
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -59,11 +59,12 @@ class _DummyAgent:
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@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 = {
|
config: RunnableConfig = {
|
||||||
"configurable": {
|
"configurable": {
|
||||||
"__is_for_execution__": True,
|
"__is_for_execution__": True,
|
||||||
"thread_id": "reviewer-thread-id",
|
"thread_id": "reviewer-thread-id",
|
||||||
|
"repo": {"owner": "acme", "name": "repo"},
|
||||||
"source": "slack",
|
"source": "slack",
|
||||||
"review_requested": True,
|
"review_requested": True,
|
||||||
},
|
},
|
||||||
|
|
@ -73,11 +74,11 @@ async def test_reviewer_uses_cached_thread_token_for_slack_review_request() -> N
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("app-token", None),
|
return_value=("app-token", None),
|
||||||
) as mock_get_thread_token,
|
) as mock_app_token,
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock) as mock_resolve_token,
|
patch("agent.reviewer.cache_github_token_for_thread") as mock_cache_token,
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -96,12 +97,45 @@ async def test_reviewer_uses_cached_thread_token_for_slack_review_request() -> N
|
||||||
metadata = config["metadata"]
|
metadata = config["metadata"]
|
||||||
assert isinstance(metadata, dict)
|
assert isinstance(metadata, dict)
|
||||||
assert "github_token_encrypted" not in metadata
|
assert "github_token_encrypted" not in metadata
|
||||||
mock_get_thread_token.assert_awaited_once_with("reviewer-thread-id")
|
# Token is resolved in this process at run start (scoped to the repo), not read
|
||||||
mock_resolve_token.assert_not_called()
|
# 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"]
|
middleware = create_agent.call_args.kwargs["middleware"]
|
||||||
assert reviewer.check_message_queue_before_model in 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
|
@pytest.mark.asyncio
|
||||||
async def test_reviewer_applies_eval_model_and_effort_overrides() -> None:
|
async def test_reviewer_applies_eval_model_and_effort_overrides() -> None:
|
||||||
config: RunnableConfig = {
|
config: RunnableConfig = {
|
||||||
|
|
@ -288,11 +322,10 @@ async def test_reviewer_inlines_agents_md_into_system_prompt() -> None:
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -648,11 +681,10 @@ async def test_reviewer_injects_pr_review_threads_into_first_review_context() ->
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -720,11 +752,10 @@ async def test_reviewer_injects_pr_review_threads_into_re_review_context() -> No
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -784,11 +815,10 @@ async def test_reviewer_omits_threads_block_when_fetch_returns_empty() -> None:
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -844,11 +874,10 @@ async def test_reviewer_continues_when_thread_fetch_raises() -> None:
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -912,11 +941,10 @@ async def test_reviewer_populates_diff_line_set_from_github_api() -> None:
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -978,11 +1006,10 @@ async def test_reviewer_leaves_validation_disabled_when_diff_fetch_fails() -> No
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
@ -1040,11 +1067,10 @@ async def test_reviewer_injects_pr_title_and_body_into_context() -> None:
|
||||||
|
|
||||||
with (
|
with (
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.get_github_token_from_thread",
|
"agent.reviewer.get_github_app_installation_token_with_expiry",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value=("gh-token", None),
|
return_value=("gh-token", None),
|
||||||
),
|
),
|
||||||
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
|
|
||||||
patch(
|
patch(
|
||||||
"agent.reviewer.ensure_sandbox_for_thread",
|
"agent.reviewer.ensure_sandbox_for_thread",
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
|
|
|
||||||
|
|
@ -404,7 +404,6 @@ async def test_push_event_public_repo_uses_scoped_token() -> None:
|
||||||
await webapp.process_github_push_event(payload)
|
await webapp.process_github_push_event(payload)
|
||||||
|
|
||||||
get_token.assert_awaited_once_with(repository_ids=[123])
|
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
|
_, kwargs = fake_client.runs.create.await_args
|
||||||
assert kwargs["config"]["configurable"]["repo_private"] is False
|
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)
|
await webapp.process_github_push_event(payload)
|
||||||
|
|
||||||
assert get_token.await_args_list == [call(), call(repository_ids=[456])]
|
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
|
_, kwargs = fake_client.runs.create.await_args
|
||||||
assert kwargs["config"]["configurable"]["repo_private"] is False
|
assert kwargs["config"]["configurable"]["repo_private"] is False
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue