fix(mcp): reload args on config.yaml changes (#72839)

[webtecnica]

Summary

Fixes #72839: MCP servers cached their startup config in _config on the MCPServerTask but never checked whether the config had changed when register_mcp_servers() was called again. Any edit to config.yaml (e.g. changing --browser firefox to --browser chromium for @playwright/mcp) was silently ignored — the old server process survived all reconnect attempts and ran alongside a duplicate process spawned with the new args.

Root cause

register_mcp_servers() filtered out every server already listed in _servers:

new_servers = {
    k: v for k, v in servers.items()
    if k not in _servers  # ← skipped already-connected servers unconditionally
    and ...
}

Because an already-connected server was never removed from _servers, config edits were invisible until a full process restart. During restart the old orphan subprocess persisted because no PID check discovered it.

Fix

1. Config digest — _compute_config_digest() hashes the config fields that affect the spawned transport/subprocess (command, args, env, url, headers, transport, auth, timeouts, sampling/elicitation configs).

2. Store digest on startup — MCPServerTask.run() stores _config_digest after assigning self._config.

3. Detect and restart — register_mcp_servers() now compares each already-connected server's stored digest against the current config digest. When they diverge:

   • Removes the old server from _servers
   • Signals shutdown on its transport task (fire-and-forget; orphan subprocesses are reaped by _run_stdio's existing _kill_orphaned_mcp_children())
   • Clears connect-failure state so the new attempt isn't blocked by a stale cooldown
   • Deregisters stale tool entries
   • Falls through to the normal new_servers connection path, which picks up the fresh config

Also fixes a related gap: register_mcp_servers() now checks _server_connecting to prevent duplicate spawns from concurrent calls (regression #72818).

Testing

• Existing MCP tests continue to pass
• The digest comparison gracefully handles legacy servers that lack _config_digest (returns None → comparison skipped)
• Orphan reaping in _run_stdio covers the case where the old subprocess is still alive when the new one spawns

Related

• MCP reconnect path spawns servers with a bare command, ignoring configured absolute path + env.PATH #72754 (MCP reconnect path) — this bug is a companion: reconnect worked but ignored config changes
• Concurrent MCP registration spawns duplicate stdio server processes and leaks PIPE FDs #72818 (concurrent _server_connecting race) — fixed in this PR

[teknium1]

Thanks for isolating the concurrent registration race. The narrow _server_connecting guard is still needed on current main: tools/mcp_tool.py:5926 selects servers based only on _servers, then claims them at :5946; discover_mcp_tools() has the same omission at :6087.

Problems

• The config-digest restart path duplicates existing behavior. cli.py:11178-11231 detects changed mcp_servers and launches _reload_mcp; cli.py:11447-11450 shuts down all MCP connections and rediscoveries from fresh config.
• The new direct srv._task.cancel() bypasses the loop-owned shutdown path. Current shutdown_mcp_servers() schedules server.shutdown() on the MCP loop (tools/mcp_tool.py:6510-6564), matching the dedicated task lifecycle documented at tools/mcp_tool.py:1821-1826.
• The added test only covers the concurrent-connecting guard; it does not exercise the claimed config-change restart or child-process cleanup.
• The agent-runtime hook and Himalaya documentation changes are unrelated to this MCP fix.

Suggested changes

• Salvage the two _server_connecting exclusions plus their regression test as the focused fix for the confirmed race.
• Split the unrelated commits and omit the redundant per-server config-restart path.

Automated hermes-sweeper review.

[GottZ]

This was generated by AI during triage.

Summary

One PR addresses issue #72839. #72864 adds config-digest-based MCP replacement and concurrent-registration guards, but the diff does not establish safe cleanup of stale subprocesses and does not address the reported hermes config set list-serialization defect.

Related pull requests

• fix(mcp): reload args on config.yaml changes (#72839) #72864 best fix — (+212/-143) — best available partial fix; keep open with a focused salvage path: The _server_connecting exclusions in both registration paths, together with their regression test, prevent concurrent duplicate starts. Consistent with the keep_open review on fix(mcp): reload args on config.yaml changes (#72839) #72864, the digest-restart path duplicates the existing CLI reload, directly cancels the task outside the loop-owned shutdown path, lacks config-change and child-process-cleanup coverage, and is bundled with unrelated tool-hook and Himalaya documentation changes.

Suggested consolidation

Keep #72864 open with a salvage path: retain the two _server_connecting guards and their focused regression test, while removing the redundant digest-restart implementation and splitting out the unrelated agent-runtime hook and Himalaya documentation changes. It is the only PR in this complex, so there are no duplicates to close; stale-process cleanup and hermes config set list serialization remain unresolved parts of #72839.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I72839(["issue #72839 (open)"])
    P72864["PR #72864 (open)"]
    P72864 -->|best fix| I72839
    class I72839 open
    class P72864 open
    class P72864 best
    class P72864 target
    click I72839 "https://github.com/NousResearch/hermes-agent/issues/72839"
    click P72864 "https://github.com/NousResearch/hermes-agent/pull/72864"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 22 kB of PR diffs, 3 kB of issue/PR text, 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.