diff --git a/CHANGELOG.md b/CHANGELOG.md index 478a3cfe..6b703d36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Hermes Web UI -- Changelog +## [Unreleased] + +### Fixed + +- **bug(profile/mcp): non-default profile MCP servers never load** ([#1968](https://github.com/nesquena/hermes-webui/issues/1968)). `_run_agent_streaming` called `discover_mcp_tools()` ~100 lines BEFORE the per-session `os.environ['HERMES_HOME'] = _profile_home` mutation, so MCP discovery always read the default profile's `~/.hermes/config.yaml` regardless of which profile the session was stamped with. Result: switching profiles in the WebUI dropdown was effectively cosmetic for MCP — non-default profiles never registered their stdio (npx/node) MCP servers. Fix relocates the `discover_mcp_tools()` call past the `_ENV_LOCK` env-mutation block so `get_hermes_home()` resolves to the session's actual profile home. Adds 4 static regression tests (`tests/test_issue1968_mcp_profile_discovery.py`) pinning the call ordering, lock-release placement, single call site, and try/except wrapping. **Caveat (out of scope, agent-side):** `_servers` in `tools/mcp_tool.py` is a process-global dict keyed only by server name, so concurrent use of multiple non-default profiles in the same WebUI process still has a "first profile wins per name" issue. Fully fixing that requires keying `_servers` by `(profile_home, name)` upstream in hermes-agent. This PR ships layer 1 only. + ## [v0.51.31] — 2026-05-09 — Release H (12-PR contributor batch: image-mode + race fixes + composer drafts + locale parity) ### Added diff --git a/api/streaming.py b/api/streaming.py index 5fcd1ae9..a1a8ac1b 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -1919,15 +1919,10 @@ def _run_agent_streaming( old_hermes_home = None old_profile_env = {} - # ── MCP Server Discovery (lazy import, idempotent) ── - # discover_mcp_tools() is called here (rather than at server startup) so that - # the hermes-agent package is fully initialized before we try to connect. - # It is safe to call multiple times — already-connected servers are skipped. - try: - from tools.mcp_tool import discover_mcp_tools - discover_mcp_tools() - except Exception: - pass # MCP not available or not configured — non-fatal + # MCP discovery moved to AFTER the per-profile HERMES_HOME mutation below + # (was here at v0.51.30) — the previous placement always read the default + # profile's mcp_servers because os.environ['HERMES_HOME'] hadn't been + # rewritten yet. See https://github.com/nesquena/hermes-webui/issues/1968. # Sprint 10: create a cancel event for this stream cancel_event = threading.Event() @@ -2052,6 +2047,27 @@ def _run_agent_streaming( if _profile_home: os.environ['HERMES_HOME'] = _profile_home # Lock released — agent runs without holding it + # ── MCP Server Discovery (lazy import, idempotent) ── + # MUST run AFTER the HERMES_HOME mutation above — `discover_mcp_tools()` + # reads `~/.hermes/config.yaml` via `get_hermes_home()`, which uses + # `os.environ['HERMES_HOME']`. Calling it before the mutation always + # loaded the default profile's `mcp_servers`, even when the session + # was stamped with a non-default profile. See issue #1968. + # + # NOTE: `_servers` in `tools/mcp_tool.py` is a process-global registry + # keyed by server name. This means once profile A registers a server + # named e.g. `postgres`, profile B's discovery sees it as already + # connected and skips it — even if B's config points at a different + # binary. Fully fixing multi-profile concurrent use requires keying + # `_servers` by `(profile_home, name)` upstream in hermes-agent; that + # lives outside this WebUI repo. This change fixes the headline bug + # for users who run a single non-default profile per WebUI process. + try: + from tools.mcp_tool import discover_mcp_tools + discover_mcp_tools() + except Exception: + pass # MCP not available or not configured — non-fatal + # Register a gateway-style notify callback so the approval system can # push the `approval` SSE event the moment a dangerous command is # detected, without waiting for the next on_tool() poll cycle. diff --git a/tests/test_issue1968_mcp_profile_discovery.py b/tests/test_issue1968_mcp_profile_discovery.py new file mode 100644 index 00000000..999d3f49 --- /dev/null +++ b/tests/test_issue1968_mcp_profile_discovery.py @@ -0,0 +1,107 @@ +"""Regression test for issue #1968 — non-default profile MCP servers never load. + +The bug: `discover_mcp_tools()` was called at the top of `_run_agent_streaming` +before the `HERMES_HOME` env mutation that stamps the per-session profile. +Result: `_load_mcp_config()` always read the default profile's +`~/.hermes/config.yaml`, never the non-default profile's MCP servers. + +The fix moves the call past the `_ENV_LOCK` env-mutation block so +`discover_mcp_tools()` runs with the correct `HERMES_HOME` for the session's +profile. + +This is a static check (source ordering) rather than a runtime test, because +mocking the entire agent stack to reach the call site would be brittle and +miss the actual lexical ordering that's the load-bearing fix. +""" +from pathlib import Path +import re + +ROOT = Path(__file__).resolve().parents[1] +STREAMING_PY = (ROOT / "api" / "streaming.py").read_text(encoding="utf-8") + + +def _line_of(pattern: str) -> int: + """Return the 1-indexed line number of the first match for `pattern`.""" + for idx, line in enumerate(STREAMING_PY.splitlines(), start=1): + if re.search(pattern, line): + return idx + raise AssertionError(f"pattern not found in api/streaming.py: {pattern!r}") + + +def test_discover_mcp_tools_called_after_hermes_home_mutation(): + """The fix for #1968: `discover_mcp_tools()` must execute AFTER the + `HERMES_HOME = _profile_home` assignment, otherwise non-default profile + MCP servers are never discovered. + """ + home_set_line = _line_of(r"os\.environ\['HERMES_HOME'\]\s*=\s*_profile_home") + discover_call_line = _line_of(r"discover_mcp_tools\(\)\s*$") + assert discover_call_line > home_set_line, ( + f"discover_mcp_tools() at line {discover_call_line} must be AFTER the " + f"HERMES_HOME mutation at line {home_set_line} (issue #1968). " + "Otherwise non-default profile MCP servers never load." + ) + + +def test_discover_mcp_tools_called_after_env_lock_release(): + """`discover_mcp_tools()` should run AFTER the `_ENV_LOCK` block releases — + discovery itself can take up to 120s (per `_run_on_mcp_loop` timeout in + hermes-agent), and holding the env lock across that would serialize all + concurrent sessions through MCP discovery. + + Lexical check: the discover call must come after the `# Lock released` marker + that follows the `with _ENV_LOCK:` block. + """ + lock_release_marker = _line_of(r"# Lock released — agent runs without holding it") + discover_call_line = _line_of(r"discover_mcp_tools\(\)\s*$") + assert discover_call_line > lock_release_marker, ( + f"discover_mcp_tools() at line {discover_call_line} should run AFTER " + f"the _ENV_LOCK release at line {lock_release_marker}, not inside the " + "lock block (which would serialize MCP discovery across sessions)." + ) + + +def test_discover_mcp_tools_only_called_once_in_streaming(): + """Sanity check: only one *actual call* to `discover_mcp_tools()` in + `api/streaming.py` — not counting prose mentions inside comments. + + The fix relocates the existing call rather than adding a second one. If a + later refactor reintroduces a pre-mutation call site, this test catches it. + """ + call_lines = [ + line for line in STREAMING_PY.splitlines() + if "discover_mcp_tools()" in line + and not line.lstrip().startswith("#") + ] + assert len(call_lines) == 1, ( + f"Expected exactly 1 `discover_mcp_tools()` call line in api/streaming.py " + f"(comments excluded), found {len(call_lines)}: {call_lines!r}. A " + "duplicate call site would re-introduce the #1968 bug if placed before " + "the HERMES_HOME mutation." + ) + + +def test_discover_mcp_tools_call_is_inside_try_except(): + """MCP discovery is best-effort — failures must not crash the chat stream. + Verify the call site is wrapped in `try: ... except Exception: pass`. + + Looks at the 6 lines immediately surrounding the call (which is the actual + structural block, regardless of how chatty the preceding comment is). + """ + lines = STREAMING_PY.splitlines() + call_idx = None + for idx, line in enumerate(lines): + if "discover_mcp_tools()" in line and not line.lstrip().startswith("#"): + call_idx = idx + break + assert call_idx is not None, "discover_mcp_tools() call line not found" + # Look at the 4 lines before and 4 lines after the call. + block_start = max(0, call_idx - 4) + block_end = min(len(lines), call_idx + 5) + block = "\n".join(lines[block_start:block_end]) + assert "try:" in block, ( + f"discover_mcp_tools() at line {call_idx + 1} must be inside a try block " + "so MCP failures don't crash the chat stream. Surrounding code:\n" + block + ) + assert "except" in block, ( + f"discover_mcp_tools() at line {call_idx + 1} must have an except clause." + )