From d3d769f01e840c51269c930b31d3364a867ef184 Mon Sep 17 00:00:00 2001 From: Davy <95214375+thedavidweng@users.noreply.github.com> Date: Wed, 10 Jun 2026 23:04:20 -0700 Subject: [PATCH] fix(cli): accept configured MCP prefixed toolsets before discovery --- cli.py | 15 +- hermes_cli/mcp_toolsets.py | 77 ++++++++++ hermes_cli/oneshot.py | 39 ++--- tests/cli/test_mcp_toolset_validation.py | 74 +++++++++ tests/hermes_cli/test_tui_resume_flow.py | 183 +++++++++++++++++++++++ tests/test_tui_gateway_server.py | 22 +++ tui_gateway/server.py | 18 +-- 7 files changed, 392 insertions(+), 36 deletions(-) create mode 100644 hermes_cli/mcp_toolsets.py create mode 100644 tests/cli/test_mcp_toolset_validation.py diff --git a/cli.py b/cli.py index c4a3b0ae56c30..0dc1271700f49 100644 --- a/cli.py +++ b/cli.py @@ -2743,9 +2743,18 @@ def _init_toolsets(self, toolsets): self.disabled_toolsets = parse_config_string_list(CLI_CONFIG["agent"].get("disabled_toolsets")) if toolsets and "all" not in toolsets and "*" not in toolsets: - # MCP server names only resolve after discover_mcp_tools runs; skip them here. - mcp_names = set((CLI_CONFIG.get("mcp_servers") or {}).keys()) - invalid = [t for t in toolsets if not validate_toolset(t) and t not in mcp_names] + # Validate each toolset — MCP server toolsets are registered as + # ``mcp-`` aliases during discover_mcp_tools(), but + # discovery hasn't run yet at this point, so exclude configured MCP + # aliases from the early static warning. + from hermes_cli.mcp_toolsets import mcp_toolset_aliases_for_servers + + mcp_aliases = mcp_toolset_aliases_for_servers(CLI_CONFIG.get("mcp_servers")) + invalid = [ + t + for t in toolsets + if not validate_toolset(t) and t not in mcp_aliases + ] if invalid: self._console_print(f"[bold red]Warning: Unknown toolsets: {', '.join(invalid)}[/]") diff --git a/hermes_cli/mcp_toolsets.py b/hermes_cli/mcp_toolsets.py new file mode 100644 index 0000000000000..2f5fc07b7ba94 --- /dev/null +++ b/hermes_cli/mcp_toolsets.py @@ -0,0 +1,77 @@ +"""Helpers for configured MCP server toolset names. + +MCP discovery registers runtime toolsets as ``mcp-`` and also exposes +bare server-name aliases. Several startup paths validate explicit toolset lists +before discovery has run, so they need a config-only view of both spellings. +""" + +from __future__ import annotations + +from collections.abc import Mapping + + +def _parse_enabled_flag(value: object, default: bool = True) -> bool: + """Parse bool-like config values used by MCP server settings.""" + if value is None: + return default + if isinstance(value, bool): + return value + if isinstance(value, int): + return value != 0 + if isinstance(value, str): + lowered = value.strip().lower() + if lowered in {"true", "1", "yes", "on"}: + return True + if lowered in {"false", "0", "no", "off"}: + return False + return default + + +def _server_enabled(server_cfg: object) -> bool: + """Return whether an MCP server config is enabled by config semantics.""" + if isinstance(server_cfg, Mapping): + return _parse_enabled_flag(server_cfg.get("enabled", True), default=True) + return True + + +def mcp_toolset_aliases_for_servers( + mcp_servers: object, + *, + enabled: bool | None = None, +) -> set[str]: + """Return configured MCP toolset spellings for startup validation. + + Args: + mcp_servers: The ``config.yaml`` ``mcp_servers`` mapping. + enabled: ``True`` for only enabled servers, ``False`` for only disabled + servers, ``None`` for every configured server. + + Each configured server contributes both accepted toolset spellings: + ```` and ``mcp-``. The latter is the runtime toolset name + registered by MCP discovery; the former is the historical alias supported + by earlier startup validators. + """ + if not isinstance(mcp_servers, Mapping): + return set() + + aliases: set[str] = set() + for name, server_cfg in mcp_servers.items(): + if not isinstance(server_cfg, Mapping): + continue + name = str(name) + is_enabled = _server_enabled(server_cfg) + if enabled is not None and is_enabled is not enabled: + continue + aliases.add(name) + aliases.add(f"mcp-{name}") + return aliases + + +def split_configured_mcp_toolset_aliases( + mcp_servers: object, +) -> tuple[set[str], set[str]]: + """Return ``(enabled_aliases, disabled_aliases)`` for configured MCP servers.""" + return ( + mcp_toolset_aliases_for_servers(mcp_servers, enabled=True), + mcp_toolset_aliases_for_servers(mcp_servers, enabled=False), + ) diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index a6ed16273ce26..c64915ead4d60 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -68,26 +68,6 @@ def _build_preloaded_skills_prompt(skills: object = None) -> str | None: return skills_prompt or None -def _configured_mcp_servers() -> tuple[set[str], set[str]]: - """``(enabled, disabled)`` MCP server names from config; both empty on any error.""" - try: - from hermes_cli.config import read_raw_config - from hermes_cli.tools_config import _parse_enabled_flag - - cfg = read_raw_config() - mcp_servers = cfg.get("mcp_servers") if isinstance(cfg.get("mcp_servers"), dict) else {} - enabled: set[str] = set() - disabled: set[str] = set() - for name, server_cfg in mcp_servers.items(): - if not isinstance(server_cfg, dict): - continue - target = enabled if _parse_enabled_flag(server_cfg.get("enabled", True), default=True) else disabled - target.add(str(name)) - return enabled, disabled - except Exception: - return set(), set() - - def _validate_explicit_toolsets(toolsets: object = None) -> tuple[list[str] | None, str | None]: normalized = _normalize_toolsets(toolsets) if normalized is None: @@ -121,10 +101,23 @@ def _validate_explicit_toolsets(toolsets: object = None) -> tuple[list[str] | No ) return None, None - mcp_names, mcp_disabled = _configured_mcp_servers() if unresolved else (set(), set()) - mcp_valid = [name for name in unresolved if name in mcp_names] + mcp_aliases: set[str] = set() + mcp_disabled: set[str] = set() + if unresolved: + try: + from hermes_cli.config import read_raw_config + from hermes_cli.mcp_toolsets import split_configured_mcp_toolset_aliases + + cfg = read_raw_config() + mcp_servers = cfg.get("mcp_servers") if isinstance(cfg.get("mcp_servers"), dict) else {} + mcp_aliases, mcp_disabled = split_configured_mcp_toolset_aliases(mcp_servers) + except Exception: + mcp_aliases = set() + mcp_disabled = set() + + mcp_valid = [name for name in unresolved if name in mcp_aliases] disabled = [name for name in unresolved if name in mcp_disabled] - unknown = [name for name in unresolved if name not in mcp_names and name not in mcp_disabled] + unknown = [name for name in unresolved if name not in mcp_aliases and name not in mcp_disabled] valid = built_in + mcp_valid if unknown: diff --git a/tests/cli/test_mcp_toolset_validation.py b/tests/cli/test_mcp_toolset_validation.py new file mode 100644 index 0000000000000..7cf6e85c62c3e --- /dev/null +++ b/tests/cli/test_mcp_toolset_validation.py @@ -0,0 +1,74 @@ +from copy import deepcopy + +import cli + + +def _init_cli_and_capture_warnings(monkeypatch, toolsets): + cfg = deepcopy(cli.CLI_CONFIG) + cfg["model"] = {"default": "stub-model", "provider": "auto", "base_url": ""} + cfg.setdefault("agent", {})["disabled_toolsets"] = [] + cfg.setdefault("display", {}) + cfg["mcp_servers"] = { + "agentmail": {"enabled": True}, + "notion-findit": {"enabled": True}, + } + + warnings = [] + monkeypatch.setattr(cli, "CLI_CONFIG", cfg) + monkeypatch.setattr(cli, "validate_toolset", lambda name: name == "terminal") + monkeypatch.setattr( + cli.HermesCLI, + "_console_print", + lambda self, message, *args, **kwargs: warnings.append(str(message)), + ) + + cli.HermesCLI( + model="stub-model", + provider="auto", + toolsets=toolsets, + compact=True, + ) + return warnings + + +def test_cli_startup_accepts_mcp_prefixed_toolsets_before_discovery(monkeypatch): + warnings = _init_cli_and_capture_warnings( + monkeypatch, + ["terminal", "mcp-agentmail", "mcp-notion-findit"], + ) + + assert warnings == [] + + +def test_cli_startup_still_warns_for_non_mcp_unknown_toolsets(monkeypatch): + warnings = _init_cli_and_capture_warnings( + monkeypatch, + ["terminal", "mcp-agentmail", "not-real"], + ) + + assert warnings == ["[bold red]Warning: Unknown toolsets: not-real[/]"] + + +def test_mcp_toolset_aliases_skips_non_mapping_entries(): + """Non-mapping server entries are skipped to avoid validating toolsets + that discovery cannot register.""" + from hermes_cli.mcp_toolsets import mcp_toolset_aliases_for_servers + + result = mcp_toolset_aliases_for_servers({ + "good": {"enabled": True}, + "bad": "just-a-string", + "also-bad": 42, + }) + assert result == {"good", "mcp-good"} + + +def test_split_configured_mcp_toolset_aliases_skips_non_mapping(): + from hermes_cli.mcp_toolsets import split_configured_mcp_toolset_aliases + + enabled, disabled = split_configured_mcp_toolset_aliases({ + "active": {"enabled": True}, + "inactive": {"enabled": False}, + "malformed": "not-a-dict", + }) + assert enabled == {"active", "mcp-active"} + assert disabled == {"inactive", "mcp-inactive"} diff --git a/tests/hermes_cli/test_tui_resume_flow.py b/tests/hermes_cli/test_tui_resume_flow.py index fe6ca2c8b43cc..ef48e5708cf77 100644 --- a/tests/hermes_cli/test_tui_resume_flow.py +++ b/tests/hermes_cli/test_tui_resume_flow.py @@ -147,6 +147,189 @@ def _stub_plugin_discovery(monkeypatch): ) +def test_oneshot_rejects_invalid_only_toolsets(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + from hermes_cli.oneshot import run_oneshot + + assert run_oneshot("hello", toolsets="nope") == 2 + err = capsys.readouterr().err + assert "nope" in err + assert "did not contain any valid toolsets" in err + + +def test_oneshot_fails_closed_on_empty_final_response(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.oneshot as oneshot_mod + + monkeypatch.setattr(oneshot_mod, "_run_agent", lambda *_args, **_kwargs: ("", {})) + + assert oneshot_mod.run_oneshot("hello") == 1 + captured = capsys.readouterr() + assert captured.out == "" + assert "no final response" in captured.err + + +def test_oneshot_prints_nonempty_final_response(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.oneshot as oneshot_mod + + monkeypatch.setattr(oneshot_mod, "_run_agent", lambda *_args, **_kwargs: ("done", {})) + + assert oneshot_mod.run_oneshot("hello") == 0 + captured = capsys.readouterr() + assert captured.out == "done\n" + assert captured.err == "" + + +def test_oneshot_fails_closed_on_agent_exception(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.oneshot as oneshot_mod + + def _boom(*_args, **_kwargs): + raise OSError("not a TTY") + + monkeypatch.setattr(oneshot_mod, "_run_agent", _boom) + + assert oneshot_mod.run_oneshot("hello") == 1 + captured = capsys.readouterr() + assert captured.out == "" + assert "agent failed" in captured.err + assert "not a TTY" in captured.err + + +def test_oneshot_reraises_keyboard_interrupt(monkeypatch): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.oneshot as oneshot_mod + import pytest as _pytest + + def _interrupt(*_args, **_kwargs): + raise KeyboardInterrupt + + monkeypatch.setattr(oneshot_mod, "_run_agent", _interrupt) + + with _pytest.raises(KeyboardInterrupt): + oneshot_mod.run_oneshot("hello") + + +def test_oneshot_filters_invalid_toolsets_before_redirect(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + from hermes_cli.oneshot import _validate_explicit_toolsets + + valid, error = _validate_explicit_toolsets("web,nope") + + assert valid == ["web"] + assert error is None + assert "nope" in capsys.readouterr().err + + +def test_oneshot_all_toolsets_means_all_not_configured_cli(): + from hermes_cli.oneshot import _validate_explicit_toolsets + + valid, error = _validate_explicit_toolsets("all") + + assert valid is None + assert error is None + + +def test_oneshot_all_toolsets_warns_about_ignored_extra_entries(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + from hermes_cli.oneshot import _validate_explicit_toolsets + + valid, error = _validate_explicit_toolsets("all,nope") + + assert valid is None + assert error is None + assert "ignoring additional entries: nope" in capsys.readouterr().err + + +def test_oneshot_accepts_plugin_toolset_after_discovery(monkeypatch): + import toolsets + + from hermes_cli.oneshot import _validate_explicit_toolsets + + discovered = {"ready": False} + original_validate = toolsets.validate_toolset + + def fake_validate(name): + return name == "plugin_demo" and discovered["ready"] or original_validate(name) + + monkeypatch.setattr(toolsets, "validate_toolset", fake_validate) + monkeypatch.setitem( + sys.modules, + "hermes_cli.plugins", + types.SimpleNamespace( + discover_plugins=lambda: discovered.update({"ready": True}) + ), + ) + + valid, error = _validate_explicit_toolsets("plugin_demo") + + assert valid == ["plugin_demo"] + assert error is None + + +def test_oneshot_accepts_configured_mcp_prefixed_toolset_before_discovery( + monkeypatch, capsys +): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.config as config_mod + + from hermes_cli.oneshot import _validate_explicit_toolsets + + monkeypatch.setattr( + config_mod, + "read_raw_config", + lambda: {"mcp_servers": {"brain": {"enabled": True}}}, + ) + + valid, error = _validate_explicit_toolsets("web,mcp-brain") + + assert valid == ["web", "mcp-brain"] + assert error is None + assert capsys.readouterr().err == "" + + +def test_oneshot_rejects_disabled_mcp_toolset(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.config as config_mod + + from hermes_cli.oneshot import _validate_explicit_toolsets + + monkeypatch.setattr( + config_mod, + "read_raw_config", + lambda: {"mcp_servers": {"off": {"enabled": False}}}, + ) + + valid, error = _validate_explicit_toolsets("mcp-off") + + assert valid is None + assert error == "hermes -z: --toolsets did not contain any valid toolsets.\n" + err = capsys.readouterr().err + assert "ignoring disabled MCP servers" in err + assert "mcp-off" in err + + +def test_oneshot_distinguishes_disabled_mcp_from_unknown(monkeypatch, capsys): + _stub_plugin_discovery(monkeypatch) + import hermes_cli.config as config_mod + + from hermes_cli.oneshot import _validate_explicit_toolsets + + monkeypatch.setattr( + config_mod, + "read_raw_config", + lambda: {"mcp_servers": {"mcp-off": {"enabled": False}}}, + ) + + valid, error = _validate_explicit_toolsets("web,mcp-off,nope") + + assert valid == ["web"] + assert error is None + err = capsys.readouterr().err + assert "ignoring unknown --toolsets entries: nope" in err + assert "ignoring disabled MCP servers" in err + assert "mcp-off" in err def test_oneshot_wires_session_db_for_recall(monkeypatch): diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 54e29465fb90c..d12e0076f43b7 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -2588,6 +2588,28 @@ def test_load_enabled_toolsets_folds_project_into_focus_posture(monkeypatch): assert server._load_enabled_toolsets("tui") == ["coding", "figma", "project"] +def test_load_enabled_toolsets_accepts_configured_mcp_prefixed_env_before_discovery( + monkeypatch, capsys +): + monkeypatch.setenv("HERMES_TUI_TOOLSETS", "web,mcp-brain") + monkeypatch.setitem( + sys.modules, + "hermes_cli.plugins", + types.SimpleNamespace(discover_plugins=lambda: None), + ) + + import hermes_cli.config as config_mod + + monkeypatch.setattr( + config_mod, + "read_raw_config", + lambda: {"mcp_servers": {"brain": {"enabled": True}}}, + ) + + assert server._load_enabled_toolsets() == ["web", "mcp-brain"] + assert capsys.readouterr().err == "" + + def test_load_enabled_toolsets_rejects_disabled_mcp_env(monkeypatch, capsys): monkeypatch.setenv("HERMES_TUI_TOOLSETS", "mcp-off") monkeypatch.setitem( diff --git a/tui_gateway/server.py b/tui_gateway/server.py index e3c486b48e591..49626adaa09bd 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -1806,21 +1806,19 @@ def _resolve_explicit_toolsets(explicit: list[str], validate_toolset) -> list[st return None if not unresolved: return built_in - try: # (enabled, disabled) MCP server names from raw config; both empty on any failure + try: # (enabled, disabled) MCP toolset aliases from raw config; both empty on any failure from hermes_cli.config import read_raw_config - from hermes_cli.tools_config import _parse_enabled_flag + from hermes_cli.mcp_toolsets import split_configured_mcp_toolset_aliases raw_cfg = read_raw_config() mcp_servers = raw_cfg.get("mcp_servers") if isinstance(raw_cfg.get("mcp_servers"), dict) else {} - mcp_names, mcp_disabled = set(), set() - for name, server_cfg in mcp_servers.items(): - if isinstance(server_cfg, dict): - on = _parse_enabled_flag(server_cfg.get("enabled", True), default=True) - (mcp_names if on else mcp_disabled).add(str(name)) + # Both spellings: bare ```` and the ``mcp-`` runtime + # name registered at discovery (which hasn't run yet on this path). + mcp_aliases, mcp_disabled = split_configured_mcp_toolset_aliases(mcp_servers) except Exception: - mcp_names, mcp_disabled = set(), set() - mcp_valid = [name for name in unresolved if name in mcp_names] + mcp_aliases, mcp_disabled = set(), set() + mcp_valid = [name for name in unresolved if name in mcp_aliases] disabled = [name for name in unresolved if name in mcp_disabled] - unknown = [name for name in unresolved if name not in mcp_names and name not in mcp_disabled] + unknown = [name for name in unresolved if name not in mcp_aliases and name not in mcp_disabled] if unknown: _tui_notice(f"[tui] ignoring unknown HERMES_TUI_TOOLSETS entries: {', '.join(unknown)}") if disabled: