From 0d8a1575c577bde3103c45e7fa1cbf33c48b8d98 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:06:12 -0700 Subject: [PATCH] refactor(mcp): keep the passive status RPC, drop the SDK contract and reason codes mcp.servers.status now rides the shared _mcp_rpc decorator (profile scope, 4064, 5024 with the real message) instead of a hand-rolled try/finally with a blanket except. Drop the _MCPConnectErrorText str subclass and reason taxonomy: the existing status/error fields already carry the state, and a whitelist on the RPC keeps error text out of the wire. The Desktop connections.health contribution contract is held back until its consumer plugin is public. Tests trimmed to the scope invariants (per-profile runtime visibility, scoped shutdown clears only its own status, launch runtime never leaks into another profile). --- tests/tools/test_mcp_tool.py | 75 +--------------------- tests/tui_gateway/test_mcp_profile_rpcs.py | 57 ---------------- tools/mcp_tool_discovery.py | 57 +++++----------- tools/mcp_tool_errors.py | 23 ------- tui_gateway/methods_tools.py | 41 ++++-------- 5 files changed, 30 insertions(+), 223 deletions(-) diff --git a/tests/tools/test_mcp_tool.py b/tests/tools/test_mcp_tool.py index 449befc63f..6e0dece041 100644 --- a/tests/tools/test_mcp_tool.py +++ b/tests/tools/test_mcp_tool.py @@ -4,11 +4,9 @@ All tests use mocks -- no real MCP servers or subprocesses are started. """ import asyncio -import copy import json import logging import os -import pickle import sys import threading import time @@ -274,19 +272,6 @@ class TestMCPStatus: import tools.mcp_tool as mcp_tool from tools import mcp_tool_config as _mcp_config from tools import mcp_tool_discovery as _mcp_discovery - from tools import mcp_tool_errors as _mcp_errors - from tools.mcp_oauth import OAuthNonInteractiveError - - assert _mcp_errors._connect_failure_reason(RuntimeError("network")) == "check_failed" - assert _mcp_errors._connect_failure_reason( - OAuthNonInteractiveError("browser required") - ) == "auth_required" - tagged = _mcp_errors._connect_error_text( - "Connection closed", OAuthNonInteractiveError("browser required") - ) - for cloned in (copy.copy(tagged), copy.deepcopy(tagged), pickle.loads(pickle.dumps(tagged))): - assert cloned == "Connection closed" - assert getattr(cloned, "reason") == "auth_required" monkeypatch.setattr( _mcp_config, "_load_mcp_config", @@ -299,17 +284,13 @@ class TestMCPStatus: ) with mcp_tool._lock: saved_servers = dict(mcp_tool._servers) - saved_scopes = dict(mcp_tool._server_scope_keys) saved_connecting = set(mcp_tool._server_connecting) saved_errors = dict(mcp_tool._server_connect_errors) mcp_tool._servers.clear() - mcp_tool._server_scope_keys.clear() mcp_tool._server_connecting.clear() mcp_tool._server_connect_errors.clear() mcp_tool._server_connecting.add("connecting") - mcp_tool._server_connect_errors["failed"] = _mcp_errors._connect_error_text( - "Connection closed", OAuthNonInteractiveError("browser required") - ) + mcp_tool._server_connect_errors["failed"] = "Connection closed" try: statuses = { @@ -320,8 +301,6 @@ class TestMCPStatus: with mcp_tool._lock: mcp_tool._servers.clear() mcp_tool._servers.update(saved_servers) - mcp_tool._server_scope_keys.clear() - mcp_tool._server_scope_keys.update(saved_scopes) mcp_tool._server_connecting.clear() mcp_tool._server_connecting.update(saved_connecting) mcp_tool._server_connect_errors.clear() @@ -331,10 +310,8 @@ class TestMCPStatus: assert statuses["configured"]["connected"] is False assert statuses["configured"]["disabled"] is False assert statuses["connecting"]["status"] == "connecting" - assert statuses["connecting"]["reason"] == "stale" assert statuses["failed"]["status"] == "failed" assert statuses["failed"]["error"] == "Connection closed" - assert statuses["failed"]["reason"] == "auth_required" assert statuses["disabled"]["status"] == "disabled" assert statuses["disabled"]["disabled"] is True @@ -372,10 +349,8 @@ class TestMCPStatus: mcp_tool._server_scope_keys.update(saved_scopes) assert status["status"] == "configured" - assert status["reason"] == "stale" assert status["tools"] == 0 assert unscoped_status["status"] == "configured" - assert unscoped_status["reason"] == "stale" def test_scoped_shutdown_clears_only_its_connection_status(self, monkeypatch): @@ -448,6 +423,7 @@ class TestMCPStatus: mcp_tool._server_connect_failures.update(saved_failures) + class TestLifecycleConfig: def test_get_lifecycle_seconds_accepts_top_level_and_nested_values(self): from tools.mcp_tool_common import _get_lifecycle_seconds @@ -2820,53 +2796,6 @@ class TestSanitizeMcpNameComponent: class TestRegisterMcpServers: - def test_records_profile_scope_before_connect(self): - import tools.mcp_tool as mcp_tool - from tools import mcp_tool_lifecycle, mcp_tool_loop, mcp_tool_discovery - - seen = [] - - async def fake_register(name, _cfg): - with mcp_tool._lock: - seen.append(( - name in mcp_tool._server_connecting, - mcp_tool._server_scope_keys.get(name), - )) - raise RuntimeError("expected connect failure") - - with mcp_tool._lock: - saved_servers = dict(mcp_tool._servers) - saved_scopes = dict(mcp_tool._server_scope_keys) - saved_connecting = set(mcp_tool._server_connecting) - saved_errors = dict(mcp_tool._server_connect_errors) - mcp_tool._servers.clear() - mcp_tool._server_scope_keys.clear() - mcp_tool._server_connecting.clear() - mcp_tool._server_connect_errors.clear() - - try: - with patch("tools.mcp_tool._MCP_AVAILABLE", True), \ - patch("tools.mcp_tool_config._filter_suspicious_mcp_servers", side_effect=lambda value: value), \ - patch("tools.mcp_tool_discovery._connect_cooldown_active", return_value=False), \ - patch("tools.mcp_tool._mcp_registry_scope", return_value="profile:work"), \ - patch("tools.mcp_tool_discovery._discover_and_register_server", side_effect=fake_register): - mcp_tool_discovery.register_mcp_servers({"scoped": {"command": "test"}}) - - assert seen == [(True, "profile:work")] - with mcp_tool._lock: - assert mcp_tool._server_connect_errors["scoped"].reason == "check_failed" - assert mcp_tool._server_scope_keys["scoped"] == "profile:work" - finally: - with mcp_tool._lock: - mcp_tool._servers.clear() - mcp_tool._servers.update(saved_servers) - mcp_tool._server_scope_keys.clear() - mcp_tool._server_scope_keys.update(saved_scopes) - mcp_tool._server_connecting.clear() - mcp_tool._server_connecting.update(saved_connecting) - mcp_tool._server_connect_errors.clear() - mcp_tool._server_connect_errors.update(saved_errors) - """Verify the new register_mcp_servers() public API.""" def test_mcp_not_available_returns_empty(self): diff --git a/tests/tui_gateway/test_mcp_profile_rpcs.py b/tests/tui_gateway/test_mcp_profile_rpcs.py index 493734d9cc..17708893fc 100644 --- a/tests/tui_gateway/test_mcp_profile_rpcs.py +++ b/tests/tui_gateway/test_mcp_profile_rpcs.py @@ -136,68 +136,11 @@ def test_status_is_profile_scoped_and_credential_safe(hermes_root): "connected": False, "disabled": False, "status": "configured", - "reason": "stale", } ] assert "error" not in str(payload) -def test_status_redacts_internal_failures(hermes_root, monkeypatch): - from tools import mcp_tool_discovery - - def fail(*args, **kwargs): - raise RuntimeError("token=must-not-leak") - - monkeypatch.setattr(mcp_tool_discovery, "get_mcp_status", fail) - response = _call("mcp.servers.status", {"profile": "work"}) - - assert response["error"]["message"] == "MCP status unavailable" - assert "must-not-leak" not in str(response) - - -def test_status_omits_raw_server_errors(hermes_root, monkeypatch): - from tools import mcp_tool_discovery - - monkeypatch.setattr( - mcp_tool_discovery, - "get_mcp_status", - lambda configured, include_runtime: [{ - "name": "svc", - "transport": "stdio", - "tools": 0, - "connected": False, - "disabled": False, - "status": "failed", - "reason": "auth_required", - "error": "token=must-not-leak", - }], - ) - - payload = _result(_call("mcp.servers.status")) - assert payload["servers"][0]["reason"] == "auth_required" - assert "error" not in payload["servers"][0] - assert "must-not-leak" not in str(payload) - - -def test_status_does_not_run_the_runtime_config_loader(hermes_root, monkeypatch): - from tools import mcp_tool_config - - _result( - _call( - "mcp.servers.add", - {"profile": "work", "name": "svc-a", "config": {"command": "svc-a-bin"}}, - ) - ) - - def forbidden(): - raise AssertionError("runtime config loader executed") - - monkeypatch.setattr(mcp_tool_config, "_load_mcp_config", forbidden) - - payload = _result(_call("mcp.servers.status", {"profile": "work"})) - assert [server["name"] for server in payload["servers"]] == ["svc-a"] - - def test_status_does_not_mix_launch_runtime_into_another_profile(hermes_root): import tools.mcp_tool as mcp_tool diff --git a/tools/mcp_tool_discovery.py b/tools/mcp_tool_discovery.py index cb33323db5..71843b01f0 100644 --- a/tools/mcp_tool_discovery.py +++ b/tools/mcp_tool_discovery.py @@ -112,7 +112,7 @@ def _note_connect_failure(name: str, exc: BaseException) -> str: message = _errors._format_connect_error(exc) with _core._lock: _core._server_connecting.discard(name) - _core._server_connect_errors[name] = _errors._connect_error_text(message, exc) + _core._server_connect_errors[name] = message _record_connect_failure(name) return message @@ -329,9 +329,7 @@ def _run_discovery_pass(new_servers: Dict[str, dict]) -> None: "connecting set: %s", how, len(stale), ", ".join(stale)) _core._server_connecting.difference_update(stale) for _sn in stale: - message = f"Connection attempt {how} during discovery" - _core._server_connect_errors.setdefault( - _sn, _errors._MCPConnectErrorText(message, "check_failed")) + _core._server_connect_errors.setdefault(_sn, f"Connection attempt {how} during discovery") raise finally: if _was_interrupted: @@ -457,36 +455,24 @@ def is_mcp_tool_parallel_safe(tool_name: str) -> bool: return bool(server_name and server_name in _core._parallel_safe_servers) -def get_mcp_status( - configured: Optional[Dict[str, dict]] = None, - *, - include_runtime: bool = True, -) -> List[dict]: - """Per-server cached status without initiating connections.""" +def get_mcp_status(configured: Optional[Dict[str, dict]] = None, *, include_runtime: bool = True) -> List[dict]: + """Per-server status dicts for banner/TUI: name, transport, tools, connected, disabled, + status (connected / disabled / connecting / failed / configured) and error for failed. + Reads cached runtime state only; never connects.""" configured = _config._load_mcp_config() if configured is None else dict(configured) if not configured: return [] current_scope = _core._mcp_registry_scope() with _core._lock: - scope_keys = dict(_core._server_scope_keys) if include_runtime else {} + def visible(name: str) -> bool: + # Runtime state belongs to the profile that adopted it; under a multiplexer only that + # profile's view may show it, and ``include_runtime=False`` hides the launch profile's + # servers from a status read scoped to a different profile. + return include_runtime and _core._server_scope_keys.get(name, None) == current_scope - def visible_in_current_scope(name: str) -> bool: - if name in scope_keys: - return scope_keys[name] == current_scope - return current_scope is None - - active_servers = { - name: server for name, server in _core._servers.items() - if include_runtime and visible_in_current_scope(name) - } - connecting = { - name for name in _core._server_connecting - if include_runtime and visible_in_current_scope(name) - } - connect_errors = { - name: error for name, error in _core._server_connect_errors.items() - if include_runtime and visible_in_current_scope(name) - } + active_servers = {n: s for n, s in _core._servers.items() if visible(n)} + connecting = {n for n in _core._server_connecting if visible(n)} + connect_errors = {n: e for n, e in _core._server_connect_errors.items() if visible(n)} result: List[dict] = [] for name, cfg in configured.items(): @@ -495,13 +481,8 @@ def get_mcp_status( live = server is not None and server.session is not None status = ("connected" if live else "disabled" if not enabled else "connecting" if name in connecting else "failed" if name in connect_errors else "configured") - reason = { - "connected": "healthy", "disabled": "not_configured", - "connecting": "stale", "configured": "stale", - }.get(status, "check_failed") entry = {"name": name, "transport": cfg.get("transport", "http") if "url" in cfg else "stdio", - "tools": 0, "connected": False, "disabled": status == "disabled", - "status": status, "reason": reason} + "tools": 0, "connected": False, "disabled": status == "disabled", "status": status} if live: entry["connected"] = True entry["tools"] = (len(server._registered_tool_names) if hasattr(server, "_registered_tool_names") @@ -509,13 +490,7 @@ def get_mcp_status( if server._sampling: entry["sampling"] = dict(server._sampling.metrics) elif status == "failed": - stored_error = connect_errors[name] - failure = getattr(server, "_error", None) if server is not None else None - failure_reason = getattr(stored_error, "reason", None) - if failure_reason is None and isinstance(failure, BaseException): - failure_reason = _errors._connect_failure_reason(failure) - entry["reason"] = failure_reason or "check_failed" - entry["error"] = str(stored_error) + entry["error"] = connect_errors[name] result.append(entry) return result diff --git a/tools/mcp_tool_errors.py b/tools/mcp_tool_errors.py index 283bcd8410..fc0b5eca1b 100644 --- a/tools/mcp_tool_errors.py +++ b/tools/mcp_tool_errors.py @@ -283,29 +283,6 @@ def _is_auth_error(exc: BaseException) -> bool: return getattr(exc.response, "status_code", None) == 401 if isinstance(exc, http_types) else True -def _connect_failure_reason(exc: BaseException) -> str: - """Reduce an MCP connect exception to a credential-safe health reason.""" - return "auth_required" if _is_auth_error(_unwrap_exception_group(exc)) else "check_failed" - - -class _MCPConnectErrorText(str): - """Backward-compatible error text carrying a credential-safe reason.""" - - reason: str - - def __new__(cls, message: str, reason: str): - value = super().__new__(cls, message) - value.reason = reason - return value - - def __reduce__(self): - return type(self), (str(self), self.reason) - - -def _connect_error_text(message: str, exc: BaseException) -> str: - return _MCPConnectErrorText(message, _connect_failure_reason(exc)) - - # Lower-cased substrings meaning the transport session expired / was GC'd (OAuth token still valid). # Substrings (lower-cased match) that indicate the MCP server rejected the request because its server-side # transport session expired / was garbage-collected. See #13383. diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index f59b0ab2dc..a7ea1e5505 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -1171,37 +1171,20 @@ def _(rid, params: dict) -> dict: return _ok(rid, {"servers": [_mcp_summarize_server(name, cfg) for name, cfg in sorted(servers.items())]}) -@method("mcp.servers.status") +@_mcp_rpc("status", required=()) def _(rid, params: dict) -> dict: - """Return credential-safe cached MCP runtime state without connecting.""" + """``{servers: [{name, transport, tools, connected, disabled, status}], checked_at}`` from cached + runtime state; never connects, probes, or starts auth. Under a multiplexer the runtime view is the + scoped profile's; otherwise it is shown only when ``profile`` is the launch profile.""" + import time hc = _tools_mod("hermes_constants") - runtime_home_key = hc.hermes_home_key() - token = None - try: - if profile := _str_arg(params, "profile"): - profile_dir = _tools_mod("hermes_cli.profiles").get_profile_dir(profile) - if not profile_dir or not profile_dir.is_dir(): - return _err(rid, 4064, f"profile '{profile}' not found") - token = hc.set_hermes_home_override(str(profile_dir)) - - import time - from agent.secret_scope import is_multiplex_active - - configured = _tools_mod("hermes_cli.mcp_config")._get_mcp_servers() - get_status = _tools_mod("tools.mcp_tool_discovery").get_mcp_status - safe_fields = ("name", "transport", "tools", "connected", "disabled", "status", "reason") - servers = get_status( - configured, - include_runtime=is_multiplex_active() or hc.hermes_home_key() == runtime_home_key, - ) - return _ok(rid, { - "servers": [{key: entry[key] for key in safe_fields if key in entry} for entry in servers], - "checked_at": int(time.time() * 1000), - }) - except Exception: - return _err(rid, 5024, "MCP status unavailable") - finally: - _mcp_reset_profile(token) + configured = _tools_mod("hermes_cli.mcp_config")._get_mcp_servers() + include_runtime = (_tools_mod("agent.secret_scope").is_multiplex_active() + or hc.hermes_home_key() == hc.hermes_home_key(hc.get_process_hermes_home())) + safe = ("name", "transport", "tools", "connected", "disabled", "status") + servers = _tools_mod("tools.mcp_tool_discovery").get_mcp_status(configured, include_runtime=include_runtime) + return _ok(rid, {"servers": [{k: e[k] for k in safe if k in e} for e in servers], + "checked_at": int(time.time() * 1000)}) @_mcp_rpc("add")