fix(mcp): ensure MCP discovery completes before agent build in non-interactive sessions

Non-interactive sessions (hermes chat -q, hermes -z) snapshot the tool
registry at AIAgent construction time. If background MCP discovery hasn't
finished, MCP tools are invisible for the entire session — and unlike
interactive mode, there is no between-turns late-binding refresh to recover.

Root cause: 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. Oneshot._run_agent() didn't call it
at all.

Fix:
- Add ensure_mcp_discovery_before_agent_build() helper to mcp_startup.py:
  idempotently starts discovery if needed + bounded wait. Fail-open on errors.
- Add single_query parameter to _resolve_discovery_timeout/wait_for_mcp_discovery:
  uses mcp_single_query_discovery_timeout (default 15s) instead of the
  interactive mcp_discovery_timeout (1.5s) because one-shot sessions have no
  second turn to recover.
- Wire into CLI _init_agent (single_query from _single_query_mode flag set
  in cli.py's single-query path) and oneshot._run_agent (single_query=True).
- Interactive sessions unchanged: keep 1.5s bound (between-turns refresh covers).

Closes #38448, #51316, #37013, #68137
Composite salvage of #60017 (chrishart0), #51322 (Bartok9), #38620 (buptwz),
#43544 (halonke), #36882 (vanhoof).
This commit is contained in:
kshitij 2026-08-01 11:47:42 +05:30
parent d1c40a731d
commit 3572d4bca1
6 changed files with 417 additions and 10 deletions

4
cli.py
View File

@ -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:

View File

@ -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:

View File

@ -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": {

View File

@ -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,
)

View File

@ -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

View File

@ -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