fix(profile/mcp): discover MCP tools after per-session HERMES_HOME mutation (#1968)

[nesquena-hermes]

Summary

Fixes the headline symptom of #1968 — non-default profile MCP servers never load in the WebUI. Switching profiles in the dropdown was effectively cosmetic for MCP — every session, regardless of profile, only saw the default profile's mcp_servers.

Reporter @tatleoat's diagnosis was correct: the WebUI backend reads MCP config based on HERMES_HOME, and the per-session profile env switch happened AFTER MCP discovery had already run.

Root cause

api/streaming.py:1922 called discover_mcp_tools() at the TOP of _run_agent_streaming(), about 100 lines BEFORE the per-session env mutation block:

# Line 1922 (BEFORE — broken):
try:
    from tools.mcp_tool import discover_mcp_tools
    discover_mcp_tools()                            # ← reads default HERMES_HOME
except Exception:
    pass
...
# Line 2053:
with _ENV_LOCK:
    ...
    if _profile_home:
        os.environ['HERMES_HOME'] = _profile_home   # ← too late

discover_mcp_tools() → _load_mcp_config() → hermes_cli.config.load_config() → get_config_path() → get_hermes_home() → reads os.environ['HERMES_HOME']. So at the call site on line 1928, HERMES_HOME was always whatever the WebUI server process had at startup (the default profile), regardless of what the session was stamped with.

Fix

Relocate the discover_mcp_tools() call past the _ENV_LOCK env-mutation block. Same try/except wrapping, same idempotency, same lazy import — only the call site changes:

        with _ENV_LOCK:
            ...
            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 — see issue #1968.
        try:
            from tools.mcp_tool import discover_mcp_tools
            discover_mcp_tools()
        except Exception:
            pass  # MCP not available or not configured — non-fatal

I deliberately kept the call OUTSIDE the lock rather than inside — discover_mcp_tools() can take up to 120s in the worst case (parallel server connection with timeouts), and holding _ENV_LOCK across that would serialize all concurrent sessions through MCP discovery. That's the same call-after-release pattern the rest of the env-dependent code already uses (the documented race in #195).

Caveat — out of scope (agent-side)

This PR ships layer 1 only. There's a second-order issue that lives in hermes-agent, not the WebUI:

_servers in tools/mcp_tool.py:1607 is a process-global Dict[str, MCPServerTask] keyed only by server name. Once profile A registers a server named postgres, profile B's discovery sees 'postgres' in _servers and skips it — even if B's postgres config points at a different binary, env, or DB.

# tools/mcp_tool.py:3065-3068 (hermes-agent, READ-ONLY here):
new_servers = {
    k: v
    for k, v in servers.items()
    if k not in _servers and _parse_boolish(v.get("enabled", True), default=True)
}

Fully fixing concurrent multi-profile use would require keying _servers by (profile_home, name) upstream in hermes-agent and threading a profile-home arg through discover_mcp_tools(). That's an upstream change.

Practical impact of layer 1 only: users running a single non-default profile per WebUI process — the exact case @tatleoat reported — get a working fix today. Users actively switching between profiles in the same process will still hit "first profile wins per server name" for MCP. I'll file the upstream issue.

The dotenv@17.3.1 banner stdout-corruption that @tatleoat also flagged is a separate npm/dotenv interaction with no clean WebUI fix — covered in my comment on the issue.

Tests

New file: tests/test_issue1968_mcp_profile_discovery.py (4 static tests):

1. test_discover_mcp_tools_called_after_hermes_home_mutation — pins the load-bearing invariant: the call line number must be greater than the os.environ['HERMES_HOME'] = _profile_home line number.
2. test_discover_mcp_tools_called_after_env_lock_release — confirms the call sits AFTER the # Lock released marker, not inside the lock block.
3. test_discover_mcp_tools_only_called_once_in_streaming — guards against a future refactor accidentally adding back a pre-mutation call site.
4. test_discover_mcp_tools_call_is_inside_try_except — preserves the best-effort failure semantics.

Mutation-safety verified: I temporarily reverted the fix (re-added a discover_mcp_tools() call before the HERMES_HOME mutation) and confirmed test 3 fails with the right error. Test 1 also catches the original bug shape.

Verification

• node -c and python -m py_compile clean
• Focused tests (MCP + streaming + profile): 108 passed in 7.90s
• Full suite: 5047 passed, 4 skipped, 3 xpassed, 0 regressions in 146.53s on Python 3.11
• Server starts cleanly with the patched build (HERMES_HOME=/tmp/hermes-test-home-1968 test profile, /health returns 200)

Files changed

File	Change	LOC
api/streaming.py	Relocate discover_mcp_tools() past env mutation; replace original call with explanatory comment block	+20 / -9
tests/test_issue1968_mcp_profile_discovery.py	4 static regression tests	+109
CHANGELOG.md	[Unreleased] entry with caveat about agent-side layer-2 work	+6

Closes #1968

[nesquena]

Review — end-to-end ✅ (clean approve; layer-1 fix is correct, agent-side layer-2 caveat is honest and correctly bounded)

What this ships

Surgical relocation of discover_mcp_tools() in api/streaming.py from the top of _run_agent_streaming (line 1922, BEFORE the per-session env mutation) to immediately after the _ENV_LOCK env-mutation block at line 2049. Same try/except wrapping, same lazy-import pattern, same idempotency. +25 / -9 LOC in streaming.py + 107 LOC in 4 new static regression tests + CHANGELOG entry.

Closes #1968 (the headline symptom — non-default profile MCP servers never load in single-non-default-profile WebUI processes, the exact case @tatleoat reported).

Cross-tool trace (verified against fresh hermes-agent tarball)

The chain is:

discover_mcp_tools() (tools/mcp_tool.py:3131)
→ _load_mcp_config() (tools/mcp_tool.py:2105)
→ hermes_cli.config.load_config()
→ get_config_path() → get_hermes_home() / "config.yaml"
→ get_hermes_home() (hermes_constants.py:30)
→ os.environ.get("HERMES_HOME", "")

So _load_mcp_config() reads ~/<HERMES_HOME>/config.yaml at the moment discover_mcp_tools() is called. Pre-fix, that was always the WebUI server process's startup HERMES_HOME (the default profile). The session's os.environ['HERMES_HOME'] = _profile_home mutation at api/streaming.py:2048 ran ~125 lines AFTER discovery — too late. The fix moves discovery past that mutation. ✓

Notable: get_hermes_home() upstream has its own one-shot warning to errors.log for the case where HERMES_HOME is unset but an active_profile file indicates a non-default profile is sticky (per the #18594 commentary). The WebUI explicitly avoids this warning path by always setting HERMES_HOME via the env-mutation block. ✓

End-to-end trace

Pre-fix flow (api/streaming.py:1922-1930 at v0.51.30):

1. _run_agent_streaming enters; discover_mcp_tools() called.
2. _load_mcp_config() reads os.environ['HERMES_HOME'] → process startup default (e.g. ~/.hermes).
3. Default profile's mcp_servers registered.
4. ~125 lines later: os.environ['HERMES_HOME'] = _profile_home for non-default profile.
5. Agent runs under non-default profile env. But MCP servers were already registered under default, and _servers global skips re-registration.
6. Result: switching profiles in dropdown is cosmetic for MCP. ❌

Post-fix flow (api/streaming.py:2050-2069):

1. _run_agent_streaming enters.
2. _ENV_LOCK acquired; os.environ['HERMES_HOME'] = _profile_home set.
3. Lock released.
4. discover_mcp_tools() called — _load_mcp_config() reads correct HERMES_HOME for the session's profile.
5. Non-default profile's mcp_servers registered. ✓

What I caught — none. The fix is correct and the caveat is honest

I traced this carefully looking for whether the relocation introduces or worsens any race. It does not. The relocation places the call where the agent itself already runs (post-lock-release), and the only race I can construct is the pre-existing _ENV_LOCK TOCTOU pattern inherited from the existing architecture: thread A sets HERMES_HOME=A, releases lock, thread B sets HERMES_HOME=B, thread A's discover_mcp_tools() reads B's HERMES_HOME mid-flight. This race already affects the agent itself (which also reads os.environ['HERMES_HOME'] post-lock-release for SOUL.md, context files, and provider resolution). The PR doesn't introduce or worsen it. Holding the lock through MCP discovery would serialize all concurrent sessions for up to 120s — explicitly rejected for valid reason in the PR body.

The agent-side _servers global registry caveat the PR documents is the real architectural limit:

# tools/mcp_tool.py:3131 (hermes-agent, READ-ONLY here):
new_servers = {
    k: v for k, v in servers.items()
    if k not in _servers and _parse_boolish(v.get("enabled", True), default=True)
}

_servers: Dict[str, MCPServerTask] is keyed by server name, not by (profile_home, name). So once profile A registers postgres, profile B's discovery sees 'postgres' in _servers and skips it — even if B's postgres config points at a different binary, env, or DB. Fully fixing concurrent multi-profile use requires keying _servers by (profile_home, name) upstream in hermes-agent and threading a profile-home arg through discover_mcp_tools().

The PR caveat captures this exactly. Practical impact:

• ✅ Single non-default profile per WebUI process (the @tatleoat repro): works.
• ⚠️ Concurrent multi-profile in same WebUI process: still "first profile wins per server name" — needs upstream agent change.
• ✅ Multi-process WebUI: each process has its own _servers, so no inter-process bleed.

Documented honestly in both the PR body and the inline comment at api/streaming.py:2057-2064.

Other audit — things that are correct already

• ✅ Lock-scope reasoning is sound. discover_mcp_tools() outside the lock matches the rest of the post-mutation code (the agent itself also runs without holding _ENV_LOCK). The lock only protects the env WRITE, not subsequent READS.
• ✅ Try/except wrapping preserved. MCP failures stay non-fatal — chat stream continues even if discovery times out or hits an import error.
• ✅ Lazy import preserved. from tools.mcp_tool import discover_mcp_tools stays inside the try block, so tools.mcp_tool import failures (missing dep, etc.) are also non-fatal.
• ✅ Idempotency unchanged. Same _servers in _servers guard upstream — repeated calls within the same profile are no-ops.
• ✅ Comment at the OLD call site explains why nothing was done there (relocated; references the issue). Future readers won't be confused by the absence.
• ✅ Inline comment at the new call site explains BOTH the layer-1 fix AND the layer-2 caveat. Honest documentation of architectural limit.
• ✅ Subprocess env-snapshot semantics: when MCP servers are spawned via npx, they capture os.environ at fork/exec time. So a subprocess spawned with HERMES_HOME=A keeps HERMES_HOME=A in its env regardless of what the parent does later. The race I identified above is bounded to the parent's _load_mcp_config config-read step, not the subprocess env. ✓
• ✅ Test mutation-safety verified per PR body: author temporarily reverted the fix and confirmed test_discover_mcp_tools_only_called_once_in_streaming and the ordering test fail with the right error.
• ✅ Static tests are appropriate scope. The bug is purely a lexical-ordering issue; runtime tests would require mocking the entire agent stack (_AIAgent, _ENV_LOCK, _set_thread_env, etc.) which is brittle and would obscure the actual invariant. The 4 tests cover ordering + post-lock placement + single call site + try/except wrapping.
• ✅ Regex precision in tests: r"discover_mcp_tools\(\)\s*$" matches the call line discover_mcp_tools() and correctly excludes the import line from tools.mcp_tool import discover_mcp_tools (no trailing ()).
• ✅ Test 3 sentinel: if "discover_mcp_tools()" in line and not line.lstrip().startswith("#") correctly filters out the explanatory comment block at the old call site that now contains the prose mention "discover_mcp_tools".

Edge-case trace

Scenario	Expected	Actual
Single non-default profile, fresh session	MCP servers register under non-default profile	✅
Default profile session	MCP servers register under default	✅
Profile switch in dropdown, new session, single profile in process	new profile's MCP servers register	✅
Profile A session → profile B session in same process (sequential)	A's servers stick, B's blocked by _servers keyed-by-name	⚠️ caveat-documented (upstream fix needed)
Concurrent A and B sessions	TOCTOU race on env; pre-existing for agent too	⚠️ same as agent itself
Multi-process WebUI (e.g., per-profile process)	each process has own _servers, no bleed	✅
tools.mcp_tool import fails (dep missing)	try/except, non-fatal	✅
Discovery timeout (120s on connect)	non-fatal, agent still runs	✅
Subprocess MCP server spawned via npx	inherits parent env at fork; HERMES_HOME pinned at spawn time	✅
Cross-tool: agent CLI doesn't use this code path	n/a (CLI runs outside WebUI streaming)	✅
dotenv@17.3.1 banner stdout corruption (reporter's secondary symptom)	out of scope, separate npm/dotenv issue	✅ explicitly deferred

Tests

• tests/test_issue1968_mcp_profile_discovery.py: 4/4 pass on the PR branch.
• MCP/streaming/profile subset (pytest -k "mcp or streaming or profile"): 399 passed, 6 skipped, 3 xpassed, 0 failed in 5.05s.
• Per PR body: full suite 5047 passed, 4 skipped, 3 xpassed, 0 regressions.

Minor observations (non-blocking)

1. Layer-2 caveat is real and worth filing upstream now, not after multi-profile users complain. The keying change in _servers is local to tools/mcp_tool.py and threading a profile_home arg through discover_mcp_tools() is mechanical. If the agent maintainers accept the patch, the WebUI side is essentially zero-change after.
2. The race I identified (parent's _load_mcp_config reading os.environ['HERMES_HOME'] after the lock release) is theoretical and bounded — even if hit, the symptom is "wrong profile's servers register for one session" rather than crash or data corruption. The subsequent attempt under the correct profile would be skipped by _servers keyed-by-name, but that's the same caveat as layer 2.
3. _set_thread_env at api/streaming.py:1957 sets HERMES_HOME in a thread-local dict, but tools/mcp_tool.py's config-load path uses os.environ directly via get_hermes_home(). Threading the thread-local through to the agent's _load_mcp_config would be the cleanest layer-2 solution but is upstream scope.
4. The comment block at the old call site (5 lines explaining the relocation + issue link) is mildly defensive but useful — future contributors searching for discover_mcp_tools in streaming.py will land on the comment first and follow the issue link.
5. api/streaming.py:1928 comment marker for the new call site uses both an issue reference AND the architectural caveat, making it self-explanatory without needing PR archaeology.

Recommendation

✅ Approved. Surgical relocation that fixes the headline symptom (single non-default profile MCP servers never loading) with correct scope. Cross-tool trace verified against fresh hermes-agent tarball — discover_mcp_tools → _load_mcp_config → load_config → get_config_path → get_hermes_home → os.environ['HERMES_HOME']. The race I traced (post-lock-release TOCTOU on env) is pre-existing in the architecture and affects the agent itself the same way; the PR does not introduce or worsen it. The agent-side layer-2 caveat (_servers keyed by name) is honestly documented in both the PR body and the inline comment, with a clear escalation path (key by (profile_home, name) upstream).

Tests are static-shape but appropriate — the bug IS a lexical-ordering issue, and runtime tests would require mocking the full agent stack to reach the call site. The 4 tests pin all four invariants worth pinning: post-mutation ordering, post-lock-release placement, single call site, try/except wrapping. Pre-fix mutation-safety verified by author.

The dotenv@17.3.1 stdout-corruption issue the reporter also flagged is correctly deferred as a separate concern (npm/dotenv interaction, not solvable cleanly at the WebUI layer).

Parked at approval — ready for the release agent's merge/tag pipeline.

[nesquena-hermes]

Opus advisor — VERDICT: SHIP-AS-IS

Ran Opus as the second-set-of-eyes per project policy on this self-built PR (nesquena APPROVED + headless cross-tool trace was the first set). Verbatim verdict:

SHIP-AS-IS.

The fix is a textbook surgical relocation. nesquena's approval is correct on all load-bearing claims (lock-scope, idempotency, test invariants); the one technical inaccuracy (subprocess env mechanism) doesn't change the conclusion. The agent-side _servers keying limitation is a real and stricter bound on the fix's effective scope than the PR body suggests, but the inline comment captures it honestly. Tests are static-shape and that is the appropriate scope for a lexical-ordering bug. CI green on 3.11/3.12/3.13. Merge cleanly; track the upstream layer-2 issue separately.

What Opus verified beyond nesquena's review

• Lock-scope reasoning — TOCTOU race exists but is pre-existing (affects the agent itself the same way). Holding lock through discovery would serialize sessions for up to 120s — correctly rejected.
• Subprocess env capture — caught a technical inaccuracy in nesquena's review (StdioServerParameters(env=safe_env) REPLACES the child env via _build_safe_env filter, not "captures os.environ at fork/exec"). HERMES_HOME is not in _SAFE_ENV_KEYS, so subprocesses don't see it unless explicitly forwarded via mcp_servers.<name>.env. The bounded-to-config-read conclusion still holds because _interpolate_env_vars reads os.environ at config-load time post-fix.
• First-session caveat (stricter framing) — the PR's "single non-default profile per WebUI process: works" framing is mildly optimistic. The first session must be on the non-default profile, OR the default profile must have no same-named MCP servers. If the user opens on default first, runs anything, then switches to a non-default profile with same server names, the non-default's servers are blocked by _servers keyed-by-name.
• Test invariants — confirmed all 4 are the right ones, with mild non-blocking fragility (anchor on comment string, regex end-of-line match) that's acceptable given the bug is lexical-ordering.

Pre-merge state

• mergeStateStatus: CLEAN, MERGEABLE
• CI: 3.11 / 3.12 / 3.13 all SUCCESS
• Local pytest: 5043 passed, 0 failed (152.67s) — +1 net new test
• nesquena APPROVED with full cross-tool trace
• Opus APPROVED with stricter framing on agent-side caveat

Follow-up (non-blocking, will file post-merge)

Per Opus: file an upstream hermes-agent issue for keying _servers: Dict[str, MCPServerTask] by (profile_home, name) rather than just name, and threading a profile_home arg through discover_mcp_tools(). Without that, the layer-2 hole the PR documents stays open. The keying change is local to tools/mcp_tool.py; mechanical to thread the arg through.

Merging.