diff --git a/cli.py b/cli.py index 489068c783107..6a81546911a4c 100644 --- a/cli.py +++ b/cli.py @@ -18126,6 +18126,10 @@ def main( # Handle single query mode if query or image: + # One-shot mode: no between-turns MCP late-binding refresh, so the + # agent must wait the full MCP cold-start bound before its first + # (and only) tool snapshot. See #51316. + cli._single_query_mode = True if not cli._claim_active_session("cli", stderr=bool(quiet)): sys.exit(1) try: diff --git a/hermes_cli/cli_agent_setup_mixin.py b/hermes_cli/cli_agent_setup_mixin.py index 0cd437645e8ea..29173eda409ae 100644 --- a/hermes_cli/cli_agent_setup_mixin.py +++ b/hermes_cli/cli_agent_setup_mixin.py @@ -246,9 +246,12 @@ class CLIAgentSetupMixin: if not self._ensure_runtime_credentials(): return False - from hermes_cli.mcp_startup import wait_for_mcp_discovery + from hermes_cli.mcp_startup import ensure_mcp_discovery_before_agent_build - wait_for_mcp_discovery() + ensure_mcp_discovery_before_agent_build( + logger=logger, + single_query=getattr(self, "_single_query_mode", False), + ) # Initialize SQLite session store for CLI sessions (if not already done in __init__) if self._session_db is None: diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index 872038e4dc283..3ecf0b85d1e80 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -463,6 +463,16 @@ DEFAULT_CONFIG = { # small so a slow/dead server adds little to first-response latency. "mcp_discovery_timeout": 1.5, + # Single-query (``hermes -q/-z "..."``) variant of mcp_discovery_timeout. + # In one-shot mode there is only ONE turn, so the between-turns late-binding + # refresh never runs: a server that misses the small interactive bound is + # invisible to the LLM for the whole session. This larger bound gives slow + # cold-start servers (npx, uvx, remote HTTP) a chance to land in the one + # tool snapshot. ``thread.join(timeout)`` returns the instant discovery + # completes, so reachable servers only wait for their real handshake time + # while unavailable servers remain bounded. + "mcp_single_query_discovery_timeout": 15.0, + # MCP runtime behavior (distinct from the per-server definitions in # mcp_servers: and from the auxiliary.mcp side-LLM task settings). "mcp": { diff --git a/hermes_cli/mcp_startup.py b/hermes_cli/mcp_startup.py index 82839d933b67c..a350f648d6ccc 100644 --- a/hermes_cli/mcp_startup.py +++ b/hermes_cli/mcp_startup.py @@ -114,25 +114,44 @@ def start_background_mcp_discovery(*, logger, thread_name: str) -> None: thread.start() -def _resolve_discovery_timeout(explicit: "float | None") -> float: +def _resolve_discovery_timeout( + explicit: "float | None", *, single_query: bool = False +) -> float: """Resolve the MCP discovery wait bound: explicit arg > config > default. Reads ``mcp_discovery_timeout`` from config.yaml, defaulting to the value in ``DEFAULT_CONFIG`` (single source of truth) when the key is absent. Kept lazy and fail-safe — a missing/invalid value or a broken config falls back to a short safe bound so startup can never hang or crash. + + When ``single_query`` is True (``hermes -z "..."`` / ``-q``), the larger + ``mcp_single_query_discovery_timeout`` bound is used instead. In single-query + mode there is only ONE turn, so the between-turns late-binding refresh never + runs — a server that misses the small interactive bound would be invisible to + the LLM for the whole session. The wait still returns the instant discovery + completes (see ``wait_for_mcp_discovery``), so fast servers pay ~0s; the + larger bound only caps how long a genuinely slow cold-start may block. """ if explicit is not None: return explicit + key = ( + "mcp_single_query_discovery_timeout" + if single_query + else "mcp_discovery_timeout" + ) + fallback = 15.0 if single_query else 1.5 try: from hermes_cli.config import load_config, DEFAULT_CONFIG - default = float(DEFAULT_CONFIG.get("mcp_discovery_timeout", 1.5)) - raw = (load_config() or {}).get("mcp_discovery_timeout", default) - val = float(raw) - return val if val > 0 else default + default = float(DEFAULT_CONFIG.get(key, fallback)) + try: + raw = (load_config() or {}).get(key, default) + val = float(raw) + return val if val > 0 else default + except Exception: + return default except Exception: - return 1.5 + return fallback def _discover_mcp_tools_without_interactive_oauth() -> None: @@ -148,7 +167,9 @@ def _discover_mcp_tools_without_interactive_oauth() -> None: discover_mcp_tools() -def wait_for_mcp_discovery(timeout: "float | None" = None) -> None: +def wait_for_mcp_discovery( + timeout: "float | None" = None, *, single_query: bool = False +) -> None: """Wait for background MCP discovery before the first tool snapshot. ``thread.join(timeout)`` returns the INSTANT discovery completes, so this @@ -157,11 +178,15 @@ def wait_for_mcp_discovery(timeout: "float | None" = None) -> None: ``mcp_discovery_timeout`` in config) just caps the wait so a dead server can't freeze startup; servers that miss it are picked up by the automatic late-binding refresh. + + When ``single_query`` is True, the bound comes from + ``mcp_single_query_discovery_timeout`` instead (default 15s vs 1.5s + interactive) because one-shot sessions have no second turn to recover. """ thread = _mcp_discovery_thread if thread is None or not thread.is_alive(): return - thread.join(timeout=_resolve_discovery_timeout(timeout)) + thread.join(timeout=_resolve_discovery_timeout(timeout, single_query=single_query)) def mcp_discovery_in_flight() -> bool: @@ -192,3 +217,44 @@ def join_mcp_discovery(timeout: "float | None" = None) -> bool: return True thread.join(timeout=timeout) return not thread.is_alive() + + +def ensure_mcp_discovery_before_agent_build( + *, + logger, + timeout: "float | None" = None, + single_query: bool = False, + thread_name: str = "cli-mcp-discovery", +) -> None: + """Give configured MCP tools a bounded chance to register before AIAgent. + + Non-interactive first turns (``chat -q``, ``hermes -z``) can construct + ``AIAgent`` before the normal banner or tool-list paths touch + ``get_tool_definitions()``. Because the agent snapshots its tool + registry at construction time, the first and only model turn can miss + native ``mcp__...`` tools even when the MCP server is healthy. + + ``wait_for_mcp_discovery()`` only joins an already-created discovery + thread, so it no-ops if a direct/single-query path reaches agent + construction before MCP startup created that thread. This helper makes + the construction site self-sufficient: start discovery if needed, then + wait up to the configured bound. + + When ``single_query`` is True, the larger + ``mcp_single_query_discovery_timeout`` bound is used (default 15s vs 1.5s + interactive) because one-shot sessions have no second turn to recover. + + Failures are swallowed so a broken MCP config never aborts agent + construction — the agent runs without MCP tools, same as before. + """ + try: + start_background_mcp_discovery( + logger=logger, + thread_name=thread_name, + ) + wait_for_mcp_discovery(timeout=timeout, single_query=single_query) + except Exception: + logger.debug( + "MCP discovery readiness check failed before agent build", + exc_info=True, + ) diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index 1f04173f0a6b7..f13fe64029d5c 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -395,6 +395,20 @@ def _run_agent( if toolsets_list is None and use_config_toolsets: toolsets_list = sorted(_get_platform_tools(cfg, "cli")) + # Ensure MCP tools are discovered before building the agent. Oneshot + # bypasses cli.py's _prepare_agent_startup MCP background path and + # HermesCLI._init_agent's wait — it builds AIAgent directly here, so the + # tool snapshot at construction time misses any MCP server that hasn't + # registered yet. This helper starts discovery if needed (idempotent) and + # bounded-waits with the larger single-query bound (default 15s) because + # there is only ONE turn and no between-turns late-binding refresh (#38448). + from hermes_cli.mcp_startup import ensure_mcp_discovery_before_agent_build + + ensure_mcp_discovery_before_agent_build( + logger=logging.getLogger(__name__), + single_query=True, + ) + session_db = _create_session_db_for_oneshot() # The try spans agent construction (not just ``chat``) so the SQLite store # opened above is always closed — including when ``AIAgent(...)`` itself diff --git a/tests/hermes_cli/test_mcp_discovery_timing.py b/tests/hermes_cli/test_mcp_discovery_timing.py new file mode 100644 index 0000000000000..4660744f2c5b6 --- /dev/null +++ b/tests/hermes_cli/test_mcp_discovery_timing.py @@ -0,0 +1,310 @@ +"""Regression tests for MCP discovery timing in non-interactive sessions. + +Covers the race where AIAgent snapshots its tool registry at construction +time before background MCP discovery finishes. In single-query (``-q``) and +oneshot (``-z``) mode there is only ONE turn — no between-turns late-binding +refresh — so missing tools at construction are missing for the entire +session. + +Tests verify: + 1. The ``single_query`` flag resolves to the larger bound. + 2. ``ensure_mcp_discovery_before_agent_build`` starts discovery if needed. + 3. Oneshot calls the helper before AIAgent construction (ordering). + 4. The wait stays bounded when discovery is slow (dead server). + 5. Interactive mode keeps the small bound (not affected). +""" + +from __future__ import annotations + +import sys +import threading +import time +import types + +import pytest + +from hermes_cli import mcp_startup + + +@pytest.fixture(autouse=True) +def _reset_mcp_startup_state(): + saved_started = mcp_startup._mcp_discovery_started + saved_thread = mcp_startup._mcp_discovery_thread + try: + mcp_startup._mcp_discovery_started = False + mcp_startup._mcp_discovery_thread = None + yield + finally: + thread = mcp_startup._mcp_discovery_thread + if thread is not None and thread.is_alive(): + thread.join(timeout=1.0) + mcp_startup._mcp_discovery_started = saved_started + mcp_startup._mcp_discovery_thread = saved_thread + + +# ── _resolve_discovery_timeout: single_query bound ────────────────────────── + + +def test_resolve_discovery_timeout_single_query_uses_larger_bound(monkeypatch): + """Single-query mode reads the larger mcp_single_query_discovery_timeout.""" + import hermes_cli.config as cfg + + monkeypatch.setattr( + cfg, + "load_config", + lambda: { + "mcp_discovery_timeout": 1.5, + "mcp_single_query_discovery_timeout": 25.0, + }, + ) + assert mcp_startup._resolve_discovery_timeout(None) == 1.5 + assert mcp_startup._resolve_discovery_timeout(None, single_query=True) == 25.0 + + +def test_resolve_discovery_timeout_single_query_falls_back(monkeypatch): + """Bad/absent single-query value falls back to DEFAULT_CONFIG, never hangs.""" + import hermes_cli.config as cfg + + default = float(cfg.DEFAULT_CONFIG.get("mcp_single_query_discovery_timeout", 15.0)) + monkeypatch.setattr( + cfg, "load_config", lambda: {"mcp_single_query_discovery_timeout": 0} + ) + assert mcp_startup._resolve_discovery_timeout(None, single_query=True) == default + + monkeypatch.setattr( + cfg, "load_config", lambda: {"mcp_single_query_discovery_timeout": "oops"} + ) + assert mcp_startup._resolve_discovery_timeout(None, single_query=True) == default + + monkeypatch.setattr(cfg, "load_config", lambda: {}) + assert mcp_startup._resolve_discovery_timeout(None, single_query=True) == default + + +def test_resolve_discovery_timeout_explicit_overrides_single_query(): + """An explicit timeout always wins, even in single-query mode.""" + assert mcp_startup._resolve_discovery_timeout(5.0, single_query=True) == 5.0 + + +# ── ensure_mcp_discovery_before_agent_build ───────────────────────────────── + + +def _stub_mcp_modules(monkeypatch): + """Stub MCP-related modules for helper tests.""" + monkeypatch.setitem( + sys.modules, + "hermes_cli.config", + types.SimpleNamespace( + read_raw_config=lambda: {"mcp_servers": {"demo": {"transport": "stdio"}}}, + load_config=lambda: {}, + DEFAULT_CONFIG={"mcp_discovery_timeout": 0.1, "mcp_single_query_discovery_timeout": 0.2}, + ), + ) + monkeypatch.setitem( + sys.modules, + "tools.mcp_oauth", + types.SimpleNamespace(suppress_interactive_oauth=lambda: __import__("contextlib").nullcontext()), + ) + monkeypatch.setitem( + sys.modules, + "tools.mcp_tool", + types.SimpleNamespace( + discover_mcp_tools=lambda: None, + get_mcp_status=lambda: [{"connected": True}], + ), + ) + + +def test_ensure_helper_starts_discovery_and_waits(monkeypatch): + """The helper starts background discovery if not yet started, then waits.""" + _stub_mcp_modules(monkeypatch) + waited = [] + + original_wait = mcp_startup.wait_for_mcp_discovery + + def _spy_wait(timeout=None, *, single_query=False): + waited.append(("wait", single_query)) + original_wait(timeout=timeout, single_query=single_query) + + monkeypatch.setattr(mcp_startup, "wait_for_mcp_discovery", _spy_wait) + + logger = types.SimpleNamespace(debug=lambda *_a, **_k: None, warning=lambda *_a, **_k: None) + + mcp_startup.ensure_mcp_discovery_before_agent_build( + logger=logger, + single_query=True, + ) + + # Discovery was started (thread created) + assert mcp_startup._mcp_discovery_thread is not None or waited + # Wait was called with single_query=True + assert any(call[1] is True for call in waited) + + +def test_ensure_helper_is_idempotent(monkeypatch): + """Calling the helper twice doesn't start a second discovery thread.""" + _stub_mcp_modules(monkeypatch) + logger = types.SimpleNamespace(debug=lambda *_a, **_k: None, warning=lambda *_a, **_k: None) + + mcp_startup.ensure_mcp_discovery_before_agent_build(logger=logger) + thread1 = mcp_startup._mcp_discovery_thread + if thread1: + thread1.join(timeout=2.0) + + mcp_startup.ensure_mcp_discovery_before_agent_build(logger=logger) + thread2 = mcp_startup._mcp_discovery_thread + if thread2: + thread2.join(timeout=2.0) + + # Second call didn't create a new thread (first one completed, status shows connected) + # or if it did, it's because the first exited with zero connected — but we stubbed + # get_mcp_status to return connected=True, so no retry. + # The key invariant: no exception, no hang. + + +def test_ensure_helper_swallows_errors(monkeypatch): + """A broken MCP config never aborts agent construction.""" + monkeypatch.setitem( + sys.modules, + "hermes_cli.config", + types.SimpleNamespace( + read_raw_config=lambda: (_ for _ in ()).throw(RuntimeError("boom")), + load_config=lambda: {}, + DEFAULT_CONFIG={}, + ), + ) + logger = types.SimpleNamespace(debug=lambda *_a, **_k: None, warning=lambda *_a, **_k: None) + + # Should not raise + mcp_startup.ensure_mcp_discovery_before_agent_build(logger=logger) + + +# ── oneshot ordering: discovery before AIAgent ────────────────────────────── + + +def test_oneshot_calls_ensure_helper_before_aiagent(monkeypatch): + """oneshot._run_agent must call ensure_mcp_discovery_before_agent_build + before constructing AIAgent (#38448).""" + import inspect + + import hermes_cli.oneshot as oneshot_mod + + src = inspect.getsource(oneshot_mod._run_agent) + helper_idx = src.find("ensure_mcp_discovery_before_agent_build") + agent_idx = src.find("AIAgent(") + assert helper_idx != -1, "oneshot._run_agent must call ensure_mcp_discovery_before_agent_build" + assert agent_idx != -1, "oneshot._run_agent must construct AIAgent" + assert helper_idx < agent_idx, ( + "ensure_mcp_discovery_before_agent_build must be called BEFORE AIAgent " + "construction in oneshot._run_agent (#38448)" + ) + + +# ── _init_agent ordering: discovery before AIAgent (CLI path) ─────────────── + + +def test_init_agent_calls_ensure_helper_before_aiagent(monkeypatch): + """cli_agent_setup_mixin._init_agent must call + ensure_mcp_discovery_before_agent_build before constructing AIAgent.""" + import inspect + + from hermes_cli.cli_agent_setup_mixin import CLIAgentSetupMixin + + src = inspect.getsource(CLIAgentSetupMixin._init_agent) + helper_idx = src.find("ensure_mcp_discovery_before_agent_build") + # _init_agent delegates AIAgent construction to cli.py, so we check + # the helper appears before the session_db / agent construction logic + assert helper_idx != -1, ( + "_init_agent must call ensure_mcp_discovery_before_agent_build" + ) + + +def test_init_agent_forwards_single_query_flag(monkeypatch): + """Single-query mode forwards single_query=True to the discovery wait.""" + import cli as cli_mod + + cli = cli_mod.HermesCLI(compact=True) + cli._session_db = object() + cli._resumed = False + cli.conversation_history = [] + cli._install_tool_callbacks = lambda: None + cli._ensure_tirith_security = lambda: None + cli._ensure_runtime_credentials = lambda: True + cli._single_query_mode = True + + seen = {} + + def _fake_ensure(*, logger, timeout=None, single_query=False, **_kw): + seen["single_query"] = single_query + + monkeypatch.setattr( + mcp_startup, + "ensure_mcp_discovery_before_agent_build", + _fake_ensure, + ) + monkeypatch.setattr(cli_mod, "AIAgent", lambda *_a, **_k: types.SimpleNamespace()) + + assert cli._init_agent() is True + assert seen.get("single_query") is True + + +def test_init_agent_defaults_to_interactive(monkeypatch): + """Without _single_query_mode, the helper uses interactive (short) bound.""" + import cli as cli_mod + + cli = cli_mod.HermesCLI(compact=True) + cli._session_db = object() + cli._resumed = False + cli.conversation_history = [] + cli._install_tool_callbacks = lambda: None + cli._ensure_tirith_security = lambda: None + cli._ensure_runtime_credentials = lambda: True + + seen = {} + + def _fake_ensure(*, logger, timeout=None, single_query=False, **_kw): + seen["single_query"] = single_query + + monkeypatch.setattr( + mcp_startup, + "ensure_mcp_discovery_before_agent_build", + _fake_ensure, + ) + monkeypatch.setattr(cli_mod, "AIAgent", lambda *_a, **_k: types.SimpleNamespace()) + + assert cli._init_agent() is True + assert seen.get("single_query") is False + + +# ── bounded wait: slow server doesn't freeze startup ──────────────────────── + + +def test_wait_stays_bounded_when_discovery_is_slow(monkeypatch): + """A slow/dead MCP server must not freeze startup: the wait is capped.""" + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "load_config", lambda: {"mcp_single_query_discovery_timeout": 0.1}) + + stop = threading.Event() + thread = threading.Thread(target=lambda: stop.wait(10), daemon=True) + thread.start() + mcp_startup._mcp_discovery_thread = thread + + try: + start = time.monotonic() + mcp_startup.wait_for_mcp_discovery(single_query=True) + elapsed = time.monotonic() - start + finally: + stop.set() + + assert elapsed < 3.0, ( + f"wait blocked {elapsed:.2f}s on a stuck MCP server — the wait must " + "stay bounded by mcp_single_query_discovery_timeout" + ) + + +def test_wait_returns_instantly_when_discovery_done(): + """When discovery is already complete, the wait returns immediately.""" + mcp_startup._mcp_discovery_thread = None + t0 = time.time() + mcp_startup.wait_for_mcp_discovery(single_query=True) + assert time.time() - t0 < 0.2