From 16642e2769e2b9ea6490756ef3491384cec9b58b Mon Sep 17 00:00:00 2001 From: alt-glitch Date: Fri, 19 Jun 2026 23:27:18 +0530 Subject: [PATCH] fix(mcp): revert ACP rebuild to original; harden generation guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught 3 ACP test failures (tests/acp/test_server.py, tests/acp/test_mcp_e2e.py). Root cause: routing ACP's tool-surface rebuild through the shared refresh_agent_mcp_tools helper (added in the round-2 pass) broke a deliberate, pre-existing ACP contract: - the ACP tests assert `agent.tools is ` (object identity) and an exact get_tool_definitions(enabled_toolsets=[...], disabled_toolsets=..., quiet_mode=True) call signature; the shared helper list()-copies and re-derives differently, breaking identity; and - the tests use a MagicMock agent whose _tool_snapshot_generation is a mock, so the new `int < published_gen` generation guard raised TypeError and the whole ACP refresh silently failed. ACP already preserves memory-provider tools (its own inject call) and excludes context_engine, so there was no bug to fix there — only over-reach. Reverted ACP to its original rebuild. (Same lesson as the gateway path: leave call sites that carry their own tested contract alone; a reviewer's "inert today, fragile" note meant leave-it, not change-it.) Also hardened the generation guard defensively: tolerate a non-int _tool_snapshot_generation (mock / partially-built agent) instead of throwing TypeError and silently failing the refresh. --- acp_adapter/server.py | 19 +++++++++++-------- tools/mcp_tool.py | 7 ++++++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/acp_adapter/server.py b/acp_adapter/server.py index 7b0129bc2..a51db91d4 100644 --- a/acp_adapter/server.py +++ b/acp_adapter/server.py @@ -823,21 +823,24 @@ class HermesACPAgent(acp.Agent): return try: - from tools.mcp_tool import refresh_agent_mcp_tools + from model_tools import get_tool_definitions + from agent.memory_manager import inject_memory_provider_tools enabled_toolsets = _expand_acp_enabled_toolsets( getattr(state.agent, "enabled_toolsets", None) or ["hermes-acp"], mcp_server_names=[server.name for server in mcp_servers], ) - # Route through the shared helper (name-diff, atomic publish, and — - # critically — additive-preserving so memory-provider AND context- - # engine tools survive). enabled_override applies the ACP-expanded - # toolset and stores it on the agent, matching prior behavior. - refresh_agent_mcp_tools( - state.agent, - enabled_override=enabled_toolsets, + state.agent.enabled_toolsets = enabled_toolsets + disabled_toolsets = getattr(state.agent, "disabled_toolsets", None) + state.agent.tools = get_tool_definitions( + enabled_toolsets=enabled_toolsets, + disabled_toolsets=disabled_toolsets, quiet_mode=True, ) + state.agent.valid_tool_names = { + tool["function"]["name"] for tool in state.agent.tools or [] + } + inject_memory_provider_tools(state.agent) invalidate = getattr(state.agent, "_invalidate_system_prompt", None) if callable(invalidate): invalidate() diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 4b021e499..48cb39085 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -4343,7 +4343,12 @@ def refresh_agent_mcp_tools( # with what was actually published, even under concurrent callers, and a # stale (older-generation) rebuild can't overwrite a newer published one. with _agent_tools_lock: - published_gen = getattr(agent, "_tool_snapshot_generation", -1) + # Defensive: the published generation should be an int, but tolerate an + # agent that never set it (or set a non-int, e.g. a test mock) rather + # than throwing TypeError on the comparison and silently failing the + # whole refresh. + published_gen_raw = getattr(agent, "_tool_snapshot_generation", -1) + published_gen = published_gen_raw if isinstance(published_gen_raw, int) else -1 if snapshot_generation < published_gen: # A newer snapshot already won; our set is stale — drop it. return set()