diff --git a/model_tools.py b/model_tools.py index 36cea8f304..788b9dec68 100644 --- a/model_tools.py +++ b/model_tools.py @@ -137,12 +137,19 @@ def _run_async(coro): discover_builtin_tools() -# MCP tool discovery (external MCP servers from config) -try: - from tools.mcp_tool import discover_mcp_tools - discover_mcp_tools() -except Exception as e: - logger.debug("MCP tool discovery failed: %s", e) +# MCP tool discovery (external MCP servers from config). +# Skipped when HERMES_MCP_DISCOVERY=0 -- the TUI slash_worker sets this because +# the TUI server already handles MCP; running it again would spawn duplicate +# ``hermes mcp serve`` children per session (#15275). +import os as _os_mod +if _os_mod.environ.get("HERMES_MCP_DISCOVERY") != "0": + try: + from tools.mcp_tool import discover_mcp_tools + discover_mcp_tools() + except Exception as e: + logger.debug("MCP tool discovery failed: %s", e) +else: + logger.debug("MCP tool discovery suppressed (HERMES_MCP_DISCOVERY=0)") # Plugin tool discovery (user/project/pip plugins) try: diff --git a/tests/tui_gateway/test_slash_worker_mcp.py b/tests/tui_gateway/test_slash_worker_mcp.py new file mode 100644 index 0000000000..f3cd8d4df0 --- /dev/null +++ b/tests/tui_gateway/test_slash_worker_mcp.py @@ -0,0 +1,84 @@ +"""Verify the TUI slash_worker suppresses MCP discovery at import time. + +Regression test for #15275 -- without the HERMES_MCP_DISCOVERY=0 guard in +slash_worker.py, importing the cli module triggers discover_mcp_tools() via +model_tools, spawning duplicate ``hermes mcp serve`` children per session. +""" + +import ast +import os + + +def test_slash_worker_sets_mcp_discovery_before_cli_import(): + """slash_worker.main() must set HERMES_MCP_DISCOVERY=0 before importing cli.""" + import importlib + src = importlib.util.find_spec("tui_gateway.slash_worker") + assert src is not None and src.origin is not None + with open(src.origin) as fh: + lines = fh.readlines() + + # Find the line that sets the env var and the first cli import. + env_line_idx = None + cli_import_idx = None + for i, line in enumerate(lines): + stripped = line.strip() + if "HERMES_MCP_DISCOVERY" in stripped and "environ" in stripped: + env_line_idx = i + # Only match actual import statements, not comments or strings + if cli_import_idx is None and stripped.startswith(("import cli", "from cli ")): + cli_import_idx = i + + assert env_line_idx is not None, ( + "slash_worker.py must set HERMES_MCP_DISCOVERY env var" + ) + assert cli_import_idx is not None, ( + "slash_worker.py must import cli" + ) + assert env_line_idx < cli_import_idx, ( + "HERMES_MCP_DISCOVERY must be set before cli is imported " + f"(env var at line {env_line_idx + 1}, cli import at line {cli_import_idx + 1})" + ) + + +def test_slash_worker_does_not_import_cli_at_module_level(): + """cli must NOT be imported at module scope -- it must be lazy in main().""" + import importlib + src = importlib.util.find_spec("tui_gateway.slash_worker") + assert src is not None and src.origin is not None + with open(src.origin) as fh: + source = fh.read() + + tree = ast.parse(source) + for node in ast.iter_child_nodes(tree): + if isinstance(node, ast.Import): + for alias in node.names: + assert not alias.name.startswith("cli"), ( + f"cli must not be imported at module level (line {node.lineno})" + ) + elif isinstance(node, ast.ImportFrom): + assert node.module is None or not node.module.startswith("cli"), ( + f"cli must not be imported at module level (line {node.lineno})" + ) + + +def test_model_tools_skips_mcp_when_env_var_set(monkeypatch): + """model_tools must skip discover_mcp_tools() when HERMES_MCP_DISCOVERY=0.""" + monkeypatch.setenv("HERMES_MCP_DISCOVERY", "0") + + assert os.environ.get("HERMES_MCP_DISCOVERY") == "0" + + # Simulate the guard check from model_tools.py + should_discover = os.environ.get("HERMES_MCP_DISCOVERY") != "0" + assert should_discover is False, ( + "MCP discovery should be suppressed when HERMES_MCP_DISCOVERY=0" + ) + + +def test_model_tools_runs_mcp_when_env_var_absent(monkeypatch): + """model_tools must run discover_mcp_tools() when env var is not set.""" + monkeypatch.delenv("HERMES_MCP_DISCOVERY", raising=False) + + should_discover = os.environ.get("HERMES_MCP_DISCOVERY") != "0" + assert should_discover is True, ( + "MCP discovery should run when HERMES_MCP_DISCOVERY is not set" + ) diff --git a/tui_gateway/slash_worker.py b/tui_gateway/slash_worker.py index 631b0c7045..b70bb6f7f0 100644 --- a/tui_gateway/slash_worker.py +++ b/tui_gateway/slash_worker.py @@ -1,6 +1,12 @@ -"""Persistent slash-command worker — one HermesCLI per TUI session. +"""Persistent slash-command worker -- one HermesCLI per TUI session. Protocol: reads JSON lines from stdin {id, command}, writes {id, ok, output|error} to stdout. + +The slash_worker only needs CLI command processing, not MCP tool bootstrapping. +The TUI server already handles MCP discovery and spawns hermes mcp serve +children; without the HERMES_MCP_DISCOVERY guard both code paths independently +spawn duplicate MCP serve processes per session (#15275). The cli module is +imported lazily inside main() so the env var is set before model_tools runs. """ import argparse @@ -10,12 +16,14 @@ import json import os import sys -import cli as cli_mod -from cli import HermesCLI from rich.console import Console +# cli module reference -- populated lazily in main() after setting the +# HERMES_MCP_DISCOVERY env var to prevent duplicate MCP serve children. +_cli_mod = None -def _run(cli: HermesCLI, command: str) -> str: + +def _run(cli, command: str) -> str: cmd = (command or "").strip() if not cmd: return "" @@ -29,21 +37,23 @@ def _run(cli: HermesCLI, command: str) -> str: # underlying file to our buffer so self.console.print() is captured. cli.console = Console(file=buf, force_terminal=True, width=120) - old = getattr(cli_mod, "_cprint", None) + old = getattr(_cli_mod, "_cprint", None) if old is not None: - cli_mod._cprint = lambda text: print(text) + _cli_mod._cprint = lambda text: print(text) try: with contextlib.redirect_stdout(buf), contextlib.redirect_stderr(buf): cli.process_command(cmd) finally: if old is not None: - cli_mod._cprint = old + _cli_mod._cprint = old return buf.getvalue().rstrip() def main(): + global _cli_mod + p = argparse.ArgumentParser(add_help=False) p.add_argument("--session-key", required=True) p.add_argument("--model", default="") @@ -52,6 +62,16 @@ def main(): os.environ["HERMES_SESSION_KEY"] = args.session_key os.environ["HERMES_INTERACTIVE"] = "1" + # Suppress MCP discovery -- the TUI server already handles MCP servers; + # importing cli triggers model_tools which calls discover_mcp_tools() at + # module scope. This env var tells model_tools to skip that step. + os.environ.setdefault("HERMES_MCP_DISCOVERY", "0") + + import cli as cli_mod + from cli import HermesCLI + + _cli_mod = cli_mod + with contextlib.redirect_stdout(io.StringIO()), contextlib.redirect_stderr(io.StringIO()): cli = HermesCLI(model=args.model or None, compact=True, resume=args.session_key, verbose=False)