From e24e06cbb89a9db1c43368ca45ec171662a0da12 Mon Sep 17 00:00:00 2001 From: kinolo <2279317070@qq.com> Date: Wed, 30 Sep 2026 11:40:43 +0800 Subject: [PATCH] skills: apply tool_filter in SkillToolSet.get_tools Filter both initial and cached tool lists using the current invocation context while retaining the full tool cache. Fixes trpc-group/trpc-agent-python#352 RELEASE NOTES: SkillToolSet now honors configured tool filters when is_include_all_tools is false. --- tests/skills/test_toolset.py | 63 +++++++++++++++++++++++++++++++ trpc_agent_sdk/skills/_toolset.py | 8 ++-- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/tests/skills/test_toolset.py b/tests/skills/test_toolset.py index d630d26b..1ce6a0cb 100644 --- a/tests/skills/test_toolset.py +++ b/tests/skills/test_toolset.py @@ -16,6 +16,10 @@ from unittest.mock import MagicMock +import pytest + +from trpc_agent_sdk.context import reset_invocation_ctx +from trpc_agent_sdk.context import set_invocation_ctx from trpc_agent_sdk.skills._dynamic_toolset import SkillToolSetWithDynamicTools from trpc_agent_sdk.skills._toolset import SkillToolSet @@ -74,6 +78,65 @@ async def test_get_tools_sets_metadata(self, tmp_path): ctx.agent_context.with_metadata.assert_called() +@pytest.mark.parametrize("toolset_cls", [SkillToolSet, SkillToolSetWithDynamicTools]) +class TestSkillToolSetFiltering: + + async def test_name_filter_on_first_and_cached_calls(self, tmp_path, toolset_cls): + allowed = ["skill_load", "skill_list", "workspace_exec"] + ts = toolset_cls(paths=[str(tmp_path)], tool_filter=allowed, is_include_all_tools=False) + + for _ in range(2): + tools = await ts.get_tools(_make_ctx()) + assert {tool.name for tool in tools} == set(allowed) + assert len(tools) == len(allowed) + + async def test_predicate_uses_current_context_without_filtering_cache(self, tmp_path, toolset_cls): + calls = [] + + def predicate(tool, ctx): + calls.append((tool.name, ctx)) + return tool.name in ctx.allowed_tools + + ts = toolset_cls(paths=[str(tmp_path)], tool_filter=predicate, is_include_all_tools=False) + for allowed in (set(), {"skill_run"}, {"skill_load"}): + ctx = _make_ctx() + ctx.allowed_tools = allowed + calls.clear() + tools = await ts.get_tools(ctx) + assert {tool.name for tool in tools} == allowed + assert calls + assert all(call_ctx is ctx for _, call_ctx in calls) + + async def test_predicate_receives_implicit_context(self, tmp_path, toolset_cls): + predicate = MagicMock(side_effect=lambda tool, ctx: tool.name == "skill_load") + ts = toolset_cls(paths=[str(tmp_path)], tool_filter=predicate, is_include_all_tools=False) + ctx = _make_ctx() + token = set_invocation_ctx(ctx) + try: + for _ in range(2): + predicate.reset_mock() + tools = await ts.get_tools() + assert [tool.name for tool in tools] == ["skill_load"] + assert predicate.called + assert all(call.args[1] is ctx for call in predicate.call_args_list) + finally: + reset_invocation_ctx(token) + + @pytest.mark.parametrize("tool_filter,is_include_all_tools", [ + (None, False), + ([], False), + (["skill_load"], True), + (lambda tool, ctx: False, True), + ]) + async def test_unfiltered_behavior_is_preserved(self, tmp_path, toolset_cls, tool_filter, is_include_all_tools): + ts = toolset_cls(paths=[str(tmp_path)], tool_filter=tool_filter, is_include_all_tools=is_include_all_tools) + first = [tool.name for tool in await ts.get_tools(_make_ctx())] + second = [tool.name for tool in await ts.get_tools(_make_ctx())] + assert {"skill_load", "skill_run", "skill_exec", "workspace_exec", "skill_list"} <= set(first) + assert first == second + assert len(first) == len(set(first)) + + class TestSkillToolSetWithDynamicTools: async def test_get_tools_includes_dynamic_selection_helpers(self, tmp_path): ts = SkillToolSetWithDynamicTools(paths=[str(tmp_path)]) diff --git a/trpc_agent_sdk/skills/_toolset.py b/trpc_agent_sdk/skills/_toolset.py index a975df05..6161a45b 100644 --- a/trpc_agent_sdk/skills/_toolset.py +++ b/trpc_agent_sdk/skills/_toolset.py @@ -149,10 +149,10 @@ async def get_tools(self, invocation_context: Optional[InvocationContext] = None """Get all tools from registered skills. Args: - invocation_context: Optional invocation context (not used currently) + invocation_context: Optional invocation context for metadata and tool filtering. Returns: - List of tools from all registered skills + List of tools selected for the current invocation """ if self._repo_resolver is not None: repository = self._repo_resolver(invocation_context) @@ -167,7 +167,7 @@ async def get_tools(self, invocation_context: Optional[InvocationContext] = None if not is_exist_skill_config(agent_context): set_skill_config(agent_context, self._skill_config) if self._default_tools: - return self._default_tools.copy() + return [tool for tool in self._default_tools if self._is_tool_selected(tool, invocation_context)] tools: List[ToolABC] = [] tools.append(self._load_tool) @@ -184,4 +184,4 @@ async def get_tools(self, invocation_context: Optional[InvocationContext] = None logger.warning("Failed to get tools from skill '%s': %s", skill_function.__name__, ex) continue self._default_tools.extend(tools) - return tools + return [tool for tool in tools if self._is_tool_selected(tool, invocation_context)]