fix(mcp): revert ACP rebuild to original; harden generation guard
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 <get_tool_definitions return>` (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.
This commit is contained in:
parent
f3e967aae5
commit
16642e2769
2 changed files with 17 additions and 9 deletions
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue