From af8d698b418ede7dc8b078a8116510699ec94eb7 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:02:33 -0700 Subject: [PATCH] fix(mcp): propagate profile HERMES_HOME override into shared discovery owner; restore stdio startup + WSTransport tests Follow-up to @LionGateOS's #72135 salvage: - Route ensure_mcp_discovery_started through hermes_cli.mcp_startup's shared owner instead of a hand-rolled bare thread, keeping the start lock, retry-after-zero-connected allowance, and interactive-OAuth suppression. The shared owner now captures the caller's context-local HERMES_HOME override and re-installs it inside the discovery thread, so discovery reads the selected profile's mcp_servers (#67605). - Restore stdio TUI startup discovery in main() and the _mcp_discovery_enabled retry gate in wait_for_mcp_discovery, both dropped by the original branch. - Restore the 3 WSTransport regression tests (send serialization, cross-batch ordering, drained-token ordering) deleted by the PR. - Harden the profile-scoped discovery test against sibling-state leaks. --- hermes_cli/mcp_startup.py | 28 ++++++++ tests/test_tui_gateway_server.py | 15 ++-- tests/test_tui_gateway_ws.py | 118 +++++++++++++++++++++++++++++++ tui_gateway/entry.py | 117 ++++++++++++++++-------------- 4 files changed, 220 insertions(+), 58 deletions(-) diff --git a/hermes_cli/mcp_startup.py b/hermes_cli/mcp_startup.py index a8744161d563..82839d933b67 100644 --- a/hermes_cli/mcp_startup.py +++ b/hermes_cli/mcp_startup.py @@ -58,7 +58,28 @@ def start_background_mcp_discovery(*, logger, thread_name: str) -> None: if not _has_configured_mcp_servers(): return + # Capture the caller's context-local HERMES_HOME override (profile + # scoping in multi-profile processes like the dashboard/desktop + # backend) and re-install it inside the discovery thread. ContextVars + # do not propagate into bare threads, so without this a session + # "switched" to profile X would discover the LAUNCH profile's + # mcp_servers instead (#67605). The config gate above already runs on + # the caller's thread, so it sees the same override. + try: + from hermes_constants import get_hermes_home_override + + home_override = get_hermes_home_override() + except Exception: + home_override = None + def _discover() -> None: + token = None + try: + from hermes_constants import set_hermes_home_override + + token = set_hermes_home_override(home_override) + except Exception: + token = None try: _discover_mcp_tools_without_interactive_oauth() try: @@ -73,6 +94,13 @@ def start_background_mcp_discovery(*, logger, thread_name: str) -> None: except Exception: logger.debug("Background MCP tool discovery failed", exc_info=True) finally: + if token is not None: + try: + from hermes_constants import reset_hermes_home_override + + reset_hermes_home_override(token) + except Exception: + pass with _mcp_discovery_lock: global _mcp_discovery_thread, _mcp_discovery_started _mcp_discovery_thread = None diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index efc0b180f8a1..cd751aaa9f10 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -569,6 +569,7 @@ def _write_profile_cfg(home: Path, cwd: str | None) -> Path: def test_profile_scoped_mcp_discovery_uses_target_home(monkeypatch, tmp_path): """MCP discovery must start under the selected profile's HERMES_HOME.""" + from hermes_cli import mcp_startup from hermes_constants import get_hermes_home from tui_gateway import entry @@ -587,20 +588,26 @@ def test_profile_scoped_mcp_discovery_uses_target_home(monkeypatch, tmp_path): seen = [] - monkeypatch.setattr(entry, "_mcp_discovery_thread", None) + monkeypatch.setattr(mcp_startup, "_mcp_discovery_started", False) + monkeypatch.setattr(mcp_startup, "_mcp_discovery_thread", None) + # ensure_mcp_discovery_started flips this module global; monkeypatch it so + # the enablement doesn't leak into sibling tests in this file. + monkeypatch.setattr(entry, "_mcp_discovery_enabled", False) monkeypatch.setattr( - "tools.mcp_tool.discover_mcp_tools", + mcp_startup, + "_discover_mcp_tools_without_interactive_oauth", lambda: seen.append(str(get_hermes_home())), ) try: entry.ensure_mcp_discovery_started() - thread = entry._mcp_discovery_thread + thread = mcp_startup._mcp_discovery_thread assert thread is not None thread.join(timeout=2) finally: reset_hermes_home_override(token) - entry._mcp_discovery_thread = None + mcp_startup._mcp_discovery_thread = None + mcp_startup._mcp_discovery_started = False assert seen == [str(profile_home)] diff --git a/tests/test_tui_gateway_ws.py b/tests/test_tui_gateway_ws.py index da3c9be3446a..ad41ba9dade6 100644 --- a/tests/test_tui_gateway_ws.py +++ b/tests/test_tui_gateway_ws.py @@ -218,3 +218,121 @@ def test_ws_starts_mcp_discovery_before_ready(monkeypatch): # should not start MCP discovery before a profile has been bound. assert calls == [] assert events == ["accept", "ready_after_0"] + + +def test_ws_transport_serializes_concurrent_sends(): + active_sends = 0 + max_active_sends = 0 + sent = [] + + class FakeWS: + async def send_text(self, line): + nonlocal active_sends, max_active_sends + active_sends += 1 + max_active_sends = max(max_active_sends, active_sends) + try: + await asyncio.sleep(0.05) + sent.append(line) + finally: + active_sends -= 1 + + loop = asyncio.new_event_loop() + thread = threading.Thread(target=loop.run_forever, daemon=True) + thread.start() + try: + transport = ws_mod.WSTransport(FakeWS(), loop, peer="serialize-test") + with concurrent.futures.ThreadPoolExecutor(max_workers=2) as pool: + futures = [ + pool.submit(transport.write, {"idx": 1}), + pool.submit(transport.write, {"idx": 2}), + ] + assert [f.result(timeout=2) for f in futures] == [True, True] + + assert len(sent) == 2 + assert max_active_sends == 1 + assert transport._closed is False + finally: + loop.call_soon_threadsafe(loop.stop) + thread.join(timeout=2) + loop.close() + + +def test_ws_transport_preserves_cross_batch_order(): + async def scenario(): + entered = [] + first_entered = asyncio.Event() + release_first = asyncio.Event() + second_started = asyncio.Event() + + class FakeWS: + async def send_text(self, line): + entered.append(line) + if line == "A1": + first_entered.set() + await release_first.wait() + + transport = ws_mod.WSTransport( + FakeWS(), asyncio.get_running_loop(), peer="batch-order-test" + ) + first = asyncio.create_task(transport._safe_send_many(["A1", "A2"])) + await first_entered.wait() + + async def send_second(): + second_started.set() + await transport._safe_send_many(["B1", "B2"]) + + second = asyncio.create_task(send_second()) + await second_started.wait() + + # The second task has reached the transport. Without whole-batch + # serialization it runs B1/B2 before this task can resume. + assert entered == ["A1"] + + release_first.set() + await asyncio.gather(first, second) + assert entered == ["A1", "A2", "B1", "B2"] + + asyncio.run(scenario()) + + +def test_ws_write_async_keeps_drained_tokens_with_current_frame(): + async def scenario(): + entered = [] + first_entered = asyncio.Event() + release_first = asyncio.Event() + current_started = asyncio.Event() + + class FakeWS: + async def send_text(self, line): + entered.append(line) + if line == "A1": + first_entered.set() + await release_first.wait() + + transport = ws_mod.WSTransport( + FakeWS(), asyncio.get_running_loop(), peer="async-order-test" + ) + transport._pending_tokens.append("pending-token") + + first = asyncio.create_task(transport._safe_send_many(["A1", "A2"])) + await first_entered.wait() + + async def send_current(): + current_started.set() + await transport.write_async({"id": "current"}) + + current = asyncio.create_task(send_current()) + await current_started.wait() + later = asyncio.create_task(transport._safe_send_many(["later-batch"])) + + release_first.set() + await asyncio.gather(first, current, later) + assert entered == [ + "A1", + "A2", + "pending-token", + json.dumps({"id": "current"}), + "later-batch", + ] + + asyncio.run(scenario()) diff --git a/tui_gateway/entry.py b/tui_gateway/entry.py index b7bdb9eecbff..bf41c1201816 100644 --- a/tui_gateway/entry.py +++ b/tui_gateway/entry.py @@ -24,20 +24,23 @@ from tui_gateway.transport import TeeTransport logger = logging.getLogger(__name__) -# Handle for the background MCP tool-discovery thread (see main()). The first -# agent build briefly joins this so already-spawning fast servers land before -# the agent snapshots its tool list (see wait_for_mcp_discovery). +# Handle for the background MCP tool-discovery thread (see +# ensure_mcp_discovery_started). The first agent build briefly joins this so +# already-spawning fast servers land before the agent snapshots its tool list +# (see wait_for_mcp_discovery). Stays None when discovery is delegated to the +# shared owner in hermes_cli.mcp_startup — the wait/in-flight/join helpers +# below consult both owners. _mcp_discovery_thread = None -# True once main() decided this TUI process has MCP servers configured and -# spawned discovery through the shared owner. Lets wait_for_mcp_discovery -# re-invoke the (idempotent) spawn on later agent builds so the -# retry-after-zero-connected allowance in -# hermes_cli.mcp_startup.start_background_mcp_discovery can actually fire for -# the stdio TUI — without this, main()'s single spawn is the only call and a -# first run that connected nothing latches the process MCP-less. Kept as a -# flag (rather than re-probing config) so non-MCP sessions never pay the -# tools.mcp_tool import on the per-agent-build wait path. +# True once ensure_mcp_discovery_started decided this process has MCP servers +# configured and spawned discovery through the shared owner. Lets +# wait_for_mcp_discovery re-invoke the (idempotent) spawn on later agent +# builds so the retry-after-zero-connected allowance in +# hermes_cli.mcp_startup.start_background_mcp_discovery can actually fire — +# without this, the single spawn is the only call and a first run that +# connected nothing latches the process MCP-less. Kept as a flag (rather than +# re-probing config) so non-MCP sessions never pay the tools.mcp_tool import +# on the per-agent-build wait path. _mcp_discovery_enabled = False @@ -243,17 +246,18 @@ def wait_for_mcp_discovery(timeout: "float | None" = None) -> None: bound = timeout if timeout is not None else 0.75 thread.join(timeout=bound) return - # The stdio TUI spawns discovery via the shared owner (see main()); wait - # on it so the first agent build still catches fast servers. Re-invoke - # the idempotent spawn first: if the previous run finished with zero - # connected servers, start_background_mcp_discovery's - # retry-after-zero-connected allowance kicks off a fresh discovery run - # here instead of leaving the TUI latched MCP-less for the session. - # Only the stdio TUI (which spawned discovery through the shared owner) - # should delegate to the startup wait here — for every other surface - # (dashboard /api/ws) _make_agent already calls - # hermes_cli.mcp_startup.wait_for_mcp_discovery directly, and delegating - # unconditionally would make that bounded wait run twice per agent build. + # Discovery is spawned via the shared owner (ensure_mcp_discovery_started + # → hermes_cli.mcp_startup); wait on it so the first agent build still + # catches fast servers. Re-invoke the idempotent spawn first: if the + # previous run finished with zero connected servers, + # start_background_mcp_discovery's retry-after-zero-connected allowance + # kicks off a fresh discovery run here instead of leaving the process + # latched MCP-less for the session. In multi-profile processes this + # retry runs under the CALLER's profile context (agent build binds the + # session profile's HERMES_HOME first), so a launch profile with no + # mcp_servers no longer starves selected profiles of discovery (#67605). + # Gated on _mcp_discovery_enabled so non-MCP sessions never pay the + # tools.mcp_tool import on the per-agent-build wait path. if not _mcp_discovery_enabled: return try: @@ -356,48 +360,53 @@ def _has_configured_mcp_servers() -> bool: def ensure_mcp_discovery_started() -> None: - """Start background MCP discovery once for gateway entrypoints. + """Start background MCP discovery for the current profile context, once. - ``main()`` covers the stdio/TUI path. WebSocket/Desktop entrypoints can - accept sessions without running ``main()``, so they must call this helper - before the first agent snapshots its tool list. + ``main()`` calls this for the stdio/TUI path. WebSocket/Desktop + entrypoints can accept sessions without running ``main()``, so the + agent-build path (``server._start_agent_build``) also calls it AFTER + binding the session profile's HERMES_HOME override — the shared owner in + ``hermes_cli.mcp_startup`` captures the caller's context-local override + and propagates it into the discovery thread, so discovery reads the + SELECTED profile's ``mcp_servers``, not the launch profile's (#67605). + + Delegating to the shared owner (instead of a hand-rolled thread) keeps + the process-wide start lock, the retry-after-zero-connected allowance, + and interactive-OAuth suppression. + + Known limitation: MCP tool registration is process-global, so in a + multi-profile process the FIRST profile that builds an agent wins the + discovery slot. Full per-profile MCP registries are tracked in #67605. """ - global _mcp_discovery_thread - - from hermes_constants import get_hermes_home_override, reset_hermes_home_override, set_hermes_home_override - - if _mcp_discovery_thread is not None: - return + global _mcp_discovery_enabled if not _has_configured_mcp_servers(): return + _mcp_discovery_enabled = True + try: + from hermes_cli.mcp_startup import start_background_mcp_discovery - home_override = get_hermes_home_override() - - def _discover_mcp_background() -> None: - token = set_hermes_home_override(home_override) - try: - from tools.mcp_tool import discover_mcp_tools - - discover_mcp_tools() - except Exception: - logger.warning("Background MCP tool discovery failed", exc_info=True) - finally: - reset_hermes_home_override(token) - - import threading as _mcp_threading - - _mcp_discovery_thread = _mcp_threading.Thread( - target=_discover_mcp_background, - name="tui-mcp-discovery", - daemon=True, - ) - _mcp_discovery_thread.start() + start_background_mcp_discovery( + logger=logger, thread_name="tui-mcp-discovery" + ) + except Exception: + logger.warning( + "Background MCP tool discovery failed to start", exc_info=True + ) def main(): _install_sidecar_publisher() + # MCP tool discovery — backgrounded so a slow or unreachable MCP server + # can't freeze TUI startup (a dead stdio/http server burns 1+2+4s of + # connect retries → ~7s of dead air before the composer appears). The + # agent isn't built until the first prompt, at which point _make_agent + # briefly joins the discovery thread (wait_for_mcp_discovery, bounded) so + # already-spawning fast servers land in the tool snapshot. The config + # gate inside ensure_mcp_discovery_started keeps the ~200ms MCP SDK + # import cost entirely off the path for users with no mcp_servers. + ensure_mcp_discovery_started() if not write_json({ "jsonrpc": "2.0",