fix(tools): Prevent broken skill tool references when prefix is set and support tool_filter

- Dynamic prefixing for load_skill, load_skill_resource, run_skill_script, and search_skills when toolset has a tool_name_prefix configured.
- Respect tool_filter in SkillToolset to correctly filter core and dynamic tools using the _is_tool_selected predicate.

TAG=agy
CONV=d82d2790-2c30-4d48-9d57-3c8f340280e2

Change-Id: I591065b097b03bc6dba5a9b3cb13ca95de62ed6b

Author: wukath

src/google/adk/tools/skill_toolset.py

@@ -38,6 +38,7 @@
 from ..skills import SkillRegistry
 from .base_tool import BaseTool
 from .base_toolset import BaseToolset
+from .base_toolset import ToolPredicate
 from .function_tool import FunctionTool
 from .tool_context import ToolContext
 
@@ -56,34 +57,37 @@
     " conversation history for you to analyze."
 )
 
-DEFAULT_SKILL_SYSTEM_INSTRUCTION = (
-    "You can use specialized 'skills' to help you with complex tasks. "
-    "You MUST use the skill tools to interact with these skills.\n\n"
-    "Skills are folders of instructions and resources that extend your "
-    "capabilities for specialized tasks. Each skill folder contains:\n"
-    "- **SKILL.md** (required): The main instruction file with skill "
-    "metadata and detailed markdown instructions.\n"
-    "- **references/** (Optional): Additional documentation or examples for "
-    "skill usage.\n"
-    "- **assets/** (Optional): Templates, scripts or other resources used by "
-    "the skill.\n"
-    "- **scripts/** (Optional): Executable scripts that can be run via "
-    "bash.\n\n"
-    "This is very important:\n\n"
-    "1. If a skill seems relevant to the current user query, you MUST use "
-    'the `load_skill` tool with `skill_name="<SKILL_NAME>"` to read '
-    "its full instructions before proceeding.\n"
-    "2. Once you have read the instructions, follow them exactly as "
-    "documented before replying to the user. For example, If the "
-    "instruction lists multiple steps, please make sure you complete all "
-    "of them in order.\n"
-    "3. The `load_skill_resource` tool is for viewing files within a "
-    "skill's directory (e.g., `references/*`, `assets/*`, `scripts/*`). "
-    "Do NOT use other tools to access these files.\n"
-    "4. Use `run_skill_script` to run scripts from a skill's `scripts/` "
-    "directory. Use `load_skill_resource` to view script content first if "
-    "needed.\n"
-)
+def _build_skill_system_instruction(prefix: str | None = None) -> str:
+  p = f"{prefix}_" if prefix else ""
+
+  return (
+      "You can use specialized 'skills' to help you with complex tasks. "
+      "You MUST use the skill tools to interact with these skills.\n\n"
+      "Skills are folders of instructions and resources that extend your "
+      "capabilities for specialized tasks. Each skill folder contains:\n"
+      "- **SKILL.md** (required): The main instruction file with skill "
+      "metadata and detailed markdown instructions.\n"
+      "- **references/** (Optional): Additional documentation or examples for "
+      "skill usage.\n"
+      "- **assets/** (Optional): Templates, scripts or other resources used by "
+      "the skill.\n"
+      "- **scripts/** (Optional): Executable scripts that can be run via "
+      "bash.\n\n"
+      "This is very important:\n\n"
+      f"1. If a skill seems relevant to the current user query, you MUST use "
+      f"the `{p}load_skill` tool with `skill_name=\"<SKILL_NAME>\"` to read "
+      "its full instructions before proceeding.\n"
+      "2. Once you have read the instructions, follow them exactly as "
+      "documented before replying to the user. For example, If the "
+      "instruction lists multiple steps, please make sure you complete all "
+      "of them in order.\n"
+      f"3. The `{p}load_skill_resource` tool is for viewing files within a "
+      "skill's directory (e.g., `references/*`, `assets/*`, `scripts/*`). "
+      "Do NOT use other tools to access these files.\n"
+      f"4. Use `{p}run_skill_script` to run scripts from a skill's `scripts/` "
+      f"directory. Use `{p}load_skill_resource` to view script content first if "
+      "needed.\n"
+  )
 
 
 class ListSkillsTool(BaseTool):
@@ -899,6 +903,8 @@ def __init__(
       code_executor: BaseCodeExecutor | None = None,
       script_timeout: int = _DEFAULT_SCRIPT_TIMEOUT,
       additional_tools: list[ToolUnion] | None = None,
+      tool_name_prefix: str | None = None,
+      tool_filter: ToolPredicate | list[str] | None = None,
   ):
     """Initializes the SkillToolset.
 
@@ -911,8 +917,10 @@ def __init__(
         scripts executed via exec().
       additional_tools: Optional list of `BaseTool` or `BaseToolset` instances
         to be made available to the agent when certain skills are activated.
+      tool_name_prefix: Optional prefix to prepend to tool names.
+      tool_filter: Optional filter to select specific tools.
     """
-    super().__init__()
+    super().__init__(tool_filter=tool_filter, tool_name_prefix=tool_name_prefix)
 
     skills = skills or []
 
@@ -964,7 +972,8 @@ async def get_tools(
     dynamic_tools = await self._resolve_additional_tools_from_state(
         readonly_context
     )
-    return self._tools + dynamic_tools
+    all_tools = self._tools + dynamic_tools
+    return [t for t in all_tools if self._is_tool_selected(t, readonly_context)]
 
   async def _resolve_additional_tools_from_state(
       self, readonly_context: ReadonlyContext | None
@@ -1076,7 +1085,7 @@ async def process_llm_request(
       self, *, tool_context: ToolContext, llm_request: LlmRequest
   ) -> None:
     """Processes the outgoing LLM request to include available skills."""
-    instructions = [DEFAULT_SKILL_SYSTEM_INSTRUCTION]
+    instructions = [_build_skill_system_instruction(prefix=self.tool_name_prefix)]
 
     has_list_skills = any(isinstance(t, ListSkillsTool) for t in self._tools)
 
@@ -1086,9 +1095,10 @@ async def process_llm_request(
       instructions.append(skills_xml)
 
     if self._registry:
+      p = f"{self.tool_name_prefix}_" if self.tool_name_prefix else ""
       instructions.append(
           "\nIf the locally available skills are not sufficient to complete "
-          "your task, you can use the `search_skills` tool to discover "
+          f"your task, you can use the `{p}search_skills` tool to discover "
           "additional skills from the registry."
       )
 
@@ -1103,3 +1113,5 @@ async def close(self) -> None:
           cached.cancel()
     self._fetched_skill_cache.clear()
     await super().close()
+
+DEFAULT_SKILL_SYSTEM_INSTRUCTION = _build_skill_system_instruction()

tests/unittests/tools/test_skill_toolset.py

@@ -1917,3 +1917,90 @@ async def test_close_cancels_futures_and_clears_cache():
   assert fut1.cancelled()
   assert not fut2.cancelled()  # Done futures shouldn't/can't be cancelled
   assert not toolset._fetched_skill_cache
+
+
+@pytest.mark.asyncio
+async def test_process_llm_request_with_tool_name_prefix(
+    mock_skill1, mock_skill2, tool_context_instance, mock_registry
+):
+  toolset = skill_toolset.SkillToolset(
+      [mock_skill1, mock_skill2],
+      registry=mock_registry,
+      tool_name_prefix="my_prefix",
+  )
+
+  # Manually remove ListSkillsTool from self._tools to simulate it not being available
+  # so that instructions[1] is generated with available_skills
+  toolset._tools = [
+      t
+      for t in toolset._tools
+      if not isinstance(t, skill_toolset.ListSkillsTool)
+  ]
+
+  llm_req = mock.create_autospec(llm_request_model.LlmRequest, instance=True)
+
+  await toolset.process_llm_request(
+      tool_context=tool_context_instance, llm_request=llm_req
+  )
+
+  llm_req.append_instructions.assert_called_once()
+  args, _ = llm_req.append_instructions.call_args
+  instructions = args[0]
+  assert len(instructions) == 3
+  assert "`my_prefix_load_skill`" in instructions[0]
+  assert "`my_prefix_load_skill_resource`" in instructions[0]
+  assert "`my_prefix_run_skill_script`" in instructions[0]
+  assert "my_prefix_search_skills" in instructions[2]
+
+
+@pytest.mark.asyncio
+async def test_skill_toolset_with_list_tool_filter():
+  toolset = skill_toolset.SkillToolset(
+      tool_filter=["list_skills", "load_skill"]
+  )
+  tools = await toolset.get_tools()
+  tool_names = [t.name for t in tools]
+  assert "list_skills" in tool_names
+  assert "load_skill" in tool_names
+  assert "load_skill_resource" not in tool_names
+  assert "run_skill_script" not in tool_names
+
+
+@pytest.mark.asyncio
+async def test_skill_toolset_with_predicate_tool_filter():
+  # Filter to only tools containing 'resource' in their name
+  toolset = skill_toolset.SkillToolset(
+      tool_filter=lambda tool, ctx=None: "resource" in tool.name
+  )
+  tools = await toolset.get_tools()
+  tool_names = [t.name for t in tools]
+  assert tool_names == ["load_skill_resource"]
+
+
+@pytest.mark.asyncio
+async def test_skill_toolset_with_dynamic_tools_filter(
+    mock_skill1, tool_context_instance
+):
+  mock_skill1.frontmatter.metadata = {
+      "adk_additional_tools": ["my_custom_tool"]
+  }
+
+  custom_tool = mock.create_autospec(skill_toolset.BaseTool, instance=True)
+  custom_tool.name = "my_custom_tool"
+
+  toolset = skill_toolset.SkillToolset(
+      skills=[mock_skill1],
+      additional_tools=[custom_tool],
+      tool_filter=["list_skills", "my_custom_tool"],
+  )
+
+  state_key = "_adk_activated_skill_test_agent"
+  tool_context_instance.state.get.side_effect = lambda key, default=None: (
+      ["skill1"] if key == state_key else default
+  )
+
+  tools = await toolset.get_tools(readonly_context=tool_context_instance)
+  tool_names = [t.name for t in tools]
+  assert "list_skills" in tool_names
+  assert "my_custom_tool" in tool_names
+  assert "load_skill" not in tool_names