mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-08-13 17:16:12 +00:00
polish(mcp): simplify-pass folds on the lazy-startup salvage
Five review findings folded: - schema cache writes via utils.atomic_json_write (fsync; was bare tmp+replace), file moved to cache/mcp_schema_cache.json with 0o600 (sibling precedent: registry discovery cache) - phantom-tool reconciliation: after a lazy server's first-use connect, cached tools the live server no longer offers are deregistered (were permanent registry ghosts burning circuit-breaker strikes on every 'Unknown tool' round-trip); stale fingerprint logged - cache-load path now runs _scan_mcp_description like the eager path (cache file is user-writable JSON; defense-in-depth) - write-through skips the disk rewrite when the entry is unchanged (a flapping stdio server was rewriting byte-identical JSON per revival) - _lazy_server_fingerprints no longer write-only dead state (consumed by the reconciliation logging) 444 mcp tests green (440 pre-fold + 4 new guards); phantom-dereg and write-skip mutation-checked.
This commit is contained in:
@@ -232,11 +232,15 @@ class TestLazyFirstUseConnect:
|
||||
mock_run.assert_not_called()
|
||||
|
||||
def test_lazy_connect_success_clears_lazy_state(self):
|
||||
mcp._lazy_server_configs["playwright"] = {"command": "npx", "lazy": True}
|
||||
config = {"command": "npx", "lazy": True}
|
||||
mcp._lazy_server_configs["playwright"] = dict(config)
|
||||
mcp._lazy_server_fingerprints["playwright"] = "abc"
|
||||
mcp._lazy_server_tool_names["playwright"] = ["mcp_playwright_browser_navigate"]
|
||||
|
||||
connected = SimpleNamespace(session=MagicMock())
|
||||
connected = SimpleNamespace(
|
||||
session=MagicMock(),
|
||||
_registered_tool_names=["mcp_playwright_browser_navigate"],
|
||||
)
|
||||
|
||||
def _fake_run(coro_or_factory, timeout=30):
|
||||
mcp._servers["playwright"] = connected
|
||||
@@ -252,6 +256,37 @@ class TestLazyFirstUseConnect:
|
||||
assert "playwright" not in mcp._lazy_server_fingerprints
|
||||
assert "playwright" not in mcp._lazy_server_tool_names
|
||||
|
||||
def test_lazy_connect_deregisters_phantom_cached_tools(self):
|
||||
# Stale-cache reconciliation: the cached manifest advertised tool X,
|
||||
# but the live server only registers tool Y → X must be deregistered
|
||||
# after the first-use connect so the model stops seeing a phantom.
|
||||
from tools.registry import registry
|
||||
|
||||
mcp._lazy_server_configs["playwright"] = {"command": "npx", "lazy": True}
|
||||
mcp._lazy_server_fingerprints["playwright"] = "stale-fp"
|
||||
mcp._lazy_server_tool_names["playwright"] = [
|
||||
"mcp_playwright_tool_x",
|
||||
"mcp_playwright_tool_y",
|
||||
]
|
||||
|
||||
connected = SimpleNamespace(
|
||||
session=MagicMock(),
|
||||
_registered_tool_names=["mcp_playwright_tool_y"],
|
||||
)
|
||||
|
||||
def _fake_run(coro_or_factory, timeout=30):
|
||||
mcp._servers["playwright"] = connected
|
||||
coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory
|
||||
coro.close()
|
||||
return ["mcp_playwright_tool_y"]
|
||||
|
||||
with patch.object(mcp, "_ensure_mcp_loop"), \
|
||||
patch.object(mcp, "_run_on_mcp_loop", side_effect=_fake_run), \
|
||||
patch.object(registry, "deregister") as mock_dereg:
|
||||
assert mcp._ensure_lazy_server_connected("playwright") is True
|
||||
|
||||
mock_dereg.assert_called_once_with("mcp_playwright_tool_x")
|
||||
|
||||
def test_lazy_connect_failure_records_cooldown(self):
|
||||
mcp._lazy_server_configs["playwright"] = {"command": "npx", "lazy": True}
|
||||
|
||||
@@ -270,6 +305,21 @@ class TestLazyFirstUseConnect:
|
||||
assert "playwright" in mcp._lazy_server_configs
|
||||
|
||||
|
||||
class TestCacheLoadDescriptionScan:
|
||||
def test_scan_runs_on_cache_load_path(self):
|
||||
# Defense-in-depth: the cache file is user-writable JSON, so the
|
||||
# cache-load registration path must run the same injection scan as
|
||||
# eager discovery.
|
||||
entry = _fake_cache_entry()
|
||||
config = {"command": "npx", "args": [], "lazy": True}
|
||||
with patch.object(mcp, "_scan_mcp_description", return_value=[]) as mock_scan, \
|
||||
patch.object(mcp, "_convert_mcp_schema", side_effect=RuntimeError("stop")), \
|
||||
pytest.raises(RuntimeError):
|
||||
mcp._register_from_cache_sync("playwright", config, entry)
|
||||
|
||||
mock_scan.assert_called_once_with("playwright", "browser_navigate", "Navigate")
|
||||
|
||||
|
||||
class TestResolveServerLazy:
|
||||
def test_default_off(self):
|
||||
assert mcp._resolve_server_lazy("s", {"command": "npx"}) is False
|
||||
|
||||
@@ -72,3 +72,41 @@ class TestCacheRoundTrip:
|
||||
def test_malformed_entry_shapes_are_tolerated(self):
|
||||
assert msc.tools_from_cache_entry({"tools": "nope"}) == []
|
||||
assert msc.utility_tools_from_cache_entry({}) == []
|
||||
|
||||
|
||||
class TestCacheFileLocation:
|
||||
def test_cache_lives_under_hermes_home_cache_dir_with_0600(
|
||||
self, monkeypatch, tmp_path
|
||||
):
|
||||
# Real path (no _cache_path monkeypatch): HERMES_HOME/cache/…, 0o600,
|
||||
# matching the discovery-cache precedent in tools/registry.py.
|
||||
import hermes_constants
|
||||
|
||||
monkeypatch.setattr(hermes_constants, "get_hermes_home", lambda: tmp_path)
|
||||
path = msc._cache_path()
|
||||
assert path == tmp_path / "cache" / "mcp_schema_cache.json"
|
||||
msc.write_cache_entry("srv", "fp", tools=[], utility_tools=[])
|
||||
assert path.exists()
|
||||
assert (path.stat().st_mode & 0o777) == 0o600
|
||||
|
||||
|
||||
class TestWriteSkip:
|
||||
def test_identical_payload_skips_rewrite(self, monkeypatch, tmp_path):
|
||||
monkeypatch.setattr(msc, "_cache_path", lambda: tmp_path / "cache.json")
|
||||
saves = []
|
||||
real_save = msc._save_all
|
||||
|
||||
def _counting_save(data):
|
||||
saves.append(1)
|
||||
real_save(data)
|
||||
|
||||
monkeypatch.setattr(msc, "_save_all", _counting_save)
|
||||
tools = [{"name": "t1", "description": "d", "inputSchema": {}}]
|
||||
msc.write_cache_entry("srv", "fp1", tools=tools, utility_tools=[])
|
||||
assert len(saves) == 1
|
||||
# Identical payload (reconnect / list_changed refresh) → no rewrite.
|
||||
msc.write_cache_entry("srv", "fp1", tools=list(tools), utility_tools=[])
|
||||
assert len(saves) == 1
|
||||
# Changed payload → rewrite.
|
||||
msc.write_cache_entry("srv", "fp2", tools=tools, utility_tools=[])
|
||||
assert len(saves) == 2
|
||||
|
||||
@@ -24,7 +24,7 @@ _cache_lock = threading.Lock()
|
||||
def _cache_path() -> Path:
|
||||
from hermes_constants import get_hermes_home
|
||||
|
||||
return get_hermes_home() / _CACHE_FILENAME
|
||||
return get_hermes_home() / "cache" / _CACHE_FILENAME
|
||||
|
||||
|
||||
def config_fingerprint(config: dict) -> str:
|
||||
@@ -55,11 +55,12 @@ def _load_all() -> Dict[str, Any]:
|
||||
|
||||
|
||||
def _save_all(data: Dict[str, Any]) -> None:
|
||||
path = _cache_path()
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
tmp = path.with_suffix(".tmp")
|
||||
tmp.write_text(json.dumps(data, indent=2, ensure_ascii=False), encoding="utf-8")
|
||||
tmp.replace(path)
|
||||
from utils import atomic_json_write
|
||||
|
||||
# Cache dir + 0o600: sibling precedent in tools/registry.py
|
||||
# _save_discovery_cache; the cache file is trusted input on the lazy
|
||||
# registration path, so keep it user-only.
|
||||
atomic_json_write(_cache_path(), data, mode=0o600)
|
||||
|
||||
|
||||
def get_cached_entry(server_name: str, fingerprint: str) -> Optional[dict]:
|
||||
@@ -85,13 +86,19 @@ def write_cache_entry(
|
||||
utility_tools: Optional[List[dict]] = None,
|
||||
) -> None:
|
||||
"""Persist tool schemas after a successful live connect."""
|
||||
entry = {
|
||||
"fingerprint": fingerprint,
|
||||
"tools": tools,
|
||||
"utility_tools": utility_tools or [],
|
||||
}
|
||||
with _cache_lock:
|
||||
data = _load_all()
|
||||
data[server_name] = {
|
||||
"fingerprint": fingerprint,
|
||||
"tools": tools,
|
||||
"utility_tools": utility_tools or [],
|
||||
}
|
||||
# Write-through fires on every registration (reconnects,
|
||||
# list_changed refreshes); skip the load-all+rewrite churn when the
|
||||
# entry is byte-identical to what is already on disk.
|
||||
if data.get(server_name) == entry:
|
||||
return
|
||||
data[server_name] = entry
|
||||
_save_all(data)
|
||||
|
||||
|
||||
|
||||
@@ -4821,9 +4821,28 @@ def _ensure_lazy_server_connected(server_name: str) -> bool:
|
||||
_server_connecting.discard(server_name)
|
||||
_clear_connect_failure(server_name)
|
||||
_lazy_server_configs.pop(server_name, None)
|
||||
_lazy_server_fingerprints.pop(server_name, None)
|
||||
_lazy_server_tool_names.pop(server_name, None)
|
||||
stale_fingerprint = _lazy_server_fingerprints.pop(server_name, None)
|
||||
cached_names = _lazy_server_tool_names.pop(server_name, None) or []
|
||||
server = _servers.get(server_name)
|
||||
live_names = set(
|
||||
getattr(server, "_registered_tool_names", []) or []
|
||||
)
|
||||
# Stale-cache reconciliation: the cached manifest may advertise tools
|
||||
# the live server no longer serves. Deregister those phantoms so the
|
||||
# model stops seeing tools that can never succeed.
|
||||
phantom_names = [n for n in cached_names if n not in live_names]
|
||||
if phantom_names:
|
||||
from tools.registry import registry
|
||||
|
||||
for tool_name in phantom_names:
|
||||
registry.deregister(tool_name)
|
||||
_forget_mcp_tool_server(tool_name)
|
||||
logger.info(
|
||||
"MCP server '%s': deregistered %d phantom cached tool(s) not "
|
||||
"served live (stale schema-cache fingerprint %s): %s",
|
||||
server_name, len(phantom_names), stale_fingerprint,
|
||||
", ".join(phantom_names),
|
||||
)
|
||||
return server is not None and server.session is not None
|
||||
|
||||
|
||||
@@ -6061,6 +6080,9 @@ def _register_from_cache_sync(name: str, config: dict, entry: dict) -> List[str]
|
||||
raw.get("description") or "",
|
||||
raw_schema if isinstance(raw_schema, dict) else {},
|
||||
)
|
||||
# Defense-in-depth: the cache file is user-writable JSON, so run the
|
||||
# same injection scan the eager discovery path applies.
|
||||
_scan_mcp_description(name, mcp_tool.name, mcp_tool.description or "")
|
||||
schema = _convert_mcp_schema(name, mcp_tool)
|
||||
registry_name = schema["name"]
|
||||
existing_toolset = registry.get_toolset_for_tool(registry_name)
|
||||
|
||||
Reference in New Issue
Block a user