diff --git a/pyproject.toml b/pyproject.toml index 4aed2ff4c..ae9027198 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -112,6 +112,18 @@ dependencies = [ "wcwidth>=0.2", # ``typing.Self`` backport for Python 3.10 (added to stdlib in 3.11). "typing_extensions>=4.16.0; python_version < '3.11'", + # OS keychain backing for MCP OAuth token storage + # (services/mcp/auth.py McpTokenStore). Undeclared until now even though + # auth.py's backend allowlist is written against keyring 25.x class names + # and 27 MCP tests failed closed without it. Backends are resolved at + # runtime and PlaintextKeyring is explicitly rejected, so the package + # alone degrades to "no secure storage" rather than leaking tokens. + "keyring>=25", + # anyio raises this backport on Python 3.10 for TaskGroup exception + # groups; services/mcp/client.py resolves it to unwrap connection-error + # groups. Without it, `_unwrap_exception_group_message` returns the + # opaque "unhandled errors in a TaskGroup" text it exists to strip. + "exceptiongroup>=1.1; python_version < '3.11'", ] [project.optional-dependencies] diff --git a/src/services/mcp/client.py b/src/services/mcp/client.py index a3b256404..ea49b1d99 100644 --- a/src/services/mcp/client.py +++ b/src/services/mcp/client.py @@ -1,6 +1,7 @@ from __future__ import annotations import asyncio +import builtins import json import logging import os @@ -49,14 +50,21 @@ def _parse_server_capabilities(caps: Any) -> ServerCapabilities: which the ch15 list_changed refresh wiring gates on — is directly testable. ``tools``/``prompts``/``resources`` collapse to a bool (present-or-not); ``tools_list_changed`` is the nested - ``{tools: {listChanged: true}}`` sub-flag.""" + ``{tools: {listChanged: true}}`` sub-flag. + + Presence is tested with ``is not None``, not truthiness: the MCP spec makes + capability members *objects* whose presence signals support and whose + contents are optional, so ``{"tools": {}}`` is a spec-valid advertisement + of full tool support. ``bool({})`` is False and would report such a server + as having no tools, silently hiding all of them (observed against a live + server advertising 89 tools).""" if not isinstance(caps, dict): caps = {} tools_cap = caps.get("tools") return ServerCapabilities( - tools=bool(tools_cap), - prompts=bool(caps.get("prompts")), - resources=bool(caps.get("resources")), + tools=tools_cap is not None, + prompts=caps.get("prompts") is not None, + resources=caps.get("resources") is not None, tools_list_changed=bool( isinstance(tools_cap, dict) and tools_cap.get("listChanged") ), @@ -85,6 +93,23 @@ def _is_remote_config(config: Any) -> bool: ) +def _exception_group_cls() -> type[BaseException] | None: + """The ``BaseExceptionGroup`` class in effect on this interpreter. + + 3.11+ has it as a builtin; on 3.10 anyio raises the ``exceptiongroup`` + backport instead, so prefer the backport when the builtin is absent — + ``isinstance`` against the wrong class silently fails to unwrap. + """ + builtin = getattr(builtins, "BaseExceptionGroup", None) + if builtin is not None: + return builtin + try: + from exceptiongroup import BaseExceptionGroup as backport + except ImportError: + return None + return backport + + def _unwrap_exception_group_message(exc: BaseException) -> str: """Extract the most actionable error string from a (possibly nested) ``BaseExceptionGroup``. @@ -94,12 +119,15 @@ def _unwrap_exception_group_message(exc: BaseException) -> str: is the opaque ``"unhandled errors in a TaskGroup (1 sub-exception)"``. Walk the group tree and return the leaf exception's message — that's what the user actually needs to debug an unreachable server. + + ``BaseExceptionGroup`` is a builtin from 3.11; on 3.10 the backport + (``exceptiongroup``) is what anyio actually raises, so resolve it from + there. Guarding only with ``except NameError`` returned the builtin-less + fallback on 3.10 — i.e. exactly the opaque message this function exists + to strip — so ``requires-python = ">=3.10"`` needs the backport path. """ - try: - eg_cls = BaseExceptionGroup # 3.11+ builtin # type: ignore[name-defined] - except NameError: # pragma: no cover - Python < 3.11 - return str(exc) or type(exc).__name__ - if isinstance(exc, eg_cls) and exc.exceptions: + eg_cls = _exception_group_cls() + if eg_cls is not None and isinstance(exc, eg_cls) and exc.exceptions: return _unwrap_exception_group_message(exc.exceptions[0]) return str(exc) or type(exc).__name__ diff --git a/tests/test_mcp_client_full.py b/tests/test_mcp_client_full.py index d313b4658..ba9f32ca9 100644 --- a/tests/test_mcp_client_full.py +++ b/tests/test_mcp_client_full.py @@ -7,6 +7,7 @@ McpClient, MAX_RECONNECT_ATTEMPTS, _cache_key_for, + _exception_group_cls, _unwrap_exception_group_message, ) from src.services.mcp.types import ( @@ -185,18 +186,22 @@ def test_plain_exception_passthrough(self): assert _unwrap_exception_group_message(exc) == "port 1 closed" def test_unwraps_single_subexception(self): + eg_cls = _exception_group_cls() + assert eg_cls is not None, "no BaseExceptionGroup on this interpreter" try: inner_exc = ConnectionRefusedError("nobody home") - raise BaseExceptionGroup("unhandled errors", [inner_exc]) - except BaseExceptionGroup as eg: + raise eg_cls("unhandled errors", [inner_exc]) + except eg_cls as eg: assert _unwrap_exception_group_message(eg) == "nobody home" def test_recurses_through_nested_groups(self): + eg_cls = _exception_group_cls() + assert eg_cls is not None, "no BaseExceptionGroup on this interpreter" try: inner = TimeoutError("connect timed out") - mid = BaseExceptionGroup("inner group", [inner]) - raise BaseExceptionGroup("outer group", [mid]) - except BaseExceptionGroup as eg: + mid = eg_cls("inner group", [inner]) + raise eg_cls("outer group", [mid]) + except eg_cls as eg: assert _unwrap_exception_group_message(eg) == "connect timed out" def test_falls_back_to_class_name_when_str_is_empty(self): diff --git a/tests/test_r5_ch15_mcp_refresh_correctness.py b/tests/test_r5_ch15_mcp_refresh_correctness.py index 2d71e3972..8e5b2a9a4 100644 --- a/tests/test_r5_ch15_mcp_refresh_correctness.py +++ b/tests/test_r5_ch15_mcp_refresh_correctness.py @@ -111,9 +111,10 @@ def test_nested_listchanged_parsed_from_init_result(self): self.assertTrue(adv.tools) # tools present # listChanged absent → False (must NOT wire refresh). The m3 gate - # cares only about tools_list_changed. (Note: an empty {} tools cap - # collapses tools→False under the existing bool() parse — a - # pre-existing quirk, unchanged here and orthogonal to m3.) + # cares only about tools_list_changed. An empty {} tools cap is a + # spec-valid advertisement of tool support (presence signals support, + # contents optional) — tools must stay True while listChanged + # stays False. no_lc = _parse_server_capabilities({"tools": {"other": 1}}) self.assertFalse(no_lc.tools_list_changed) self.assertTrue(no_lc.tools) # non-empty dict → tools present @@ -126,6 +127,28 @@ def test_nested_listchanged_parsed_from_init_result(self): self.assertFalse(_parse_server_capabilities({}).tools_list_changed) self.assertFalse(_parse_server_capabilities(None).tools_list_changed) + def test_empty_capability_objects_count_as_present(self): + # Regression: capability members are objects whose *presence* signals + # support and whose contents are optional, so `{"tools": {}}` is a + # spec-valid advertisement. The parse used bool(), and bool({}) is + # False, so such a server reported no tools and list_tools() returned + # [] — silently hiding 89 tools on a live server. + from src.services.mcp.client import _parse_server_capabilities + + caps = _parse_server_capabilities( + {"tools": {}, "prompts": {}, "resources": {}} + ) + self.assertTrue(caps.tools) + self.assertTrue(caps.prompts) + self.assertTrue(caps.resources) + self.assertFalse(caps.tools_list_changed) # empty obj has no sub-flags + + # Absent members stay absent — presence, not truthiness. + empty = _parse_server_capabilities({}) + self.assertFalse(empty.tools) + self.assertFalse(empty.prompts) + self.assertFalse(empty.resources) + def test_capability_property_exposed(self): # The gate reads client.capabilities.tools_list_changed — confirm the # client exposes it (parsed at connect from the nested listChanged).