mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(mcp): move auto-reload opt-out to top-level mcp: section + regression tests
Follow-up on the salvaged #67449: auxiliary.mcp is the side-LLM task provider block (provider/model/timeout for MCP aux calls) — a watcher behavior toggle doesn't belong there. Move it to a new top-level mcp: runtime section and read it from the same freshly-parsed config.yaml the watcher already diffs (no second load_config() per tick, and flipping the toggle + editing mcp_servers in one edit behaves correctly). Also adds a regression test for the salvaged #55701 false-positive fix: ${VAR} templates in mcp_servers made the raw-yaml-vs-expanded-snapshot comparison permanently unequal, so ANY save_config_value() rewrite (e.g. /reasoning changing agent.reasoning_effort) fired a full MCP reconnect. Credits: @OYLFLMH (#55701 env-expand fix), @TurgutKural (#67449 opt-out).
This commit is contained in:
Teknium
parent
1abcccdeba
commit
60092f728c
3 changed files with 115 additions and 50 deletions
22
cli.py
22
cli.py
|
|
@ -10062,18 +10062,16 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
|||
# next message re-sends the full input prefix, which is expensive on
|
||||
# long-context / high-reasoning models).
|
||||
#
|
||||
# The toggle lives under ``auxiliary.mcp.auto_reload_on_config_change``
|
||||
# in DEFAULT_CONFIG (same section as the MCP aux-task provider
|
||||
# settings), so resolve it through that path, not a top-level ``mcp``
|
||||
# key that does not exist in the loaded config shape.
|
||||
try:
|
||||
from hermes_cli.config import load_config as _load_cfg
|
||||
_cfg = _load_cfg()
|
||||
_aux = _cfg.get("auxiliary") if isinstance(_cfg, dict) else None
|
||||
_mcp = _aux.get("mcp") if isinstance(_aux, dict) else None
|
||||
_auto = _mcp.get("auto_reload_on_config_change", True) if isinstance(_mcp, dict) else True
|
||||
except Exception:
|
||||
_auto = True
|
||||
# The toggle is the top-level ``mcp.auto_reload_on_config_change``
|
||||
# key (see DEFAULT_CONFIG). Read it from the config we just parsed
|
||||
# so the user can flip it in the same edit that changes mcp_servers;
|
||||
# missing key means default-on.
|
||||
_mcp_cfg = new_cfg.get("mcp")
|
||||
_auto = (
|
||||
_mcp_cfg.get("auto_reload_on_config_change", True)
|
||||
if isinstance(_mcp_cfg, dict)
|
||||
else True
|
||||
)
|
||||
|
||||
self._config_mcp_servers = new_mcp
|
||||
|
||||
|
|
|
|||
|
|
@ -1409,6 +1409,20 @@ DEFAULT_CONFIG = {
|
|||
# small so a slow/dead server adds little to first-response latency.
|
||||
"mcp_discovery_timeout": 1.5,
|
||||
|
||||
# MCP runtime behavior (distinct from the per-server definitions in
|
||||
# mcp_servers: and from the auxiliary.mcp side-LLM task settings).
|
||||
"mcp": {
|
||||
# Auto-reload MCP connections when config.yaml's mcp_servers section
|
||||
# changes at runtime (CLI file watcher, default on).
|
||||
# Set to false to stop the automatic reload: every automatic reload
|
||||
# rebuilds the agent tool surface and INVALIDATES the provider
|
||||
# prompt cache (the next message re-sends the full input prefix),
|
||||
# which is expensive on long-context / high-reasoning models.
|
||||
# When disabled, the watcher still detects the change and prints
|
||||
# guidance to apply it deliberately via /reload-mcp.
|
||||
"auto_reload_on_config_change": True,
|
||||
},
|
||||
|
||||
# Tool-output truncation thresholds. When terminal output or a
|
||||
# single read_file page exceeds these limits, Hermes truncates the
|
||||
# payload sent to the model (keeping head + tail for terminal,
|
||||
|
|
@ -1670,14 +1684,6 @@ DEFAULT_CONFIG = {
|
|||
"timeout": 30,
|
||||
"extra_body": {},
|
||||
"reasoning_effort": "", # per-task thinking level: none|minimal|low|medium|high|xhigh|max|ultra (empty = provider default)
|
||||
# Auto-reload MCP connections when config.yaml's mcp_servers section
|
||||
# changes at runtime (default on, matches pre-#1474 behaviour).
|
||||
# Set to False to stop the automatic reload: every automatic reload
|
||||
# rebuilds the agent tool surface and INVALIDATES the provider
|
||||
# prompt cache (the next message re-sends the full input prefix),
|
||||
# which is expensive on long-context / high-reasoning models.
|
||||
# MCP servers can still be reloaded manually via /reload-mcp.
|
||||
"auto_reload_on_config_change": True,
|
||||
},
|
||||
"title_generation": {
|
||||
"enabled": True,
|
||||
|
|
|
|||
|
|
@ -106,17 +106,17 @@ class TestMCPConfigWatch:
|
|||
obj._reload_mcp.assert_not_called()
|
||||
|
||||
def test_optout_disables_auto_reload(self, tmp_path, capsys):
|
||||
"""When auxiliary.mcp.auto_reload_on_config_change is False, a changed
|
||||
"""When mcp.auto_reload_on_config_change is False, a changed
|
||||
mcp_servers section must NOT trigger an automatic reload — but the
|
||||
change is still detected and the user is told how to apply it.
|
||||
|
||||
This protects the provider prompt cache: every automatic reload
|
||||
rebuilds the agent tool surface and invalidates cached prefixes.
|
||||
|
||||
The toggle lives under ``auxiliary.mcp`` in DEFAULT_CONFIG (alongside
|
||||
the MCP aux-task provider settings), so the mocked config must mirror
|
||||
that shape — a top-level ``mcp`` key does not exist in the loaded
|
||||
config and the watcher resolves through ``auxiliary.mcp``.
|
||||
The toggle is the top-level ``mcp:`` section in config.yaml, and the
|
||||
watcher reads it from the same freshly-parsed file it diffs — so
|
||||
flipping the toggle and editing mcp_servers in one edit behaves
|
||||
correctly.
|
||||
"""
|
||||
import yaml
|
||||
obj, cfg_file = _make_cli(
|
||||
|
|
@ -124,16 +124,14 @@ class TestMCPConfigWatch:
|
|||
mcp_servers={},
|
||||
)
|
||||
|
||||
# Simulate a changed mcp_servers section
|
||||
cfg_file.write_text(yaml.dump({"mcp_servers": {"github": {"url": "https://mcp.github.com"}}}))
|
||||
# Simulate a changed mcp_servers section with auto-reload opted out.
|
||||
cfg_file.write_text(yaml.dump({
|
||||
"mcp": {"auto_reload_on_config_change": False},
|
||||
"mcp_servers": {"github": {"url": "https://mcp.github.com"}},
|
||||
}))
|
||||
obj._config_mtime = 0.0 # force stale mtime
|
||||
|
||||
# Opt out via the loaded config (the watcher reads load_config(),
|
||||
# not obj.config, so we patch the loader). Match the real shape:
|
||||
# DEFAULT_CONFIG["auxiliary"]["mcp"]["auto_reload_on_config_change"].
|
||||
mocked_cfg = {"auxiliary": {"mcp": {"auto_reload_on_config_change": False}}}
|
||||
with patch("hermes_cli.config.get_config_path", return_value=cfg_file), \
|
||||
patch("hermes_cli.config.load_config", return_value=mocked_cfg):
|
||||
with patch("hermes_cli.config.get_config_path", return_value=cfg_file):
|
||||
obj._check_config_mcp_changes()
|
||||
|
||||
obj._reload_mcp.assert_not_called()
|
||||
|
|
@ -143,32 +141,95 @@ class TestMCPConfigWatch:
|
|||
assert "/reload-mcp" in out
|
||||
assert "prompt cache" in out
|
||||
|
||||
def test_optout_path_is_auxiliary_mcp_not_top_level(self, tmp_path, capsys):
|
||||
"""Regression guard: the opt-out toggle lives under
|
||||
``auxiliary.mcp.auto_reload_on_config_change`` in DEFAULT_CONFIG,
|
||||
NOT under a top-level ``mcp`` key.
|
||||
def test_optout_updates_snapshot_so_reload_mcp_applies_cleanly(self, tmp_path):
|
||||
"""After an opted-out change, the watcher must not re-notify every
|
||||
tick: the snapshot is updated so the same content compares equal on
|
||||
the next pass."""
|
||||
import yaml
|
||||
obj, cfg_file = _make_cli(tmp_path, mcp_servers={})
|
||||
|
||||
A config that sets ONLY ``mcp.auto_reload_on_config_change: false``
|
||||
(top-level, wrong path) must NOT disable the reload — otherwise the
|
||||
watcher is reading the wrong key and the declared default never
|
||||
takes effect at runtime. This test pins the config-path contract
|
||||
so a future regression to ``_cfg.get("mcp")`` is caught.
|
||||
"""
|
||||
cfg_file.write_text(yaml.dump({
|
||||
"mcp": {"auto_reload_on_config_change": False},
|
||||
"mcp_servers": {"github": {"url": "https://mcp.github.com"}},
|
||||
}))
|
||||
obj._config_mtime = 0.0
|
||||
|
||||
with patch("hermes_cli.config.get_config_path", return_value=cfg_file):
|
||||
obj._check_config_mcp_changes()
|
||||
# Second pass: same file content, new mtime — no reload, no change.
|
||||
obj._last_config_check = 0.0
|
||||
obj._config_mtime = 0.0
|
||||
obj._check_config_mcp_changes()
|
||||
|
||||
obj._reload_mcp.assert_not_called()
|
||||
assert obj._config_mcp_servers == {"github": {"url": "https://mcp.github.com"}}
|
||||
|
||||
def test_optout_path_is_top_level_mcp_not_auxiliary(self, tmp_path):
|
||||
"""Regression guard: the opt-out toggle is the top-level
|
||||
``mcp.auto_reload_on_config_change`` key, NOT ``auxiliary.mcp``
|
||||
(which holds side-LLM task provider settings).
|
||||
|
||||
A config that sets ONLY ``auxiliary.mcp.auto_reload_on_config_change:
|
||||
false`` must NOT disable the reload."""
|
||||
import yaml
|
||||
obj, cfg_file = _make_cli(
|
||||
tmp_path,
|
||||
mcp_servers={},
|
||||
)
|
||||
|
||||
cfg_file.write_text(yaml.dump({"mcp_servers": {"github": {"url": "https://mcp.github.com"}}}))
|
||||
cfg_file.write_text(yaml.dump({
|
||||
"auxiliary": {"mcp": {"auto_reload_on_config_change": False}},
|
||||
"mcp_servers": {"github": {"url": "https://mcp.github.com"}},
|
||||
}))
|
||||
obj._config_mtime = 0.0
|
||||
|
||||
# Wrong shape: top-level "mcp" (not where DEFAULT_CONFIG puts the
|
||||
# toggle). The watcher must NOT honour this, so a reload is expected.
|
||||
wrong_shape_cfg = {"mcp": {"auto_reload_on_config_change": False}}
|
||||
with patch("hermes_cli.config.get_config_path", return_value=cfg_file), \
|
||||
patch("hermes_cli.config.load_config", return_value=wrong_shape_cfg):
|
||||
with patch("hermes_cli.config.get_config_path", return_value=cfg_file):
|
||||
obj._check_config_mcp_changes()
|
||||
|
||||
# Reload happened because the wrong-path opt-out is ignored.
|
||||
# Reload happened because the aux-task path is not the toggle.
|
||||
obj._reload_mcp.assert_called()
|
||||
|
||||
def test_env_var_templates_do_not_false_positive_on_unrelated_saves(
|
||||
self, tmp_path, monkeypatch, capsys
|
||||
):
|
||||
"""Regression for the '/reasoning triggers MCP reload' bug (#55701).
|
||||
|
||||
Init snapshots mcp_servers from the loaded config, which has been
|
||||
through _expand_env_vars() — so ``${MCP_GH_API_KEY}`` is stored
|
||||
expanded. The watcher re-parses the RAW yaml. Without expanding the
|
||||
watcher side too, the comparison is always unequal whenever any
|
||||
template is in use, so EVERY config.yaml rewrite (e.g.
|
||||
save_config_value('agent.reasoning_effort', ...) from /reasoning)
|
||||
fired a full MCP reconnect.
|
||||
"""
|
||||
import yaml
|
||||
monkeypatch.setenv("MCP_GH_API_KEY", "sekrit-token")
|
||||
|
||||
raw_servers = {
|
||||
"github": {
|
||||
"url": "https://mcp.github.com",
|
||||
"headers": {"Authorization": "Bearer ${MCP_GH_API_KEY}"},
|
||||
}
|
||||
}
|
||||
expanded_servers = {
|
||||
"github": {
|
||||
"url": "https://mcp.github.com",
|
||||
"headers": {"Authorization": "Bearer sekrit-token"},
|
||||
}
|
||||
}
|
||||
# Init snapshot holds the EXPANDED form (as load_cli_config produces).
|
||||
obj, cfg_file = _make_cli(tmp_path, mcp_servers=expanded_servers)
|
||||
|
||||
# Unrelated-key save: mcp_servers content identical (raw templates),
|
||||
# only reasoning_effort changed — mtime moves.
|
||||
cfg_file.write_text(yaml.dump({
|
||||
"agent": {"reasoning_effort": "high"},
|
||||
"mcp_servers": raw_servers,
|
||||
}))
|
||||
obj._config_mtime = 0.0
|
||||
|
||||
with patch("hermes_cli.config.get_config_path", return_value=cfg_file):
|
||||
obj._check_config_mcp_changes()
|
||||
|
||||
obj._reload_mcp.assert_not_called()
|
||||
assert "MCP server config changed" not in capsys.readouterr().out
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue