mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-18 04:41:56 +00:00
4e89c53082
Wraps every sync->async coroutine-scheduling site in the codebase with a new agent.async_utils.safe_schedule_threadsafe() helper that closes the coroutine on scheduling failure (closed loop, shutdown race, etc.) instead of leaking it as 'coroutine was never awaited' RuntimeWarnings plus reference leaks. 22 production call sites migrated across the codebase: - acp_adapter/events.py, acp_adapter/permissions.py - agent/lsp/manager.py - cron/scheduler.py (media + text delivery paths) - gateway/platforms/feishu.py (5 sites, via existing _submit_on_loop helper which now delegates to safe_schedule_threadsafe) - gateway/run.py (10 sites: telegram rename, agent:step hook, status callback, interim+bg-review, clarify send, exec-approval button+text, temp-bubble cleanup, channel-directory refresh) - plugins/memory/hindsight, plugins/platforms/google_chat - tools/browser_supervisor.py (3), browser_cdp_tool.py, computer_use/cua_backend.py, slash_confirm.py - tools/environments/modal.py (_AsyncWorker) - tools/mcp_tool.py (2 + 8 _run_on_mcp_loop callers converted to factory-style so the coroutine is never constructed on a dead loop) - tui_gateway/ws.py Tests: new tests/agent/test_async_utils.py covers helper behavior under live loop, dead loop, None loop, and scheduling exceptions. Regression tests added at three PR-original sites (acp events, acp permissions, mcp loop runner) mirroring contributor's intent. Live-tested end-to-end: - Helper stress test: 1500 schedules across live/dead/race scenarios, zero leaked coroutines - Race exercised: 5000 schedules with loop killed mid-flight, 100 ok / 4900 None returns, zero leaks - hermes chat -q with terminal tool call (exercises step_callback bridge) - MCP probe against failing subprocess servers + factory path - Real gateway daemon boot + SIGINT shutdown across multiple platform adapter inits - WSTransport 100 live + 50 dead-loop writes - Cron delivery path live + dead loop Salvages PR #2657 — adopts contributor's intent over a much wider site list and a single centralized helper instead of inline try/except at each site. 3 of the original PR's 6 sites no longer exist on main (environments/patches.py deleted, DingTalk refactored to native async); the equivalent fix lives in tools/environments/modal.py instead. Co-authored-by: JithendraNara <jithendranaidunara@gmail.com>
219 lines
8.6 KiB
Python
219 lines
8.6 KiB
Python
"""Tests for probe_mcp_server_tools() in tools.mcp_tool."""
|
|
|
|
import asyncio
|
|
from types import SimpleNamespace
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _reset_mcp_state():
|
|
"""Ensure clean MCP module state before/after each test."""
|
|
import tools.mcp_tool as mcp
|
|
old_loop = mcp._mcp_loop
|
|
old_thread = mcp._mcp_thread
|
|
old_servers = dict(mcp._servers)
|
|
yield
|
|
mcp._servers.clear()
|
|
mcp._servers.update(old_servers)
|
|
mcp._mcp_loop = old_loop
|
|
mcp._mcp_thread = old_thread
|
|
|
|
|
|
class TestProbeMcpServerTools:
|
|
"""Tests for the lightweight probe_mcp_server_tools function."""
|
|
|
|
def test_returns_empty_when_mcp_not_available(self):
|
|
with patch("tools.mcp_tool._MCP_AVAILABLE", False):
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
assert result == {}
|
|
|
|
def test_returns_empty_when_no_config(self):
|
|
with patch("tools.mcp_tool._load_mcp_config", return_value={}):
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
assert result == {}
|
|
|
|
def test_returns_empty_when_all_servers_disabled(self):
|
|
config = {
|
|
"github": {"command": "npx", "enabled": False},
|
|
"slack": {"command": "npx", "enabled": "off"},
|
|
}
|
|
with patch("tools.mcp_tool._load_mcp_config", return_value=config):
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
assert result == {}
|
|
|
|
def test_returns_tools_from_successful_server(self):
|
|
"""Successfully probed server returns its tools list."""
|
|
config = {
|
|
"github": {"command": "npx", "connect_timeout": 5},
|
|
}
|
|
mock_tool_1 = SimpleNamespace(name="create_issue", description="Create a new issue")
|
|
mock_tool_2 = SimpleNamespace(name="search_repos", description="Search repositories")
|
|
|
|
mock_server = MagicMock()
|
|
mock_server._tools = [mock_tool_1, mock_tool_2]
|
|
mock_server.shutdown = AsyncMock()
|
|
|
|
async def fake_connect(name, cfg):
|
|
return mock_server
|
|
|
|
with patch("tools.mcp_tool._MCP_AVAILABLE", True), \
|
|
patch("tools.mcp_tool._load_mcp_config", return_value=config), \
|
|
patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \
|
|
patch("tools.mcp_tool._ensure_mcp_loop"), \
|
|
patch("tools.mcp_tool._run_on_mcp_loop") as mock_run, \
|
|
patch("tools.mcp_tool._stop_mcp_loop"):
|
|
|
|
# Simulate running the async probe
|
|
def run_coro(coro_or_factory, timeout=120):
|
|
coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory
|
|
loop = asyncio.new_event_loop()
|
|
try:
|
|
return loop.run_until_complete(coro)
|
|
finally:
|
|
loop.close()
|
|
|
|
mock_run.side_effect = run_coro
|
|
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
|
|
assert "github" in result
|
|
assert len(result["github"]) == 2
|
|
assert result["github"][0] == ("create_issue", "Create a new issue")
|
|
assert result["github"][1] == ("search_repos", "Search repositories")
|
|
mock_server.shutdown.assert_awaited_once()
|
|
|
|
def test_failed_server_omitted_from_results(self):
|
|
"""Servers that fail to connect are silently skipped."""
|
|
config = {
|
|
"github": {"command": "npx", "connect_timeout": 5},
|
|
"broken": {"command": "nonexistent", "connect_timeout": 5},
|
|
}
|
|
mock_tool = SimpleNamespace(name="create_issue", description="Create")
|
|
mock_server = MagicMock()
|
|
mock_server._tools = [mock_tool]
|
|
mock_server.shutdown = AsyncMock()
|
|
|
|
async def fake_connect(name, cfg):
|
|
if name == "broken":
|
|
raise ConnectionError("Server not found")
|
|
return mock_server
|
|
|
|
with patch("tools.mcp_tool._MCP_AVAILABLE", True), \
|
|
patch("tools.mcp_tool._load_mcp_config", return_value=config), \
|
|
patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \
|
|
patch("tools.mcp_tool._ensure_mcp_loop"), \
|
|
patch("tools.mcp_tool._run_on_mcp_loop") as mock_run, \
|
|
patch("tools.mcp_tool._stop_mcp_loop"):
|
|
|
|
def run_coro(coro_or_factory, timeout=120):
|
|
coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory
|
|
loop = asyncio.new_event_loop()
|
|
try:
|
|
return loop.run_until_complete(coro)
|
|
finally:
|
|
loop.close()
|
|
|
|
mock_run.side_effect = run_coro
|
|
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
|
|
assert "github" in result
|
|
assert "broken" not in result
|
|
|
|
def test_handles_tool_without_description(self):
|
|
"""Tools without descriptions get empty string."""
|
|
config = {"github": {"command": "npx", "connect_timeout": 5}}
|
|
mock_tool = SimpleNamespace(name="my_tool") # no description attribute
|
|
|
|
mock_server = MagicMock()
|
|
mock_server._tools = [mock_tool]
|
|
mock_server.shutdown = AsyncMock()
|
|
|
|
async def fake_connect(name, cfg):
|
|
return mock_server
|
|
|
|
with patch("tools.mcp_tool._MCP_AVAILABLE", True), \
|
|
patch("tools.mcp_tool._load_mcp_config", return_value=config), \
|
|
patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \
|
|
patch("tools.mcp_tool._ensure_mcp_loop"), \
|
|
patch("tools.mcp_tool._run_on_mcp_loop") as mock_run, \
|
|
patch("tools.mcp_tool._stop_mcp_loop"):
|
|
|
|
def run_coro(coro_or_factory, timeout=120):
|
|
coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory
|
|
loop = asyncio.new_event_loop()
|
|
try:
|
|
return loop.run_until_complete(coro)
|
|
finally:
|
|
loop.close()
|
|
|
|
mock_run.side_effect = run_coro
|
|
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
|
|
assert result["github"][0] == ("my_tool", "")
|
|
|
|
def test_cleanup_called_even_on_failure(self):
|
|
"""_stop_mcp_loop is called even when probe fails."""
|
|
config = {"github": {"command": "npx", "connect_timeout": 5}}
|
|
|
|
with patch("tools.mcp_tool._MCP_AVAILABLE", True), \
|
|
patch("tools.mcp_tool._load_mcp_config", return_value=config), \
|
|
patch("tools.mcp_tool._ensure_mcp_loop"), \
|
|
patch("tools.mcp_tool._run_on_mcp_loop", side_effect=RuntimeError("boom")), \
|
|
patch("tools.mcp_tool._stop_mcp_loop") as mock_stop:
|
|
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
|
|
assert result == {}
|
|
mock_stop.assert_called_once()
|
|
|
|
def test_skips_disabled_servers(self):
|
|
"""Disabled servers are not probed."""
|
|
config = {
|
|
"github": {"command": "npx", "connect_timeout": 5},
|
|
"disabled_one": {"command": "npx", "enabled": False},
|
|
}
|
|
mock_tool = SimpleNamespace(name="create_issue", description="Create")
|
|
mock_server = MagicMock()
|
|
mock_server._tools = [mock_tool]
|
|
mock_server.shutdown = AsyncMock()
|
|
|
|
connect_calls = []
|
|
|
|
async def fake_connect(name, cfg):
|
|
connect_calls.append(name)
|
|
return mock_server
|
|
|
|
with patch("tools.mcp_tool._MCP_AVAILABLE", True), \
|
|
patch("tools.mcp_tool._load_mcp_config", return_value=config), \
|
|
patch("tools.mcp_tool._connect_server", side_effect=fake_connect), \
|
|
patch("tools.mcp_tool._ensure_mcp_loop"), \
|
|
patch("tools.mcp_tool._run_on_mcp_loop") as mock_run, \
|
|
patch("tools.mcp_tool._stop_mcp_loop"):
|
|
|
|
def run_coro(coro_or_factory, timeout=120):
|
|
coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory
|
|
loop = asyncio.new_event_loop()
|
|
try:
|
|
return loop.run_until_complete(coro)
|
|
finally:
|
|
loop.close()
|
|
|
|
mock_run.side_effect = run_coro
|
|
|
|
from tools.mcp_tool import probe_mcp_server_tools
|
|
result = probe_mcp_server_tools()
|
|
|
|
assert "github" in result
|
|
assert "disabled_one" not in result
|
|
assert "disabled_one" not in connect_calls
|