fix(acp): filter disabled_toolsets from /tools listing; add behavioral coverage
Review follow-up: _cmd_tools rebuilt its listing without state.agent.disabled_toolsets, so a config-disabled toolset was filtered from execution but still advertised by /tools. Pass it through, matching the session tool-surface rebuild. New tests exercise the real get_tool_definitions (no patching) and assert a disabled toolset is absent from both the /tools listing and the rebuilt valid_tool_names — with a baseline assertion that the toolset is present when nothing is disabled, so the check cannot pass vacuously.
This commit is contained in:
@@ -157,11 +157,17 @@ class SlashCommandsMixin:
|
||||
from agent.memory_manager import inject_memory_provider_tools
|
||||
|
||||
toolsets = _expand_acp_enabled_toolsets(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,
|
||||
disabled_toolsets=getattr(state.agent, "disabled_toolsets", None),
|
||||
quiet_mode=True,
|
||||
)
|
||||
tool_view = SimpleNamespace(
|
||||
tools=list(tools or []),
|
||||
valid_tool_names={t.get("function", {}).get("name") for t in tools or [] if isinstance(t, dict)},
|
||||
enabled_toolsets=toolsets, _memory_manager=getattr(state.agent, "_memory_manager", None),
|
||||
enabled_toolsets=toolsets,
|
||||
disabled_toolsets=getattr(state.agent, "disabled_toolsets", None),
|
||||
_memory_manager=getattr(state.agent, "_memory_manager", None),
|
||||
)
|
||||
inject_memory_provider_tools(tool_view)
|
||||
tools = tool_view.tools
|
||||
|
||||
@@ -748,3 +748,82 @@ class TestRegisterSessionMcpServers:
|
||||
with patch("tools.mcp_tool_discovery.register_mcp_servers", side_effect=RuntimeError("boom")):
|
||||
# Should not raise
|
||||
await agent._register_session_mcp_servers(state, [server])
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# disabled_toolsets filter the ACP tool surface (real get_tool_definitions)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestDisabledToolsetsFilterToolSurface:
|
||||
"""Config-disabled toolsets must be absent from ACP-generated definitions.
|
||||
|
||||
These run the real ``get_tool_definitions`` (no patching) so they cover
|
||||
the actual filtering behavior, not just which kwargs were forwarded.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _listed_names(listing: str) -> set:
|
||||
return {
|
||||
line.strip().split(":", 1)[0]
|
||||
for line in listing.splitlines()[1:]
|
||||
}
|
||||
|
||||
def test_cmd_tools_strips_configured_disabled_toolsets(self, agent, mock_manager):
|
||||
state = mock_manager.create_session(cwd="/tmp")
|
||||
state.agent.enabled_toolsets = ["hermes-acp"]
|
||||
state.agent._memory_manager = None
|
||||
|
||||
state.agent.disabled_toolsets = None
|
||||
baseline = agent._cmd_tools("", state)
|
||||
assert baseline.startswith("Available tools"), baseline
|
||||
assert "execute_code" in self._listed_names(baseline)
|
||||
|
||||
state.agent.disabled_toolsets = ["code_execution"]
|
||||
filtered = agent._cmd_tools("", state)
|
||||
assert filtered.startswith("Available tools"), filtered
|
||||
assert "execute_code" not in self._listed_names(filtered)
|
||||
|
||||
def test_cmd_tools_hides_memory_provider_tools_when_memory_disabled(self, agent, mock_manager):
|
||||
"""Disabling the memory toolset must also withhold external provider tools.
|
||||
|
||||
``get_tool_definitions`` drops the built-in ``memory`` tool, but
|
||||
``inject_memory_provider_tools`` re-adds provider schemas unless the
|
||||
view it is handed also carries ``disabled_toolsets``.
|
||||
"""
|
||||
state = mock_manager.create_session(cwd="/tmp")
|
||||
state.agent.enabled_toolsets = ["hermes-acp"]
|
||||
state.agent._memory_manager = SimpleNamespace(
|
||||
providers=[SimpleNamespace(name="holo")],
|
||||
get_all_tool_schemas=lambda: [
|
||||
{"name": "fact_store", "description": "d", "parameters": {}}
|
||||
],
|
||||
)
|
||||
|
||||
state.agent.disabled_toolsets = None
|
||||
assert "fact_store" in self._listed_names(agent._cmd_tools("", state))
|
||||
|
||||
state.agent.disabled_toolsets = ["memory"]
|
||||
assert "fact_store" not in self._listed_names(agent._cmd_tools("", state))
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mcp_tool_rebuild_strips_configured_disabled_toolsets(
|
||||
self, agent, mock_manager
|
||||
):
|
||||
from acp.schema import McpServerStdio
|
||||
|
||||
state = mock_manager.create_session(cwd="/tmp")
|
||||
state.agent.enabled_toolsets = ["hermes-acp"]
|
||||
state.agent.disabled_toolsets = ["code_execution"]
|
||||
state.agent.tools = []
|
||||
state.agent.valid_tool_names = set()
|
||||
state.agent._memory_manager = None
|
||||
|
||||
server = McpServerStdio(name="srv", command="/bin/test", args=[], env=[])
|
||||
|
||||
with patch("tools.mcp_tool_discovery.register_mcp_servers", return_value=[]) as register:
|
||||
await agent._register_session_mcp_servers(state, [server])
|
||||
|
||||
assert register.called, "MCP registration was not intercepted"
|
||||
assert state.agent.valid_tool_names, "tool surface rebuild produced no tools"
|
||||
assert "execute_code" not in state.agent.valid_tool_names
|
||||
|
||||
@@ -229,6 +229,36 @@ class TestCreateSession:
|
||||
|
||||
assert state.agent.kwargs.get("disabled_toolsets") == ["todo", "browser"]
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"configured, expected",
|
||||
[
|
||||
("code_execution", ["code_execution"]), # scalar string = one name
|
||||
("['todo', 'browser']", ["todo", "browser"]), # `hermes config set` stores JSON strings
|
||||
([" todo ", "", "browser"], ["todo", "browser"]),
|
||||
],
|
||||
)
|
||||
def test_make_agent_normalizes_disabled_toolsets_string_shapes(
|
||||
self, monkeypatch, configured, expected
|
||||
):
|
||||
"""A bare ``list()`` would explode a string into single characters.
|
||||
|
||||
``hermes config set`` persists lists as quoted JSON strings and a scalar
|
||||
string is a valid one-name shape, so this must go through
|
||||
``parse_config_string_list`` the way ``hermes_cli/tools_config.py`` does.
|
||||
"""
|
||||
self._patch_make_agent_env(
|
||||
monkeypatch,
|
||||
{
|
||||
"model": {"default": "fake-model", "provider": "fake-provider"},
|
||||
"mcp_servers": {},
|
||||
"agent": {"disabled_toolsets": configured},
|
||||
},
|
||||
)
|
||||
|
||||
state = SessionManager(db=None).create_session(cwd="/tmp/project")
|
||||
|
||||
assert state.agent.kwargs.get("disabled_toolsets") == expected
|
||||
|
||||
def test_make_agent_omits_disabled_toolsets_when_none_configured(self, monkeypatch):
|
||||
self._patch_make_agent_env(
|
||||
monkeypatch,
|
||||
|
||||
Reference in New Issue
Block a user