fix(mcp): consolidate OAuth handling, pick up external token refreshes (#11383)

* feat(mcp-oauth): scaffold MCPOAuthManager Central manager for per-server MCP OAuth state. Provides get_or_build_provider (cached), remove (evicts cache + deletes disk), invalidate_if_disk_changed (mtime watch, core fix for external-refresh workflow), and handle_401 (dedup'd recovery). No behavior change yet — existing call sites still use build_oauth_auth directly. Task 1 of 8 in the MCP OAuth consolidation (fixes Cthulhu's BetterStack reliability issues). * feat(mcp-oauth): add HermesMCPOAuthProvider with pre-flow disk watch Subclasses the MCP SDK's OAuthClientProvider to inject a disk mtime check before every async_auth_flow, via the central manager. When a subclass instance is used, external token refreshes (cron, another CLI instance) are picked up before the next API call. Still dead code: the manager's _build_provider still delegates to build_oauth_auth and returns the plain OAuthClientProvider. Task 4 wires this subclass in. Task 2 of 8. * refactor(mcp-oauth): extract build_oauth_auth helpers Decomposes build_oauth_auth into _configure_callback_port, _build_client_metadata, _maybe_preregister_client, and _parse_base_url. Public API preserved. These helpers let MCPOAuthManager._build_provider reuse the same logic in Task 4 instead of duplicating the construction dance. Also updates the SDK version hint in the warning from 1.10.0 to 1.26.0 (which is what we actually require for the OAuth types used here). Task 3 of 8. * feat(mcp-oauth): manager now builds HermesMCPOAuthProvider directly _build_provider constructs the disk-watching subclass using the helpers from Task 3, instead of delegating to the plain build_oauth_auth factory. Any consumer using the manager now gets pre-flow disk-freshness checks automatically. build_oauth_auth is preserved as the public API for backwards compatibility. The code path is now: MCPOAuthManager.get_or_build_provider -> _build_provider -> _configure_callback_port _build_client_metadata _maybe_preregister_client _parse_base_url HermesMCPOAuthProvider(...) Task 4 of 8. * feat(mcp): wire OAuth manager + add _reconnect_event MCPServerTask gains _reconnect_event alongside _shutdown_event. When set, _run_http / _run_stdio exit their async-with blocks cleanly (no exception), and the outer run() loop re-enters the transport to rebuild the MCP session with fresh credentials. This is the recovery path for OAuth failures that the SDK's in-place httpx.Auth cannot handle (e.g. cron externally consumed the refresh_token, or server-side session invalidation). _run_http now asks MCPOAuthManager for the OAuth provider instead of calling build_oauth_auth directly. Config-time, runtime, and reconnect paths all share one provider instance with pre-flow disk-watch active. shutdown() defensively sets both events so there is no race between reconnect and shutdown signalling. Task 5 of 8. * feat(mcp): detect auth failures in tool handlers, trigger reconnect All 5 MCP tool handlers (tool call, list_resources, read_resource, list_prompts, get_prompt) now detect auth failures and route through MCPOAuthManager.handle_401: 1. If the manager says recovery is viable (disk has fresh tokens, or SDK can refresh in-place), signal MCPServerTask._reconnect_event to tear down and rebuild the MCP session with fresh credentials, then retry the tool call once. 2. If no recovery path exists, return a structured needs_reauth JSON error so the model stops hallucinating manual refresh attempts (the 'let me curl the token endpoint' loop Cthulhu pasted from Discord). _is_auth_error catches OAuthFlowError, OAuthTokenError, OAuthNonInteractiveError, and httpx.HTTPStatusError(401). Non-auth exceptions still surface via the generic error path unchanged. Task 6 of 8. * feat(mcp-cli): route add/remove through manager, add 'hermes mcp login' cmd_mcp_add and cmd_mcp_remove now go through MCPOAuthManager instead of calling build_oauth_auth / remove_oauth_tokens directly. This means CLI config-time state and runtime MCP session state are backed by the same provider cache — removing a server evicts the live provider, adding a server populates the same cache the MCP session will read from. New 'hermes mcp login <name>' command: - Wipes both the on-disk tokens file and the in-memory MCPOAuthManager cache - Triggers a fresh OAuth browser flow via the existing probe path - Intended target for the needs_reauth error Task 6 returns to the model Task 7 of 8. * test(mcp-oauth): end-to-end integration tests Five new tests exercising the full consolidation with real file I/O and real imports (no transport mocks): 1. external_refresh_picked_up_without_restart — Cthulhu's cron workflow. External process writes fresh tokens to disk; on the next auth flow the manager's mtime-watch flips _initialized and the SDK re-reads from storage. 2. handle_401_deduplicates_concurrent_callers — 10 concurrent handlers for the same failed token fire exactly ONE recovery attempt (thundering-herd protection). 3. handle_401_returns_false_when_no_provider — defensive path for unknown servers. 4. invalidate_if_disk_changed_handles_missing_file — pre-auth state returns False cleanly. 5. provider_is_reused_across_reconnects — cache stickiness so reconnects preserve the disk-watch baseline mtime. Task 8 of 8 — consolidation complete.
2026-04-25 00:51:20 +00:00 · 2026-04-16 21:57:10 -07:00 · 2026-04-16 21:57:10 -07:00 · 70768665a4
commit 70768665a4
parent 436a7359cd
11 changed files with 1566 additions and 90 deletions
--- a/tests/tools/test_mcp_oauth_manager.py
+++ b/tests/tools/test_mcp_oauth_manager.py
@ -0,0 +1,141 @@
+"""Tests for the MCP OAuth manager (tools/mcp_oauth_manager.py).
+
+The manager consolidates the eight scattered MCP-OAuth call sites into a
+single object with disk-mtime watch, dedup'd 401 handling, and a provider
+cache. See `tools/mcp_oauth_manager.py` for design rationale.
+"""
+import json
+import os
+import time
+
+import pytest
+
+pytest.importorskip(
+    "mcp.client.auth.oauth2",
+    reason="MCP SDK 1.26.0+ required for OAuth support",
+)
+
+
+def test_manager_is_singleton():
+    """get_manager() returns the same instance across calls."""
+    from tools.mcp_oauth_manager import get_manager, reset_manager_for_tests
+    reset_manager_for_tests()
+    m1 = get_manager()
+    m2 = get_manager()
+    assert m1 is m2
+
+
+def test_manager_get_or_build_provider_caches(tmp_path, monkeypatch):
+    """Calling get_or_build_provider twice with same name returns same provider."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    p2 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert p1 is p2
+
+
+def test_manager_get_or_build_rebuilds_on_url_change(tmp_path, monkeypatch):
+    """Changing the URL discards the cached provider."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://a.example.com/mcp", None)
+    p2 = mgr.get_or_build_provider("srv", "https://b.example.com/mcp", None)
+    assert p1 is not p2
+
+
+def test_manager_remove_evicts_cache(tmp_path, monkeypatch):
+    """remove(name) evicts the provider from cache AND deletes disk files."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager
+
+    # Pre-seed tokens on disk
+    token_dir = tmp_path / "mcp-tokens"
+    token_dir.mkdir(parents=True)
+    (token_dir / "srv.json").write_text(json.dumps({
+        "access_token": "TOK",
+        "token_type": "Bearer",
+    }))
+
+    mgr = MCPOAuthManager()
+    p1 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert p1 is not None
+    assert (token_dir / "srv.json").exists()
+
+    mgr.remove("srv")
+
+    assert not (token_dir / "srv.json").exists()
+    p2 = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert p1 is not p2
+
+
+def test_hermes_provider_subclass_exists():
+    """HermesMCPOAuthProvider is defined and subclasses OAuthClientProvider."""
+    from tools.mcp_oauth_manager import _HERMES_PROVIDER_CLS
+    from mcp.client.auth.oauth2 import OAuthClientProvider
+
+    assert _HERMES_PROVIDER_CLS is not None
+    assert issubclass(_HERMES_PROVIDER_CLS, OAuthClientProvider)
+
+
+@pytest.mark.asyncio
+async def test_disk_watch_invalidates_on_mtime_change(tmp_path, monkeypatch):
+    """When the tokens file mtime changes, provider._initialized flips False.
+
+    This is the behaviour Claude Code ships as
+    invalidateOAuthCacheIfDiskChanged (CC-1096 / GH#24317) and is the core
+    fix for Cthulhu's external-cron refresh workflow.
+    """
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    from tools.mcp_oauth_manager import MCPOAuthManager, reset_manager_for_tests
+
+    reset_manager_for_tests()
+
+    token_dir = tmp_path / "mcp-tokens"
+    token_dir.mkdir(parents=True)
+    tokens_file = token_dir / "srv.json"
+    tokens_file.write_text(json.dumps({
+        "access_token": "OLD",
+        "token_type": "Bearer",
+    }))
+
+    mgr = MCPOAuthManager()
+    provider = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+    assert provider is not None
+
+    # First call: records mtime (zero -> real) -> returns True
+    changed1 = await mgr.invalidate_if_disk_changed("srv")
+    assert changed1 is True
+
+    # No file change -> False
+    changed2 = await mgr.invalidate_if_disk_changed("srv")
+    assert changed2 is False
+
+    # Touch file with a newer mtime
+    future_mtime = time.time() + 10
+    os.utime(tokens_file, (future_mtime, future_mtime))
+
+    changed3 = await mgr.invalidate_if_disk_changed("srv")
+    assert changed3 is True
+    # _initialized flipped — next async_auth_flow will re-read from disk
+    assert provider._initialized is False
+
+
+def test_manager_builds_hermes_provider_subclass(tmp_path, monkeypatch):
+    """get_or_build_provider returns HermesMCPOAuthProvider, not plain OAuthClientProvider."""
+    from tools.mcp_oauth_manager import (
+        MCPOAuthManager, _HERMES_PROVIDER_CLS, reset_manager_for_tests,
+    )
+    reset_manager_for_tests()
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    mgr = MCPOAuthManager()
+    provider = mgr.get_or_build_provider("srv", "https://example.com/mcp", None)
+
+    assert _HERMES_PROVIDER_CLS is not None
+    assert isinstance(provider, _HERMES_PROVIDER_CLS)
+    assert provider._hermes_server_name == "srv"
+