From d2a073f93b6868ed2425f9ecd9b85225adc32667 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Wed, 15 Jul 2026 14:47:01 +0000 Subject: [PATCH] fix(acp): filter disabled_toolsets from /tools listing; add behavioral coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- acp_adapter/commands.py | 10 +++- tests/acp_adapter/test_server.py | 79 +++++++++++++++++++++++++++++++ tests/acp_adapter/test_session.py | 30 ++++++++++++ 3 files changed, 117 insertions(+), 2 deletions(-) diff --git a/acp_adapter/commands.py b/acp_adapter/commands.py index 12bebb01ba..4d9a606824 100644 --- a/acp_adapter/commands.py +++ b/acp_adapter/commands.py @@ -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 diff --git a/tests/acp_adapter/test_server.py b/tests/acp_adapter/test_server.py index ffb572fdfd..df8838eab2 100644 --- a/tests/acp_adapter/test_server.py +++ b/tests/acp_adapter/test_server.py @@ -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 diff --git a/tests/acp_adapter/test_session.py b/tests/acp_adapter/test_session.py index 856c232799..332362a8b2 100644 --- a/tests/acp_adapter/test_session.py +++ b/tests/acp_adapter/test_session.py @@ -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,