diff --git a/contributing/samples/skills_agent/agent.py b/contributing/samples/skills_agent/agent.py index 39eec53c8f..9caf0ad752 100644 --- a/contributing/samples/skills_agent/agent.py +++ b/contributing/samples/skills_agent/agent.py @@ -19,7 +19,7 @@ from google.adk import Agent from google.adk.skills import load_skill_from_dir from google.adk.skills import models -from google.adk.tools import skill_toolset +from google.adk.tools.skill_toolset import SkillToolset greeting_skill = models.Skill( frontmatter=models.Frontmatter( @@ -41,18 +41,15 @@ ) weather_skill = load_skill_from_dir( - pathlib.Path(__file__).parent / "skills" / "weather_skill" + pathlib.Path(__file__).parent / "skills" / "weather-skill" ) -my_skill_toolset = skill_toolset.SkillToolset( - skills=[greeting_skill, weather_skill] -) +my_skill_toolset = SkillToolset(skills=[greeting_skill, weather_skill]) root_agent = Agent( model="gemini-2.5-flash", name="skill_user_agent", description="An agent that can use specialized skills.", - instruction=skill_toolset.DEFAULT_SKILL_SYSTEM_INSTRUCTION, tools=[ my_skill_toolset, ], diff --git a/contributing/samples/skills_agent/skills/weather_skill/SKILL.md b/contributing/samples/skills_agent/skills/weather-skill/SKILL.md similarity index 100% rename from contributing/samples/skills_agent/skills/weather_skill/SKILL.md rename to contributing/samples/skills_agent/skills/weather-skill/SKILL.md diff --git a/contributing/samples/skills_agent/skills/weather_skill/references/weather_info.md b/contributing/samples/skills_agent/skills/weather-skill/references/weather_info.md similarity index 100% rename from contributing/samples/skills_agent/skills/weather_skill/references/weather_info.md rename to contributing/samples/skills_agent/skills/weather-skill/references/weather_info.md diff --git a/src/google/adk/skills/__init__.py b/src/google/adk/skills/__init__.py index 73184b2b1b..8c0cf91baf 100644 --- a/src/google/adk/skills/__init__.py +++ b/src/google/adk/skills/__init__.py @@ -19,6 +19,8 @@ from .models import Script from .models import Skill from .utils import load_skill_from_dir +from .utils import read_skill_properties +from .utils import validate_skill_dir __all__ = [ "Frontmatter", @@ -26,4 +28,6 @@ "Script", "Skill", "load_skill_from_dir", + "read_skill_properties", + "validate_skill_dir", ] diff --git a/src/google/adk/skills/models.py b/src/google/adk/skills/models.py index 7f5d75b453..e138ee4bf2 100644 --- a/src/google/adk/skills/models.py +++ b/src/google/adk/skills/models.py @@ -16,9 +16,16 @@ from __future__ import annotations +import re from typing import Optional +import unicodedata from pydantic import BaseModel +from pydantic import ConfigDict +from pydantic import Field +from pydantic import field_validator + +_NAME_PATTERN = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*$") class Frontmatter(BaseModel): @@ -30,18 +37,58 @@ class Frontmatter(BaseModel): (required). license: License for the skill (optional). compatibility: Compatibility information for the skill (optional). - allowed_tools: Tool patterns the skill requires (optional, experimental). + allowed_tools: Tool patterns the skill requires (optional, + experimental). Accepts both ``allowed_tools`` and the YAML-friendly + ``allowed-tools`` key. metadata: Key-value pairs for client-specific properties (defaults to empty dict). """ + model_config = ConfigDict( + extra="allow", + populate_by_name=True, + ) + name: str description: str license: Optional[str] = None compatibility: Optional[str] = None - allowed_tools: Optional[str] = None + allowed_tools: Optional[str] = Field( + default=None, + alias="allowed-tools", + serialization_alias="allowed-tools", + ) metadata: dict[str, str] = {} + @field_validator("name") + @classmethod + def _validate_name(cls, v: str) -> str: + v = unicodedata.normalize("NFKC", v) + if len(v) > 64: + raise ValueError("name must be at most 64 characters") + if not _NAME_PATTERN.match(v): + raise ValueError( + "name must be lowercase kebab-case (a-z, 0-9, hyphens)," + " with no leading, trailing, or consecutive hyphens" + ) + return v + + @field_validator("description") + @classmethod + def _validate_description(cls, v: str) -> str: + if not v: + raise ValueError("description must not be empty") + if len(v) > 1024: + raise ValueError("description must be at most 1024 characters") + return v + + @field_validator("compatibility") + @classmethod + def _validate_compatibility(cls, v: Optional[str]) -> Optional[str]: + if v is not None and len(v) > 500: + raise ValueError("compatibility must be at most 500 characters") + return v + class Script(BaseModel): """Wrapper for script content.""" @@ -131,11 +178,13 @@ class Skill(BaseModel): frontmatter: Parsed skill frontmatter from SKILL.md. instructions: L2 skill content: markdown instruction from SKILL.md body. resources: L3 skill content: additional instructions, assets, and scripts. + source_path: Filesystem path to the SKILL.md that was loaded (optional). """ frontmatter: Frontmatter instructions: str resources: Resources = Resources() + source_path: Optional[str] = None @property def name(self) -> str: diff --git a/src/google/adk/skills/prompt.py b/src/google/adk/skills/prompt.py index e9840ab2f0..fdc508d302 100644 --- a/src/google/adk/skills/prompt.py +++ b/src/google/adk/skills/prompt.py @@ -18,19 +18,22 @@ import html from typing import List +from typing import Union from . import models -def format_skills_as_xml(skills: List[models.Frontmatter]) -> str: +def format_skills_as_xml( + skills: List[Union[models.Frontmatter, models.Skill]], +) -> str: """Formats available skills into a standard XML string. Args: - skills: A list of skill frontmatter objects. + skills: A list of skill frontmatter or full skill objects. Returns: XML string with block containing each skill's - name and description. + name, description, and optionally source location. """ if not skills: @@ -38,14 +41,22 @@ def format_skills_as_xml(skills: List[models.Frontmatter]) -> str: lines = [""] - for skill in skills: + for item in skills: + source_path = None + if isinstance(item, models.Skill): + source_path = item.source_path + lines.append("") lines.append("") - lines.append(html.escape(skill.name)) + lines.append(html.escape(item.name)) lines.append("") lines.append("") - lines.append(html.escape(skill.description)) + lines.append(html.escape(item.description)) lines.append("") + if source_path is not None: + lines.append("") + lines.append(html.escape(source_path)) + lines.append("") lines.append("") lines.append("") diff --git a/src/google/adk/skills/utils.py b/src/google/adk/skills/utils.py index deb10b2a21..7a756e540a 100644 --- a/src/google/adk/skills/utils.py +++ b/src/google/adk/skills/utils.py @@ -23,6 +23,15 @@ from . import models +_ALLOWED_FRONTMATTER_KEYS = frozenset({ + "name", + "description", + "license", + "allowed-tools", + "metadata", + "compatibility", +}) + def _load_dir(directory: pathlib.Path) -> dict[str, str]: """Recursively load files from a directory into a dictionary. @@ -48,21 +57,21 @@ def _load_dir(directory: pathlib.Path) -> dict[str, str]: return files -def load_skill_from_dir(skill_dir: Union[str, pathlib.Path]) -> models.Skill: - """Load a complete skill from a directory. +def _parse_skill_md( + skill_dir: pathlib.Path, +) -> tuple[dict, str, pathlib.Path]: + """Parse SKILL.md from a skill directory. Args: - skill_dir: Path to the skill directory. + skill_dir: Resolved path to the skill directory. Returns: - Skill object with all components loaded. + Tuple of (parsed_frontmatter_dict, body_string, skill_md_path). Raises: - FileNotFoundError: If the skill directory or SKILL.md is not found. + FileNotFoundError: If the directory or SKILL.md is not found. ValueError: If SKILL.md is invalid. """ - skill_dir = pathlib.Path(skill_dir).resolve() - if not skill_dir.is_dir(): raise FileNotFoundError(f"Skill directory '{skill_dir}' not found.") @@ -95,8 +104,36 @@ def load_skill_from_dir(skill_dir: Union[str, pathlib.Path]) -> models.Skill: if not isinstance(parsed, dict): raise ValueError("SKILL.md frontmatter must be a YAML mapping") - # Frontmatter class handles required field validation - frontmatter = models.Frontmatter(**parsed) + return parsed, body, skill_md + + +def load_skill_from_dir(skill_dir: Union[str, pathlib.Path]) -> models.Skill: + """Load a complete skill from a directory. + + Args: + skill_dir: Path to the skill directory. + + Returns: + Skill object with all components loaded. + + Raises: + FileNotFoundError: If the skill directory or SKILL.md is not found. + ValueError: If SKILL.md is invalid or the skill name does not match + the directory name. + """ + skill_dir = pathlib.Path(skill_dir).resolve() + + parsed, body, skill_md = _parse_skill_md(skill_dir) + + # Use model_validate to handle aliases like allowed-tools + frontmatter = models.Frontmatter.model_validate(parsed) + + # Validate that skill name matches the directory name + if skill_dir.name != frontmatter.name: + raise ValueError( + f"Skill name '{frontmatter.name}' does not match directory" + f" name '{skill_dir.name}'." + ) references = _load_dir(skill_dir / "references") assets = _load_dir(skill_dir / "assets") @@ -115,4 +152,83 @@ def load_skill_from_dir(skill_dir: Union[str, pathlib.Path]) -> models.Skill: frontmatter=frontmatter, instructions=body, resources=resources, + source_path=str(skill_md), ) + + +def validate_skill_dir( + skill_dir: Union[str, pathlib.Path], +) -> list[str]: + """Validate a skill directory without fully loading it. + + Checks that the directory exists, contains a valid SKILL.md with correct + frontmatter, and that the skill name matches the directory name. + + Args: + skill_dir: Path to the skill directory. + + Returns: + List of problem strings. Empty list means the skill is valid. + """ + problems: list[str] = [] + skill_dir = pathlib.Path(skill_dir).resolve() + + if not skill_dir.exists(): + return [f"Directory '{skill_dir}' does not exist."] + if not skill_dir.is_dir(): + return [f"'{skill_dir}' is not a directory."] + + skill_md = None + for name in ("SKILL.md", "skill.md"): + path = skill_dir / name + if path.exists(): + skill_md = path + break + if skill_md is None: + return [f"SKILL.md not found in '{skill_dir}'."] + + try: + parsed, _, _ = _parse_skill_md(skill_dir) + except (FileNotFoundError, ValueError) as e: + return [str(e)] + + unknown = set(parsed.keys()) - _ALLOWED_FRONTMATTER_KEYS + if unknown: + problems.append(f"Unknown frontmatter fields: {sorted(unknown)}") + + try: + frontmatter = models.Frontmatter.model_validate(parsed) + except Exception as e: + problems.append(f"Frontmatter validation error: {e}") + return problems + + if skill_dir.name != frontmatter.name: + problems.append( + f"Skill name '{frontmatter.name}' does not match directory" + f" name '{skill_dir.name}'." + ) + + return problems + + +def read_skill_properties( + skill_dir: Union[str, pathlib.Path], +) -> models.Frontmatter: + """Read only the frontmatter properties from a skill directory. + + This is a lightweight alternative to ``load_skill_from_dir`` when you + only need the skill metadata without loading instructions or resources. + + Args: + skill_dir: Path to the skill directory. + + Returns: + Frontmatter object with the skill's metadata. + + Raises: + FileNotFoundError: If the directory or SKILL.md is not found. + ValueError: If the frontmatter is invalid. + """ + skill_dir = pathlib.Path(skill_dir).resolve() + parsed, _, _ = _parse_skill_md(skill_dir) + return models.Frontmatter.model_validate(parsed) diff --git a/src/google/adk/tools/skill_toolset.py b/src/google/adk/tools/skill_toolset.py index 34cad5c58b..0555441284 100644 --- a/src/google/adk/tools/skill_toolset.py +++ b/src/google/adk/tools/skill_toolset.py @@ -39,12 +39,13 @@ - **SKILL.md** (required): The main instruction file with skill metadata and detailed markdown instructions. - **references/** (Optional): Additional documentation or examples for skill usage. - **assets/** (Optional): Templates, scripts or other resources used by the skill. +- **scripts/** (Optional): Executable scripts that can be run via bash. This is very important: 1. If a skill seems relevant to the current user query, you MUST use the `load_skill` tool with `name=""` to read its full instructions before proceeding. 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. -3. The `load_skill_resource` tool is for viewing files within a skill's directory (e.g., `references/*`, `assets/*`). Do NOT use other tools to access these files. +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. """ @@ -74,8 +75,8 @@ def _get_declaration(self) -> types.FunctionDeclaration | None: async def run_async( self, *, args: dict[str, Any], tool_context: ToolContext ) -> Any: - skill_frontmatters = self._toolset._list_skills() - return prompt.format_skills_as_xml(skill_frontmatters) + skills = self._toolset._list_skills() + return prompt.format_skills_as_xml(skills) @experimental(FeatureName.SKILL_TOOLSET) @@ -131,14 +132,14 @@ async def run_async( @experimental(FeatureName.SKILL_TOOLSET) class LoadSkillResourceTool(BaseTool): - """Tool to load resources (references or assets) from a skill.""" + """Tool to load resources (references, assets, or scripts) from a skill.""" def __init__(self, toolset: "SkillToolset"): super().__init__( name="load_skill_resource", description=( - "Loads a resource file (from references/ or assets/) from within a" - " skill." + "Loads a resource file (from references/, assets/, or" + " scripts/) from within a skill." ), ) self._toolset = toolset @@ -158,7 +159,8 @@ def _get_declaration(self) -> types.FunctionDeclaration | None: "type": "string", "description": ( "The relative path to the resource (e.g.," - " 'references/my_doc.md' or 'assets/template.txt')." + " 'references/my_doc.md', 'assets/template.txt'," + " or 'scripts/setup.sh')." ), }, }, @@ -197,9 +199,16 @@ async def run_async( elif resource_path.startswith("assets/"): asset_name = resource_path[len("assets/") :] content = skill.resources.get_asset(asset_name) + elif resource_path.startswith("scripts/"): + script_name = resource_path[len("scripts/") :] + script = skill.resources.get_script(script_name) + if script is not None: + content = script.src else: return { - "error": "Path must start with 'references/' or 'assets/'.", + "error": ( + "Path must start with 'references/', 'assets/', or 'scripts/'." + ), "error_code": "INVALID_RESOURCE_PATH", } @@ -222,9 +231,23 @@ async def run_async( class SkillToolset(BaseToolset): """A toolset for managing and interacting with agent skills.""" - def __init__(self, skills: list[models.Skill]): + def __init__( + self, + skills: list[models.Skill], + *, + inject_instruction: bool = True, + ): super().__init__() + + # Check for duplicate skill names + seen: set[str] = set() + for skill in skills: + if skill.name in seen: + raise ValueError(f"Duplicate skill name '{skill.name}'.") + seen.add(skill.name) + self._skills = {skill.name: skill for skill in skills} + self._inject_instruction = inject_instruction self._tools = [ ListSkillsTool(self), LoadSkillTool(self), @@ -241,14 +264,18 @@ def _get_skill(self, name: str) -> models.Skill | None: """Retrieves a skill by name.""" return self._skills.get(name) - def _list_skills(self) -> list[models.Frontmatter]: - """Lists the frontmatter of all available skills.""" - return [s.frontmatter for s in self._skills.values()] + def _list_skills(self) -> list[models.Skill]: + """Lists all available skills.""" + return list(self._skills.values()) async def process_llm_request( self, *, tool_context: ToolContext, llm_request: LlmRequest ) -> None: """Processes the outgoing LLM request to include available skills.""" - skill_frontmatters = self._list_skills() - skills_xml = prompt.format_skills_as_xml(skill_frontmatters) - llm_request.append_instructions([skills_xml]) + skills = self._list_skills() + skills_xml = prompt.format_skills_as_xml(skills) + instructions = [] + if self._inject_instruction: + instructions.append(DEFAULT_SKILL_SYSTEM_INSTRUCTION) + instructions.append(skills_xml) + llm_request.append_instructions(instructions) diff --git a/tests/unittests/skills/test_models.py b/tests/unittests/skills/test_models.py index 6ecdd51f95..45378e8726 100644 --- a/tests/unittests/skills/test_models.py +++ b/tests/unittests/skills/test_models.py @@ -15,6 +15,7 @@ """Unit tests for skill models.""" from google.adk.skills import models +from pydantic import ValidationError import pytest @@ -68,3 +69,124 @@ def test_script_to_string(): """Tests Script model.""" script = models.Script(src="print('hello')") assert str(script) == "print('hello')" + + +# --- Name validation tests --- + + +def test_name_too_long(): + with pytest.raises(ValidationError, match="at most 64 characters"): + models.Frontmatter(name="a" * 65, description="desc") + + +def test_name_uppercase_rejected(): + with pytest.raises(ValidationError, match="lowercase kebab-case"): + models.Frontmatter(name="My-Skill", description="desc") + + +def test_name_leading_hyphen(): + with pytest.raises(ValidationError, match="lowercase kebab-case"): + models.Frontmatter(name="-my-skill", description="desc") + + +def test_name_trailing_hyphen(): + with pytest.raises(ValidationError, match="lowercase kebab-case"): + models.Frontmatter(name="my-skill-", description="desc") + + +def test_name_consecutive_hyphens(): + with pytest.raises(ValidationError, match="lowercase kebab-case"): + models.Frontmatter(name="my--skill", description="desc") + + +def test_name_invalid_chars_underscore(): + with pytest.raises(ValidationError, match="lowercase kebab-case"): + models.Frontmatter(name="my_skill", description="desc") + + +def test_name_invalid_chars_ampersand(): + with pytest.raises(ValidationError, match="lowercase kebab-case"): + models.Frontmatter(name="skill&name", description="desc") + + +def test_name_valid_passes(): + fm = models.Frontmatter(name="my-skill-2", description="desc") + assert fm.name == "my-skill-2" + + +def test_name_single_word(): + fm = models.Frontmatter(name="skill", description="desc") + assert fm.name == "skill" + + +# --- Description validation tests --- + + +def test_description_empty(): + with pytest.raises(ValidationError, match="must not be empty"): + models.Frontmatter(name="my-skill", description="") + + +def test_description_too_long(): + with pytest.raises(ValidationError, match="at most 1024 characters"): + models.Frontmatter(name="my-skill", description="x" * 1025) + + +# --- Compatibility validation tests --- + + +def test_compatibility_too_long(): + with pytest.raises(ValidationError, match="at most 500 characters"): + models.Frontmatter( + name="my-skill", description="desc", compatibility="c" * 501 + ) + + +# --- Extra field rejected --- + + +def test_extra_field_allowed(): + fm = models.Frontmatter( + name="my-skill", description="desc", unknown_field="value" + ) + assert fm.name == "my-skill" + + +# --- allowed-tools alias --- + + +def test_allowed_tools_alias_via_model_validate(): + fm = models.Frontmatter.model_validate({ + "name": "my-skill", + "description": "desc", + "allowed-tools": "tool-pattern", + }) + assert fm.allowed_tools == "tool-pattern" + + +def test_allowed_tools_serialization_alias(): + fm = models.Frontmatter( + name="my-skill", description="desc", allowed_tools="tool-pattern" + ) + dumped = fm.model_dump(by_alias=True) + assert "allowed-tools" in dumped + assert dumped["allowed-tools"] == "tool-pattern" + + +# --- source_path on Skill --- + + +def test_skill_source_path(): + fm = models.Frontmatter(name="my-skill", description="desc") + skill = models.Skill( + frontmatter=fm, + instructions="do this", + source_path="/path/to/SKILL.md", + ) + assert skill.source_path == "/path/to/SKILL.md" + + +def test_skill_source_path_default_none(): + fm = models.Frontmatter(name="my-skill", description="desc") + skill = models.Skill(frontmatter=fm, instructions="do this") + assert skill.source_path is None diff --git a/tests/unittests/skills/test_prompt.py b/tests/unittests/skills/test_prompt.py index f5395f3c0f..f3b4952f44 100644 --- a/tests/unittests/skills/test_prompt.py +++ b/tests/unittests/skills/test_prompt.py @@ -42,8 +42,23 @@ def test_format_skills_as_xml_empty(self): def test_format_skills_as_xml_escaping(self): skills = [ - models.Frontmatter(name="skill&name", description="desc"), + models.Frontmatter(name="my-skill", description="desc"), ] xml = prompt.format_skills_as_xml(skills) - assert "skill&name" in xml + assert "my-skill" in xml assert "desc<ription>" in xml + + def test_format_skills_as_xml_with_skill_location(self): + fm = models.Frontmatter(name="my-skill", description="desc") + skill = models.Skill( + frontmatter=fm, + instructions="do this", + source_path="/path/to/SKILL.md", + ) + xml = prompt.format_skills_as_xml([skill]) + assert "\n/path/to/SKILL.md\n" in xml + + def test_format_skills_as_xml_frontmatter_no_location(self): + fm = models.Frontmatter(name="my-skill", description="desc") + xml = prompt.format_skills_as_xml([fm]) + assert "" not in xml diff --git a/tests/unittests/skills/test_utils.py b/tests/unittests/skills/test_utils.py index d922719d18..765cdc272c 100644 --- a/tests/unittests/skills/test_utils.py +++ b/tests/unittests/skills/test_utils.py @@ -15,6 +15,8 @@ """Unit tests for skill utilities.""" from google.adk.skills import load_skill_from_dir +from google.adk.skills import read_skill_properties +from google.adk.skills import validate_skill_dir import pytest @@ -54,3 +56,145 @@ def test_load_skill_from_dir(tmp_path): assert skill.resources.get_reference("ref1.md") == "ref1 content" assert skill.resources.get_asset("asset1.txt") == "asset1 content" assert skill.resources.get_script("script1.sh").src == "echo hello" + + +def test_allowed_tools_yaml_key(tmp_path): + """Tests that allowed-tools YAML key loads correctly.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A skill +allowed-tools: "some-tool-*" +--- +Instructions here +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + skill = load_skill_from_dir(skill_dir) + assert skill.frontmatter.allowed_tools == "some-tool-*" + + +def test_name_directory_mismatch(tmp_path): + """Tests that name-directory mismatch raises ValueError.""" + skill_dir = tmp_path / "wrong-dir" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A skill +--- +Body +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + with pytest.raises(ValueError, match="does not match directory"): + load_skill_from_dir(skill_dir) + + +def test_source_path_set(tmp_path): + """Tests that source_path is set after loading.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A skill +--- +Body +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + skill = load_skill_from_dir(skill_dir) + assert skill.source_path is not None + assert skill.source_path.endswith("SKILL.md") + + +def test_validate_skill_dir_valid(tmp_path): + """Tests validate_skill_dir with a valid skill.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A skill +--- +Body +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + problems = validate_skill_dir(skill_dir) + assert problems == [] + + +def test_validate_skill_dir_missing_dir(tmp_path): + """Tests validate_skill_dir with missing directory.""" + problems = validate_skill_dir(tmp_path / "nonexistent") + assert len(problems) == 1 + assert "does not exist" in problems[0] + + +def test_validate_skill_dir_missing_skill_md(tmp_path): + """Tests validate_skill_dir with missing SKILL.md.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + problems = validate_skill_dir(skill_dir) + assert len(problems) == 1 + assert "SKILL.md not found" in problems[0] + + +def test_validate_skill_dir_name_mismatch(tmp_path): + """Tests validate_skill_dir catches name-directory mismatch.""" + skill_dir = tmp_path / "wrong-dir" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A skill +--- +Body +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + problems = validate_skill_dir(skill_dir) + assert any("does not match" in p for p in problems) + + +def test_validate_skill_dir_unknown_fields(tmp_path): + """Tests validate_skill_dir detects unknown frontmatter fields.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A skill +unknown-field: something +--- +Body +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + problems = validate_skill_dir(skill_dir) + assert any("Unknown frontmatter" in p for p in problems) + + +def test_read_skill_properties(tmp_path): + """Tests read_skill_properties basic usage.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + skill_md = """--- +name: my-skill +description: A cool skill +license: MIT +--- +Body content +""" + (skill_dir / "SKILL.md").write_text(skill_md) + + fm = read_skill_properties(skill_dir) + assert fm.name == "my-skill" + assert fm.description == "A cool skill" + assert fm.license == "MIT" diff --git a/tests/unittests/tools/test_skill_toolset.py b/tests/unittests/tools/test_skill_toolset.py index b747d1f894..288fbc247e 100644 --- a/tests/unittests/tools/test_skill_toolset.py +++ b/tests/unittests/tools/test_skill_toolset.py @@ -39,8 +39,10 @@ def mock_skill1(mock_skill1_frontmatter): """Fixture for skill1.""" skill = mock.create_autospec(models.Skill, instance=True) skill.name = "skill1" + skill.description = "Skill 1 description" skill.instructions = "instructions for skill1" skill.frontmatter = mock_skill1_frontmatter + skill.source_path = "/path/to/skill1/SKILL.md" skill.resources = mock.MagicMock( spec=["get_reference", "get_asset", "get_script"] ) @@ -55,8 +57,14 @@ def get_asset(name): return "asset content 1" return None + def get_script(name): + if name == "setup.sh": + return models.Script(src="echo setup") + return None + skill.resources.get_reference.side_effect = get_ref skill.resources.get_asset.side_effect = get_asset + skill.resources.get_script.side_effect = get_script return skill @@ -78,8 +86,10 @@ def mock_skill2(mock_skill2_frontmatter): """Fixture for skill2.""" skill = mock.create_autospec(models.Skill, instance=True) skill.name = "skill2" + skill.description = "Skill 2 description" skill.instructions = "instructions for skill2" skill.frontmatter = mock_skill2_frontmatter + skill.source_path = None skill.resources = mock.MagicMock( spec=["get_reference", "get_asset", "get_script"] ) @@ -114,10 +124,10 @@ def test_get_skill(mock_skill1, mock_skill2): def test_list_skills(mock_skill1, mock_skill2): toolset = skill_toolset.SkillToolset([mock_skill1, mock_skill2]) - frontmatters = toolset._list_skills() - assert len(frontmatters) == 2 - assert mock_skill1.frontmatter in frontmatters - assert mock_skill2.frontmatter in frontmatters + skills = toolset._list_skills() + assert len(skills) == 2 + assert mock_skill1 in skills + assert mock_skill2 in skills @pytest.mark.asyncio @@ -203,6 +213,14 @@ async def test_load_skill_run_async( "content": "asset content 1", }, ), + ( + {"skill_name": "skill1", "path": "scripts/setup.sh"}, + { + "skill_name": "skill1", + "path": "scripts/setup.sh", + "content": "echo setup", + }, + ), ( {"skill_name": "nonexistent", "path": "references/ref1.md"}, { @@ -223,7 +241,10 @@ async def test_load_skill_run_async( ( {"skill_name": "skill1", "path": "invalid/path.txt"}, { - "error": "Path must start with 'references/' or 'assets/'.", + "error": ( + "Path must start with 'references/', 'assets/'," + " or 'scripts/'." + ), "error_code": "INVALID_RESOURCE_PATH", }, ), @@ -263,10 +284,47 @@ async def test_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) == 2 + assert instructions[0] == skill_toolset.DEFAULT_SKILL_SYSTEM_INSTRUCTION + assert "" in instructions[1] + assert "skill1" in instructions[1] + assert "skill2" in instructions[1] + + +@pytest.mark.asyncio +async def test_process_llm_request_no_inject( + mock_skill1, tool_context_instance +): + toolset = skill_toolset.SkillToolset([mock_skill1], inject_instruction=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 + ) + llm_req.append_instructions.assert_called_once() args, _ = llm_req.append_instructions.call_args instructions = args[0] assert len(instructions) == 1 assert "" in instructions[0] - assert "skill1" in instructions[0] - assert "skill2" in instructions[0] + + +def test_duplicate_skill_name_raises(mock_skill1): + skill_dup = mock.create_autospec(models.Skill, instance=True) + skill_dup.name = "skill1" + with pytest.raises(ValueError, match="Duplicate skill name"): + skill_toolset.SkillToolset([mock_skill1, skill_dup]) + + +@pytest.mark.asyncio +async def test_scripts_resource_not_found(mock_skill1, tool_context_instance): + toolset = skill_toolset.SkillToolset([mock_skill1]) + tool = skill_toolset.LoadSkillResourceTool(toolset) + result = await tool.run_async( + args={"skill_name": "skill1", "path": "scripts/nonexistent.sh"}, + tool_context=tool_context_instance, + ) + assert result["error_code"] == "RESOURCE_NOT_FOUND"