mirror of
https://github.com/langchain-ai/deepagents.git
synced 2026-07-21 17:25:26 -04:00
fix(code): suppress expected MCP reauth logs (#4359)
Expected MCP OAuth re-auth failures in non-interactive mode runs now produce the Deep Agents login hint without also dumping noisy SDK warning tracebacks. Transient refresh failures still stay visible so provider outages are not mistaken for expired credentials. ## Changes - Added a scoped OAuth log filter that suppresses only expected refresh failures (`400`, `401`, `403`) and nested re-auth errors while the non-interactive auth flow is delegating to the MCP SDK. - Wired `build_oauth_provider(interactive=False)` to enable that suppression, while preserving the SDK diagnostics for interactive sessions. - Changed MCP tool discovery to log expected re-auth skips as concise warnings with the full exception moved to debug logs, while keeping unexpected discovery failures at warning level with tracebacks. - Reformatted untrusted project MCP server warnings as a multiline list so multiple skipped servers are readable.
This commit is contained in:
@@ -13,6 +13,7 @@ from __future__ import annotations
|
||||
|
||||
import concurrent.futures
|
||||
import contextlib
|
||||
import contextvars
|
||||
import hashlib
|
||||
import html
|
||||
import json
|
||||
@@ -43,6 +44,7 @@ from mcp.shared.auth import (
|
||||
OAuthToken,
|
||||
)
|
||||
from pydantic import BaseModel, ConfigDict, ValidationError
|
||||
from typing_extensions import override
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from pathlib import Path
|
||||
@@ -109,6 +111,47 @@ class McpServerSpec(TypedDict, total=False):
|
||||
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
_SUPPRESS_EXPECTED_REAUTH_LOGS: contextvars.ContextVar[bool] = contextvars.ContextVar(
|
||||
"suppress_expected_mcp_reauth_logs",
|
||||
default=False,
|
||||
)
|
||||
|
||||
|
||||
_TOKEN_REFRESH_FAILED_PREFIX = "Token refresh failed: " # noqa: S105 # log-message prefix, not a credential
|
||||
"""Prefix of the SDK's `Token refresh failed: <status>` warning (`oauth2.py`)."""
|
||||
|
||||
_EXPECTED_REAUTH_REFRESH_STATUSES = frozenset({"400", "401", "403"})
|
||||
"""Refresh-endpoint statuses that mean the grant was rejected (token stale).
|
||||
|
||||
The SDK logs `Token refresh failed: <status>` and clears tokens for *any*
|
||||
non-200 on the refresh endpoint. Only these statuses indicate the refresh
|
||||
token itself is expired/revoked — i.e. the expected re-auth cases our hint
|
||||
replaces. Transient failures (`429`, `5xx`, gateway timeouts) must stay
|
||||
visible so a provider outage isn't silently relabeled as "go re-login".
|
||||
"""
|
||||
|
||||
|
||||
class _ExpectedReauthLogFilter(logging.Filter):
|
||||
"""Drop SDK OAuth log records that are replaced by our reauth hint."""
|
||||
|
||||
@override
|
||||
def filter(self, record: logging.LogRecord) -> bool:
|
||||
"""Return whether the SDK OAuth log record should be emitted."""
|
||||
if not _SUPPRESS_EXPECTED_REAUTH_LOGS.get():
|
||||
return True
|
||||
message = record.getMessage()
|
||||
if message.startswith(_TOKEN_REFRESH_FAILED_PREFIX):
|
||||
status = message.removeprefix(_TOKEN_REFRESH_FAILED_PREFIX)
|
||||
if status in _EXPECTED_REAUTH_REFRESH_STATUSES:
|
||||
return False
|
||||
if message == "OAuth flow error" and record.exc_info is not None:
|
||||
exc = record.exc_info[1]
|
||||
if exc is not None and find_reauth_required(exc) is not None:
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
logging.getLogger("mcp.client.auth.oauth2").addFilter(_ExpectedReauthLogFilter())
|
||||
|
||||
_REF_RE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}")
|
||||
"""Matches `${VAR}` placeholders inside config strings for env-var substitution."""
|
||||
@@ -1095,6 +1138,9 @@ class _ExpiryAwareOAuthClientProvider(OAuthClientProvider):
|
||||
"""
|
||||
|
||||
def __init__(self, *args: Any, **kwargs: Any) -> None:
|
||||
self._suppress_expected_reauth_logs = bool(
|
||||
kwargs.pop("suppress_expected_reauth_logs", False)
|
||||
)
|
||||
super().__init__(*args, **kwargs)
|
||||
_strip_duplicate_client_id_under_basic_auth(self.context)
|
||||
|
||||
@@ -1245,6 +1291,9 @@ class _ExpiryAwareOAuthClientProvider(OAuthClientProvider):
|
||||
# `asend`/`aclose` (never `athrow`), so forwarding sent values and
|
||||
# closing the inner generator on `GeneratorExit` is sufficient — no
|
||||
# `athrow` forwarding needed.
|
||||
token: contextvars.Token[bool] | None = None
|
||||
if self._suppress_expected_reauth_logs:
|
||||
token = _SUPPRESS_EXPECTED_REAUTH_LOGS.set(True)
|
||||
inner = super().async_auth_flow(request)
|
||||
try:
|
||||
# Prime with `anext()` (no response to send yet); thereafter every
|
||||
@@ -1257,6 +1306,8 @@ class _ExpiryAwareOAuthClientProvider(OAuthClientProvider):
|
||||
return
|
||||
finally:
|
||||
await inner.aclose()
|
||||
if token is not None:
|
||||
_SUPPRESS_EXPECTED_REAUTH_LOGS.reset(token)
|
||||
|
||||
|
||||
def build_oauth_provider(
|
||||
@@ -1347,6 +1398,7 @@ def build_oauth_provider(
|
||||
storage=storage,
|
||||
redirect_handler=redirect,
|
||||
callback_handler=callback,
|
||||
suppress_expected_reauth_logs=not interactive,
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -1558,21 +1558,31 @@ async def _load_tools_from_config(
|
||||
# Tokens existed (we checked above) but the OAuth provider
|
||||
# fell back to interactive reauth — the refresh attempt
|
||||
# failed. Flag unauthenticated so the user is prompted to
|
||||
# re-login, and keep the original exception in the log so
|
||||
# debugging a real provider outage is possible.
|
||||
# re-login, and keep the original exception only in debug logs
|
||||
# so expected re-auth skips don't flood non-interactive output.
|
||||
status = "unauthenticated"
|
||||
error = (
|
||||
f"{reauth} "
|
||||
"(token refresh failed; the original error is in debug logs)"
|
||||
)
|
||||
logger.warning(
|
||||
"MCP server '%s' skipped: %s",
|
||||
server_name,
|
||||
error,
|
||||
)
|
||||
logger.debug(
|
||||
"MCP server '%s' skipped: tool discovery failed",
|
||||
server_name,
|
||||
exc_info=True,
|
||||
)
|
||||
else:
|
||||
status = "error"
|
||||
error = str(exc)
|
||||
logger.warning(
|
||||
"MCP server '%s' skipped: tool discovery failed",
|
||||
server_name,
|
||||
exc_info=True,
|
||||
)
|
||||
logger.warning(
|
||||
"MCP server '%s' skipped: tool discovery failed",
|
||||
server_name,
|
||||
exc_info=True,
|
||||
)
|
||||
server_infos.append(
|
||||
MCPServerInfo(
|
||||
name=server_name,
|
||||
@@ -1791,18 +1801,19 @@ async def resolve_and_load_mcp_tools(
|
||||
# `${VAR}` references in their `headers` would exfiltrate the value
|
||||
# to the attacker URL during the discovery handshake.
|
||||
skipped = [
|
||||
f"{name} [{kind}]: {summary}" for name, kind, summary in project_servers
|
||||
f"- {name} [{kind}]: {summary}" for name, kind, summary in project_servers
|
||||
]
|
||||
skipped_list = "\n".join(skipped)
|
||||
if trust_project_mcp is False:
|
||||
logger.warning(
|
||||
"Skipped untrusted project MCP servers: %s",
|
||||
"; ".join(skipped),
|
||||
"Skipped untrusted project MCP servers:\n%s",
|
||||
skipped_list,
|
||||
)
|
||||
else:
|
||||
logger.warning(
|
||||
"Skipped untrusted project MCP servers "
|
||||
"(config changed or not yet approved): %s",
|
||||
"; ".join(skipped),
|
||||
"(config changed or not yet approved):\n%s",
|
||||
skipped_list,
|
||||
)
|
||||
|
||||
if explicit_config_path:
|
||||
|
||||
@@ -8,7 +8,7 @@ import logging
|
||||
import re
|
||||
import time
|
||||
from pathlib import Path
|
||||
from typing import Any, Literal
|
||||
from typing import Any, Literal, cast
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
@@ -512,6 +512,66 @@ class TestExpiryAwareOAuthClientProvider:
|
||||
assert "/.well-known/oauth-protected-resource" in str(discovery_request.url)
|
||||
await flow.aclose()
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("interactive", "expected"),
|
||||
[(False, True), (True, False)],
|
||||
)
|
||||
async def test_delegated_flow_toggles_reauth_log_suppression(
|
||||
self,
|
||||
fake_home: Path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
*,
|
||||
interactive: bool,
|
||||
expected: bool,
|
||||
) -> None:
|
||||
"""The contextvar is set during delegation only for non-interactive runs.
|
||||
|
||||
Guards the wiring between `build_oauth_provider(interactive=...)` and the
|
||||
filter: the SDK flow logs synchronously inside the delegated generator,
|
||||
so the suppression flag must be visible there. A fake SDK flow records
|
||||
what the contextvar reads at that point.
|
||||
"""
|
||||
del fake_home
|
||||
import httpx
|
||||
from mcp.client.auth import OAuthClientProvider
|
||||
|
||||
from deepagents_code.mcp_auth import (
|
||||
_SUPPRESS_EXPECTED_REAUTH_LOGS,
|
||||
build_oauth_provider,
|
||||
)
|
||||
|
||||
observed: dict[str, bool] = {}
|
||||
|
||||
async def fake_flow(
|
||||
self: OAuthClientProvider,
|
||||
request: httpx.Request,
|
||||
):
|
||||
del self
|
||||
observed["suppressed"] = _SUPPRESS_EXPECTED_REAUTH_LOGS.get()
|
||||
_ = yield request
|
||||
|
||||
monkeypatch.setattr(OAuthClientProvider, "async_auth_flow", fake_flow)
|
||||
|
||||
storage = FileTokenStorage("notion")
|
||||
await storage.set_client_info(_make_client_info())
|
||||
await storage.set_tokens(_make_tokens())
|
||||
|
||||
provider = build_oauth_provider(
|
||||
server_name="notion",
|
||||
server_url="https://mcp.notion.com/mcp",
|
||||
storage=storage,
|
||||
interactive=interactive,
|
||||
)
|
||||
flow = provider.async_auth_flow(
|
||||
httpx.Request("POST", "https://mcp.notion.com/mcp")
|
||||
)
|
||||
await anext(flow)
|
||||
await flow.aclose()
|
||||
|
||||
assert observed["suppressed"] is expected
|
||||
# The flag never leaks past the flow.
|
||||
assert _SUPPRESS_EXPECTED_REAUTH_LOGS.get() is False
|
||||
|
||||
async def test_delegated_flow_forwards_responses_on_every_iteration(
|
||||
self,
|
||||
fake_home: Path,
|
||||
@@ -664,6 +724,95 @@ class TestBasicAuthClientIdStripping:
|
||||
assert "client_secret" not in data
|
||||
|
||||
|
||||
class TestExpectedReauthLogFilter:
|
||||
"""Tests for suppressing noisy SDK OAuth logs during non-interactive reauth."""
|
||||
|
||||
def test_suppresses_expected_sdk_oauth_logs(
|
||||
self,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Expected non-interactive reauth logs are replaced by our login hint."""
|
||||
from deepagents_code.mcp_auth import _SUPPRESS_EXPECTED_REAUTH_LOGS
|
||||
|
||||
sdk_logger = logging.getLogger("mcp.client.auth.oauth2")
|
||||
caplog.set_level(logging.WARNING, logger="mcp.client.auth.oauth2")
|
||||
server = "notion"
|
||||
reauth = MCPReauthRequiredError(server)
|
||||
msg = "boom"
|
||||
unexpected = RuntimeError(msg)
|
||||
token = _SUPPRESS_EXPECTED_REAUTH_LOGS.set(True)
|
||||
try:
|
||||
sdk_logger.warning("Token refresh failed: 400")
|
||||
sdk_logger.error(
|
||||
"OAuth flow error",
|
||||
exc_info=(type(reauth), reauth, reauth.__traceback__),
|
||||
)
|
||||
sdk_logger.error(
|
||||
"OAuth flow error",
|
||||
exc_info=(type(unexpected), unexpected, unexpected.__traceback__),
|
||||
)
|
||||
finally:
|
||||
_SUPPRESS_EXPECTED_REAUTH_LOGS.reset(token)
|
||||
|
||||
messages = [record.getMessage() for record in caplog.records]
|
||||
assert messages == ["OAuth flow error"]
|
||||
exc_info = caplog.records[0].exc_info
|
||||
assert exc_info is not None
|
||||
assert isinstance(exc_info[1], RuntimeError)
|
||||
|
||||
def test_transient_refresh_failure_is_not_suppressed(
|
||||
self,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Transient refresh statuses (5xx/429) stay visible, not relabeled reauth.
|
||||
|
||||
The SDK logs `Token refresh failed: <status>` for any non-200. A `503`
|
||||
means the provider is down and the refresh token is still valid, so the
|
||||
operator must see it rather than be steered toward a pointless re-login.
|
||||
"""
|
||||
from deepagents_code.mcp_auth import _SUPPRESS_EXPECTED_REAUTH_LOGS
|
||||
|
||||
sdk_logger = logging.getLogger("mcp.client.auth.oauth2")
|
||||
caplog.set_level(logging.WARNING, logger="mcp.client.auth.oauth2")
|
||||
token = _SUPPRESS_EXPECTED_REAUTH_LOGS.set(True)
|
||||
try:
|
||||
sdk_logger.warning("Token refresh failed: 503")
|
||||
sdk_logger.warning("Token refresh failed: 429")
|
||||
sdk_logger.warning("Token refresh failed: 400")
|
||||
finally:
|
||||
_SUPPRESS_EXPECTED_REAUTH_LOGS.reset(token)
|
||||
|
||||
messages = [record.getMessage() for record in caplog.records]
|
||||
assert messages == [
|
||||
"Token refresh failed: 503",
|
||||
"Token refresh failed: 429",
|
||||
]
|
||||
|
||||
def test_passes_through_when_not_suppressing(
|
||||
self,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""With the contextvar unset, the process-wide filter is inert.
|
||||
|
||||
The filter is installed on the SDK logger for every consumer of that
|
||||
logger, so its default-off behavior guards against globally swallowing
|
||||
real OAuth errors outside a non-interactive reauth window.
|
||||
"""
|
||||
sdk_logger = logging.getLogger("mcp.client.auth.oauth2")
|
||||
caplog.set_level(logging.WARNING, logger="mcp.client.auth.oauth2")
|
||||
reauth = MCPReauthRequiredError("notion")
|
||||
|
||||
# No `_SUPPRESS_EXPECTED_REAUTH_LOGS.set(...)`: contextvar at default.
|
||||
sdk_logger.warning("Token refresh failed: 400")
|
||||
sdk_logger.error(
|
||||
"OAuth flow error",
|
||||
exc_info=(type(reauth), reauth, reauth.__traceback__),
|
||||
)
|
||||
|
||||
messages = [record.getMessage() for record in caplog.records]
|
||||
assert messages == ["Token refresh failed: 400", "OAuth flow error"]
|
||||
|
||||
|
||||
class TestFindReauthRequired:
|
||||
"""Tests for unwrapping nested re-auth errors."""
|
||||
|
||||
@@ -895,6 +1044,34 @@ class TestBuildOAuthProvider:
|
||||
assert metadata.redirect_uris is not None
|
||||
assert [str(uri) for uri in metadata.redirect_uris] == [_SLACK_REDIRECT_URI]
|
||||
|
||||
def test_interactive_mode_maps_to_reauth_log_suppression(
|
||||
self,
|
||||
fake_home: Path,
|
||||
) -> None:
|
||||
"""Only non-interactive providers suppress expected reauth SDK logs.
|
||||
|
||||
Interactive sessions keep the SDK's OAuth diagnostics; non-interactive
|
||||
runs replace the expected reauth noise with our login hint.
|
||||
"""
|
||||
del fake_home
|
||||
from deepagents_code.mcp_auth import build_oauth_provider
|
||||
|
||||
non_interactive = build_oauth_provider(
|
||||
server_name="notion",
|
||||
server_url="https://mcp.notion.com/mcp",
|
||||
storage=FileTokenStorage("notion"),
|
||||
interactive=False,
|
||||
)
|
||||
interactive = build_oauth_provider(
|
||||
server_name="notion",
|
||||
server_url="https://mcp.notion.com/mcp",
|
||||
storage=FileTokenStorage("notion"),
|
||||
interactive=True,
|
||||
)
|
||||
|
||||
assert cast("Any", non_interactive)._suppress_expected_reauth_logs is True
|
||||
assert cast("Any", interactive)._suppress_expected_reauth_logs is False
|
||||
|
||||
async def test_refresh_uses_cached_oauth_metadata_endpoint(
|
||||
self,
|
||||
fake_home: Path,
|
||||
|
||||
@@ -4,6 +4,7 @@ from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
from contextlib import asynccontextmanager
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING, Any, cast
|
||||
@@ -1323,8 +1324,11 @@ class TestLoadToolsFromConfigOAuth:
|
||||
assert isinstance(recorded[0].get("auth"), OAuthClientProvider)
|
||||
await manager.cleanup()
|
||||
|
||||
async def test_discovery_reauth_marks_server_unauthenticated(self) -> None:
|
||||
"""OAuth re-auth during discovery is surfaced as unauthenticated."""
|
||||
async def test_discovery_reauth_marks_server_unauthenticated(
|
||||
self,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""OAuth re-auth during discovery is surfaced without warning tracebacks."""
|
||||
from mcp.shared.auth import OAuthToken
|
||||
|
||||
storage = FileTokenStorage(
|
||||
@@ -1344,6 +1348,8 @@ class TestLoadToolsFromConfigOAuth:
|
||||
raise ExceptionGroup(msg, [MCPReauthRequiredError("notion")])
|
||||
yield
|
||||
|
||||
caplog.set_level(logging.WARNING, logger="deepagents_code.mcp_tools")
|
||||
|
||||
with patch("langchain_mcp_adapters.sessions.create_session", _fake):
|
||||
config = {
|
||||
"mcpServers": {
|
||||
@@ -1360,6 +1366,15 @@ class TestLoadToolsFromConfigOAuth:
|
||||
assert isinstance(manager, MCPSessionManager)
|
||||
assert server_infos[0].status == "unauthenticated"
|
||||
assert "re-authentication" in (server_infos[0].error or "")
|
||||
warning_records = [
|
||||
record
|
||||
for record in caplog.records
|
||||
if record.name == "deepagents_code.mcp_tools"
|
||||
and record.levelno == logging.WARNING
|
||||
]
|
||||
assert warning_records
|
||||
assert all(record.exc_info is None for record in warning_records)
|
||||
assert "Exception Group Traceback" not in caplog.text
|
||||
await manager.cleanup()
|
||||
|
||||
|
||||
@@ -1452,6 +1467,7 @@ class TestResolveAndLoadMcpTools:
|
||||
mock_classify: MagicMock,
|
||||
mock_load: AsyncMock,
|
||||
tmp_path: Path,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Project remote MCP entries do not reach the loader without trust.
|
||||
|
||||
@@ -1467,7 +1483,11 @@ class TestResolveAndLoadMcpTools:
|
||||
"transport": "http",
|
||||
"url": "http://169.254.169.254",
|
||||
"headers": {"X-Token": "${OPENAI_API_KEY}"},
|
||||
}
|
||||
},
|
||||
"docs-langchain": {
|
||||
"transport": "http",
|
||||
"url": "https://docs.langchain.com/mcp",
|
||||
},
|
||||
}
|
||||
}
|
||||
)
|
||||
@@ -1475,6 +1495,7 @@ class TestResolveAndLoadMcpTools:
|
||||
mock_discover.return_value = [project_cfg]
|
||||
mock_classify.return_value = ([], [project_cfg])
|
||||
mock_load.return_value = ([], None, [])
|
||||
caplog.set_level(logging.WARNING, logger="deepagents_code.mcp_tools")
|
||||
|
||||
tools, _manager, _infos = await resolve_and_load_mcp_tools(
|
||||
trust_project_mcp=False,
|
||||
@@ -1482,6 +1503,10 @@ class TestResolveAndLoadMcpTools:
|
||||
|
||||
assert tools == []
|
||||
assert mock_load.call_count == 0
|
||||
assert "Skipped untrusted project MCP servers:\n" in caplog.text
|
||||
assert "- evil [http]: http://169.254.169.254" in caplog.text
|
||||
assert "- docs-langchain [http]: https://docs.langchain.com/mcp" in caplog.text
|
||||
assert "; docs-langchain" not in caplog.text
|
||||
|
||||
@patch("deepagents_code.mcp_tools._load_tools_from_config")
|
||||
@patch("deepagents_code.mcp_tools.classify_discovered_configs")
|
||||
|
||||
Reference in New Issue
Block a user