From 8552e871f4b79e471b2b00c7667e8040460d02a6 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Mon, 29 Jun 2026 12:10:54 -0400 Subject: [PATCH] fix: fail closed for unbound-legacy sandboxes and catch repo mismatch Gap 1: a thread with a persisted sandbox_id but no in-memory cache and no recorded bound_repo (a pre-binding legacy thread, post-deploy) previously reconnected-and-served the sandbox to the current repo, then rebound it. Now fail closed: drop the stale id and recreate a fresh sandbox bound to this repo, logging a reconnect-with-missing-binding event. A sandbox is never served to a repo unless its binding is known and matches; new threads bind on first run unchanged. Gap 3: catch SandboxRepoMismatchError at the agent and reviewer run entrypoints, log it for alarming, and surface a clean sanitized error instead of letting an opaque deep-stack exception crash-loop the worker. --- agent/reviewer.py | 19 +++-- agent/server.py | 55 +++++++++++---- tests/test_proxy_auth.py | 9 ++- tests/test_repo_binding_isolation.py | 101 +++++++++++++++++++++++++++ 4 files changed, 165 insertions(+), 19 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index e3a39c23..c6b8794c 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -59,6 +59,7 @@ from .server import ( DEFAULT_LLM_MAX_TOKENS, DEFAULT_RECURSION_LIMIT, MODEL_CALL_RECURSION_LIMIT, + SandboxRepoMismatchError, _general_purpose_subagent, ensure_sandbox_for_thread, graph_loaded_for_execution, @@ -851,12 +852,18 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: if repo_config.get("owner") and repo_config.get("name") else None ) - sandbox_backend = await ensure_sandbox_for_thread( - thread_id, - github_proxy_token=github_proxy_token, - github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None, - repo=repo_for_snapshot, - ) + try: + sandbox_backend = await ensure_sandbox_for_thread( + thread_id, + github_proxy_token=github_proxy_token, + github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None, + repo=repo_for_snapshot, + ) + except SandboxRepoMismatchError as exc: + # Repo-binding refusal at the run boundary: log for alarming and surface the + # sanitized terminal error rather than crash-looping the reviewer worker. + logger.error("Refusing reviewer run for thread %s: %s", thread_id, exc) + raise RuntimeError(str(exc)) from exc work_dir = await aresolve_sandbox_work_dir(sandbox_backend) diff --git a/agent/server.py b/agent/server.py index 5faef2f6..558825c4 100644 --- a/agent/server.py +++ b/agent/server.py @@ -451,14 +451,19 @@ async def ensure_sandbox_for_thread( sandbox_backend = SANDBOX_BACKENDS.get(thread_id) sandbox_id = await get_sandbox_id_from_metadata(thread_id) - # Repo-binding guard (TID-COLLIDE-01): refuse to reuse a thread's sandbox for - # a repo it is not bound to, so a colliding thread_id from a different repo - # cannot bind to (or clobber) another thread's sandbox. + if sandbox_id == SANDBOX_CREATING and not sandbox_backend: + logger.info("Sandbox creation in progress for thread %s, waiting...", thread_id) + sandbox_id = await _resolve_creating_sentinel(thread_id) + + # Repo-binding guard (TID-COLLIDE-01): a sandbox is never served to a repo + # unless its binding is known and matches. current_repo = repo_cache_key(repo) bound_repo = await get_bound_repo_from_metadata(thread_id) proxy_bound = getattr(sandbox_backend, "bound_repo", None) effective_bound = bound_repo or (proxy_bound if isinstance(proxy_bound, str) else None) if current_repo and effective_bound and effective_bound != current_repo: + # Known binding that does not match the current repo: refuse outright so a + # colliding thread_id from a different repo cannot reuse/clobber it. logger.error( "Repo mismatch for thread %s: bound=%s current=%s; refusing sandbox reuse", thread_id, @@ -466,10 +471,26 @@ async def ensure_sandbox_for_thread( current_repo, ) raise SandboxRepoMismatchError(thread_id, effective_bound, current_repo) - - if sandbox_id == SANDBOX_CREATING and not sandbox_backend: - logger.info("Sandbox creation in progress for thread %s, waiting...", thread_id) - sandbox_id = await _resolve_creating_sentinel(thread_id) + if ( + current_repo + and not effective_bound + and sandbox_backend is None + and isinstance(sandbox_id, str) + and sandbox_id not in (None, SANDBOX_CREATING) + ): + # Fail CLOSED for unbound-legacy threads (migration window): a thread with a + # persisted sandbox_id but no in-memory cache and no recorded bound_repo + # cannot be confirmed to belong to the current repo, so never + # reconnect-and-serve it. Drop the stale id and recreate a fresh sandbox + # bound to this repo below. + logger.error( + "reconnect-with-missing-binding for thread %s: persisted sandbox %s has no " + "bound_repo; refusing reuse and recreating for repo %s", + thread_id, + sandbox_id, + current_repo, + ) + sandbox_id = None if sandbox_backend: logger.info("Using cached sandbox backend for thread %s", thread_id) @@ -668,11 +689,21 @@ async def get_agent(config: RunnableConfig) -> Pregel: ) team_defaults_task = asyncio.create_task(get_team_default_model_pair("agent")) profile_task = asyncio.create_task(load_profile(profile_login)) if profile_login else None - triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather( - triggering_user_identity_task, - sandbox_task, - team_defaults_task, - ) + try: + triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather( + triggering_user_identity_task, + sandbox_task, + team_defaults_task, + ) + except SandboxRepoMismatchError as exc: + # Repo-binding refusal at the run boundary: log for alarming and surface the + # already-sanitized terminal error (no sandbox/token internals) to the caller, + # rather than letting an opaque deep-stack exception crash-loop the worker. + logger.error("Refusing agent run for thread %s: %s", thread_id, exc) + for pending in (triggering_user_identity_task, team_defaults_task, profile_task): + if pending is not None and not pending.done(): + pending.cancel() + raise RuntimeError(str(exc)) from exc profile = await profile_task if profile_task is not None else None del github_token diff --git a/tests/test_proxy_auth.py b/tests/test_proxy_auth.py index cc827bcb..dfdfa903 100644 --- a/tests/test_proxy_auth.py +++ b/tests/test_proxy_auth.py @@ -309,7 +309,7 @@ class TestRefreshProxyOnSandboxReuse: @pytest.mark.asyncio async def test_refreshes_proxy_when_reconnecting_to_existing_langsmith_sandbox(self) -> None: - """Reconnected sandboxes should also get a fresh proxy token.""" + """A bound thread reconnecting to its sandbox should get a fresh proxy token.""" config = self._execution_config() mock_sandbox = MagicMock(id="sandbox-existing") @@ -325,6 +325,13 @@ class TestRefreshProxyOnSandboxReuse: new_callable=AsyncMock, return_value="sandbox-existing", ), + # Thread is bound to the current repo, so reconnect proceeds (an unbound + # legacy thread would instead fail closed and recreate). + patch( + "agent.server.get_bound_repo_from_metadata", + new_callable=AsyncMock, + return_value="langchain-ai/open-swe", + ), patch("agent.server.create_sandbox", return_value=mock_sandbox) as mock_create, patch( "agent.server.get_github_app_installation_token_with_expiry", diff --git a/tests/test_repo_binding_isolation.py b/tests/test_repo_binding_isolation.py index 3ecf288b..6e9c94bd 100644 --- a/tests/test_repo_binding_isolation.py +++ b/tests/test_repo_binding_isolation.py @@ -53,11 +53,24 @@ def test_unbound_token_served_when_repo_unknown() -> None: assert github_token.get_github_token({"configurable": {"thread_id": "tid"}}) == "ghp_legacy" +def test_cached_token_reused_for_same_repo_case_insensitive() -> None: + """``Org/Repo`` and ``org/repo`` are the same repo: no spurious refusal.""" + future = (datetime.now(UTC) + timedelta(hours=1)).isoformat() + github_token.cache_github_token_for_thread( + "tid", "ghp_repoA", expires_at=future, repo={"owner": "Acme", "name": "Alpha"} + ) + cfg = {"configurable": {"thread_id": "tid", "repo": {"owner": "acme", "name": "alpha"}}} + assert github_token.get_github_token(cfg) == "ghp_repoA" + + def test_repo_cache_key_normalizes() -> None: assert github_token.repo_cache_key({"owner": "o", "name": "r"}) == "o/r" assert github_token.repo_cache_key("o/r") == "o/r" assert github_token.repo_cache_key({"owner": "o"}) is None assert github_token.repo_cache_key(None) is None + # Casefolded so different casing of the same repo collapses to one key. + assert github_token.repo_cache_key({"owner": "Org", "name": "Repo"}) == "org/repo" + assert github_token.repo_cache_key("Org/Repo") == "org/repo" # --- sandbox repo binding ---------------------------------------------------- @@ -122,3 +135,91 @@ async def test_ensure_sandbox_allows_matching_repo(monkeypatch: pytest.MonkeyPat result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "acme", "name": "alpha"}) assert result is backend assert calls["git"] == 1 + + +@pytest.mark.asyncio +async def test_ensure_sandbox_reuses_same_repo_case_insensitive( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A bound sandbox is reused when the current repo differs only by casing.""" + + async def fake_sandbox_id(_tid: str) -> str: + return "sb-A" + + async def fake_bound_repo(_tid: str) -> str: + return "acme/alpha" + + backend = _FakeBackend("sb-A") + backend.bound_repo = "acme/alpha" + + async def fake_check(b: Any, *_a: Any, **_k: Any) -> Any: + return b + + async def fake_refresh(b: Any, *_a: Any, **_k: Any) -> Any: + return b + + async def fake_git(_b: Any) -> None: + return None + + monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id) + monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo) + monkeypatch.setattr(server, "check_or_recreate_sandbox", fake_check) + monkeypatch.setattr(server, "_refresh_github_proxy_or_recreate", fake_refresh) + monkeypatch.setattr(server, "set_sandbox_backend", lambda _tid, b, **_k: b) + monkeypatch.setattr(server, "_configure_git_identity", fake_git) + server.SANDBOX_BACKENDS["tid"] = backend # type: ignore[assignment] + + # Different casing of the same repo must not raise and must reuse the sandbox. + result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "ACME", "name": "Alpha"}) + assert result is backend + + +@pytest.mark.asyncio +async def test_legacy_unbound_sandbox_recreated_not_reused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A legacy thread (sandbox_id present, bound_repo absent) must fail closed. + + With no recorded binding, the existing sandbox cannot be confirmed to belong + to the current repo, so it is never reconnected-and-served: a fresh sandbox is + created and bound to the requesting repo instead. + """ + + async def fake_sandbox_id(_tid: str) -> str: + return "sb-legacy" + + async def fake_bound_repo(_tid: str) -> None: + return None + + def fake_reconnect(*_a: Any, **_k: Any) -> Any: # pragma: no cover - must not run + raise AssertionError("must not reconnect to a legacy unbound sandbox") + + fresh = _FakeBackend("sb-fresh") + + async def fake_create_with_proxy(*_a: Any, **_k: Any) -> Any: + return fresh + + async def fake_git(_b: Any) -> None: + return None + + class _Threads: + async def update(self, **_k: Any) -> None: + return None + + async def get(self, *_a: Any, **_k: Any) -> dict[str, Any]: + return {} + + class _Client: + threads = _Threads() + + monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id) + monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo) + monkeypatch.setattr(server, "create_sandbox", fake_reconnect) + monkeypatch.setattr(server, "_create_sandbox_with_proxy", fake_create_with_proxy) + monkeypatch.setattr(server, "_configure_git_identity", fake_git) + monkeypatch.setattr(server, "client", _Client()) + + result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "evil", "name": "beta"}) + + assert result.id == "sb-fresh" + assert server.SANDBOX_BACKENDS["tid"].bound_repo == "evil/beta"