mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-23 10:42:00 +00:00
fix(cron): layer enabled MCP servers onto per-job enabled_toolsets
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 (commitsc10fea8d2+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>
This commit is contained in:
parent
04a1d9efd7
commit
74a5905aea
3 changed files with 119 additions and 10 deletions
|
|
@ -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"))
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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 = {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue