From 99cb74329d5911dbf1d6cd4d76d510ff3722d919 Mon Sep 17 00:00:00 2001 From: thomaslprr Date: Fri, 11 Sep 2026 16:13:13 +0200 Subject: [PATCH 1/2] feat(skills): add include_list_skills to skip the discovery turn Expose a public flag so the L1 catalog can be injected into the system instruction without mutating SkillToolset._tools. Fixes #7092 --- docs/guides/skills/skill/index.md | 18 ++++- docs/guides/skills/skill_registry/index.md | 4 +- src/google/adk/tools/skill_toolset.py | 15 +++- tests/unittests/tools/test_skill_toolset.py | 81 +++++++++++++++++++++ 4 files changed, 114 insertions(+), 4 deletions(-) diff --git a/docs/guides/skills/skill/index.md b/docs/guides/skills/skill/index.md index b1b128b3ff..bb1dda32e1 100644 --- a/docs/guides/skills/skill/index.md +++ b/docs/guides/skills/skill/index.md @@ -190,7 +190,9 @@ model: `list_skills`, `load_skill`, `load_skill_resource`, and configured. The model reaches the three levels by calling those tools in order: 1. `list_skills` returns the name and description of every installed skill, - which is level 1. + which is level 1. Pass `include_list_skills=False` on `SkillToolset` to skip + this turn: the catalog is injected as `` XML in the system + instruction, and the model can call `load_skill` directly. 2. `load_skill` returns the body of the one it picked, which is level 2. 3. `load_skill_resource` or `run_skill_script` reaches a single file or script, which is level 3. @@ -332,6 +334,20 @@ from google.adk.skills import load_skills_from_dir skills = load_skills_from_dir(pathlib.Path(__file__).parent / "skills") ``` +### Inject the L1 catalog instead of calling list_skills + +* **Problem solved**: `list_skills` costs an extra model turn before + `load_skill`. With a small, stable local catalog that tax is not worth it. +* **Implementation**: `SkillToolset(..., include_list_skills=False)` hides + the `list_skills` tool and injects the names and descriptions as + `` XML in the system instruction. The model still calls + `load_skill` for the body. Registry skills, if a registry is configured, are + still discovered only through `search_skills`. + +```python +SkillToolset(skills=[weather_skill], include_list_skills=False) +``` + ### List without loading * **Problem solved**: you want a catalog of names and descriptions without diff --git a/docs/guides/skills/skill_registry/index.md b/docs/guides/skills/skill_registry/index.md index d731f0cf01..03d675c577 100644 --- a/docs/guides/skills/skill_registry/index.md +++ b/docs/guides/skills/skill_registry/index.md @@ -203,7 +203,9 @@ per result, skipping and logging any entry that fails validation. * **A registry cannot list its whole catalog.** The interface has no `list_skills`, and the model's `list_skills` tool reports only the toolset's local skills. A model that never thinks to search never learns the registry - is there at all. + is there at all. `include_list_skills=False` injects those local skills into + the system instruction instead; registry skills are still discovered only + through `search_skills`. * **Skill names must satisfy the frontmatter rules.** A store whose keys are not kebab-case cannot round-trip through `Frontmatter` validation, so map them at the boundary. diff --git a/src/google/adk/tools/skill_toolset.py b/src/google/adk/tools/skill_toolset.py index 4903e0312a..32746cef1b 100644 --- a/src/google/adk/tools/skill_toolset.py +++ b/src/google/adk/tools/skill_toolset.py @@ -1331,6 +1331,7 @@ def __init__( additional_tools: list[ToolUnion] | None = None, tool_name_prefix: str | None = None, tool_filter: ToolPredicate | list[str] | None = None, + include_list_skills: bool = True, ): """Initializes the SkillToolset. @@ -1349,6 +1350,11 @@ def __init__( 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. + include_list_skills: Whether to expose the `list_skills` discovery tool. + When True (default), the model lists the L1 catalog through a tool + call. When False, the catalog is injected into the system instruction + as `` XML so the model can call `load_skill` + directly, without a discovery turn. """ super().__init__(tool_filter=tool_filter, tool_name_prefix=tool_name_prefix) @@ -1401,13 +1407,17 @@ def __init__( ft = FunctionTool(tool_union) self._provided_tools_by_name[ft.name] = ft - # Initialize core skill tools + # Initialize core skill tools. Omitting list_skills injects the L1 catalog + # into the system instruction in process_llm_request, so the model can call + # load_skill without a discovery turn. + self._include_list_skills = include_list_skills self._tools = [ - ListSkillsTool(self), LoadSkillTool(self), LoadSkillResourceTool(self), RunSkillScriptTool(self), ] + if include_list_skills: + self._tools.insert(0, ListSkillsTool(self)) if self._registry: self._tools.append(SearchSkillsTool(self)) @@ -1586,6 +1596,7 @@ def clone_with_updated_skills( additional_tools=additional_tools, tool_name_prefix=self.tool_name_prefix, tool_filter=self.tool_filter, + include_list_skills=self._include_list_skills, ) async def process_llm_request( diff --git a/tests/unittests/tools/test_skill_toolset.py b/tests/unittests/tools/test_skill_toolset.py index d88471e16e..6f38bb9e6d 100644 --- a/tests/unittests/tools/test_skill_toolset.py +++ b/tests/unittests/tools/test_skill_toolset.py @@ -250,6 +250,25 @@ async def test_clone_with_updated_skills_keeps_filter_and_prefix( assert clone_names == original_names == ["list_skills"] +@pytest.mark.asyncio +async def test_clone_with_updated_skills_keeps_include_list_skills( + mock_skill1, mock_skill2, tool_context_instance +): + """The clone keeps include_list_skills=False, so list_skills stays hidden.""" + toolset = skill_toolset.SkillToolset([mock_skill1], include_list_skills=False) + + new_toolset = toolset.clone_with_updated_skills([mock_skill2]) + + original_names = [ + t.name for t in await toolset.get_tools(tool_context_instance) + ] + clone_names = [ + t.name for t in await new_toolset.get_tools(tool_context_instance) + ] + assert "list_skills" not in original_names + assert clone_names == original_names + + def test_init_accepts_environment(mock_skill1): """SkillToolset stores the provided environment.""" mock_env = mock.create_autospec(BaseEnvironment, instance=True) @@ -3115,6 +3134,68 @@ async def test_process_llm_request_injects_skills_xml_when_list_skills_filtered( assert "skill2" in instructions[1] +@pytest.mark.asyncio +async def test_get_tools_omits_list_skills_when_disabled(mock_skill1): + """include_list_skills=False hides list_skills and keeps the other tools.""" + toolset = skill_toolset.SkillToolset([mock_skill1], include_list_skills=False) + + tool_names = [t.name for t in await toolset.get_tools()] + + assert "list_skills" not in tool_names + assert "load_skill" in tool_names + assert "load_skill_resource" in tool_names + assert "run_skill_script" in tool_names + + +@pytest.mark.asyncio +async def test_process_llm_request_injects_skills_xml_when_list_skills_disabled( + mock_skill1, mock_skill2, tool_context_instance +): + """include_list_skills=False injects the L1 catalog into the system prompt.""" + toolset = skill_toolset.SkillToolset( + [mock_skill1, mock_skill2], include_list_skills=False + ) + 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 + ) + + args, _ = llm_req.append_instructions.call_args + instructions = args[0] + assert len(instructions) == 2 + assert "NOT available: `list_skills`" in instructions[0] + assert "" in instructions[1] + assert "skill1" in instructions[1] + assert "skill2" in instructions[1] + + +@pytest.mark.asyncio +async def test_include_list_skills_false_keeps_search_skills( + mock_skill1, mock_registry, tool_context_instance +): + """Disabling list_skills still exposes search_skills when a registry is set.""" + toolset = skill_toolset.SkillToolset( + [mock_skill1], + registry=mock_registry, + include_list_skills=False, + ) + + tool_names = [t.name for t in await toolset.get_tools(tool_context_instance)] + assert "list_skills" not in tool_names + assert "search_skills" in tool_names + assert "load_skill" in tool_names + + 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 + ) + args, _ = llm_req.append_instructions.call_args + instructions = args[0] + assert "" in instructions[1] + assert "search_skills" in instructions[2] + + @pytest.mark.asyncio async def test_process_llm_request_omits_search_skills_hint_when_filtered( mock_registry, tool_context_instance From e09ec04b12d5a4905ba77a0c2b5b761418777124 Mon Sep 17 00:00:00 2001 From: thomaslprr Date: Mon, 14 Sep 2026 14:47:40 +0200 Subject: [PATCH 2/2] refactor(skills): rename include_list_skills to SkillDiscoveryMode Use discovery_mode=EAGER|LAZY so the API names the coupled behavior (hide list_skills + inject ). Reject non-enum values with TypeError. Fixes #7092 --- docs/guides/skills/skill/index.md | 20 +++++---- docs/guides/skills/skill_registry/index.md | 6 +-- src/google/adk/tools/skill_toolset.py | 45 ++++++++++++++++----- tests/unittests/tools/test_skill_toolset.py | 36 +++++++++++------ 4 files changed, 74 insertions(+), 33 deletions(-) diff --git a/docs/guides/skills/skill/index.md b/docs/guides/skills/skill/index.md index bb1dda32e1..a0b4697684 100644 --- a/docs/guides/skills/skill/index.md +++ b/docs/guides/skills/skill/index.md @@ -190,9 +190,10 @@ model: `list_skills`, `load_skill`, `load_skill_resource`, and configured. The model reaches the three levels by calling those tools in order: 1. `list_skills` returns the name and description of every installed skill, - which is level 1. Pass `include_list_skills=False` on `SkillToolset` to skip - this turn: the catalog is injected as `` XML in the system - instruction, and the model can call `load_skill` directly. + which is level 1. Pass `discovery_mode=SkillDiscoveryMode.EAGER` on + `SkillToolset` to skip this turn: the catalog is injected as + `` XML in the system instruction, and the model can call + `load_skill` directly. 2. `load_skill` returns the body of the one it picked, which is level 2. 3. `load_skill_resource` or `run_skill_script` reaches a single file or script, which is level 3. @@ -338,14 +339,19 @@ skills = load_skills_from_dir(pathlib.Path(__file__).parent / "skills") * **Problem solved**: `list_skills` costs an extra model turn before `load_skill`. With a small, stable local catalog that tax is not worth it. -* **Implementation**: `SkillToolset(..., include_list_skills=False)` hides - the `list_skills` tool and injects the names and descriptions as +* **Implementation**: `SkillToolset(..., discovery_mode=SkillDiscoveryMode.EAGER)` + hides the `list_skills` tool and injects the names and descriptions as `` XML in the system instruction. The model still calls `load_skill` for the body. Registry skills, if a registry is configured, are - still discovered only through `search_skills`. + still discovered only through `search_skills`. The default + `SkillDiscoveryMode.LAZY` keeps `list_skills` and discovers the catalog via + a tool call. ```python -SkillToolset(skills=[weather_skill], include_list_skills=False) +from google.adk.tools.skill_toolset import SkillDiscoveryMode +from google.adk.tools.skill_toolset import SkillToolset + +SkillToolset(skills=[weather_skill], discovery_mode=SkillDiscoveryMode.EAGER) ``` ### List without loading diff --git a/docs/guides/skills/skill_registry/index.md b/docs/guides/skills/skill_registry/index.md index 03d675c577..99339c943e 100644 --- a/docs/guides/skills/skill_registry/index.md +++ b/docs/guides/skills/skill_registry/index.md @@ -203,9 +203,9 @@ per result, skipping and logging any entry that fails validation. * **A registry cannot list its whole catalog.** The interface has no `list_skills`, and the model's `list_skills` tool reports only the toolset's local skills. A model that never thinks to search never learns the registry - is there at all. `include_list_skills=False` injects those local skills into - the system instruction instead; registry skills are still discovered only - through `search_skills`. + is there at all. `discovery_mode=SkillDiscoveryMode.EAGER` injects those + local skills into the system instruction instead; registry skills are still + discovered only through `search_skills`. * **Skill names must satisfy the frontmatter rules.** A store whose keys are not kebab-case cannot round-trip through `Frontmatter` validation, so map them at the boundary. diff --git a/src/google/adk/tools/skill_toolset.py b/src/google/adk/tools/skill_toolset.py index 32746cef1b..c12b901169 100644 --- a/src/google/adk/tools/skill_toolset.py +++ b/src/google/adk/tools/skill_toolset.py @@ -20,6 +20,7 @@ import asyncio import collections +import enum import json import logging import mimetypes @@ -73,6 +74,22 @@ _RUN_SKILL_SCRIPT_TOOL_NAME = "run_skill_script" +class SkillDiscoveryMode(enum.Enum): + """How the model learns the L1 catalog of locally registered skills. + + Omitting ``list_skills`` also injects the catalog into the system prompt, so + this enum names the discovery strategy rather than only the tool presence. + """ + + LAZY = "lazy" + """Expose ``list_skills``; the model discovers the L1 catalog via a tool call.""" + + EAGER = "eager" + """Hide ``list_skills`` and inject ```` into the system + instruction so the model can call ``load_skill`` without a discovery turn. + """ + + def _build_skill_system_instruction( prefix: str | None = None, allowed_tools: set[str] | frozenset[str] | None = None, @@ -1331,7 +1348,7 @@ def __init__( additional_tools: list[ToolUnion] | None = None, tool_name_prefix: str | None = None, tool_filter: ToolPredicate | list[str] | None = None, - include_list_skills: bool = True, + discovery_mode: SkillDiscoveryMode = SkillDiscoveryMode.LAZY, ): """Initializes the SkillToolset. @@ -1350,12 +1367,18 @@ def __init__( 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. - include_list_skills: Whether to expose the `list_skills` discovery tool. - When True (default), the model lists the L1 catalog through a tool - call. When False, the catalog is injected into the system instruction - as `` XML so the model can call `load_skill` + discovery_mode: How the model learns the L1 catalog of local skills. + ``LAZY`` (default) exposes ``list_skills``. ``EAGER`` hides + ``list_skills`` and injects the catalog into the system instruction as + ```` XML so the model can call ``load_skill`` directly, without a discovery turn. """ + if not isinstance(discovery_mode, SkillDiscoveryMode): + raise TypeError( + "discovery_mode must be a SkillDiscoveryMode, got" + f" {type(discovery_mode).__name__}." + ) + super().__init__(tool_filter=tool_filter, tool_name_prefix=tool_name_prefix) skills = skills or [] @@ -1407,16 +1430,16 @@ def __init__( ft = FunctionTool(tool_union) self._provided_tools_by_name[ft.name] = ft - # Initialize core skill tools. Omitting list_skills injects the L1 catalog - # into the system instruction in process_llm_request, so the model can call - # load_skill without a discovery turn. - self._include_list_skills = include_list_skills + # Initialize core skill tools. EAGER omits list_skills and injects the L1 + # catalog into the system instruction in process_llm_request, so the model + # can call load_skill without a discovery turn. + self._discovery_mode = discovery_mode self._tools = [ LoadSkillTool(self), LoadSkillResourceTool(self), RunSkillScriptTool(self), ] - if include_list_skills: + if discovery_mode is SkillDiscoveryMode.LAZY: self._tools.insert(0, ListSkillsTool(self)) if self._registry: self._tools.append(SearchSkillsTool(self)) @@ -1596,7 +1619,7 @@ def clone_with_updated_skills( additional_tools=additional_tools, tool_name_prefix=self.tool_name_prefix, tool_filter=self.tool_filter, - include_list_skills=self._include_list_skills, + discovery_mode=self._discovery_mode, ) async def process_llm_request( diff --git a/tests/unittests/tools/test_skill_toolset.py b/tests/unittests/tools/test_skill_toolset.py index 6f38bb9e6d..46a871c037 100644 --- a/tests/unittests/tools/test_skill_toolset.py +++ b/tests/unittests/tools/test_skill_toolset.py @@ -250,12 +250,20 @@ async def test_clone_with_updated_skills_keeps_filter_and_prefix( assert clone_names == original_names == ["list_skills"] +def test_init_rejects_non_enum_discovery_mode(mock_skill1): + """discovery_mode must be a SkillDiscoveryMode instance.""" + with pytest.raises(TypeError, match="SkillDiscoveryMode"): + skill_toolset.SkillToolset([mock_skill1], discovery_mode="eager") + + @pytest.mark.asyncio -async def test_clone_with_updated_skills_keeps_include_list_skills( +async def test_clone_with_updated_skills_keeps_discovery_mode( mock_skill1, mock_skill2, tool_context_instance ): - """The clone keeps include_list_skills=False, so list_skills stays hidden.""" - toolset = skill_toolset.SkillToolset([mock_skill1], include_list_skills=False) + """The clone keeps discovery_mode=EAGER, so list_skills stays hidden.""" + toolset = skill_toolset.SkillToolset( + [mock_skill1], discovery_mode=skill_toolset.SkillDiscoveryMode.EAGER + ) new_toolset = toolset.clone_with_updated_skills([mock_skill2]) @@ -267,6 +275,7 @@ async def test_clone_with_updated_skills_keeps_include_list_skills( ] assert "list_skills" not in original_names assert clone_names == original_names + assert new_toolset._discovery_mode is skill_toolset.SkillDiscoveryMode.EAGER def test_init_accepts_environment(mock_skill1): @@ -3135,9 +3144,11 @@ async def test_process_llm_request_injects_skills_xml_when_list_skills_filtered( @pytest.mark.asyncio -async def test_get_tools_omits_list_skills_when_disabled(mock_skill1): - """include_list_skills=False hides list_skills and keeps the other tools.""" - toolset = skill_toolset.SkillToolset([mock_skill1], include_list_skills=False) +async def test_get_tools_omits_list_skills_when_eager(mock_skill1): + """EAGER hides list_skills and keeps the other tools.""" + toolset = skill_toolset.SkillToolset( + [mock_skill1], discovery_mode=skill_toolset.SkillDiscoveryMode.EAGER + ) tool_names = [t.name for t in await toolset.get_tools()] @@ -3148,12 +3159,13 @@ async def test_get_tools_omits_list_skills_when_disabled(mock_skill1): @pytest.mark.asyncio -async def test_process_llm_request_injects_skills_xml_when_list_skills_disabled( +async def test_process_llm_request_injects_skills_xml_when_eager( mock_skill1, mock_skill2, tool_context_instance ): - """include_list_skills=False injects the L1 catalog into the system prompt.""" + """EAGER injects the L1 catalog into the system prompt.""" toolset = skill_toolset.SkillToolset( - [mock_skill1, mock_skill2], include_list_skills=False + [mock_skill1, mock_skill2], + discovery_mode=skill_toolset.SkillDiscoveryMode.EAGER, ) llm_req = mock.create_autospec(llm_request_model.LlmRequest, instance=True) @@ -3171,14 +3183,14 @@ async def test_process_llm_request_injects_skills_xml_when_list_skills_disabled( @pytest.mark.asyncio -async def test_include_list_skills_false_keeps_search_skills( +async def test_eager_discovery_keeps_search_skills( mock_skill1, mock_registry, tool_context_instance ): - """Disabling list_skills still exposes search_skills when a registry is set.""" + """EAGER still exposes search_skills when a registry is set.""" toolset = skill_toolset.SkillToolset( [mock_skill1], registry=mock_registry, - include_list_skills=False, + discovery_mode=skill_toolset.SkillDiscoveryMode.EAGER, ) tool_names = [t.name for t in await toolset.get_tools(tool_context_instance)]