# NousResearch/hermes-agent · MCP tool discovery racing agent tool snapshot in one-shot/TUI ## Open now - 0 issues, 5 PRs open on GitHub. - PR #37301: fix(mcp): raise headless discovery timeout and add HERMES_MCP_DISCOVERY_WAIT override - PR #38620: Fix/mcp discovery non gateway paths - PR #51322: fix(cli): wait full MCP cold-start in single-query mode (closes #51316) - PR #60017: fix(cli): wait for MCP before first-turn agent builds - PR #107205: fix(mcp): increase discovery timeout and sync-wait in TUI agent build Most members describe the same theme: background MCP discovery can finish after the agent builds its tool snapshot, especially for slow stdio/HTTP servers in one-shot (-z), kanban worker, headless, or TUI modes. The issues and PRs propose waiting longer, making the timeout configurable, or exposing late-connecting tools. ## TL;DR - 24 member(s): 0 open issue(s), 8 closed issue(s), 5 open PR(s), 2 merged PR(s), 9 PR(s) closed without merging. - A fix was merged to main in PR #75933. Verify it addresses your case. - **Recommended:** read PR #75933 (and any competing PRs below) before filing. Confirm the merged change covers your symptom. ## Maintainer resolution verdict - Internal-tracker verdict over this cluster's issue members: `partially_resolved` · deciding PR(s): `pr-49208,pr-75933`. This is distinct from the fix status above. Weigh it yourself. ## How this cluster was grouped - Link evidence (`STRUCTURAL_PARTIAL`): some members are connected by structural evidence (shared changed files, or a closing/superseded reference); the rest are joined by an automated relevance judgement over their text. - Members outside that structural core may belong to a different underlying problem. - Pairwise link density `0.09`: the share of member pairs that carry a link, counting a structural link in full and a relevance-only link at 0.36. `1.00` means every pair is linked. ## Closed / duplicate issues - #37013: \[Bug\]: -z/headless oneshots silently drop slow-connecting MCP servers (0.75 s discovery wait too short for container/stdio-backed servers) - #38448: hermes -z can miss MCP tools because oneshot snapshots tools before MCP discovery - #41625: MCP tools discovered but not exposed to agent in TUI mode - #47121: MCP tools missing in TUI sessions due to wait_for_mcp_discovery timeout (0.75s) being shorter than MCP discovery time (~6s) - #51316: Bug: MCP tool snapshot misses stdio servers with slow startup in single-query mode (-z) - #54691: One-shot kanban workers miss large MCP servers' tools (snapshot races discovery) - #57170: MCP HTTP tools registered after agent_init are invisible (race condition) - #68137: One-shot mode (-z) builds the agent without waiting for background MCP discovery — slow stdio servers are silently dropped ## Prior & competing PRs - **#37104: fix(mcp): wait longer for headless discovery**. closed without merge · fix tier `suggested` - maintainer direction: category `uncategorized`; implication `needs_llm` - _suggested-tier fix detail withheld; re-request with `?tier=all` to include it._ - **#41630: fix(mcp): increase wait_for_mcp_discovery timeout from 0.75s to 3.0s**. closed without merge · fix tier `suggested` - maintainer direction: category `implemented_elsewhere`; implication `superseded` - _suggested-tier fix detail withheld; re-request with `?tier=all` to include it._ - **#49208: fix(mcp): expose late-connecting MCP tools to the agent (TUI/CLI/gateway)**. merged to main · fix tier `verified` - maintainer direction: category `merged` - files: `agent/agent_init.py`, `agent/background_review.py`, `agent/turn_context.py`, `cli.py`, `gateway/run.py`, `hermes_cli/config.py`, `hermes_cli/mcp_startup.py`, `tests/agent/test_turn_context.py`, `tests/tools/test_refresh_agent_mcp_tools.py`, `tools/mcp_tool.py`, `tui_gateway/entry.py`, `tui_gateway/server.py` - fix hunks: - `agent/agent_init.py` ```diff @@ -531,7 +531,14 @@ def init_agent( agent._last_activity_desc: str = "initializing" agent._current_tool: str | None = None agent._api_call_count: int = 0 - + # Opt-out flag for the between-turns MCP tool refresh (build_turn_context). + # Set on internal forks (e.g. background_review) that must keep ``tools[]`` + # byte-identical to a parent for provider cache parity. + agent._skip_mcp_refresh = False + # Registry generation the current tool snapshot was derived from. Lets a + # late/concurrent refresh reject a stale (older-generation) rebuild instead + # of clobbering a newer one. Set adjacent to the tool snapshot below. + agent._tool_snapshot_generation = 0 # Rate limit tracking — updated from x-ratelimit-* response headers # after each API call. Accessed by /usage slash command. agent._rate_limit_state: Optional["RateLimitState"] = None ``` ```diff @@ -953,7 +960,14 @@ def init_agent( print(f"🔄 Fallback chain ({len(agent._fallback_chain)} providers): " + " → ".join(f"{f['model']} ({f['provider']})" for f in agent._fallback_chain)) - # Get available tools with filtering + # Get available tools with filtering. Capture the registry generation this + # snapshot is derived from FIRST, so a later concurrent refresh can tell + # whether it holds a newer or staler view (see refresh_agent_mcp_tools). + try: + from tools.registry import registry as _snapshot_registry + agent._tool_snapshot_generation = _snapshot_registry._generation + except Exception: + agent._tool_snapshot_generation = 0 agent.tools = _ra().get_tool_definitions( enabled_toolsets=enabled_toolsets, disabled_toolsets=disabled_toolsets, ``` - `agent/background_review.py` ```diff @@ -535,6 +535,13 @@ def _bg_review_auto_deny(command, description, **kwargs): ) review_agent._memory_write_origin = "background_review" review_agent._memory_write_context = "background_review" + # The review fork pins the parent's cached system prompt and keeps + # ``tools[]`` byte-identical to the parent so its outbound request + # hits the same provider cache prefix (see the toolset-parity note + # above). The between-turns MCP refresh in build_turn_context would + # add late-connecting MCP tools to this fork and break that parity, + # so opt the review fork out of it. + review_agent._skip_mcp_refresh = True review_agent._memory_store = agent._memory_store review_agent._memory_enabled = agent._memory_enabled review_agent._user_profile_enabled = agent._user_profile_enabled ``` - `agent/turn_context.py` ```diff @@ -112,6 +112,24 @@ def build_turn_context( # Restore the primary runtime if the previous turn activated fallback. agent._restore_primary_runtime() + # Between-turns MCP refresh: an MCP server that finished connecting since + # the previous turn (slow HTTP/OAuth servers routinely take 2-6s on a cold + # connect, missing the bounded startup wait) lands in THIS turn's tool + # snapshot. This is cache-safe by construction: it runs in the per-turn + # prologue, before this turn's first API call assembles ``tools=``, so it + # only ever extends a fresh request prefix — it never mutates the cached + # prefix of an in-flight turn. No-op when no MCP servers are registered + # (the common case, gated by the cheap ``has_registered_mcp_tools`` check) + # or when the tool set is unchanged (``refresh_agent_mcp_tools`` diffs by + # name and leaves the snapshot untouched on no-change). + try: + if not getattr(agent, "_skip_mcp_refresh", False): + from tools.mcp_tool import has_registered_mcp_tools, refresh_agent_mcp_tools + if has_registered_mcp_tools(): + refresh_agent_mcp_tools(agent, quiet_mode=True) + except Exception: + logger.debug("between-turns MCP tool refresh skipped", exc_info=True) + # Sanitize surrogate characters from user input. if isinstance(user_message, str): user_message = sanitize_surrogates(user_message) ``` - `cli.py` ```diff @@ -9661,16 +9661,35 @@ def _reload_mcp(self): else: print(f" 🔧 {len(new_tools)} tool(s) available from {len(connected_servers)} server(s)") - # Refresh the agent's tool list so the model can call new tools + # Refresh the agent's tool list so the model can call new tools. + # Route through the shared helper so this CLI /reload-mcp path stays + # in lockstep with the TUI RPC / gateway reload / late-binding paths + # (name-diff, thread-safe, and — critically — additive-preserving so + # memory-provider and context-engine tools survive the rebuild). if self.agent is not None: - self.agent.tools = get_tool_definitions( - enabled_toolsets=self.agent.enabled_toolsets - if hasattr(self.agent, "enabled_toolsets") else None, + from tools.mcp_tool import refresh_agent_mcp_tools + # Explicit reload: pick up MCP servers the user ENABLED in config + # this session. self.enabled_toolsets was resolved once at + # startup; merge in any now-connected server names (unless the + # user pinned `all`/`*`, which already includes everything) so a + # freshly-added server isn't filtered out. Mirrors startup, where + # MCP server names are part of enabled_toolsets (see __init__). + enabled_override = None + et = self.enabled_toolsets + if et and "all" not in et and "*" not in et: + merged = list(et) + for _name in sorted(connected_servers): + if _name not in merged: + merged.append(_name) + enabled_override = merged + refresh_agent_mcp_tools( + self.agent, + enabled_override=enabled_override, quiet_mode=True, ) - self.agent.valid_tool_names = { - tool["function"]["name"] for tool in self.agent.tools - } if self.agent.tools else set() + # Keep the CLI's own list in sync with what the agent now uses. + if enabled_override is not None: + self.enabled_toolsets = enabled_override # Inject a message at the END of conversation history so the # model knows tools changed. Appended after all existing ``` - `gateway/run.py` ```diff @@ -11656,7 +11656,7 @@ async def _execute_mcp_reload(self, event: MessageEvent) -> str: # consented to the prompt-cache invalidation via the slash-confirm # gate in _handle_reload_mcp_command before we reach this point. try: - from model_tools import get_tool_definitions + from tools.mcp_tool import refresh_agent_mcp_tools _cache = getattr(self, "_agent_cache", None) _cache_lock = getattr(self, "_agent_cache_lock", None) if _cache_lock is not None and _cache: ``` ```diff @@ -11668,15 +11668,16 @@ async def _execute_mcp_reload(self, event: MessageEvent) -> str: continue if _agent is None: continue - new_defs = get_tool_definitions( - enabled_toolsets=getattr(_agent, "enabled_toolsets", None), - disabled_toolsets=getattr(_agent, "disabled_toolsets", None), - quiet_mode=True, - ) - _agent.tools = new_defs - _agent.valid_tool_names = { - t["function"]["name"] for t in new_defs - } if new_defs else set() + # Preserve each cached agent's build-time toolset + # selection EXACTLY: a gateway session built with a + # restricted enabled_toolsets (e.g. ["safe"]) must + # NOT silently gain tools after a reload. This is the + # opposite of the interactive CLI/TUI /reload-mcp, + # which is a single user re-applying their own config + # edit; gateway agents are per-session and may be + # deliberately locked down. (Contract is asserted by + # test_reload_mcp_preserves_per_agent_toolset_overrides.) + refresh_agent_mcp_tools(_agent, quiet_mode=True) except Exception as _exc: logger.debug( "Failed to update cached agent tools after MCP reload: %s", ``` - `hermes_cli/config.py` ```diff @@ -1201,6 +1201,21 @@ def _ensure_hermes_home_managed(home: Path): # 100K chars ≈ 25–35K tokens across typical tokenisers. "file_read_max_chars": 100_000, + # Seconds to wait at agent-build time for in-flight MCP server discovery + # to finish before the agent snapshots its tool list. MCP discovery runs + # in a background thread so a slow/dead server can't freeze startup; this + # bounds how long the first agent build blocks on it. The wait returns + # the INSTANT discovery completes, so users with no MCP servers (the common + # case) or fast servers pay ~0s regardless of this value — the bound is + # only reached when a server is genuinely still connecting. The old 0.75s + # default was a touch short for HTTP/OAuth servers on a cold connect; a + # modest bump lets more of them land in the FIRST turn's snapshot. This is + # only a turn-1 latency/UX knob: a server that misses this window is still + # picked up automatically on the next turn by the between-turns refresh + # (see agent/turn_context.py), so correctness never depends on it. Keep it + # small so a slow/dead server adds little to first-response latency. + "mcp_discovery_timeout": 1.5, + # Tool-output truncation thresholds. When terminal output or a # single read_file page exceeds these limits, Hermes truncates the # payload sent to the model (keeping head + tail for terminal, ``` - `hermes_cli/mcp_startup.py` ```diff @@ -51,9 +51,38 @@ def _discover() -> None: thread.start() -def wait_for_mcp_discovery(timeout: float = 0.75) -> None: - """Briefly wait for background MCP discovery before the first tool snapshot.""" +def _resolve_discovery_timeout(explicit: "float | None") -> 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. + """ + if explicit is not None: + return explicit + 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 + except Exception: + return 1.5 + + +def wait_for_mcp_discovery(timeout: "float | None" = None) -> None: + """Wait for background MCP discovery before the first tool snapshot. + + ``thread.join(timeout)`` returns the INSTANT discovery completes, so this + only ever blocks for the real connect time of a still-pending server — + users with no MCP servers or fast servers pay ~0s. The bound (from + ``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. + """ thread = _mcp_discovery_thread if thread is None or not thread.is_alive(): return - thread.join(timeout=timeout) + thread.join(timeout=_resolve_discovery_timeout(timeout)) ``` - `tests/agent/test_turn_context.py` ```diff @@ -47,6 +47,9 @@ def __init__(self): self.max_iterations = 90 self.tools = [] self.valid_tool_names = set() + self.enabled_toolsets = None + self.disabled_toolsets = None + self._skip_mcp_refresh = False self.compression_enabled = False self.context_compressor = types.SimpleNamespace( protect_first_n=2, protect_last_n=2 ``` ```diff @@ -185,3 +188,74 @@ def test_no_review_when_memory_disabled(): agent = _FakeAgent() ctx = _build(agent) assert ctx.should_review_memory is False + + +# ── Between-turns MCP refresh (cache-safe late-binding) ────────────────────── +# +# A slow MCP server that connects after the agent's build-time tool snapshot +# must become callable by the user's NEXT turn — without mutating an in-flight +# turn's cached request prefix. The prologue is exactly that boundary, so the +# refresh hook lives here. These assert the contract (R1/R2/R6 in the spec), +# not timing permutations. + + +def test_between_turns_refresh_adds_late_tool_when_servers_registered(): + """R1: a tool that registered since build lands in this turn's snapshot.""" + agent = _FakeAgent() + + new_def = {"type": "function", "function": {"name": "mcp_x_tool", "description": "", "parameters": {}}} + + import model_tools + with patch("tools.mcp_tool.has_registered_mcp_tools", return_value=True), \ + patch.object(model_tools, "get_tool_definitions", return_value=[new_def]): + _build(agent) + + assert "mcp_x_tool" in agent.valid_tool_names + assert any(t["function"]["name"] == "mcp_x_tool" for t in agent.tools) + + +def test_between_turns_refresh_skipped_when_no_servers(): + """R6: the common case (no MCP servers) never walks the registry.""" + agent = _FakeAgent() + import model_tools + + with patch("tools.mcp_tool.has_registered_mcp_tools", return_value=False), \ + patch.object(model_tools, "get_tool_definitions") as gtd: + _build(agent) + + gtd.assert_not_called() + + +def test_between_turns_refresh_skipped_when_skip_flag_set(): + """Internal forks (background_review) set _skip_mcp_refresh to keep tools[] + byte-identical to the parent for cache parity — the hook must honor it even + when MCP servers are registered.""" + agent = _FakeAgent() + agent._skip_mcp_refresh = True + import model_tools + + with patch("tools.mcp_tool.has_registered_mcp_tools", return_value=True), \ + patch.object(model_tools, "get_tool_definitions") as gtd: + _build(agent) + + gtd.assert_not_called() + + +def test_between_turns_refresh_no_churn_when_unchanged(): + """R2: an unchanged tool set leaves the snapshot object identity intact + (no needless swap → nothing for the next request prefix to diff against).""" + agent = _FakeAgent() + same = [{"type": "function", "function": {"name": "a", "description": "", "parameters": {}}}] + agent.tools = same + agent.valid_tool_names = {"a"} + + import model_tools + with patch("tools.mcp_tool.has_registered_mcp_tools", return_value=True), \ + patch.object( + model_tools, "get_tool_definitions", + return_value=[{"type": "function", "function": {"name": "a", "description": "", "parameters": {}}}], + ): + _build(agent) + + assert agent.tools is same # not replaced → no churn + ``` - `tests/tools/test_refresh_agent_mcp_tools.py` ```diff @@ -0,0 +1,298 @@ +"""Tests for the shared MCP agent-tool refresh helper and discovery-wait bound. + +``refresh_agent_mcp_tools`` is the single rebuild path used by the TUI +``reload.mcp`` RPC, the gateway reload, and the late-binding refresh thread — +so a slow MCP server that connects after the agent's one-time tool snapshot is +picked up everywhere identically. These assert the *contracts* those callers +rely on (name-based diff, in-place mutation, agent-scoped filtering) rather than +freezing any particular tool list. +""" + +import threading +import types + +from tools import mcp_tool + + +def _tool(name): + return {"type": "function", "function": {"name": name, "description": "", "parameters": {}}} + + +def _agent(tool_names, *, enabled=None, disabled=None): + a = types.SimpleNamespace() + a.tools = [_tool(n) for n in tool_names] + a.valid_tool_names = set(tool_names) + a.enabled_toolsets = enabled + a.disabled_toolsets = disabled + return a + + +def test_refresh_adds_late_landing_tools(monkeypatch): + """A server that registers after build → its tools land in the snapshot.""" + agent = _agent(["read_file", "terminal"]) + + new_defs = [_tool(n) for n in ("read_file", "terminal", "mcp_granola_get_account_info")] + monkeypatch.setattr(mcp_tool, "get_tool_definitions", lambda **kw: new_defs, raising=False) + # get_tool_definitions is imported inside the helper from model_tools, so patch there too. + import model_tools + monkeypatch.setattr(model_tools, "get_tool_definitions", lambda **kw: new_defs) + + added = mcp_tool.refresh_agent_mcp_tools(agent) + + assert added == {"mcp_granola_get_account_info"} + assert "mcp_granola_get_account_info" in agent.valid_tool_names + assert len(agent.tools) == 3 + + +def test_refresh_no_change_returns_empty_and_leaves_agent_untouched(monkeypatch): + """No new tools → empty set, and the snapshot object is not swapped.""" + agent = _agent(["read_file", "terminal"]) + original_tools = agent.tools + + import model_tools + monkeypatch.setattr( + model_tools, "get_tool_definitions", + lambda **kw: [_tool("read_file"), _tool("terminal")], + ) + + added = mcp_tool.refresh_agent_mcp_tools(agent) + + assert added == set() + assert agent.tools is original_tools # not replaced → no churn / no cache thrash + + +def test_refresh_detects_equal_size_swap(monkeypatch): + """Name-based diff catches an add+remove of equal count (count-compare can't).""" + agent = _agent(["a", "old_mcp_tool"]) # 2 tools + + import model_tools + # Same COUNT (2) but a different membership: old_mcp_tool removed, new added. + monkeypatch.setattr( + model_tools, "get_tool_definitions", + lambda **kw: [_tool("a"), _tool("new_mcp_tool")], + ) + + added = mcp_tool.refresh_agent_mcp_tools(agent) + + assert added == {"new_mcp_tool"} + assert agent.valid_tool_names == {"a", "new_mcp_tool"} + assert "old_mcp_tool" not in agent.valid_tool_names + + +def test_refresh_passes_agent_toolset_filters(monkeypatch): + """The rebuild re-derives with the agent's OWN enabled/disabled toolsets.""" + agent = _agent(["a"], enabled=["coding", "granola"], disabled=["messaging"]) + seen = {} + + import model_tools + + def _capture(**kw): + seen.update(kw) + return [_tool("a"), _tool("b")] + + monkeypatch.setattr(model_tools, "get_tool_definitions", _capture) + + mcp_tool.refresh_agent_mcp_tools(agent) + + assert seen["enabled_toolsets"] == ["coding", "granola"] + assert seen["disabled_toolsets"] == ["messaging"] + + +def test_refresh_preserves_memory_provider_and_context_engine_tools(monkeypatch): + """B1 regression: a rebuild must NOT drop post-build-injected tools. + + get_tool_definitions() returns only the registry-derived tools. agent_init + appends memory-provider tools (mem0/honcho/…) and context-engine tools + (lcm_*) directly onto agent.tools AFTER that. A naive + `agent.tools = get_tool_definitions()` would silently delete them on every + refresh. The helper must re-inject them. + """ + # Agent already carries: a built-in, a memory-provider tool, a context tool. + agent = _agent(["read_file", "memory_search", "lcm_grep"]) + + # Provider exposes its schemas; context compressor exposes lcm_*. + agent._memory_manager = types.SimpleNamespace( + get_all_tool_schemas=lambda: [ + {"name": "memory_search", "description": "", "parameters": {}} + ] + ) + agent.context_compressor = types.SimpleNamespace( + get_tool_schemas=lambda: [ + {"name": "lcm_grep", "description": "", "parameters": {}} + ] + ) + agent._context_engine_tool_names = {"lcm_grep"} + + import model_tools + # The registry now ALSO has a newly-connected MCP tool, but does NOT contain + # the memory/context tools (they're never in get_tool_definitions output). + monkeypatch.setattr( + model_tools, "get_tool_definitions", + lambda **kw: [_tool("read_file"), _tool("mcp_new_server_tool")], + ) + + added = mcp_tool.refresh_agent_mcp_tools(agent) + + # The new MCP tool landed AND the injected families survived. + assert "mcp_new_server_tool" in agent.valid_tool_names + assert "memory_search" in agent.valid_tool_names # not clobbered + assert "lcm_grep" in agent.valid_tool_names # not clobbered + assert added == {"mcp_new_server_tool"} + + +def test_refresh_respects_context_engine_toolset_gate(monkeypatch): + """#5544: context-engine tools must NOT be re-injected on a restricted + toolset. A platform with enabled_toolsets that excludes context_engine + must not get lcm_* leaked back in by a refresh.""" + agent = _agent(["read_file"], enabled=["coding"]) # context_engine NOT enabled + agent.context_compressor = types.SimpleNamespace( + get_tool_schemas=lambda: [{"name": "lcm_grep", "description": "", "parameters": {}}] + ) + agent._context_engine_tool_names = set() + + import model_tools + monkeypatch.setattr( + model_tools, "get_tool_definitions", + lambda **kw: [_tool("read_file"), _tool("mcp_new_tool")], + ) + + mcp_tool.refresh_agent_mcp_tools(agent) + + assert "mcp_new_tool" in agent.valid_tool_names # MCP tool still lands + assert "lcm_grep" not in agent.valid_tool_names # gated out (#5544) + + +def test_refreshed_tool_is_callable_through_valid_tool_names_guard(monkeypatch): + """The whole point: a late tool, once refreshed, passes the name guard the + run loop uses to accept/reject tool calls (agent.valid_tool_names).""" + agent = _agent(["read_file"]) + + import model_tools + monkeypatch.setattr( + model_tools, "get_tool_definitions", + lambda **kw: [_tool("read_file"), _tool("mcp_granola_list_meetings")], + ) + + # Before refresh the run loop would reject the call ("Tool does not exist"). + assert "mcp_granola_list_meetings" not in agent.valid_tool_names + + mcp_tool.refresh_agent_mcp_tools(agent) + + # After refresh the same guard accepts it AND it's in the tools= payload. + assert "mcp_granola_list_meetings" in agent.valid_tool_names + assert any(t["function"]["name"] == "mcp_granola_list_meetings" for t in agent.tools) + + +def test_refresh_is_thread_safe_under_concurrent_calls(monkeypatch): + """Concurrent refreshes keep tools / valid_tool_names coherent. + + The registry alternates between two DIFFERENT tool sets every call, so the + write path (publish) runs repeatedly rather than short-circuiting on the + no-change early return — this actually exercises the lock. The invariant: + a reader of ``valid_tool_names`` must always match ``agent.tools``, and the + final published pair must be one of the two valid sets (never a mix). + """ + agent = _agent(["a"]) + + import itertools + set_a = [_tool("a"), _tool("b")] + set_b = [_tool("a"), _tool("c")] + flip = itertools.cycle([set_a, set_b]) + flip_lock = threading.Lock() + + def _gtd(**kw): + with flip_lock: + return list(next(flip)) + + import model_tools + monkeypatch.setattr(model_tools, "get_tool_definitions", _gtd) + + errors = [] + + def _worker(): + try: + for _ in range(50): + mcp_tool.refresh_agent_mcp_tools(agent) + # Coherence invariant: the name set must match the tool list + # at every observation, never a torn cross-attribute state. + names = {t["function"]["name"] for t in agent.tools} + assert agent.valid_tool_names == names + assert names in ({"a", "b"}, {"a", "c"}) + except Exception as exc: # pragma: no cover - failure path + errors.append(exc) + + threads = [threading.Thread(target=_worker) for _ in range(4)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + + assert not errors + assert agent.valid_tool_names in ({"a", "b"}, {"a", "c"}) + + +# ── discovery-wait bound (mcp_discovery_timeout config) ────────────────────── + + +def test_resolve_discovery_timeout_explicit_wins(monkeypatch): + from hermes_cli import mcp_startup + + assert mcp_startup._resolve_discovery_timeout(2.5) == 2.5 + + +def test_resolve_discovery_timeout_reads_config(monkeypatch): + from hermes_cli import mcp_startup + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "load_config", lambda: {"mcp_discovery_timeout": 8.0}) + + assert mcp_startup._resolve_discovery_timeout(None) == 8.0 + + +def test_resolve_discovery_timeout_falls_back_on_bad_value(monkeypatch): + from hermes_cli import mcp_startup + import hermes_cli.config as cfg + + # Non-positive / unparsable → DEFAULT_CONFIG value, never hang. + default = float(cfg.DEFAULT_CONFIG.get("mcp_discovery_timeout", 1.5)) + monkeypatch.setattr(cfg, "load_config", lambda: {"mcp_discovery_timeout": 0}) + assert mcp_startup._resolve_discovery_timeout(None) == default + + monkeypatch.setattr(cfg, "load_config", lambda: {"mcp_discovery_timeout": "oops"}) + assert mcp_startup._resolve_discovery_timeout(None) == default + + +def test_stale_generation_refresh_does_not_clobber_newer(monkeypatch): + """A slower refresh that computed an OLDER registry generation must not + overwrite a snapshot a newer-generation refresh already published.""" + from tools import registry as _reg_mod + + agent = _agent(["read_file"]) + # A newer refresh already published generation = current+5, with two tools. + agent._tool_snapshot_generation = _reg_mod.registry._generation + 5 + agent.tools = [_tool("read_file"), _tool("mcp_new_tool")] + agent.valid_tool_names = {"read_file", "mcp_new_tool"} + + import model_tools + # This (stale) refresh computes only the old single-tool set. + monkeypatch.setattr(model_tools, "get_tool_definitions", lambda **kw: [_tool("read_file")]) + + added = mcp_tool.refresh_agent_mcp_tools(agent) + + # Stale write rejected: the newer tool survives. + assert added == set() + assert "mcp_new_tool" in agent.valid_tool_names + + +def test_wait_returns_instantly_when_no_discovery_thread(monkeypatch): + """The common case (no MCP / discovery done) pays ~0s regardless of bound.""" + import time + from hermes_cli import mcp_startup + + monkeypatch.setattr(mcp_startup, "_mcp_discovery_thread", None) + import hermes_cli.config as cfg + monkeypatch.setattr(cfg, "load_config", lambda: {"mcp_discovery_timeout": 999.0}) + + t0 = time.time() + mcp_startup.wait_for_mcp_discovery() + assert time.time() - t0 < 0.2 # never blocks on the bound when nothing's pending ``` - `tools/mcp_tool.py` ```diff @@ -3994,6 +3994,215 @@ async def _probe_all(): return result +# Serializes in-place mutation of an agent's tool snapshot. The reload RPC, +# the gateway reload, and the late-binding refresh thread all swap +# ``agent.tools`` / ``agent.valid_tool_names`` after the agent was built; the +# agent's run loop reads those during tool iteration, so a concurrent write +# mid-read could otherwise expose a half-updated list. +_agent_tools_lock = threading.Lock() + + +def has_registered_mcp_tools() -> bool: + """True if any MCP server has actually registered tools into the registry. + + Cheap — checks the global MCP-tool→server name map under ``_lock``, no + registry walk. Used by the per-turn refresh hook so a session with no MCP + tools (the common case, and also a connected-but-zero-tool/prompt-only + server) skips the ``get_tool_definitions`` rebuild entirely. Checks + registered TOOLS, not connected servers, so a server that registers no tools + doesn't keep the hook firing every turn. + """ + with _lock: + return bool(_mcp_tool_server_names) + + +def refresh_agent_mcp_tools( + agent, + *, + enabled_override=None, + disabled_override=None, + quiet_mode: bool = True, +) -> set: + """Re-derive an already-built agent's tool snapshot from the live registry. + + The agent snapshots ``agent.tools`` once at build time and never re-reads + the registry (see ``run_agent`` / ``agent_init``). When MCP servers connect + *after* that snapshot — a slow HTTP/OAuth server that misses the bounded + startup wait, or a ``/reload-mcp`` — their tools are invisible until the + snapshot is rebuilt. This is the single shared rebuild used by every such + caller (the TUI ``reload.mcp`` RPC, the gateway reload, the late-binding + refresh thread, and the per-turn between-turns refresh) so they can't drift + apart again. + + The rebuild respects the agent's own ``enabled_toolsets`` / + ``disabled_toolsets`` (the same filtering it was built with) and diffs by + tool **name** (not count — a count compare misses an equal-size add/remove + swap). + + Crucially it is **additive-preserving**: ``get_tool_definitions`` returns + only the registry-derived tools, but ``agent_init`` appends two further + families directly onto ``agent.tools`` *after* that — external + memory-provider tools (mem0/honcho/…) and context-engine tools + (``lcm_*``). A naive ``agent.tools = get_tool_definitions(...)`` would + silently DELETE those. So after rebuilding the registry set we re-run the + same post-build injectors ``agent_init`` used, reconstructing the full + surface. The new ``(tools, valid_tool_names)`` pair is published together + under ``_agent_tools_lock`` so a concurrent reader never sees a + cross-attribute half-swap. + + Returns the set of newly-added tool names (empty when nothing changed), so + callers can decide whether to notify the user / re-emit session info. The + caller owns the prompt-cache contract: this helper does NOT check turn state, + because each caller has a different policy (``/reload-mcp`` rebuilds after + explicit user consent; the late-binding and between-turns paths only rebuild + at a turn boundary, before that turn's ``tools=`` prefix is assembled). + """ + from model_tools import get_tool_definitions + from tools.registry import registry + + # Explicit reloads (/reload-mcp) pass freshly-resolved toolsets so a server + # the user just ENABLED in config is picked up; the agent's stored selection + # is then updated to match. The automatic paths (between-turns, late-binding) + # pass nothing and reuse the agent's build-time selection unchanged. + if enabled_override is not None or disabled_override is not None: + enabled = enabled_override if enabled_override is not None else getattr(agent, "enabled_toolsets", None) + disabled = disabled_override if disabled_override is not None else getattr(agent, "disabled_toolsets", None) + agent.enabled_toolsets = enabled + agent.disabled_toolsets = disabled + else: + enabled = getattr(agent, "enabled_toolsets", None) + disabled = getattr(agent, "disabled_toolsets", None) + + # Capture the registry generation this rebuild is derived from BEFORE the + # (potentially slow) get_tool_definitions call. Used at publish time to + # reject a stale write: if two callers race (e.g. the late-refresh daemon + # and the between-turns prologue around turn 1), a slower caller that + # computed an OLDER set must not clobber a newer set another caller already + # published. ``registry._generation`` bumps on every (de)register. + snapshot_generation = registry._generation + + # Registry-derived tools (built-ins + MCP), filtered to the agent's toolsets. + # Computed OUTSIDE the lock (get_tool_definitions can be slow); the diff and + # publish below happen together in ONE critical section so two concurrent + # callers can't torn-publish or compute overlapping ``added`` sets. + new_defs = list( + get_tool_definitions( + enabled_toolsets=enabled, + disabled_toolsets=disabled, + quiet_mode=quiet_mode, + ) + or [] + ) + new_names = {t["function"]["name"] for t in new_defs} + + # Re-append the post-build injected families that get_tool_definitions does + # NOT reproduce, so a refresh never strips them (memory-provider + context- + # engine tools). Staged entirely on LOCALS — the live ``agent.tools`` / + # ``valid_tool_names`` / ``_context_engine_tool_names`` are never touched + # until the single atomic publish below, so a concurrent reader + # (``build_api_kwargs``) can't see a partial rebuild or a cross-attribute + # half-swap. ``staged_engine_names`` are the context-engine routing names + # this rebuild actually appended (matching agent_init's dedup-aware add). + staged_engine_names = _reinject_post_build_tools(agent, new_defs, new_names) + + # Single atomic read-diff-publish so the returned ``added`` is consistent + # with what was actually published, even under concurrent callers, and a + # stale (older-generation) rebuild can't overwrite a newer published one. + with _agent_tools_lock: + # Defensive: the published generation should be an int, but tolerate an + # agent that never set it (or set a non-int, e.g. a test mock) rather + # than throwing TypeError on the comparison and silently failing the + # whole refresh. + published_gen_raw = getattr(agent, "_tool_snapshot_generation", -1) + published_gen = published_gen_raw if isinstance(published_gen_raw, int) else -1 + if snapshot_generation < published_gen: + # A newer snapshot already won; our set is stale — drop it. + return set() + current = { + t["function"]["name"] + for t in (getattr(agent, "tools", None) or []) + } + if new_names == current: + # No change → leave the live snapshot untouched (no churn), but + # record the generation so an in-flight older caller can't clobber. + agent._tool_snapshot_generation = max(published_gen, snapshot_generation) + return set() + agent.tools = new_defs + agent.valid_tool_names = new_names + # Publish context-engine routing names atomically with the snapshot. + engine_names = getattr(agent, "_context_engine_tool_names", None) + if isinstance(engine_names, set): + engine_names.clear() + engine_names.update(staged_engine_names) + agent._tool_snapshot_generation = max(published_gen, snapshot_generation) + return new_names - current + + +def _reinject_post_build_tools(agent, tools_list: list, name_set: set) -> set: + """Append memory-provider and context-engine tools onto staged locals. + + Mirrors the post-``get_tool_definitions`` injection in ``agent_init`` so a + snapshot rebuild reconstructs the FULL tool surface, not just the + registry-derived subset. Operates ONLY on the caller's staged ``tools_list`` + / ``name_set`` (never the live agent attributes) so the rebuild stays atomic. + Idempotent (skips names already present) and fail-soft. + + Returns the set of context-engine routing names actually appended by THIS + rebuild — matching ``agent_init``'s dedup behavior (a name already provided + by a registry/plugin tool is NOT claimed for context-engine routing). The + caller publishes this into ``agent._context_engine_tool_names`` atomically + with the snapshot. + """ + def _add(schema: dict) -> bool: + name = schema.get("name", "") + if not name or name in name_set: + return False + tools_list.append({"type": "function", "function": schema}) + name_set.add(name) + return True + + # Memory-provider tools (mem0/honcho/byterover/supermemory/…). + try: + memory_manager = getattr(agent, "_memory_manager", None) + get_mem_schemas = getattr(memory_manager, "get_all_tool_schemas", None) if memory_manager else None + if callable(get_mem_schemas): + # Honor the same enablement gate inject_memory_provider_tools uses. + from agent.memory_manager import memory_provider_tools_enabled + if "memory" in name_set or memory_provider_tools_enabled(getattr(agent, "enabled_toolsets", None)): + for schema in get_mem_schemas(): + if isinstance(schema, dict): + _add(schema) + except Exception: + logger.debug("Memory-provider tool re-injection skipped", exc_info=True) + + # Context-engine tools (lcm_grep/lcm_describe/…) — the `context_engine` + # toolset is intentionally empty, so these only exist via this append. + # Honor the same enabled_toolsets gate agent_init uses (#5544): without it a + # restricted-toolset platform (e.g. platform_toolsets: telegram: []) would + # re-leak lcm_* tools the build deliberately excluded, and pay the local- + # model latency penalty. + staged_engine_names: set = set() + try: + enabled = getattr(agent, "enabled_toolsets", None) + context_engine_allowed = enabled is None or "context_engine" in enabled + compressor = getattr(agent, "context_compressor", None) + get_schemas = getattr(compressor, "get_tool_schemas", None) if compressor else None + if context_engine_allowed and callable(get_schemas): + for schema in get_schemas(): + if not isinstance(schema, dict): + continue + name = schema.get("name", "") + # Only claim the routing name when WE appended the schema, so a + # name already owned by a registry/plugin tool keeps its own + # dispatch (matches agent_init.py's `continue`-before-claim). + if _add(schema) and name: + staged_engine_names.add(name) + except Exception: + logger.debug("Context-engine tool re-injection skipped", exc_info=True) + + return staged_engine_names + + def shutdown_mcp_servers(): """Close all MCP server connections and stop the background loop. ``` - `tui_gateway/entry.py` ```diff @@ -192,22 +192,32 @@ def _log_exit(reason: str) -> None: print(f"[gateway-exit] {reason}", file=sys.stderr, flush=True) -def wait_for_mcp_discovery(timeout: float = 0.75) -> None: - """Briefly block until background MCP discovery finishes, up to ``timeout``. +def wait_for_mcp_discovery(timeout: "float | None" = None) -> None: + """Block until background MCP discovery finishes, up to the resolved bound. MCP discovery runs in a daemon thread spawned at startup (see main()) so a slow/dead server can't freeze ``gateway.ready``. But the agent snapshots its tool list ONCE at build time and never re-reads it, so a reachable-but- slow server that finishes connecting *after* the first prompt would be - invisible for the whole session. Joining with a short bounded timeout - before the first agent build lets already-spawning fast servers land - without re-introducing the startup hang: a dead server simply isn't waited - on beyond ``timeout``. No-op when no discovery thread was started. + invisible for the whole session. Joining with a bounded timeout before the + first agent build lets already-spawning servers land without re-introducing + the startup hang: ``thread.join(timeout)`` returns the instant discovery + completes (so fast/no-MCP startups pay ~0s), and a dead server is simply not + waited on beyond the bound. No-op when no discovery thread was started. + + The bound comes from ``mcp_discovery_timeout`` in config (shared with the + CLI path via ``hermes_cli.mcp_startup``); ``timeout`` overrides it. """ thread = _mcp_discovery_thread if thread is None or not thread.is_alive(): return - thread.join(timeout=timeout) + try: + from hermes_cli.mcp_startup import _resolve_discovery_timeout + + bound = _resolve_discovery_timeout(timeout) + except Exception: + bound = timeout if timeout is not None else 0.75 + thread.join(timeout=bound) def mcp_discovery_in_flight() -> bool: ``` - `tui_gateway/server.py` ```diff @@ -3557,26 +3557,19 @@ def _wait_then_refresh() -> None: ): return try: - from model_tools import get_tool_definitions + from tools.mcp_tool import refresh_agent_mcp_tools - new_defs = get_tool_definitions( - enabled_toolsets=_load_enabled_toolsets(), - quiet_mode=True, - ) + added = refresh_agent_mcp_tools(agent, quiet_mode=True) except Exception as exc: logger.warning( - "Late MCP refresh: get_tool_definitions failed for %s: %s", + "Late MCP refresh: tool snapshot rebuild failed for %s: %s", sid, exc, ) return - # No change (discovery added nothing new) → don't churn the client. - if len(new_defs or []) == len(getattr(agent, "tools", []) or []): + # No new tools landed (discovery added nothing) → don't churn the client. + if not added: return - agent.tools = new_defs - agent.valid_tool_names = ( - {t["function"]["name"] for t in new_defs} if new_defs else set() - ) info = _session_info(agent, session) # Emit outside the lock — write_json must not block under _sessions_lock. _emit("session.info", sid, info) ``` ```diff @@ -8414,16 +8407,15 @@ def _(rid, params: dict) -> dict: # The user already consented to the prompt-cache invalidation via # the confirm gate above. Mirrors gateway/run.py::_execute_mcp_reload. try: - from model_tools import get_tool_definitions + from tools.mcp_tool import refresh_agent_mcp_tools - new_defs = get_tool_definitions( - enabled_toolsets=_load_enabled_toolsets(), + # Explicit reload: re-resolve enabled toolsets so a server the + # user just enabled in config this session is picked up. + refresh_agent_mcp_tools( + agent, + enabled_override=_load_enabled_toolsets(), quiet_mode=True, ) - agent.tools = new_defs - agent.valid_tool_names = ( - {t["function"]["name"] for t in new_defs} if new_defs else set() - ) except Exception as _exc: logger.warning( "Failed to refresh cached agent tools after /reload-mcp: %s", ``` - **#54695: fix(mcp): let one-shot kanban workers await large MCP discovery before snapshot**. closed without merge · fix tier `suggested` - maintainer direction: category `uncategorized`; implication `needs_llm` - **#57234: fix(agent): wait for MCP discovery before init_agent tool snapshot (#57170)**. closed without merge · fix tier `suggested` - maintainer direction: category `partial_fix`; implication `needs_llm`; refs `#57170` - _suggested-tier fix detail withheld; re-request with `?tier=all` to include it._ - **#75933: fix(mcp): ensure MCP discovery completes before agent build in non-interactive sessions**. merged to main · fix tier `verified` - maintainer direction: category `merged` - files: `cli.py`, `hermes_cli/cli_agent_setup_mixin.py`, `hermes_cli/config_defaults.py`, `hermes_cli/mcp_startup.py`, `hermes_cli/oneshot.py`, `tests/hermes_cli/test_mcp_discovery_timing.py` - **#37301: fix(mcp): raise headless discovery timeout and add HERMES_MCP_DISCOVERY_WAIT override**. open - maintainer direction: category `uncategorized`; implication `needs_llm`; refs `094aa85c3`, `#37013` - files: `cli.py`, `hermes_cli/mcp_startup.py`, `tests/hermes_cli/test_mcp_discovery_timeout.py`, `tui_gateway/entry.py` - **#37393: fix(mcp): respect HERMES_MCP_DISCOVERY_TIMEOUT env var for slow-connecting servers**. closed without merge - maintainer direction: category `uncategorized`; implication `needs_llm` - **#38620: Fix/mcp discovery non gateway paths**. open - maintainer direction: category `uncategorized`; implication `needs_llm`; refs `#38448` - files: `hermes_cli/oneshot.py`, `tests/hermes_cli/test_mcp_startup.py` - **#49160: fix(mcp): expose late-connecting MCP tools to the agent (TUI/CLI/gateway)**. closed without merge - maintainer direction: category `merged_via`; implication `superseded`; refs `#49208` - **#51322: fix(cli): wait full MCP cold-start in single-query mode (closes #51316)**. open - maintainer direction: category `uncategorized`; implication `needs_llm` - files: `cli.py`, `contributors/emails/[redacted:email]`, `tests/tools/test_refresh_agent_mcp_tools.py`, `tui_gateway/slash_worker.py` - **#60017: fix(cli): wait for MCP before first-turn agent builds**. open - maintainer direction: category `uncategorized`; implication `needs_llm`; refs `#61944` - files: `hermes_cli/cli_agent_setup_mixin.py`, `hermes_cli/config.py`, `hermes_cli/mcp_startup.py`, `hermes_cli/oneshot.py`, `tests/hermes_cli/test_mcp_startup.py`, `tests/test_tui_gateway_server.py`, `tests/tools/test_refresh_agent_mcp_tools.py`, `tests/tui_gateway/test_wait_for_mcp_discovery.py`, `tui_gateway/entry.py`, `tui_gateway/server.py` - **#68162: fix(cli): wait for background MCP discovery before building the one-shot agent (#68137)**. closed without merge - maintainer direction: category `implemented_elsewhere`; implication `superseded`; refs `3572d4bca`, `#68137` - **#68168: fix: wait for MCP discovery in one-shot mode before building agent (#68137)**. closed without merge - maintainer direction: category `duplicate_of`; implication `duplicate`; refs `#68162` - **#71277: fix: wait for MCP discovery in one-shot mode before building agent (#68137)**. closed without merge - maintainer direction: category `superseded_by_pr`; implication `superseded`; refs `#68162` - **#107205: fix(mcp): increase discovery timeout and sync-wait in TUI agent build**. open - maintainer direction: category `silent_close`; implication `active` - files: `hermes_cli/config_defaults.py`, `tests/tui_gateway/test_mcp_discovery_sync.py` ## Fix status - Merged to main in PR #75933 (cited above). No release data is tracked. Verify downstream. - Fix last verified (merged) `2026-08-01`. - Subsystem: `cron`