From af019a37161c9fbe06cf27bfba939dccce4504db Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:13:17 +0530 Subject: [PATCH] fix(cli): apply the -t/--toolsets MCP spawn filter on every discovery path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- hermes_cli/main.py | 18 +++++- hermes_cli/mcp_startup.py | 39 ++++++++++- tests/hermes_cli/test_mcp_startup.py | 96 ++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 2 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 1f3c15d8a2..d3a73ae146 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -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", diff --git a/hermes_cli/mcp_startup.py b/hermes_cli/mcp_startup.py index 77a972591d..c57b00eb43 100644 --- a/hermes_cli/mcp_startup.py +++ b/hermes_cli/mcp_startup.py @@ -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: diff --git a/tests/hermes_cli/test_mcp_startup.py b/tests/hermes_cli/test_mcp_startup.py index 9c4b94a182..45d80bfdd6 100644 --- a/tests/hermes_cli/test_mcp_startup.py +++ b/tests/hermes_cli/test_mcp_startup.py @@ -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"]