ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

refactor(mcp): keep the passive status RPC, drop the SDK contract and reason codes

Browse Source 
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).
This commit is contained in:
Teknium
2026-09-06 12:06:12 -07:00
parent a6699d60f4
commit 0d8a1575c5
5 changed files with 30 additions and 223 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/tools/test_mcp_tool.py
+2 -73
View File
@@ -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):
tests/tui_gateway/test_mcp_profile_rpcs.py
-57
View File
@@ -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
tools/mcp_tool_discovery.py
+16 -41
View File
@@ -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
tools/mcp_tool_errors.py
-23
View File
@@ -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.
tui_gateway/methods_tools.py
+12 -29
View File
@@ -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")