From 770dddc8c2394f079cd5252e28e30ebca5f18ed6 Mon Sep 17 00:00:00 2001 From: Eva Date: Fri, 10 Jul 2026 14:09:47 +0700 Subject: [PATCH] fix(oneshot): honor MCP discovery wait settings Join background MCP discovery before the one-shot tool snapshot only when the invocation's effective toolset can use an enabled MCP server. Resolve the shared mcp_discovery_timeout from config, report that bound in warnings, and cover explicit, no_mcp, timeout, and snapshot-order regressions. --- hermes_cli/oneshot.py | 126 ++++++++++++++--- tests/hermes_cli/test_oneshot_mcp_wait.py | 159 ++++++++++++++++++++++ 2 files changed, 269 insertions(+), 16 deletions(-) create mode 100644 tests/hermes_cli/test_oneshot_mcp_wait.py diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index 1f04173f0a6b7..917957759b0d3 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -50,6 +50,106 @@ def _normalize_toolsets(toolsets: object = None) -> list[str] | None: return [item for item in normalized if item] or None +def _read_mcp_server_names() -> tuple[set[str], set[str]]: + """Return (enabled, disabled) MCP server names from config.yaml. + + Shared by the ``--toolsets`` resolver and the one-shot MCP discovery wait so + both classify configured servers identically. Fail-safe: any config error + yields empty sets rather than raising. + """ + enabled: set[str] = set() + disabled: set[str] = set() + try: + from hermes_cli.config import read_raw_config + from hermes_cli.tools_config import _parse_enabled_flag + + cfg = read_raw_config() + mcp_servers = cfg.get("mcp_servers") if isinstance(cfg.get("mcp_servers"), dict) else {} + for name, server_cfg in mcp_servers.items(): + if not isinstance(server_cfg, dict): + continue + if _parse_enabled_flag(server_cfg.get("enabled", True), default=True): + enabled.add(str(name)) + else: + disabled.add(str(name)) + except Exception: + return set(), set() + return enabled, disabled + + +def _effective_oneshot_mcp_servers( + toolsets: object = None, + *, + use_config_toolsets: bool = True, +) -> set[str]: + """Return configured MCP servers that can contribute to this invocation.""" + enabled, _disabled = _read_mcp_server_names() + if not enabled: + return set() + + normalized = _normalize_toolsets(toolsets) + if not use_config_toolsets: + # ``None`` on this path is the validated representation of explicit + # ``--toolsets all`` / ``*``. Every enabled MCP server can contribute. + return enabled if normalized is None else enabled.intersection(normalized) + + try: + from hermes_cli.config import load_config + from hermes_cli.tools_config import _get_platform_tools + + effective_toolsets = _get_platform_tools(load_config(), "cli") + except Exception: + # Match the startup path's fail-safe posture: if effective toolset + # resolution breaks, do not reintroduce the original discovery race. + return enabled + return enabled.intersection(effective_toolsets) + + +def _wait_for_mcp_discovery_before_snapshot( + toolsets: object = None, + *, + use_config_toolsets: bool = True, +) -> None: + """Join background MCP discovery before the one-shot tool snapshot is built. + + ``hermes -z`` starts MCP discovery on a background thread (via + ``_prepare_agent_startup``) but, unlike the interactive CLI/TUI, never joined + it — so a configured server that had not finished connecting was silently + absent from the turn's tools. Wait here, bounded like the TUI's late-refresh + join, whenever an enabled MCP server is in this invocation's effective + toolset. Zero cost when MCP is excluded, and ~0s when discovery already + finished (``join`` returns the instant the thread is done). Servers still + pending after the configured bound are named on stderr; one-shot proceeds + with the tools that finished discovery within the bound. + + Must run BEFORE ``run_oneshot`` redirects stderr, so the warning reaches the + terminal (mirrors the ``--toolsets`` warnings above). + """ + selected_mcp_servers = _effective_oneshot_mcp_servers( + toolsets, + use_config_toolsets=use_config_toolsets, + ) + if not selected_mcp_servers: + return + try: + from hermes_cli.mcp_startup import ( + _resolve_discovery_timeout, + mcp_discovery_in_flight, + wait_for_mcp_discovery, + ) + except Exception: + return + + timeout = _resolve_discovery_timeout(None) + wait_for_mcp_discovery(timeout=timeout) + if mcp_discovery_in_flight(): + sys.stderr.write( + f"hermes -z: MCP discovery still pending after {timeout:g}s; " + "tools from these servers may be missing this turn: " + f"{', '.join(sorted(selected_mcp_servers))}\n" + ) + + def _validate_explicit_toolsets(toolsets: object = None) -> tuple[list[str] | None, str | None]: normalized = _normalize_toolsets(toolsets) if normalized is None: @@ -88,22 +188,7 @@ def _validate_explicit_toolsets(toolsets: object = None) -> tuple[list[str] | No mcp_names: set[str] = set() mcp_disabled: set[str] = set() if unresolved: - try: - from hermes_cli.config import read_raw_config - from hermes_cli.tools_config import _parse_enabled_flag - - cfg = read_raw_config() - mcp_servers = cfg.get("mcp_servers") if isinstance(cfg.get("mcp_servers"), dict) else {} - for name, server_cfg in mcp_servers.items(): - if not isinstance(server_cfg, dict): - continue - if _parse_enabled_flag(server_cfg.get("enabled", True), default=True): - mcp_names.add(str(name)) - else: - mcp_disabled.add(str(name)) - except Exception: - mcp_names = set() - mcp_disabled = set() + mcp_names, mcp_disabled = _read_mcp_server_names() mcp_valid = [name for name in unresolved if name in mcp_names] disabled = [name for name in unresolved if name in mcp_disabled] @@ -216,6 +301,15 @@ def run_oneshot( return 2 use_config_toolsets = _normalize_toolsets(toolsets) is None + # Join background MCP discovery before the agent's tool snapshot is built. + # Done here (before the stderr redirect below) so any pending-server warning + # reaches the terminal, and so a slow MCP server's tools are actually present + # for this single turn — one-shot has no interactive late-binding refresh. + _wait_for_mcp_discovery_before_snapshot( + explicit_toolsets, + use_config_toolsets=use_config_toolsets, + ) + # Auto-approve any shell / tool approvals. Non-interactive by # definition — a prompt would hang forever. os.environ["HERMES_YOLO_MODE"] = "1" diff --git a/tests/hermes_cli/test_oneshot_mcp_wait.py b/tests/hermes_cli/test_oneshot_mcp_wait.py new file mode 100644 index 0000000000000..b91b16724e117 --- /dev/null +++ b/tests/hermes_cli/test_oneshot_mcp_wait.py @@ -0,0 +1,159 @@ +"""Tests for hermes -z joining background MCP discovery before the tool snapshot. + +Regression: one-shot started MCP discovery on a background thread but never +joined it, so a still-connecting server's tools were silently absent from the +turn. ``_wait_for_mcp_discovery_before_snapshot`` now bounds-joins that thread. +""" + +from hermes_cli import mcp_startup +from hermes_cli import oneshot as oneshot_mod + + +def test_waits_with_bounded_timeout_when_mcp_configured(monkeypatch): + monkeypatch.setattr(oneshot_mod, "_read_mcp_server_names", lambda: ({"demo"}, set())) + calls: dict = {} + monkeypatch.setattr(mcp_startup, "_resolve_discovery_timeout", lambda _explicit: 1.5) + monkeypatch.setattr( + mcp_startup, + "wait_for_mcp_discovery", + lambda timeout=None: calls.__setitem__("timeout", timeout), + ) + monkeypatch.setattr(mcp_startup, "mcp_discovery_in_flight", lambda: False) + + oneshot_mod._wait_for_mcp_discovery_before_snapshot( + ["demo"], + use_config_toolsets=False, + ) + + assert calls["timeout"] == 1.5 + + +def test_no_wait_when_no_mcp_configured(monkeypatch): + monkeypatch.setattr(oneshot_mod, "_read_mcp_server_names", lambda: (set(), set())) + called = {"n": 0} + monkeypatch.setattr( + mcp_startup, + "wait_for_mcp_discovery", + lambda timeout=None: called.__setitem__("n", called["n"] + 1), + ) + + oneshot_mod._wait_for_mcp_discovery_before_snapshot() + + # Zero cost for the common no-MCP path — discovery is never joined. + assert called["n"] == 0 + + +def test_timeout_warns_and_proceeds(monkeypatch, capsys): + monkeypatch.setattr( + oneshot_mod, "_read_mcp_server_names", lambda: ({"demo", "other"}, set()) + ) + monkeypatch.setattr(mcp_startup, "_resolve_discovery_timeout", lambda _explicit: 2.75) + monkeypatch.setattr(mcp_startup, "wait_for_mcp_discovery", lambda timeout=None: None) + # Still in flight after the bound == discovery timed out. + monkeypatch.setattr(mcp_startup, "mcp_discovery_in_flight", lambda: True) + + # Must not raise — the run proceeds with whatever tools did connect. + oneshot_mod._wait_for_mcp_discovery_before_snapshot( + ["demo", "other"], + use_config_toolsets=False, + ) + + err = capsys.readouterr().err + assert err == ( + "hermes -z: MCP discovery still pending after 2.75s; tools from these " + "servers may be missing this turn: demo, other\n" + ) + + +def test_explicit_non_mcp_toolsets_skip_wait(monkeypatch, capsys): + monkeypatch.setattr(oneshot_mod, "_read_mcp_server_names", lambda: ({"demo"}, set())) + calls = {"wait": 0} + monkeypatch.setattr( + mcp_startup, + "wait_for_mcp_discovery", + lambda timeout=None: calls.__setitem__("wait", calls["wait"] + 1), + ) + monkeypatch.setattr(oneshot_mod, "_run_agent", lambda *_args, **_kwargs: ("done", {})) + + assert oneshot_mod.run_oneshot("hello", toolsets="web,terminal") == 0 + + assert calls["wait"] == 0 + assert capsys.readouterr().out == "done\n" + + +def test_no_mcp_sentinel_in_config_skips_wait(monkeypatch, capsys): + from hermes_cli import config as config_mod + + config = { + "mcp_servers": {"demo": {"command": "demo-server"}}, + "platform_toolsets": {"cli": ["web", "no_mcp"]}, + } + monkeypatch.setattr(oneshot_mod, "_read_mcp_server_names", lambda: ({"demo"}, set())) + monkeypatch.setattr(config_mod, "load_config", lambda: config) + calls = {"wait": 0} + monkeypatch.setattr( + mcp_startup, + "wait_for_mcp_discovery", + lambda timeout=None: calls.__setitem__("wait", calls["wait"] + 1), + ) + monkeypatch.setattr(oneshot_mod, "_run_agent", lambda *_args, **_kwargs: ("done", {})) + + assert oneshot_mod.run_oneshot("hello") == 0 + + assert calls["wait"] == 0 + assert capsys.readouterr().out == "done\n" + + +def test_config_resolved_timeout_reaches_wait_call(monkeypatch): + from hermes_cli import config as config_mod + + monkeypatch.setattr(oneshot_mod, "_read_mcp_server_names", lambda: ({"demo"}, set())) + monkeypatch.setattr(config_mod, "load_config", lambda: {"mcp_discovery_timeout": 7.25}) + calls: dict = {} + monkeypatch.setattr( + mcp_startup, + "wait_for_mcp_discovery", + lambda timeout=None: calls.__setitem__("timeout", timeout), + ) + monkeypatch.setattr(mcp_startup, "mcp_discovery_in_flight", lambda: False) + + oneshot_mod._wait_for_mcp_discovery_before_snapshot( + ["demo"], + use_config_toolsets=False, + ) + + assert calls["timeout"] == 7.25 + + +def test_temp_hermes_home_waits_before_oneshot_snapshot(monkeypatch, tmp_path, capsys): + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text( + "mcp_servers:\n" + " demo:\n" + " command: demo-server\n" + "platform_toolsets:\n" + " cli:\n" + " - web\n" + " - demo\n", + encoding="utf-8", + ) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + events: list[str] = [] + monkeypatch.setattr( + mcp_startup, + "wait_for_mcp_discovery", + lambda timeout=None: events.append("wait"), + ) + monkeypatch.setattr(mcp_startup, "mcp_discovery_in_flight", lambda: False) + + def _snapshot(*_args, **_kwargs): + events.append("snapshot") + return "done", {} + + monkeypatch.setattr(oneshot_mod, "_run_agent", _snapshot) + + assert oneshot_mod.run_oneshot("hello") == 0 + assert events == ["wait", "snapshot"] + assert capsys.readouterr().out == "done\n"