diff --git a/cron/scheduler.py b/cron/scheduler.py index dcf8566e89770..b203502eb830e 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -472,13 +472,20 @@ def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: def _merge_mcp_into_per_job_toolsets(per_job: list[str], cfg: dict) -> list[str]: """Layer enabled MCP servers onto a per-job ``enabled_toolsets`` allowlist (else a per-job list silently drops every MCP server). Mirrors ``_get_platform_tools``: ``no_mcp`` sentinel -> none - (stripped); any MCP server already listed -> allowlist, add nothing; else union all enabled.""" + (stripped); any MCP server already listed -> allowlist, add nothing; else union all enabled. + Servers scoped via ``mcp_servers..platforms`` follow the same platform rules as + ``_get_platform_tools(cfg, "cron")``: one scoped to other platforms is dropped even when the + job names it explicitly (server-side scope wins), and is never auto-added.""" result = [t for t in per_job if t != "no_mcp"] if "no_mcp" in per_job: return result # lazy: avoid heavy hermes_cli import at module load; shares MCP-membership with gateway/CLI from hermes_cli.tools_config import enabled_mcp_server_names - enabled_mcp = enabled_mcp_server_names(cfg) + globally_enabled = enabled_mcp_server_names(cfg) + enabled_mcp = enabled_mcp_server_names(cfg, "cron") + scoped_out = globally_enabled - enabled_mcp + if scoped_out: + result = [t for t in result if t not in scoped_out] if set(result) & enabled_mcp: return result for name in sorted(enabled_mcp): diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 2dfe8cf18c5a1..cfcff643f5ea7 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -407,16 +407,18 @@ def _parse_enabled_flag(value, default: bool = True) -> bool: return default -def enabled_mcp_server_names(config: dict) -> Set[str]: +def enabled_mcp_server_names(config: dict, platform: Optional[str] = None) -> Set[str]: """MCP servers globally enabled in config.yaml or by a plugin (shared by platform + cron resolvers). Enabled unless ``enabled`` is explicitly falsey; portable-plugin servers (in-memory) count — enabling the plugin is - the opt-in.""" - from tools.mcp_tool_common import mcp_server_enabled + the opt-in. With *platform*, servers scoped via ``mcp_servers..platforms`` to other platforms are + excluded (#110916); ``None`` keeps the legacy unfiltered set.""" + from tools.mcp_tool_common import mcp_server_allowed_for_platform, mcp_server_enabled mcp_servers = (config or {}).get("mcp_servers") or {} names = { str(name) for name, server_cfg in mcp_servers.items() if isinstance(server_cfg, dict) and mcp_server_enabled(server_cfg) + and (platform is None or mcp_server_allowed_for_platform(server_cfg, platform)) } try: from hermes_cli.plugins import get_portable_mcp_server_names_nowait @@ -619,7 +621,7 @@ def _get_platform_tools(config: dict, platform: str, *, include_default_mcp_serv # Explicit non-configurable entries (custom toolsets, MCP server names) pass through. explicit_passthrough = {ts for ts in toolset_names if ts not in explicit_known_keys and ts not in platform_default_keys} - enabled_toolsets |= _merge_mcp_servers(config, toolset_names, explicit_passthrough, include_default_mcp_servers) + enabled_toolsets |= _merge_mcp_servers(config, toolset_names, explicit_passthrough, include_default_mcp_servers, platform) # Legacy profile opt-in is a fallback only. A saved platform list (even # empty) is authoritative, so a later disable cannot silently re-enable it. @@ -683,17 +685,21 @@ def _recover_platform_native_toolsets(enabled_toolsets: Set[str], platform: str, def _merge_mcp_servers( - config: dict, toolset_names: List[str], explicit_passthrough: Set[str], include_default_mcp_servers: bool + config: dict, toolset_names: List[str], explicit_passthrough: Set[str], include_default_mcp_servers: bool, + platform: Optional[str] = None, ) -> Set[str]: """Explicit passthrough entries plus this platform's MCP servers: listed names form an allowlist, else every - globally enabled server (when ``include_default_mcp_servers``); the ``no_mcp`` sentinel disables all.""" - enabled_mcp_servers = enabled_mcp_server_names(config) - result = explicit_passthrough - enabled_mcp_servers + globally enabled server (when ``include_default_mcp_servers``); the ``no_mcp`` sentinel disables all. + A server scoped via ``mcp_servers..platforms`` to another platform is excluded even when explicitly + listed (#110916: server-side scope wins over the per-platform list).""" + globally_enabled = enabled_mcp_server_names(config) + allowed = enabled_mcp_server_names(config, platform) if platform else globally_enabled + result = explicit_passthrough - globally_enabled if "no_mcp" in toolset_names: return result - {"no_mcp"} - explicit_mcp_servers = explicit_passthrough & enabled_mcp_servers + explicit_mcp_servers = explicit_passthrough & allowed if include_default_mcp_servers and not explicit_mcp_servers: - return result | enabled_mcp_servers + return result | allowed return result | explicit_mcp_servers diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 9d29b5bba75e1..41cc7f59dc1d3 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -107,6 +107,21 @@ def test_explicit_mcp_name_is_treated_as_allowlist(self): assert result == ["web", "finnhub"] assert "playwright" not in result + def test_platform_scoped_server_never_reaches_a_cron_job(self): + # platforms: [discord] = server-side scope wins: not auto-added, and dropped even + # when the job names it explicitly (parity with _get_platform_tools). + cfg = { + "mcp_servers": { + "discord_admin": {"enabled": True, "platforms": ["discord"]}, + "finnhub": {"enabled": True}, + } + } + merged = _merge_mcp_into_per_job_toolsets(["web"], cfg) + assert "finnhub" in merged and "discord_admin" not in merged + named = _merge_mcp_into_per_job_toolsets(["web", "discord_admin"], cfg) + assert "discord_admin" not in named + assert "finnhub" in named + def test_no_mcp_sentinel_opts_out_and_is_stripped(self): result = _merge_mcp_into_per_job_toolsets(["web", "no_mcp"], self.CFG) assert result == ["web"] diff --git a/tests/hermes_cli/test_tools_config.py b/tests/hermes_cli/test_tools_config.py index 991589d9100b7..db267198ba727 100644 --- a/tests/hermes_cli/test_tools_config.py +++ b/tests/hermes_cli/test_tools_config.py @@ -387,6 +387,46 @@ def test_numeric_mcp_server_name_does_not_crash_sorted(): sorted(enabled) +def test_mcp_server_platforms_scopes_server_to_listed_platforms(): + """https://github.com/NousResearch/hermes-agent/issues/110916: a server with + ``mcp_servers..platforms`` must not leak to other platforms, even when + explicitly listed in their ``platform_toolsets``; unscoped servers are unaffected.""" + config = { + "mcp_servers": { + "discord_admin": {"command": "x", "platforms": ["discord"]}, + "open_srv": {"command": "x"}, + }, + } + + api = _get_platform_tools(config, "api_server") + assert "discord_admin" not in api + assert "open_srv" in api + discord = _get_platform_tools(config, "discord") + assert "discord_admin" in discord + + explicit = dict(config) + explicit["platform_toolsets"] = {"api_server": ["discord_admin"]} + assert "discord_admin" not in _get_platform_tools(explicit, "api_server") + + +def test_mcp_server_platforms_wildcard_serves_every_platform(): + """``["*"]`` (and its all/any aliases) means every platform — not a literal platform name.""" + for token in ("*", "all", "any"): + config = {"mcp_servers": {"everywhere": {"command": "x", "platforms": [token]}}} + for platform in ("discord", "api_server", "cron"): + assert "everywhere" in _get_platform_tools(config, platform), (token, platform) + + +def test_mcp_server_platforms_typo_warns_and_stays_excluded(caplog): + """A scope naming no known platform (e.g. a typo) must warn once instead of silently + disabling the server everywhere.""" + config = {"mcp_servers": {"typo_srv": {"command": "x", "platforms": ["dscord"]}}} + with caplog.at_level(logging.WARNING, logger="tools.mcp_tool"): + assert "typo_srv" not in _get_platform_tools(config, "discord") + warnings = [record.getMessage() for record in caplog.records] + assert any("dscord" in message for message in warnings), warnings + + # ─── Imagegen Backend Picker Wiring ──────────────────────────────────────── diff --git a/tools/mcp_tool_common.py b/tools/mcp_tool_common.py index 8104ee628cc46..0e3587c7c82f9 100644 --- a/tools/mcp_tool_common.py +++ b/tools/mcp_tool_common.py @@ -144,6 +144,60 @@ def mcp_server_enabled(cfg: dict) -> bool: return _parse_boolish(cfg.get("enabled", True), default=True) +#: ``platforms`` list entries that mean "serve every platform" rather than a platform name. +_ALL_PLATFORM_TOKENS = frozenset({"*", "all", "any"}) +#: Warn once per distinct scope list: resolution runs per turn, and a config typo would +#: otherwise warn on every one of them. +_WARNED_UNKNOWN_PLATFORM_SCOPES: set = set() + + +def _known_platform_names() -> set: + """Platform keys a ``platforms`` scope can name: gateway platforms (incl. bundled plugin + adapters) plus the non-messaging runtimes that resolve toolsets through ``_get_platform_tools`` + (CLI, cron, ACP). Best-effort — used for typo warnings only.""" + known: set = {"cli", "cron", "acp"} + try: + from gateway.config import Platform + + known.update(member.value for member in Platform) + try: + bundled, _aliases = Platform._scan_bundled_plugin_platforms() + known.update(bundled) + except Exception: + pass + except Exception: + pass + return known + + +def mcp_server_allowed_for_platform(cfg: dict, platform: str) -> bool: + """Whether ``mcp_servers.`` may serve *platform*. Absent/``null`` ``platforms`` = every + platform (backward compatible); a list scopes the server to those platforms only (#110916). + ``*``/``all``/``any`` in the list mean every platform; a listed name matching no known + platform is warned about (once per distinct list) instead of silently disabling the server.""" + raw = cfg.get("platforms", None) + if raw is None: + return True + if isinstance(raw, str): + raw = [raw] + if not isinstance(raw, (list, tuple, set)): + logger.warning("MCP config expected a list for 'platforms', got %r; ignoring", raw) + return True + allowed = {str(p).strip().lower() for p in raw if str(p).strip()} + if allowed & _ALL_PLATFORM_TOKENS: + return True + known = _known_platform_names() + unknown = allowed - known + if unknown and frozenset(allowed) not in _WARNED_UNKNOWN_PLATFORM_SCOPES: + _WARNED_UNKNOWN_PLATFORM_SCOPES.add(frozenset(allowed)) + logger.warning( + "MCP 'platforms' scope %s contains unknown name(s) %s (known: %s); use '*', " + "'all' or 'any' to serve every platform", + sorted(allowed), sorted(unknown), ", ".join(sorted(known)), + ) + return str(platform or "").strip().lower() in allowed + + def _get_lifecycle_seconds(config: dict, key: str) -> Optional[float]: """Optional positive lifecycle timeout from top-level/nested ``lifecycle`` config (``0`` disables; negatives and non-numbers are warned about and ignored)."""