fix(cli): apply the -t/--toolsets MCP spawn filter on every discovery path
The cherry-picked commit added the allowed_mcp_names filter to discover_mcp_tools(). Since then CLI startup grew a second discovery path — start_background_mcp_discovery / the deferred desktop start in hermes_cli/mcp_startup.py — so wiring the filter only into the inline call would leave `hermes chat -t terminal` (the default backgrounded path) still spawning every server. Store the filter once in mcp_startup (set_mcp_server_filter, called from _prepare_agent_startup from args.toolsets; `all`/`*`/empty clears it) and have both the inline and the background discovery honor it. The unfiltered call shape is unchanged so zero-arg test stubs keep working. Dropped from the original PR: the atexit/SIGINT/SIGTERM oneshot MCP reap — main already does this in _cleanup_oneshot_runtime() -> shutdown_mcp_servers(). E2E (3 configured stdio servers, real subprocesses, 5 runs median): no filter 3 spawned / 2.0 s; `-t terminal` 0 spawned / 1 ms.
This commit is contained in:
+17
-1
@@ -12852,6 +12852,17 @@ def _prepare_agent_startup(args) -> None:
|
||||
"plugin discovery failed at CLI startup",
|
||||
exc_info=True,
|
||||
)
|
||||
# -t/--toolsets narrows which configured MCP servers get spawned, on
|
||||
# every discovery path (inline below, background thread, TUI/desktop
|
||||
# deferred start). Built-in toolset names never match a server key, so
|
||||
# `-t terminal` simply spawns nothing; `-t all` keeps the full set.
|
||||
try:
|
||||
from hermes_cli.mcp_startup import set_mcp_server_filter
|
||||
|
||||
set_mcp_server_filter(getattr(args, "toolsets", None))
|
||||
except Exception:
|
||||
logger.debug("MCP server filter setup failed", exc_info=True)
|
||||
|
||||
_run_inline_mcp_discovery = True
|
||||
if _is_tui_chat_launch(args):
|
||||
# The TUI launcher hands off to a dedicated startup path that already
|
||||
@@ -12880,9 +12891,14 @@ def _prepare_agent_startup(args) -> None:
|
||||
try:
|
||||
# MCP tool discovery remains synchronous for entrypoints that do
|
||||
# not own a later bounded/executor startup path.
|
||||
from hermes_cli.mcp_startup import get_mcp_server_filter
|
||||
from tools.mcp_tool import discover_mcp_tools
|
||||
|
||||
discover_mcp_tools()
|
||||
_mcp_filter = get_mcp_server_filter()
|
||||
if _mcp_filter is None:
|
||||
discover_mcp_tools()
|
||||
else:
|
||||
discover_mcp_tools(allowed_mcp_names=_mcp_filter)
|
||||
except Exception:
|
||||
logger.debug(
|
||||
"MCP tool discovery failed at CLI startup",
|
||||
|
||||
@@ -10,6 +10,37 @@ _mcp_discovery_lock = threading.Lock()
|
||||
_mcp_discovery_started = False
|
||||
_mcp_discovery_thread: Optional[threading.Thread] = None
|
||||
_mcp_discovery_deferred: Optional[threading.Timer] = None
|
||||
# Process-wide MCP server-name allowlist derived from ``-t/--toolsets``.
|
||||
# ``None`` = no filter (spawn every configured server). Set once at CLI
|
||||
# startup by ``set_mcp_server_filter`` and honored by every discovery path
|
||||
# in this module (inline, background, deferred), so a ``-t terminal``
|
||||
# oneshot never cold-starts MCP subprocesses it cannot use.
|
||||
_mcp_server_filter: Optional[list[str]] = None
|
||||
|
||||
|
||||
def set_mcp_server_filter(toolsets: object) -> Optional[list[str]]:
|
||||
"""Derive the MCP spawn allowlist from a ``-t/--toolsets`` value.
|
||||
|
||||
Built-in toolset names in the list are harmless (they never match a
|
||||
configured ``mcp_servers`` key). ``all``/``*`` or an empty/absent value
|
||||
clears the filter. Returns the stored list for logging/tests.
|
||||
"""
|
||||
global _mcp_server_filter
|
||||
names: list[str] = []
|
||||
if isinstance(toolsets, str):
|
||||
names = [t.strip() for t in toolsets.split(",") if t.strip()]
|
||||
elif isinstance(toolsets, (list, tuple, set)):
|
||||
for item in toolsets:
|
||||
names.extend(t.strip() for t in str(item).split(",") if t.strip())
|
||||
if not names or "all" in names or "*" in names:
|
||||
_mcp_server_filter = None
|
||||
else:
|
||||
_mcp_server_filter = names
|
||||
return _mcp_server_filter
|
||||
|
||||
|
||||
def get_mcp_server_filter() -> Optional[list[str]]:
|
||||
return _mcp_server_filter
|
||||
|
||||
|
||||
def _has_configured_mcp_servers() -> bool:
|
||||
@@ -170,7 +201,13 @@ def _discover_mcp_tools_without_interactive_oauth() -> None:
|
||||
with suppress_interactive_oauth():
|
||||
from tools.mcp_tool import discover_mcp_tools
|
||||
|
||||
discover_mcp_tools()
|
||||
# Only pass the kwarg when a filter is set: many tests (and any
|
||||
# out-of-tree caller) stub discover_mcp_tools with a zero-arg
|
||||
# callable, and the unfiltered call shape is unchanged.
|
||||
if _mcp_server_filter is None:
|
||||
discover_mcp_tools()
|
||||
else:
|
||||
discover_mcp_tools(allowed_mcp_names=_mcp_server_filter)
|
||||
|
||||
|
||||
def defer_background_mcp_discovery(*, logger, thread_name: str, delay: float) -> None:
|
||||
|
||||
@@ -199,3 +199,99 @@ def _install_retry_stubs(monkeypatch, *, connected: bool, calls: dict):
|
||||
)
|
||||
|
||||
|
||||
|
||||
|
||||
# --- -t/--toolsets MCP spawn filter (#19000) --------------------------------
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def _reset_mcp_server_filter():
|
||||
saved = mcp_startup._mcp_server_filter
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
mcp_startup._mcp_server_filter = saved
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("toolsets", "expected"),
|
||||
[
|
||||
(None, None),
|
||||
("", None),
|
||||
("all", None),
|
||||
(["*"], None),
|
||||
("terminal,web", ["terminal", "web"]),
|
||||
(["terminal", "code-mcp,web"], ["terminal", "code-mcp", "web"]),
|
||||
],
|
||||
)
|
||||
def test_set_mcp_server_filter_normalizes(_reset_mcp_server_filter, toolsets, expected):
|
||||
assert mcp_startup.set_mcp_server_filter(toolsets) == expected
|
||||
assert mcp_startup.get_mcp_server_filter() == expected
|
||||
|
||||
|
||||
def test_discover_mcp_tools_spawns_only_allowed_servers(monkeypatch):
|
||||
"""The filter must narrow the spawn set before any server is connected;
|
||||
built-in toolset names in the list are ignored."""
|
||||
from tools import mcp_tool
|
||||
|
||||
servers = {
|
||||
"code-mcp": {"command": "true"},
|
||||
"docs-mcp": {"command": "true"},
|
||||
}
|
||||
seen: dict[str, dict] = {}
|
||||
|
||||
monkeypatch.setattr(mcp_tool, "_load_mcp_config", lambda: dict(servers))
|
||||
monkeypatch.setattr(mcp_tool, "_ensure_mcp_sdk", lambda: True)
|
||||
monkeypatch.setattr(mcp_tool, "_try_acquire_mcp_discovery_lock", lambda: mcp_tool._LOCK_UNAVAILABLE)
|
||||
monkeypatch.setattr(mcp_tool, "_release_mcp_discovery_lock", lambda *_a, **_k: None, raising=False)
|
||||
|
||||
def _fake_register(cfgs):
|
||||
seen.update(cfgs)
|
||||
return []
|
||||
|
||||
monkeypatch.setattr(mcp_tool, "register_mcp_servers", _fake_register)
|
||||
monkeypatch.setattr(mcp_tool, "_servers", {})
|
||||
monkeypatch.setattr(mcp_tool, "_server_connecting", set())
|
||||
|
||||
# Everything (no filter) — both would be registered.
|
||||
mcp_tool.discover_mcp_tools()
|
||||
assert set(seen) == {"code-mcp", "docs-mcp"}
|
||||
|
||||
# `-t terminal,code-mcp` — only the matching server; "terminal" is a no-op.
|
||||
seen.clear()
|
||||
mcp_tool.discover_mcp_tools(allowed_mcp_names=["terminal", "code-mcp"])
|
||||
assert set(seen) == {"code-mcp"}
|
||||
|
||||
# `-t terminal` — no MCP server in the filter: skip the whole MCP load.
|
||||
seen.clear()
|
||||
assert mcp_tool.discover_mcp_tools(allowed_mcp_names=["terminal"]) == []
|
||||
assert seen == {}
|
||||
|
||||
|
||||
def test_background_discovery_honors_server_filter(monkeypatch, _reset_mcp_server_filter):
|
||||
calls: list = []
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"tools.mcp_tool",
|
||||
types.SimpleNamespace(discover_mcp_tools=lambda allowed_mcp_names=None: calls.append(allowed_mcp_names)),
|
||||
)
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"tools.mcp_oauth",
|
||||
types.SimpleNamespace(suppress_interactive_oauth=nullcontext),
|
||||
)
|
||||
mcp_startup.set_mcp_server_filter("terminal,code-mcp")
|
||||
mcp_startup._discover_mcp_tools_without_interactive_oauth()
|
||||
assert calls == [["terminal", "code-mcp"]]
|
||||
|
||||
|
||||
def test_prepare_agent_startup_installs_server_filter(monkeypatch, _reset_mcp_server_filter):
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"hermes_cli.plugins",
|
||||
types.SimpleNamespace(discover_plugins=lambda: None),
|
||||
)
|
||||
monkeypatch.setattr(main_mod, "_should_background_mcp_startup", lambda args: False)
|
||||
monkeypatch.setattr(main_mod, "_command_has_dedicated_mcp_startup", lambda args: True)
|
||||
main_mod._prepare_agent_startup(_agent_args(toolsets="terminal,code-mcp"))
|
||||
assert mcp_startup.get_mcp_server_filter() == ["terminal", "code-mcp"]
|
||||
|
||||
Reference in New Issue
Block a user