fix(mcp): treat empty capability objects as present; declare keyring

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]

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__
 

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):

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).