fix(profile/mcp): discover MCP tools after per-session HERMES_HOME mutation
Issue #1968: switching to a non-default profile in the WebUI dropdown had no effect on which MCP servers were available. Every chat session, regardless of profile, only saw the default profile's mcp_servers from ~/.hermes/config.yaml. Non-default profile MCP servers (postgres, custom stdio servers, anything in <profile>/config.yaml) never registered. Root cause: api/streaming.py:1922 called discover_mcp_tools() at the TOP of _run_agent_streaming(), about 100 lines BEFORE the per-session 'os.environ["HERMES_HOME"] = _profile_home' mutation at line 2053. discover_mcp_tools() reads ~/.hermes/config.yaml via get_hermes_home(), which uses os.environ['HERMES_HOME']. So at the call site, HERMES_HOME was still whatever the WebUI server process had at startup — the default profile, every time. Fix: relocate the discover_mcp_tools() call past the _ENV_LOCK block so get_hermes_home() resolves to the session's actual profile home. Same try/except wrapping is preserved; same idempotency semantics on already-connected servers; same lazy-import pattern. Caveat (out of scope, agent-side): _servers in tools/mcp_tool.py is a process-global Dict[str, MCPServerTask] keyed only by server name. So once profile A registers a server named e.g. 'postgres', profile B's discovery sees 'postgres' as already connected and skips it — even if B's config points at a different binary or DB. Concurrent multi-profile WebUI processes will still hit 'first profile wins per server name'. Fully fixing that requires keying _servers by (profile_home, name) upstream in hermes-agent. This PR ships layer 1 only — fixes the single-non-default-profile case (the headline symptom). Tests: tests/test_issue1968_mcp_profile_discovery.py — 4 static tests pinning the lexical ordering invariants. Verified mutation-safety: a proof-of-concept revert (re-adding a discover call before the HERMES_HOME mutation) makes the 'only called once' test fail. Test suite: 5047 passed, 4 skipped, 3 xpassed, 0 regressions. Closes #1968
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
107
tests/test_issue1968_mcp_profile_discovery.py
Normal file
107
tests/test_issue1968_mcp_profile_discovery.py
Normal file
@@ -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."
|
||||
)
|
||||
Reference in New Issue
Block a user