From ebf967ff2cecdc040cd9701b0cf52059c7e8da4b Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:15:52 +0530 Subject: [PATCH] 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. --- tests/tools/test_mcp_lazy_start.py | 54 ++++++++++++++++++++++++++-- tests/tools/test_mcp_schema_cache.py | 38 ++++++++++++++++++++ tools/mcp_schema_cache.py | 29 +++++++++------ tools/mcp_tool.py | 26 ++++++++++++-- 4 files changed, 132 insertions(+), 15 deletions(-) diff --git a/tests/tools/test_mcp_lazy_start.py b/tests/tools/test_mcp_lazy_start.py index 5dddfe6935bf2..85312c0fc2a13 100644 --- a/tests/tools/test_mcp_lazy_start.py +++ b/tests/tools/test_mcp_lazy_start.py @@ -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 diff --git a/tests/tools/test_mcp_schema_cache.py b/tests/tools/test_mcp_schema_cache.py index ab54cb74dd09a..cc6df9a6d29cd 100644 --- a/tests/tools/test_mcp_schema_cache.py +++ b/tests/tools/test_mcp_schema_cache.py @@ -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 diff --git a/tools/mcp_schema_cache.py b/tools/mcp_schema_cache.py index a0c95033be07b..0fef2cdc32c8a 100644 --- a/tools/mcp_schema_cache.py +++ b/tools/mcp_schema_cache.py @@ -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) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 28db756791dd4..993d9a13c80f5 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -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)