fix(cron): block a job whose requested MCP server resolves to zero tools
Under a multiplexer MCP tools are registered per profile overlay while the server toolset alias is process-global, so a cron job naming a server in enabled_toolsets that is connected only for another profile validated as a known toolset, resolved to zero tools, and ran tool-less with quiet_mode hiding the only diagnostic; the run was booked success (#109050). After cron MCP discovery, an explicitly requested enabled MCP server that resolves empty in this profile scope now takes the existing blocked_config path (incident, alert-once, visible last_status). The implicit merge of all enabled servers is not judged; only servers the job asked for.
This commit is contained in:
@@ -1473,7 +1473,11 @@ def _preflight_or_block(job: dict, job_id: str, job_name: str, cfg: dict) -> Opt
|
||||
_pf_reason = None
|
||||
if not _pf_reason:
|
||||
return None
|
||||
return _blocked_config_result(job_id, job_name, _pf_reason)
|
||||
|
||||
|
||||
def _blocked_config_result(job_id: str, job_name: str, _pf_reason: str) -> tuple:
|
||||
"""The ``blocked_config`` failure tuple for *_pf_reason*, alerting once per job."""
|
||||
logger.warning(
|
||||
"Job '%s' (ID: %s): BLOCKED by pre-dispatch config validation — %s (no LLM call was made)",
|
||||
job_name, job_id, _pf_reason)
|
||||
@@ -2164,6 +2168,12 @@ def _resolve_cron_agent_setup(job: dict, job_id: str, job_name: str, jc) -> _Cro
|
||||
setup.credential_pool = _load_credential_pool(setup.runtime, job_id)
|
||||
# MCP servers must be registered before AIAgent is constructed.
|
||||
_init_cron_mcp_tools(job_id)
|
||||
# Only now can a requested MCP toolset be judged: its alias is process-global but its tools live
|
||||
# in this profile's registry overlay, and quiet_mode hides the empty resolution (#109050).
|
||||
if _cron_preflight_enabled(_cfg):
|
||||
_mcp_reason = _empty_requested_mcp_toolsets(job, _cfg)
|
||||
if _mcp_reason:
|
||||
setup.blocked = _blocked_config_result(job_id, job_name, _mcp_reason)
|
||||
return setup
|
||||
|
||||
|
||||
@@ -3848,7 +3858,7 @@ from cron.scheduler_prompt import ( # noqa: E402
|
||||
)
|
||||
from cron.scheduler_preflight import ( # noqa: E402
|
||||
BLOCKED_CONFIG_MARKER, BLOCKED_CONFIG_SILENT_MARKER, _cron_preflight_enabled,
|
||||
_is_transient_provider_resolve_error, _preflight_job_config,
|
||||
_empty_requested_mcp_toolsets, _is_transient_provider_resolve_error, _preflight_job_config,
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -312,6 +312,29 @@ def _preflight_check_skills(job: dict) -> Optional[str]:
|
||||
return None
|
||||
|
||||
|
||||
def _empty_requested_mcp_toolsets(job: dict, cfg: dict) -> Optional[str]:
|
||||
"""Reason when an MCP server the job's own ``enabled_toolsets`` names resolves to zero tools.
|
||||
|
||||
Runs AFTER cron MCP discovery. The server's toolset alias is process-global while its tools
|
||||
are registered per profile overlay, so under a multiplexer a job can name a server that is
|
||||
connected for another profile and build a tool-less agent that ``quiet_mode`` never reports.
|
||||
Only servers the job explicitly asked for count; the implicit enabled-server merge does not.
|
||||
"""
|
||||
requested = [str(name) for name in (job.get("enabled_toolsets") or [])]
|
||||
if not requested:
|
||||
return None
|
||||
from hermes_cli.tools_config import enabled_mcp_server_names
|
||||
from toolsets import resolve_toolset
|
||||
missing = [name for name in requested
|
||||
if name in enabled_mcp_server_names(cfg) and not resolve_toolset(name)]
|
||||
if not missing:
|
||||
return None
|
||||
return (
|
||||
f"MCP server(s) {', '.join(sorted(missing))} named in this job's enabled_toolsets "
|
||||
"resolved to zero tools for this profile (not connected, or connected for another "
|
||||
"profile only). Fix the server or remove it from the job's toolsets.")
|
||||
|
||||
|
||||
def _preflight_job_config(job: dict, cfg: dict) -> Optional[str]:
|
||||
"""Pre-dispatch validation: return a reason (missing key, unconfigured delivery, unready skill)
|
||||
so the caller refuses BEFORE building agent machinery or burning an LLM call. Every check fails
|
||||
|
||||
89
tests/cron/test_cron_mcp_toolset_empty_block.py
Normal file
89
tests/cron/test_cron_mcp_toolset_empty_block.py
Normal file
@@ -0,0 +1,89 @@
|
||||
"""A cron job whose ``enabled_toolsets`` names an MCP server that resolves to zero tools is
|
||||
blocked as ``blocked_config`` instead of running tool-less and booking success (#109050).
|
||||
|
||||
Under a multiplexer the server's toolset alias is process-global while its tools live in the
|
||||
discovering profile's registry overlay, so another profile's job sees the name but no tools.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import cron.jobs as cron_jobs
|
||||
from cron.scheduler import run_job
|
||||
|
||||
_RUNTIME = {"api_key": "k", "base_url": "https://example.invalid/v1", "provider": "openrouter",
|
||||
"api_mode": "chat_completions"}
|
||||
|
||||
|
||||
def _job(**overrides):
|
||||
job = {
|
||||
"id": "mcpjob", "name": "mcp job", "prompt": "hello", "enabled": True, "state": "scheduled",
|
||||
"schedule": {"kind": "interval", "minutes": 5, "display": "every 5m"}, "deliver": "local",
|
||||
"model": None, "provider": None, "base_url": None,
|
||||
}
|
||||
job.update(overrides)
|
||||
return job
|
||||
|
||||
|
||||
def _run(job, tmp_path):
|
||||
(tmp_path / "config.yaml").write_text(
|
||||
"model:\n default: test-model\nmcp_servers:\n notion:\n url: https://mcp.invalid\n", encoding="utf-8")
|
||||
with patch("run_agent.AIAgent") as agent_cls, \
|
||||
patch("cron.scheduler._hermes_home", tmp_path), \
|
||||
patch("cron.scheduler_delivery._resolve_origin", return_value=None), \
|
||||
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
|
||||
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
|
||||
patch("hermes_state_registry.acquire", return_value=MagicMock()), \
|
||||
patch("tools.mcp_tool_discovery.discover_mcp_tools", return_value=[]), \
|
||||
patch("hermes_cli.runtime_provider.resolve_runtime_provider", return_value=dict(_RUNTIME)):
|
||||
agent_cls.return_value.run_conversation.return_value = {"final_response": "ok"}
|
||||
with cron_jobs.use_cron_store(tmp_path):
|
||||
cron_jobs.save_jobs([job])
|
||||
result = run_job(job)
|
||||
return result, agent_cls.called
|
||||
|
||||
|
||||
def _register_notion_in_scope(scope):
|
||||
from tools.registry import registry
|
||||
registry.register(
|
||||
name="mcp__notion__search", toolset="mcp-notion",
|
||||
schema={"name": "mcp__notion__search", "description": "x",
|
||||
"parameters": {"type": "object", "properties": {}}},
|
||||
handler=lambda a, **k: "{}", scope=scope)
|
||||
registry.register_toolset_alias("notion", "mcp-notion")
|
||||
return lambda: registry.deregister("mcp__notion__search", scope=scope)
|
||||
|
||||
|
||||
def test_requested_mcp_server_owned_by_other_profile_blocks_run(tmp_path):
|
||||
from agent.secret_scope import set_multiplex_active
|
||||
from hermes_constants import hermes_home_key, reset_hermes_home_override, set_hermes_home_override
|
||||
|
||||
set_multiplex_active(True)
|
||||
token = set_hermes_home_override(tmp_path / "other")
|
||||
try:
|
||||
undo = _register_notion_in_scope(hermes_home_key())
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
try:
|
||||
(success, _output, _final, error), agent_built = _run(
|
||||
_job(enabled_toolsets=["terminal", "notion"]), tmp_path)
|
||||
finally:
|
||||
undo()
|
||||
set_multiplex_active(False)
|
||||
|
||||
assert agent_built is False
|
||||
assert success is False
|
||||
assert error is not None and "[blocked_config]" in error and "notion" in error
|
||||
|
||||
|
||||
def test_requested_mcp_server_with_tools_runs(tmp_path):
|
||||
undo = _register_notion_in_scope(None)
|
||||
try:
|
||||
(success, _output, _final, error), agent_built = _run(
|
||||
_job(enabled_toolsets=["terminal", "notion"]), tmp_path)
|
||||
finally:
|
||||
undo()
|
||||
|
||||
assert agent_built is True
|
||||
assert success is True and error is None
|
||||
Reference in New Issue
Block a user