From 60092f728c325c9bcd894c694dc332161e41904f Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 19 Jul 2026 23:54:30 -0700 Subject: [PATCH] fix(mcp): move auto-reload opt-out to top-level mcp: section + regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- cli.py | 22 ++--- hermes_cli/config.py | 22 +++-- tests/cli/test_cli_mcp_config_watch.py | 121 +++++++++++++++++++------ 3 files changed, 115 insertions(+), 50 deletions(-) diff --git a/cli.py b/cli.py index c4d12a9f4ec..b5971e08746 100644 --- a/cli.py +++ b/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 diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 9b8d9c5427b..b946be16b86 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -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, diff --git a/tests/cli/test_cli_mcp_config_watch.py b/tests/cli/test_cli_mcp_config_watch.py index fa1eb142b79..921e5408083 100644 --- a/tests/cli/test_cli_mcp_config_watch.py +++ b/tests/cli/test_cli_mcp_config_watch.py @@ -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