diff --git a/agent/middleware/workflow_push_guard.py b/agent/middleware/workflow_push_guard.py index 50b8b0b5..ccaab820 100644 --- a/agent/middleware/workflow_push_guard.py +++ b/agent/middleware/workflow_push_guard.py @@ -35,7 +35,7 @@ from ..utils.github_app import ( RUNTIME_PROXY_TOKEN_PERMISSIONS, WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS, ) -from ..utils.github_proxy import refresh_proxy_token +from ..utils.github_proxy import get_recorded_proxy_permissions, refresh_proxy_token from ..utils.sandbox_state import SANDBOX_BACKENDS from ..utils.slack import post_slack_thread_reply_with_ts @@ -876,6 +876,13 @@ async def _run_with_workflow_token( if sandbox_type != "langsmith": return await run() + # Capture the run's standing scope *before* elevating so we restore exactly + # what the install resolved to (RUNTIME, or CORE when actions:read is absent) + # rather than a hardcoded RUNTIME that 422s on installs lacking actions:read. + # The standing ladder never carries workflows:write, so this is always a real + # downscope. Fall back to RUNTIME if nothing was recorded yet. + baseline_scope = get_recorded_proxy_permissions(thread_id) or RUNTIME_PROXY_TOKEN_PERMISSIONS + elevated = await refresh_proxy_token( thread_id, permissions=WORKFLOW_RUNTIME_PROXY_TOKEN_PERMISSIONS ) @@ -904,21 +911,22 @@ async def _run_with_workflow_token( try: return await run() finally: - restored = await refresh_proxy_token(thread_id, permissions=RUNTIME_PROXY_TOKEN_PERMISSIONS) - if not restored: + restored = await refresh_proxy_token(thread_id, permissions=baseline_scope) + if not restored and baseline_scope != CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS: logger.error( - "SECURITY: failed to downscope proxy token for thread %s after an approved " - "workflow push; retrying without actions:read.", + "SECURITY: failed to restore baseline proxy scope for thread %s after an " + "approved workflow push; retrying at the guaranteed core scope.", thread_id, ) - if not await refresh_proxy_token( + restored = await refresh_proxy_token( thread_id, permissions=CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS - ): - logger.error( - "SECURITY: proxy token downscope fully failed for thread %s; the sandbox " - "may retain workflows:write for the remainder of this run.", - thread_id, - ) + ) + if not restored: + logger.error( + "SECURITY: proxy token downscope fully failed for thread %s; the sandbox " + "may retain workflows:write for the remainder of this run.", + thread_id, + ) class WorkflowPushGuardMiddleware(AgentMiddleware): diff --git a/agent/server.py b/agent/server.py index 0fe3beab..72658ec7 100644 --- a/agent/server.py +++ b/agent/server.py @@ -227,8 +227,8 @@ async def _resolve_proxy_token( continue if index: logger.warning( - "GitHub proxy token minted with reduced scope %s; installation is " - "missing higher-privilege grants", + "GitHub proxy token minted with reduced scope %s; a higher-privilege " + "scope was unavailable (missing grant or transient mint failure)", sorted(scope), ) return token, expires_at, scope diff --git a/agent/utils/github_app.py b/agent/utils/github_app.py index f6907c98..3a488683 100644 --- a/agent/utils/github_app.py +++ b/agent/utils/github_app.py @@ -189,9 +189,32 @@ async def get_github_app_installation_token_with_expiry( if isinstance(token, str) and token and parsed is not None: _TOKEN_CACHE[key] = (token, expires_at, parsed - _TOKEN_CACHE_MARGIN) return token, expires_at + except httpx.HTTPStatusError as exc: + status = exc.response.status_code if exc.response is not None else None + if status == 422: + # Expected when the installation hasn't granted a requested permission: + # the ladder caller descends to a smaller scope. Not a transient error, + # so keep it at debug even on the terminal rung. + logger.debug( + "GitHub App token mint rejected for requested scope (HTTP 422)", exc_info=True + ) + elif log_errors: + logger.exception("Failed to get GitHub App installation token") + else: + # A non-422 failure is NOT a missing grant; surface it so a transient + # error doesn't silently downscope a run that would otherwise qualify. + logger.warning( + "GitHub App token mint failed (HTTP %s); scope may degrade transiently", + status, + exc_info=True, + ) + return None, None except Exception: if log_errors: logger.exception("Failed to get GitHub App installation token") else: - logger.debug("Failed to get GitHub App installation token", exc_info=True) + logger.warning( + "GitHub App token mint failed unexpectedly; scope may degrade transiently", + exc_info=True, + ) return None, None diff --git a/agent/utils/github_proxy.py b/agent/utils/github_proxy.py index c92762a7..20e6f89f 100644 --- a/agent/utils/github_proxy.py +++ b/agent/utils/github_proxy.py @@ -91,6 +91,23 @@ def clear_proxy_token_expiry(thread_id: str | None) -> None: _PROXY_TOKEN_EXPIRY.pop(thread_id, None) +def get_recorded_proxy_permissions(thread_id: str | None) -> dict[str, str] | None: + """The permission scope last minted for ``thread_id``'s proxy token, if any. + + Lets a caller that temporarily elevates the proxy scope restore the exact + baseline the run resolved to (e.g. an install granted workflows:write but not + actions:read resolves to core), instead of guessing a fixed scope that may + 422 on restore. + """ + if not thread_id: + return None + record = _PROXY_TOKEN_EXPIRY.get(thread_id) + if record is None: + return None + *_, permission_key = _unpack_proxy_token_record(record) + return dict(permission_key) if permission_key else None + + def _unpack_proxy_token_record(record: tuple[Any, ...]) -> ProxyTokenRecord: expires_at, recorded_at, repositories, *rest = record permissions = rest[0] if rest else () diff --git a/tests/test_github_app.py b/tests/test_github_app.py index f5f923d1..ec3e43ec 100644 --- a/tests/test_github_app.py +++ b/tests/test_github_app.py @@ -1,8 +1,10 @@ from __future__ import annotations +import logging from datetime import UTC, datetime, timedelta from typing import Any +import httpx import pytest from agent.utils import github_app @@ -183,6 +185,68 @@ def test_proxy_token_ladder_descends_to_core() -> None: ) +class _HTTPStatusErrorClient: + """Raises an ``HTTPStatusError`` with a configurable status on mint.""" + + status = 500 + + def __init__(self, **kwargs: Any) -> None: + pass + + async def __aenter__(self) -> _HTTPStatusErrorClient: + return self + + async def __aexit__(self, exc_type: object, exc: object, tb: object) -> None: + return None + + async def post(self, url: str, **kwargs: Any) -> Any: + request = httpx.Request("POST", url) + response = httpx.Response(type(self).status, request=request) + raise httpx.HTTPStatusError("mint failed", request=request, response=response) + + +@pytest.mark.asyncio +async def test_missing_grant_422_descends_quietly( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """A 422 (ungranted permission) is the ladder's expected descend signal, so it + must not be logged as a transient failure even on a non-terminal rung.""" + + class Client(_HTTPStatusErrorClient): + status = 422 + + _configure(monkeypatch, Client) + + with caplog.at_level(logging.DEBUG, logger="agent.utils.github_app"): + token, _ = await github_app.get_github_app_installation_token_with_expiry( + permissions={"actions": "read"}, log_errors=False + ) + + assert token is None + assert not any(r.levelno >= logging.WARNING for r in caplog.records) + + +@pytest.mark.asyncio +async def test_transient_mint_error_warns_even_when_errors_suppressed( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """A non-422 failure is not a missing grant; it must surface at WARNING even on + a non-terminal rung so a blip doesn't silently downscope a whole run.""" + + class Client(_HTTPStatusErrorClient): + status = 503 + + _configure(monkeypatch, Client) + + with caplog.at_level(logging.DEBUG, logger="agent.utils.github_app"): + token, _ = await github_app.get_github_app_installation_token_with_expiry( + permissions={"actions": "read"}, log_errors=False + ) + + assert token is None + assert any(r.levelno == logging.WARNING for r in caplog.records) + + @pytest.mark.asyncio async def test_installation_token_includes_permissions(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(github_app, "GITHUB_APP_ID", "1") diff --git a/tests/test_workflow_push_guard.py b/tests/test_workflow_push_guard.py index b6737810..3ce890a2 100644 --- a/tests/test_workflow_push_guard.py +++ b/tests/test_workflow_push_guard.py @@ -420,6 +420,47 @@ async def test_workflow_push_restoration_falls_back_when_actions_read_unavailabl assert "workflows" not in refreshed[2] +async def test_workflow_push_restores_recorded_baseline_scope( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Restore targets the run's recorded baseline scope (here: core, because the + install never granted actions:read) — not a hardcoded RUNTIME that would 422.""" + guard.SANDBOX_BACKENDS["thread-1"] = _Backend() + refreshed: list[dict[str, str]] = [] + + async def fake_approved(thread_id: str, fingerprint: str) -> bool: + return True + + async def fake_refresh(thread_id: str | None, *, permissions: dict[str, str]) -> bool: + refreshed.append(dict(permissions)) + return True + + async def fake_find_approval(*args: Any, **kwargs: Any) -> dict[str, Any] | None: + return None + + monkeypatch.setattr(guard, "workflow_push_approved", fake_approved) + monkeypatch.setattr(guard, "find_workflow_push_approval", fake_find_approval) + monkeypatch.setattr(guard, "refresh_proxy_token", fake_refresh) + monkeypatch.setattr( + guard, + "get_recorded_proxy_permissions", + lambda _thread_id: dict(guard.CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS), + ) + + async def handler(_request: Any) -> ToolMessage: + return ToolMessage(content="pushed", tool_call_id="call-1") + + await guard.WorkflowPushGuardMiddleware().awrap_tool_call(_Request(), handler) + + # Elevate to workflows:write, then restore straight to the recorded core scope: + # no spurious RUNTIME attempt (which would 422 for this install) and no false + # "failed to downscope" error. + assert refreshed[0]["workflows"] == "write" + assert refreshed[1] == guard.CORE_RUNTIME_PROXY_TOKEN_PERMISSIONS + assert "workflows" not in refreshed[1] + assert len(refreshed) == 2 + + async def test_non_workflow_push_runs_without_approval(monkeypatch: pytest.MonkeyPatch) -> None: # Head contains workflow files, but they are identical to the trusted remote base, # so no approval is required for a code-only change.