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