fix(acp): preserve memory provider tools
This commit is contained in:
parent
2a5dc0ef3d
commit
2d474e39c7
@ -824,6 +824,7 @@ class HermesACPAgent(acp.Agent):
|
|||||||
|
|
||||||
try:
|
try:
|
||||||
from model_tools import get_tool_definitions
|
from model_tools import get_tool_definitions
|
||||||
|
from agent.memory_manager import inject_memory_provider_tools
|
||||||
|
|
||||||
enabled_toolsets = _expand_acp_enabled_toolsets(
|
enabled_toolsets = _expand_acp_enabled_toolsets(
|
||||||
getattr(state.agent, "enabled_toolsets", None) or ["hermes-acp"],
|
getattr(state.agent, "enabled_toolsets", None) or ["hermes-acp"],
|
||||||
@ -839,6 +840,7 @@ class HermesACPAgent(acp.Agent):
|
|||||||
state.agent.valid_tool_names = {
|
state.agent.valid_tool_names = {
|
||||||
tool["function"]["name"] for tool in state.agent.tools or []
|
tool["function"]["name"] for tool in state.agent.tools or []
|
||||||
}
|
}
|
||||||
|
inject_memory_provider_tools(state.agent)
|
||||||
invalidate = getattr(state.agent, "_invalidate_system_prompt", None)
|
invalidate = getattr(state.agent, "_invalidate_system_prompt", None)
|
||||||
if callable(invalidate):
|
if callable(invalidate):
|
||||||
invalidate()
|
invalidate()
|
||||||
@ -1779,10 +1781,25 @@ class HermesACPAgent(acp.Agent):
|
|||||||
def _cmd_tools(self, args: str, state: SessionState) -> str:
|
def _cmd_tools(self, args: str, state: SessionState) -> str:
|
||||||
try:
|
try:
|
||||||
from model_tools import get_tool_definitions
|
from model_tools import get_tool_definitions
|
||||||
|
from types import SimpleNamespace
|
||||||
|
from agent.memory_manager import inject_memory_provider_tools
|
||||||
|
|
||||||
toolsets = _expand_acp_enabled_toolsets(
|
toolsets = _expand_acp_enabled_toolsets(
|
||||||
getattr(state.agent, "enabled_toolsets", None) or ["hermes-acp"]
|
getattr(state.agent, "enabled_toolsets", None) or ["hermes-acp"]
|
||||||
)
|
)
|
||||||
tools = get_tool_definitions(enabled_toolsets=toolsets, quiet_mode=True)
|
tools = get_tool_definitions(enabled_toolsets=toolsets, quiet_mode=True)
|
||||||
|
tool_view = SimpleNamespace(
|
||||||
|
tools=list(tools or []),
|
||||||
|
valid_tool_names={
|
||||||
|
tool.get("function", {}).get("name")
|
||||||
|
for tool in tools or []
|
||||||
|
if isinstance(tool, dict)
|
||||||
|
},
|
||||||
|
enabled_toolsets=toolsets,
|
||||||
|
_memory_manager=getattr(state.agent, "_memory_manager", None),
|
||||||
|
)
|
||||||
|
inject_memory_provider_tools(tool_view)
|
||||||
|
tools = tool_view.tools
|
||||||
if not tools:
|
if not tools:
|
||||||
return "No tools available."
|
return "No tools available."
|
||||||
lines = [f"Available tools ({len(tools)}):"]
|
lines = [f"Available tools ({len(tools)}):"]
|
||||||
|
|||||||
@ -1193,38 +1193,8 @@ def init_agent(
|
|||||||
_ra().logger.warning("Memory provider plugin init failed: %s", _mpe)
|
_ra().logger.warning("Memory provider plugin init failed: %s", _mpe)
|
||||||
agent._memory_manager = None
|
agent._memory_manager = None
|
||||||
|
|
||||||
# Inject memory provider tool schemas into the tool surface.
|
from agent.memory_manager import inject_memory_provider_tools as _inject_memory_provider_tools
|
||||||
# Skip tools whose names already exist (plugins may register the
|
_inject_memory_provider_tools(agent)
|
||||||
# same tools via ctx.register_tool(), which lands in agent.tools
|
|
||||||
# through _ra().get_tool_definitions()). Duplicate function names cause
|
|
||||||
# 400 errors on providers that enforce unique names (e.g. Xiaomi
|
|
||||||
# MiMo via Nous Portal).
|
|
||||||
#
|
|
||||||
# Respect the platform's enabled_toolsets configuration (#5544):
|
|
||||||
# enabled_toolsets is None → no filter, inject (backward compat)
|
|
||||||
# "memory" in enabled_toolsets → user opted in, inject
|
|
||||||
# otherwise (incl. []) → user excluded memory, skip injection
|
|
||||||
#
|
|
||||||
# Without this gate, `platform_toolsets: telegram: []` still leaks memory
|
|
||||||
# provider tools (fact_store, etc.) into the tool surface — a 10x latency
|
|
||||||
# penalty on local models and a frequent trigger of tool-call loops.
|
|
||||||
if agent._memory_manager and agent.tools is not None and (
|
|
||||||
agent.enabled_toolsets is None or "memory" in agent.enabled_toolsets
|
|
||||||
):
|
|
||||||
_existing_tool_names = {
|
|
||||||
t.get("function", {}).get("name")
|
|
||||||
for t in agent.tools
|
|
||||||
if isinstance(t, dict)
|
|
||||||
}
|
|
||||||
for _schema in agent._memory_manager.get_all_tool_schemas():
|
|
||||||
_tname = _schema.get("name", "")
|
|
||||||
if _tname and _tname in _existing_tool_names:
|
|
||||||
continue # already registered via plugin path
|
|
||||||
_wrapped = {"type": "function", "function": _schema}
|
|
||||||
agent.tools.append(_wrapped)
|
|
||||||
if _tname:
|
|
||||||
agent.valid_tool_names.add(_tname)
|
|
||||||
_existing_tool_names.add(_tname)
|
|
||||||
|
|
||||||
# Skills config: nudge interval for skill creation reminders
|
# Skills config: nudge interval for skill creation reminders
|
||||||
agent._skill_nudge_interval = 10
|
agent._skill_nudge_interval = 10
|
||||||
|
|||||||
@ -44,6 +44,66 @@ logger = logging.getLogger(__name__)
|
|||||||
_SYNC_DRAIN_TIMEOUT_S = 5.0
|
_SYNC_DRAIN_TIMEOUT_S = 5.0
|
||||||
|
|
||||||
|
|
||||||
|
def memory_provider_tools_enabled(enabled_toolsets: Optional[List[str]]) -> bool:
|
||||||
|
"""Return whether external memory-provider tools should be exposed."""
|
||||||
|
if enabled_toolsets is None:
|
||||||
|
return True
|
||||||
|
if not enabled_toolsets:
|
||||||
|
return False
|
||||||
|
if "memory" in enabled_toolsets:
|
||||||
|
return True
|
||||||
|
|
||||||
|
try:
|
||||||
|
from toolsets import resolve_toolset
|
||||||
|
|
||||||
|
return any("memory" in resolve_toolset(name) for name in enabled_toolsets)
|
||||||
|
except Exception:
|
||||||
|
logger.debug("Failed to resolve enabled toolsets for memory-provider tools", exc_info=True)
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
def inject_memory_provider_tools(agent: Any) -> int:
|
||||||
|
"""Append external memory-provider tool schemas to an agent tool surface."""
|
||||||
|
memory_manager = getattr(agent, "_memory_manager", None)
|
||||||
|
tools = getattr(agent, "tools", None)
|
||||||
|
if not memory_manager or tools is None:
|
||||||
|
return 0
|
||||||
|
|
||||||
|
existing_tool_names = {
|
||||||
|
tool.get("function", {}).get("name")
|
||||||
|
for tool in tools
|
||||||
|
if isinstance(tool, dict)
|
||||||
|
}
|
||||||
|
if (
|
||||||
|
"memory" not in existing_tool_names
|
||||||
|
and not memory_provider_tools_enabled(getattr(agent, "enabled_toolsets", None))
|
||||||
|
):
|
||||||
|
return 0
|
||||||
|
|
||||||
|
get_schemas = getattr(memory_manager, "get_all_tool_schemas", None)
|
||||||
|
if not callable(get_schemas):
|
||||||
|
return 0
|
||||||
|
|
||||||
|
valid_tool_names = getattr(agent, "valid_tool_names", None)
|
||||||
|
if valid_tool_names is None:
|
||||||
|
valid_tool_names = set()
|
||||||
|
agent.valid_tool_names = valid_tool_names
|
||||||
|
|
||||||
|
added = 0
|
||||||
|
for schema in get_schemas():
|
||||||
|
if not isinstance(schema, dict):
|
||||||
|
continue
|
||||||
|
tool_name = schema.get("name", "")
|
||||||
|
if not tool_name or tool_name in existing_tool_names:
|
||||||
|
continue
|
||||||
|
tools.append({"type": "function", "function": schema})
|
||||||
|
valid_tool_names.add(tool_name)
|
||||||
|
existing_tool_names.add(tool_name)
|
||||||
|
added += 1
|
||||||
|
|
||||||
|
return added
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Context fencing helpers
|
# Context fencing helpers
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
@ -1797,6 +1797,11 @@ class TestRegisterSessionMcpServers:
|
|||||||
state.agent.tools = []
|
state.agent.tools = []
|
||||||
state.agent.valid_tool_names = set()
|
state.agent.valid_tool_names = set()
|
||||||
state.agent._cached_system_prompt = "old prompt"
|
state.agent._cached_system_prompt = "old prompt"
|
||||||
|
state.agent._memory_manager = SimpleNamespace(
|
||||||
|
get_all_tool_schemas=lambda: [
|
||||||
|
{"name": "hindsight_recall", "description": "Recall", "parameters": {}}
|
||||||
|
]
|
||||||
|
)
|
||||||
|
|
||||||
server = McpServerStdio(
|
server = McpServerStdio(
|
||||||
name="srv",
|
name="srv",
|
||||||
@ -1807,6 +1812,7 @@ class TestRegisterSessionMcpServers:
|
|||||||
|
|
||||||
fake_tools = [
|
fake_tools = [
|
||||||
{"function": {"name": "mcp_srv_search"}},
|
{"function": {"name": "mcp_srv_search"}},
|
||||||
|
{"function": {"name": "memory"}},
|
||||||
{"function": {"name": "terminal"}},
|
{"function": {"name": "terminal"}},
|
||||||
]
|
]
|
||||||
|
|
||||||
@ -1820,8 +1826,21 @@ class TestRegisterSessionMcpServers:
|
|||||||
quiet_mode=True,
|
quiet_mode=True,
|
||||||
)
|
)
|
||||||
assert state.agent.enabled_toolsets == ["hermes-acp", "mcp-srv"]
|
assert state.agent.enabled_toolsets == ["hermes-acp", "mcp-srv"]
|
||||||
assert state.agent.tools == fake_tools
|
assert state.agent.tools is fake_tools
|
||||||
assert state.agent.valid_tool_names == {"mcp_srv_search", "terminal"}
|
assert state.agent.tools[-1] == {
|
||||||
|
"type": "function",
|
||||||
|
"function": {
|
||||||
|
"name": "hindsight_recall",
|
||||||
|
"description": "Recall",
|
||||||
|
"parameters": {},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
assert state.agent.valid_tool_names == {
|
||||||
|
"hindsight_recall",
|
||||||
|
"memory",
|
||||||
|
"mcp_srv_search",
|
||||||
|
"terminal",
|
||||||
|
}
|
||||||
# _invalidate_system_prompt should have been called
|
# _invalidate_system_prompt should have been called
|
||||||
state.agent._invalidate_system_prompt.assert_called_once()
|
state.agent._invalidate_system_prompt.assert_called_once()
|
||||||
|
|
||||||
|
|||||||
@ -2,10 +2,11 @@
|
|||||||
|
|
||||||
import json
|
import json
|
||||||
import pytest
|
import pytest
|
||||||
|
from types import SimpleNamespace
|
||||||
from unittest.mock import MagicMock
|
from unittest.mock import MagicMock
|
||||||
|
|
||||||
from agent.memory_provider import MemoryProvider
|
from agent.memory_provider import MemoryProvider
|
||||||
from agent.memory_manager import MemoryManager
|
from agent.memory_manager import MemoryManager, inject_memory_provider_tools
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Concrete test provider
|
# Concrete test provider
|
||||||
@ -1320,38 +1321,25 @@ class TestMemoryToolToolsetGate:
|
|||||||
causing 10x latency on local models (Qwen3-30B: 1.7s → 42s) and
|
causing 10x latency on local models (Qwen3-30B: 1.7s → 42s) and
|
||||||
tool-call loops on small models.
|
tool-call loops on small models.
|
||||||
|
|
||||||
These tests mirror the gate logic in agent/agent_init.py around the
|
These tests exercise the shared gate used by agent init and ACP refreshes.
|
||||||
memory provider tool injection block. The gate condition is:
|
The gate condition is:
|
||||||
|
|
||||||
enabled_toolsets is None → no filter, inject (backward compat)
|
enabled_toolsets is None → no filter, inject (backward compat)
|
||||||
"memory" in enabled_toolsets → user opted in, inject
|
selected toolsets include memory → user opted in, inject
|
||||||
otherwise (incl. []) → skip injection
|
otherwise (incl. []) → skip injection
|
||||||
"""
|
"""
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _run_memory_injection(enabled_toolsets, memory_manager):
|
def _run_memory_injection(enabled_toolsets, memory_manager):
|
||||||
"""Simulate the gated memory-tool injection block from agent_init.py."""
|
"""Run the shared memory-tool injection helper against a fake agent."""
|
||||||
tools = []
|
fake_agent = SimpleNamespace(
|
||||||
valid_tool_names = set()
|
_memory_manager=memory_manager,
|
||||||
|
enabled_toolsets=enabled_toolsets,
|
||||||
if memory_manager and tools is not None and (
|
tools=[],
|
||||||
enabled_toolsets is None or "memory" in enabled_toolsets
|
valid_tool_names=set(),
|
||||||
):
|
)
|
||||||
_existing = {
|
inject_memory_provider_tools(fake_agent)
|
||||||
t.get("function", {}).get("name")
|
return fake_agent.tools, fake_agent.valid_tool_names
|
||||||
for t in tools
|
|
||||||
if isinstance(t, dict)
|
|
||||||
}
|
|
||||||
for _schema in memory_manager.get_all_tool_schemas():
|
|
||||||
_tname = _schema.get("name", "")
|
|
||||||
if _tname and _tname in _existing:
|
|
||||||
continue
|
|
||||||
tools.append({"type": "function", "function": _schema})
|
|
||||||
if _tname:
|
|
||||||
valid_tool_names.add(_tname)
|
|
||||||
_existing.add(_tname)
|
|
||||||
|
|
||||||
return tools, valid_tool_names
|
|
||||||
|
|
||||||
def _mgr_with_tools(self, *tool_names):
|
def _mgr_with_tools(self, *tool_names):
|
||||||
"""Build a MemoryManager whose providers expose the named tool schemas."""
|
"""Build a MemoryManager whose providers expose the named tool schemas."""
|
||||||
@ -1376,6 +1364,13 @@ class TestMemoryToolToolsetGate:
|
|||||||
tools, names = self._run_memory_injection(["terminal", "memory", "web"], mgr)
|
tools, names = self._run_memory_injection(["terminal", "memory", "web"], mgr)
|
||||||
assert "fact_store" in names
|
assert "fact_store" in names
|
||||||
|
|
||||||
|
def test_composite_toolset_with_memory_injects(self):
|
||||||
|
"""Composite toolsets that include memory should inject provider tools."""
|
||||||
|
mgr = self._mgr_with_tools("hindsight_recall")
|
||||||
|
tools, names = self._run_memory_injection(["hermes-acp"], mgr)
|
||||||
|
assert "hindsight_recall" in names
|
||||||
|
assert any(t["function"]["name"] == "hindsight_recall" for t in tools)
|
||||||
|
|
||||||
def test_empty_toolsets_blocks_injection(self):
|
def test_empty_toolsets_blocks_injection(self):
|
||||||
"""`platform_toolsets: telegram: []` must suppress memory tools. (#5544)"""
|
"""`platform_toolsets: telegram: []` must suppress memory tools. (#5544)"""
|
||||||
mgr = self._mgr_with_tools("fact_store")
|
mgr = self._mgr_with_tools("fact_store")
|
||||||
@ -1384,7 +1379,7 @@ class TestMemoryToolToolsetGate:
|
|||||||
assert names == set()
|
assert names == set()
|
||||||
|
|
||||||
def test_toolsets_without_memory_blocks_injection(self):
|
def test_toolsets_without_memory_blocks_injection(self):
|
||||||
"""Toolset list that doesn't name 'memory' must suppress injection."""
|
"""Toolsets that don't include memory must suppress injection."""
|
||||||
mgr = self._mgr_with_tools("fact_store")
|
mgr = self._mgr_with_tools("fact_store")
|
||||||
tools, names = self._run_memory_injection(["terminal", "web"], mgr)
|
tools, names = self._run_memory_injection(["terminal", "web"], mgr)
|
||||||
assert tools == []
|
assert tools == []
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user