From 74a5905aea6f29374e624bbfd030357026d468cf Mon Sep 17 00:00:00 2001 From: sherman-yang <58446328+sherman-yang@users.noreply.github.com> Date: Sun, 21 Jun 2026 16:39:57 +0530 Subject: [PATCH] fix(cron): layer enabled MCP servers onto per-job enabled_toolsets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cron job that sets `enabled_toolsets` to a list of *native* toolsets (e.g. `["web", "terminal"]`) silently got ZERO MCP tools, while a job with no per-job list got every globally-enabled MCP server. `_resolve_cron_enabled_ toolsets` returned the per-job list verbatim, bypassing the MCP-merge that the platform-fallback branch performs via `_get_platform_tools`. So `discover_mcp_tools()` registered the MCP tools into the registry, but `get_tool_definitions(enabled_toolsets=...)` kept only the named native toolsets — the agent then rejected every `mcp_*` call as "Unknown tool". (R2 of #23997.) Fix: `_merge_mcp_into_per_job_toolsets` layers MCP membership onto a per-job allowlist with the SAME semantics as `_get_platform_tools`: * `no_mcp` sentinel present -> no MCP servers (sentinel stripped) * one or more MCP server names already listed -> treat as an allowlist * otherwise -> union in every globally-enabled MCP server To avoid duplicating the "which MCP servers are enabled" computation (it already existed inline in `_get_platform_tools`), this extracts a shared `enabled_mcp_server_names(config)` helper in `hermes_cli.tools_config` and has BOTH the gateway/CLI platform resolver and the cron per-job resolver call it — so every path agrees on MCP membership (extend, don't duplicate). Note: the issue's *headline* — bare MCP server names rejected, registry never includes them — was already fixed on main (commits c10fea8d2 + 04918345e, both before the issue was filed). This PR closes the remaining cron-specific gap (R2). The `server:*` / `mcp:server` alias-notation rejection (R1) and the quiet-mode silent-drop (R3) are tracked separately. Salvaged from #32788 by sherman-yang (credited below). Reworked to reuse the shared `enabled_mcp_server_names` helper instead of re-implementing the MCP membership set in cron/scheduler.py. Fixes #23997 Co-authored-by: sherman-yang <58446328+sherman-yang@users.noreply.github.com> --- cron/scheduler.py | 37 ++++++++++++++++++-- hermes_cli/tools_config.py | 26 ++++++++++---- tests/cron/test_scheduler.py | 66 +++++++++++++++++++++++++++++++++++- 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/cron/scheduler.py b/cron/scheduler.py index b7d662e61a4..99f910d8630 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -135,12 +135,45 @@ def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: return disabled +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. + + A per-job list scopes the *native* toolsets, but on its own it silently + drops every MCP server: ``discover_mcp_tools()`` registers the tools into + the global registry, yet ``get_tool_definitions(enabled_toolsets=...)`` + only keeps toolsets named in the list. The agent then rejects every + ``mcp_*`` call with "Unknown tool". This restores parity with + ``_get_platform_tools`` MCP semantics: + + * ``no_mcp`` sentinel present -> no MCP servers (sentinel stripped) + * one or more MCP server names already listed -> treat as an allowlist, + add nothing further (the user named exactly the servers they want) + * otherwise -> union in every globally-enabled MCP server + """ + result = [t for t in per_job if t != "no_mcp"] + if "no_mcp" in per_job: + return result + # lazy import: avoid heavy hermes_cli import at cron module load (matches + # _resolve_cron_enabled_toolsets' fallback) and share one MCP-membership + # computation with the gateway/CLI platform resolver. + from hermes_cli.tools_config import enabled_mcp_server_names + enabled_mcp = enabled_mcp_server_names(cfg) + if set(result) & enabled_mcp: + return result + for name in sorted(enabled_mcp): + if name not in result: + result.append(name) + return result + + def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: """Resolve the toolset list for a cron job. Precedence: 1. Per-job ``enabled_toolsets`` (set via ``cronjob`` tool on create/update). - Keeps the agent's job-scoped toolset override intact — #6130. + Keeps the agent's job-scoped toolset override intact — #6130. Enabled + MCP servers are layered on per ``_merge_mcp_into_per_job_toolsets`` so a + native-toolset allowlist does not silently strip MCP tools. 2. Per-platform ``hermes tools`` config for the ``cron`` platform. Mirrors gateway behavior (``_get_platform_tools(cfg, platform_key)``) so users can gate cron toolsets globally without recreating every job. @@ -154,7 +187,7 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: """ per_job = job.get("enabled_toolsets") if per_job: - return per_job + return _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) try: from hermes_cli.tools_config import _get_platform_tools # lazy: avoid heavy import at cron module load return sorted(_get_platform_tools(cfg or {}, "cron")) diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 5eec978e180..f3664c06698 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -1284,6 +1284,24 @@ def _parse_enabled_flag(value, default: bool = True) -> bool: return default +def enabled_mcp_server_names(config: dict) -> Set[str]: + """Names of MCP servers globally enabled in config.yaml. + + Shared by the gateway/CLI platform resolver (``_get_platform_tools``) and + the cron per-job toolset resolver (``cron.scheduler``) so every path agrees + on MCP membership. A server is enabled unless its config sets an explicitly + falsey ``enabled`` (per ``_parse_enabled_flag``: false/0/no/off) — a missing + flag or an unrecognized value is treated as enabled. + """ + mcp_servers = (config or {}).get("mcp_servers") or {} + return { + str(name) + for name, server_cfg in mcp_servers.items() + if isinstance(server_cfg, dict) + and _parse_enabled_flag(server_cfg.get("enabled", True), default=True) + } + + def _get_platform_tools( config: dict, platform: str, @@ -1503,13 +1521,7 @@ def _get_platform_tools( # If the platform explicitly lists one or more MCP server names, treat that # as an allowlist. Otherwise include every globally enabled MCP server. # Special sentinel: "no_mcp" in the toolset list disables all MCP servers. - mcp_servers = config.get("mcp_servers") or {} - enabled_mcp_servers = { - str(name) - for name, server_cfg in mcp_servers.items() - if isinstance(server_cfg, dict) - and _parse_enabled_flag(server_cfg.get("enabled", True), default=True) - } + enabled_mcp_servers = enabled_mcp_server_names(config) # Allow "no_mcp" sentinel to opt out of all MCP servers for this platform if "no_mcp" in toolset_names: explicit_mcp_servers = set() diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 27613e7e1ca..a3c17048bb6 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -7,11 +7,75 @@ from unittest.mock import AsyncMock, patch, MagicMock import pytest -from cron.scheduler import _resolve_origin, _resolve_delivery_target, _deliver_result, _send_media_via_adapter, run_job, SILENT_MARKER, _build_job_prompt +from cron.scheduler import _resolve_origin, _resolve_delivery_target, _deliver_result, _send_media_via_adapter, run_job, SILENT_MARKER, _build_job_prompt, _resolve_cron_enabled_toolsets, _merge_mcp_into_per_job_toolsets from tools.env_passthrough import clear_env_passthrough from tools.credential_files import clear_credential_files +class TestPerJobToolsetMcpMerge: + """A per-job enabled_toolsets allowlist must not silently drop MCP servers.""" + + CFG = { + "mcp_servers": { + "finnhub": {"enabled": True}, + "playwright": {"enabled": True}, + "disabled_one": {"enabled": False}, + "string_enabled": {"enabled": "true"}, + "not_a_dict": "ignored", + } + } + + def _enabled_names(self): + return {"finnhub", "playwright", "string_enabled"} + + def test_native_only_list_gets_all_enabled_mcp_servers(self): + result = _merge_mcp_into_per_job_toolsets(["web", "terminal"], self.CFG) + assert result[:2] == ["web", "terminal"] + assert set(result) == {"web", "terminal"} | self._enabled_names() + + def test_disabled_servers_are_not_added(self): + result = _merge_mcp_into_per_job_toolsets(["web"], self.CFG) + assert "disabled_one" not in result + + def test_explicit_mcp_name_is_treated_as_allowlist(self): + # User named one server -> add nothing further. + result = _merge_mcp_into_per_job_toolsets(["web", "finnhub"], self.CFG) + assert result == ["web", "finnhub"] + assert "playwright" not in result + + 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"] + assert not (set(result) & self._enabled_names()) + + def test_no_mcp_config_adds_nothing(self): + result = _merge_mcp_into_per_job_toolsets(["web"], {}) + assert result == ["web"] + + def test_no_duplicate_when_listed_name_also_globally_enabled(self): + result = _merge_mcp_into_per_job_toolsets(["finnhub", "finnhub"], self.CFG) + assert result.count("finnhub") == 2 # input dups preserved, none added + + def test_resolver_uses_merge_for_per_job_lists(self): + job = {"enabled_toolsets": ["web", "terminal"]} + result = _resolve_cron_enabled_toolsets(job, self.CFG) + assert set(result) == {"web", "terminal"} | self._enabled_names() + + def test_resolver_empty_per_job_falls_through_to_platform(self): + # No per-job list -> must delegate to _get_platform_tools (the platform + # fallback), NOT the per-job merge. Stub the platform resolver and assert + # it is the path taken and its result is returned. + job = {"enabled_toolsets": None} + sentinel = ["web", "finnhub"] + with patch("hermes_cli.tools_config._get_platform_tools", + return_value=set(sentinel)) as m_platform: + result = _resolve_cron_enabled_toolsets(job, self.CFG) + m_platform.assert_called_once() + # _get_platform_tools args: (cfg, "cron") + assert m_platform.call_args[0][1] == "cron" + assert set(result) == set(sentinel) + + class TestResolveOrigin: def test_full_origin(self): job = {