From c24601886cc015972714ff1be8140484de1efedf Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 15:55:37 +0000 Subject: [PATCH 1/7] feat: add AgentSkills standard fields to Skill model Add support for AgentSkills standard fields (https://agentskills.io/specification): - description: Brief description of what the skill does - license: License under which the skill is distributed - compatibility: Environment requirements or compatibility notes - metadata: Arbitrary key-value metadata for extensibility - allowed_tools: List of pre-approved tools for the skill Also adds skills-ref as an optional dependency for future validation and prompt generation utilities. Closes #1474 Co-authored-by: openhands --- .../openhands/sdk/context/skills/skill.py | 116 +++++++ openhands-sdk/pyproject.toml | 3 + .../context/skill/test_agentskills_fields.py | 299 ++++++++++++++++++ 3 files changed, 418 insertions(+) create mode 100644 tests/sdk/context/skill/test_agentskills_fields.py diff --git a/openhands-sdk/openhands/sdk/context/skills/skill.py b/openhands-sdk/openhands/sdk/context/skills/skill.py index e0037a3100..a0032f2eac 100644 --- a/openhands-sdk/openhands/sdk/context/skills/skill.py +++ b/openhands-sdk/openhands/sdk/context/skills/skill.py @@ -40,6 +40,9 @@ class Skill(BaseModel): - None: Always active, for repository-specific guidelines - KeywordTrigger: Activated when keywords appear in user messages - TaskTrigger: Activated for specific tasks, may require user input + + This model supports both OpenHands-specific fields and AgentSkills standard + fields (https://agentskills.io/specification) for cross-platform compatibility. """ name: str @@ -74,6 +77,43 @@ class Skill(BaseModel): description="Input metadata for the skill (task skills only)", ) + # AgentSkills standard fields (https://agentskills.io/specification) + description: str | None = Field( + default=None, + description=( + "A brief description of what the skill does and when to use it. " + "AgentSkills standard field (max 1024 characters)." + ), + ) + license: str | None = Field( + default=None, + description=( + "The license under which the skill is distributed. " + "AgentSkills standard field (e.g., 'Apache-2.0', 'MIT')." + ), + ) + compatibility: str | None = Field( + default=None, + description=( + "Environment requirements or compatibility notes for the skill. " + "AgentSkills standard field (e.g., 'Requires git and docker')." + ), + ) + metadata: dict[str, str] | None = Field( + default=None, + description=( + "Arbitrary key-value metadata for the skill. " + "AgentSkills standard field for extensibility." + ), + ) + allowed_tools: list[str] | None = Field( + default=None, + description=( + "List of pre-approved tools for this skill. " + "AgentSkills standard field (parsed from space-delimited string)." + ), + ) + PATH_TO_THIRD_PARTY_SKILL_NAME: ClassVar[dict[str, str]] = { ".cursorrules": "cursorrules", "agents.md": "agents", @@ -119,6 +159,73 @@ def _handle_third_party(cls, path: Path, file_content: str) -> Union["Skill", No return None + @classmethod + def _parse_agentskills_fields(cls, metadata_dict: dict) -> dict: + """Parse AgentSkills standard fields from frontmatter metadata. + + Args: + metadata_dict: The frontmatter metadata dictionary. + + Returns: + Dictionary with AgentSkills fields (description, license, + compatibility, metadata, allowed_tools). + """ + agentskills_fields: dict = {} + + # Parse description (string, max 1024 chars per spec) + if "description" in metadata_dict: + desc = metadata_dict["description"] + if isinstance(desc, str): + agentskills_fields["description"] = desc + else: + raise SkillValidationError("description must be a string") + + # Parse license (string) + if "license" in metadata_dict: + lic = metadata_dict["license"] + if isinstance(lic, str): + agentskills_fields["license"] = lic + else: + raise SkillValidationError("license must be a string") + + # Parse compatibility (string) + if "compatibility" in metadata_dict: + compat = metadata_dict["compatibility"] + if isinstance(compat, str): + agentskills_fields["compatibility"] = compat + else: + raise SkillValidationError("compatibility must be a string") + + # Parse metadata (dict[str, str]) + if "metadata" in metadata_dict: + meta = metadata_dict["metadata"] + if isinstance(meta, dict): + # Convert all values to strings for consistency + agentskills_fields["metadata"] = { + str(k): str(v) for k, v in meta.items() + } + else: + raise SkillValidationError("metadata must be a dictionary") + + # Parse allowed-tools (space-delimited string or list) + # AgentSkills spec uses "allowed-tools" with hyphen + allowed_tools_key = ( + "allowed-tools" + if "allowed-tools" in metadata_dict + else ("allowed_tools" if "allowed_tools" in metadata_dict else None) + ) + if allowed_tools_key: + tools = metadata_dict[allowed_tools_key] + if isinstance(tools, str): + # Parse space-delimited string + agentskills_fields["allowed_tools"] = tools.split() + elif isinstance(tools, list): + agentskills_fields["allowed_tools"] = [str(t) for t in tools] + else: + raise SkillValidationError("allowed-tools must be a string or list") + + return agentskills_fields + @classmethod def load( cls, @@ -129,6 +236,9 @@ def load( """Load a skill from a markdown file with frontmatter. The agent's name is derived from its path relative to the skill_dir. + + Supports both OpenHands-specific frontmatter fields and AgentSkills + standard fields (https://agentskills.io/specification). """ path = Path(path) if isinstance(path, str) else path @@ -162,6 +272,9 @@ def load( # Use name from frontmatter if provided, otherwise use derived name agent_name = str(metadata_dict.get("name", skill_name)) + # Parse AgentSkills standard fields + agentskills_fields = cls._parse_agentskills_fields(metadata_dict) + # Get trigger keywords from metadata keywords = metadata_dict.get("triggers", []) if not isinstance(keywords, list): @@ -188,6 +301,7 @@ def load( source=str(path), trigger=TaskTrigger(triggers=keywords), inputs=inputs, + **agentskills_fields, ) elif metadata_dict.get("triggers", None): @@ -196,6 +310,7 @@ def load( content=content, source=str(path), trigger=KeywordTrigger(keywords=keywords), + **agentskills_fields, ) else: # No triggers, default to None (always active) @@ -208,6 +323,7 @@ def load( source=str(path), trigger=None, mcp_tools=mcp_tools, + **agentskills_fields, ) # Field-level validation for mcp_tools diff --git a/openhands-sdk/pyproject.toml b/openhands-sdk/pyproject.toml index 276295e37c..6c41f4bba8 100644 --- a/openhands-sdk/pyproject.toml +++ b/openhands-sdk/pyproject.toml @@ -19,6 +19,9 @@ dependencies = [ [project.optional-dependencies] boto3 = ["boto3>=1.35.0"] +agentskills = [ + "skills-ref @ git+https://github.com/agentskills/agentskills.git#subdirectory=skills-ref" +] [build-system] requires = ["setuptools>=61.0", "wheel"] diff --git a/tests/sdk/context/skill/test_agentskills_fields.py b/tests/sdk/context/skill/test_agentskills_fields.py new file mode 100644 index 0000000000..a61226735a --- /dev/null +++ b/tests/sdk/context/skill/test_agentskills_fields.py @@ -0,0 +1,299 @@ +"""Tests for AgentSkills standard fields in the Skill model.""" + +from pathlib import Path + +import pytest + +from openhands.sdk.context.skills import Skill, SkillValidationError + + +def test_skill_with_all_agentskills_fields(): + """Test loading a skill with all AgentSkills standard fields.""" + skill_content = """--- +name: pdf-processing +description: Extract text and tables from PDF files, fill forms, merge documents. +license: Apache-2.0 +compatibility: Requires poppler-utils and ghostscript +metadata: + author: example-org + version: "1.0" +allowed-tools: Bash(pdftotext:*) Read Write +--- + +# PDF Processing Skill + +Instructions for processing PDF files. +""" + skill = Skill.load(Path("pdf-processing.md"), file_content=skill_content) + + assert skill.name == "pdf-processing" + assert skill.description == ( + "Extract text and tables from PDF files, fill forms, merge documents." + ) + assert skill.license == "Apache-2.0" + assert skill.compatibility == "Requires poppler-utils and ghostscript" + assert skill.metadata == {"author": "example-org", "version": "1.0"} + assert skill.allowed_tools == ["Bash(pdftotext:*)", "Read", "Write"] + assert skill.trigger is None # No triggers = always active + + +def test_skill_with_description_only(): + """Test loading a skill with only description field.""" + skill_content = """--- +name: simple-skill +description: A simple skill for testing. +--- + +# Simple Skill + +Content here. +""" + skill = Skill.load(Path("simple.md"), file_content=skill_content) + + assert skill.name == "simple-skill" + assert skill.description == "A simple skill for testing." + assert skill.license is None + assert skill.compatibility is None + assert skill.metadata is None + assert skill.allowed_tools is None + + +def test_skill_with_allowed_tools_as_list(): + """Test loading a skill with allowed-tools as a YAML list.""" + skill_content = """--- +name: list-tools-skill +description: Skill with tools as list. +allowed-tools: + - Bash(git:*) + - Read + - Write +--- + +# List Tools Skill + +Content here. +""" + skill = Skill.load(Path("list-tools.md"), file_content=skill_content) + + assert skill.allowed_tools == ["Bash(git:*)", "Read", "Write"] + + +def test_skill_with_allowed_tools_underscore(): + """Test loading a skill with allowed_tools (underscore variant).""" + skill_content = """--- +name: underscore-tools-skill +description: Skill with underscore variant. +allowed_tools: Bash Read Write +--- + +# Underscore Tools Skill + +Content here. +""" + skill = Skill.load(Path("underscore-tools.md"), file_content=skill_content) + + assert skill.allowed_tools == ["Bash", "Read", "Write"] + + +def test_skill_with_metadata_numeric_values(): + """Test that metadata values are converted to strings.""" + skill_content = """--- +name: numeric-metadata-skill +description: Skill with numeric metadata values. +metadata: + version: 2 + priority: 1.5 + enabled: true +--- + +# Numeric Metadata Skill + +Content here. +""" + skill = Skill.load(Path("numeric-metadata.md"), file_content=skill_content) + + # All values should be converted to strings + assert skill.metadata == { + "version": "2", + "priority": "1.5", + "enabled": "True", + } + + +def test_skill_agentskills_fields_with_triggers(): + """Test AgentSkills fields work with keyword triggers.""" + skill_content = """--- +name: triggered-skill +description: A skill that activates on keywords. +license: MIT +triggers: + - pdf + - document +--- + +# Triggered Skill + +Content here. +""" + skill = Skill.load(Path("triggered.md"), file_content=skill_content) + + assert skill.name == "triggered-skill" + assert skill.description == "A skill that activates on keywords." + assert skill.license == "MIT" + assert skill.trigger is not None + assert skill.match_trigger("process this pdf") == "pdf" + + +def test_skill_agentskills_fields_with_task_trigger(): + """Test AgentSkills fields work with task triggers.""" + skill_content = """--- +name: task-skill +description: A task skill with inputs. +compatibility: Requires Python 3.12+ +inputs: + - name: filename + description: The file to process +--- + +# Task Skill + +Process ${filename} here. +""" + skill = Skill.load(Path("task.md"), file_content=skill_content) + + assert skill.name == "task-skill" + assert skill.description == "A task skill with inputs." + assert skill.compatibility == "Requires Python 3.12+" + assert len(skill.inputs) == 1 + assert skill.inputs[0].name == "filename" + + +def test_skill_invalid_description_type(): + """Test that non-string description raises error.""" + skill_content = """--- +name: invalid-desc +description: + - not + - a + - string +--- + +Content. +""" + with pytest.raises(SkillValidationError) as excinfo: + Skill.load(Path("invalid.md"), file_content=skill_content) + + assert "description must be a string" in str(excinfo.value) + + +def test_skill_invalid_license_type(): + """Test that non-string license raises error.""" + skill_content = """--- +name: invalid-license +license: 123 +--- + +Content. +""" + with pytest.raises(SkillValidationError) as excinfo: + Skill.load(Path("invalid.md"), file_content=skill_content) + + assert "license must be a string" in str(excinfo.value) + + +def test_skill_invalid_compatibility_type(): + """Test that non-string compatibility raises error.""" + skill_content = """--- +name: invalid-compat +compatibility: + requires: git +--- + +Content. +""" + with pytest.raises(SkillValidationError) as excinfo: + Skill.load(Path("invalid.md"), file_content=skill_content) + + assert "compatibility must be a string" in str(excinfo.value) + + +def test_skill_invalid_metadata_type(): + """Test that non-dict metadata raises error.""" + skill_content = """--- +name: invalid-meta +metadata: not-a-dict +--- + +Content. +""" + with pytest.raises(SkillValidationError) as excinfo: + Skill.load(Path("invalid.md"), file_content=skill_content) + + assert "metadata must be a dictionary" in str(excinfo.value) + + +def test_skill_invalid_allowed_tools_type(): + """Test that invalid allowed-tools type raises error.""" + skill_content = """--- +name: invalid-tools +allowed-tools: 123 +--- + +Content. +""" + with pytest.raises(SkillValidationError) as excinfo: + Skill.load(Path("invalid.md"), file_content=skill_content) + + assert "allowed-tools must be a string or list" in str(excinfo.value) + + +def test_skill_serialization_with_agentskills_fields(): + """Test that AgentSkills fields serialize and deserialize correctly.""" + skill = Skill( + name="test-skill", + content="Test content", + description="Test description", + license="MIT", + compatibility="Python 3.12+", + metadata={"author": "test", "version": "1.0"}, + allowed_tools=["Read", "Write"], + ) + + # Serialize + serialized = skill.model_dump() + assert serialized["description"] == "Test description" + assert serialized["license"] == "MIT" + assert serialized["compatibility"] == "Python 3.12+" + assert serialized["metadata"] == {"author": "test", "version": "1.0"} + assert serialized["allowed_tools"] == ["Read", "Write"] + + # Deserialize + deserialized = Skill.model_validate(serialized) + assert deserialized.description == "Test description" + assert deserialized.license == "MIT" + assert deserialized.compatibility == "Python 3.12+" + assert deserialized.metadata == {"author": "test", "version": "1.0"} + assert deserialized.allowed_tools == ["Read", "Write"] + + +def test_skill_backward_compatibility(): + """Test that existing skills without AgentSkills fields still work.""" + skill_content = """--- +name: legacy-skill +triggers: + - legacy +--- + +# Legacy Skill + +This skill has no AgentSkills fields. +""" + skill = Skill.load(Path("legacy.md"), file_content=skill_content) + + assert skill.name == "legacy-skill" + assert skill.description is None + assert skill.license is None + assert skill.compatibility is None + assert skill.metadata is None + assert skill.allowed_tools is None + assert skill.match_trigger("use legacy feature") == "legacy" From c03396609e2eb9894948119119ae92271ce379ca Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 16:00:49 +0000 Subject: [PATCH 2/7] chore: remove unused agentskills optional dependency The skills-ref library will be added when validation and prompt generation utilities are implemented (issue #1478). Co-authored-by: openhands --- openhands-sdk/pyproject.toml | 3 --- 1 file changed, 3 deletions(-) diff --git a/openhands-sdk/pyproject.toml b/openhands-sdk/pyproject.toml index 6c41f4bba8..276295e37c 100644 --- a/openhands-sdk/pyproject.toml +++ b/openhands-sdk/pyproject.toml @@ -19,9 +19,6 @@ dependencies = [ [project.optional-dependencies] boto3 = ["boto3>=1.35.0"] -agentskills = [ - "skills-ref @ git+https://github.com/agentskills/agentskills.git#subdirectory=skills-ref" -] [build-system] requires = ["setuptools>=61.0", "wheel"] From c7b67b612aeb62ec2b8e7714d84e4ab828a273c6 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 16:07:15 +0000 Subject: [PATCH 3/7] feat: support SKILL.md file convention and name validation - Add find_skill_md() function to locate SKILL.md files (case-insensitive) - Add validate_skill_name() function for AgentSkills spec validation - Update load_skills_from_dir() to support skill-name/SKILL.md directories - Add directory_name and validate_name parameters to Skill.load() - Export new functions from __init__.py - Add 27 unit tests for new functionality Closes #1475 Co-authored-by: openhands --- .../openhands/sdk/context/skills/__init__.py | 4 + .../openhands/sdk/context/skills/skill.py | 177 +++++++- .../context/skill/test_skill_md_convention.py | 405 ++++++++++++++++++ 3 files changed, 567 insertions(+), 19 deletions(-) create mode 100644 tests/sdk/context/skill/test_skill_md_convention.py diff --git a/openhands-sdk/openhands/sdk/context/skills/__init__.py b/openhands-sdk/openhands/sdk/context/skills/__init__.py index fc667f89ab..3e4b2b9fdf 100644 --- a/openhands-sdk/openhands/sdk/context/skills/__init__.py +++ b/openhands-sdk/openhands/sdk/context/skills/__init__.py @@ -1,10 +1,12 @@ from openhands.sdk.context.skills.exceptions import SkillValidationError from openhands.sdk.context.skills.skill import ( Skill, + find_skill_md, load_project_skills, load_public_skills, load_skills_from_dir, load_user_skills, + validate_skill_name, ) from openhands.sdk.context.skills.trigger import ( BaseTrigger, @@ -25,4 +27,6 @@ "load_project_skills", "load_public_skills", "SkillValidationError", + "find_skill_md", + "validate_skill_name", ] diff --git a/openhands-sdk/openhands/sdk/context/skills/skill.py b/openhands-sdk/openhands/sdk/context/skills/skill.py index a0032f2eac..5ecc4c428d 100644 --- a/openhands-sdk/openhands/sdk/context/skills/skill.py +++ b/openhands-sdk/openhands/sdk/context/skills/skill.py @@ -2,7 +2,6 @@ import re import shutil import subprocess -from itertools import chain from pathlib import Path from typing import Annotated, ClassVar, Union @@ -26,6 +25,62 @@ # These files are always active, so we want to keep them reasonably sized THIRD_PARTY_SKILL_MAX_CHARS = 10_000 +# Regex pattern for valid AgentSkills names +# - 1-64 characters +# - Lowercase alphanumeric + hyphens only (a-z, 0-9, -) +# - Must not start or end with hyphen +# - Must not contain consecutive hyphens (--) +SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*$") + + +def find_skill_md(skill_dir: Path) -> Path | None: + """Find SKILL.md file in a directory (case-insensitive). + + Args: + skill_dir: Path to the skill directory to search. + + Returns: + Path to SKILL.md if found, None otherwise. + """ + if not skill_dir.is_dir(): + return None + for item in skill_dir.iterdir(): + if item.is_file() and item.name.lower() == "skill.md": + return item + return None + + +def validate_skill_name(name: str, directory_name: str | None = None) -> list[str]: + """Validate skill name according to AgentSkills spec. + + Args: + name: The skill name to validate. + directory_name: Optional directory name to check for match. + + Returns: + List of validation error messages (empty if valid). + """ + errors = [] + + if not name: + errors.append("Name cannot be empty") + return errors + + if len(name) > 64: + errors.append(f"Name exceeds 64 characters: {len(name)}") + + if not SKILL_NAME_PATTERN.match(name): + errors.append( + "Name must be lowercase alphanumeric with single hyphens " + "(e.g., 'my-skill', 'pdf-tools')" + ) + + if directory_name and name != directory_name: + errors.append(f"Name '{name}' does not match directory '{directory_name}'") + + return errors + + # Union type for all trigger types TriggerType = Annotated[ KeywordTrigger | TaskTrigger, @@ -232,19 +287,35 @@ def load( path: str | Path, skill_dir: Path | None = None, file_content: str | None = None, + directory_name: str | None = None, + validate_name: bool = False, ) -> "Skill": """Load a skill from a markdown file with frontmatter. - The agent's name is derived from its path relative to the skill_dir. + The agent's name is derived from its path relative to the skill_dir, + or from the directory name for AgentSkills-style SKILL.md files. Supports both OpenHands-specific frontmatter fields and AgentSkills standard fields (https://agentskills.io/specification). + + Args: + path: Path to the skill file. + skill_dir: Base directory for skills (used to derive relative names). + file_content: Optional file content (if not provided, reads from path). + directory_name: For SKILL.md files, the parent directory name. + Used to derive skill name and validate name matches directory. + validate_name: If True, validate the skill name according to + AgentSkills spec and raise SkillValidationError if invalid. """ path = Path(path) if isinstance(path, str) else path # Calculate derived name from relative path if skill_dir is provided - skill_name = None - if skill_dir is not None: + skill_name: str | None = None + + # For SKILL.md files, use directory name as the skill name + if directory_name is not None: + skill_name = directory_name + elif skill_dir is not None: # Special handling for files which are not in skill_dir skill_name = cls.PATH_TO_THIRD_PARTY_SKILL_NAME.get( path.name.lower() @@ -272,6 +343,14 @@ def load( # Use name from frontmatter if provided, otherwise use derived name agent_name = str(metadata_dict.get("name", skill_name)) + # Validate skill name if requested + if validate_name: + name_errors = validate_skill_name(agent_name, directory_name) + if name_errors: + raise SkillValidationError( + f"Invalid skill name '{agent_name}': {'; '.join(name_errors)}" + ) + # Parse AgentSkills standard fields agentskills_fields = cls._parse_agentskills_fields(metadata_dict) @@ -401,13 +480,20 @@ def requires_user_input(self) -> bool: def load_skills_from_dir( skill_dir: str | Path, + validate_names: bool = False, ) -> tuple[dict[str, Skill], dict[str, Skill]]: """Load all skills from the given directory. + Supports both formats: + - OpenHands format: skills/*.md files + - AgentSkills format: skills/skill-name/SKILL.md directories + Note, legacy repo instructions will not be loaded here. Args: skill_dir: Path to the skills directory (e.g. .openhands/skills) + validate_names: If True, validate skill names according to AgentSkills + spec and raise SkillValidationError for invalid names. Returns: Tuple of (repo_skills, knowledge_skills) dictionaries. @@ -417,14 +503,14 @@ def load_skills_from_dir( if isinstance(skill_dir, str): skill_dir = Path(skill_dir) - repo_skills = {} - knowledge_skills = {} + repo_skills: dict[str, Skill] = {} + knowledge_skills: dict[str, Skill] = {} # Load all agents from skills directory logger.debug(f"Loading agents from {skill_dir}") # Always check for .cursorrules and AGENTS.md files in repo root - special_files = [] + special_files: list[Path] = [] repo_root = skill_dir.parent.parent # Check for third party rules: .cursorrules, AGENTS.md, etc @@ -434,26 +520,79 @@ def load_skills_from_dir( special_files.append(repo_root / variant) break # Only add the first one found to avoid duplicates - # Collect .md files from skills directory if it exists - md_files = [] + # Track directories with SKILL.md to avoid double-loading + skill_md_dirs: set[Path] = set() + + # Collect AgentSkills-style directories (skill-name/SKILL.md) + skill_md_files: list[tuple[Path, str]] = [] # (path, directory_name) if skill_dir.exists(): - md_files = [f for f in skill_dir.rglob("*.md") if f.name != "README.md"] + for subdir in skill_dir.iterdir(): + if subdir.is_dir(): + skill_md = find_skill_md(subdir) + if skill_md: + skill_md_files.append((skill_md, subdir.name)) + skill_md_dirs.add(subdir) + + # Collect .md files from skills directory (excluding SKILL.md files) + md_files: list[Path] = [] + if skill_dir.exists(): + for f in skill_dir.rglob("*.md"): + # Skip README.md + if f.name == "README.md": + continue + # Skip SKILL.md files (already collected above) + if f.name.lower() == "skill.md": + continue + # Skip files in directories that have SKILL.md + if any(f.is_relative_to(d) for d in skill_md_dirs): + continue + md_files.append(f) + + def add_skill(skill: Skill) -> None: + """Add skill to appropriate dictionary.""" + if skill.trigger is None: + repo_skills[skill.name] = skill + else: + knowledge_skills[skill.name] = skill - # Process all files in one loop - for file in chain(special_files, md_files): + # Process special files (third-party rules) + for file in special_files: try: skill = Skill.load(file, skill_dir) - if skill.trigger is None: - repo_skills[skill.name] = skill - else: - # KeywordTrigger and TaskTrigger skills - knowledge_skills[skill.name] = skill + add_skill(skill) + except SkillValidationError as e: + error_msg = f"Error loading skill from {file}: {str(e)}" + raise SkillValidationError(error_msg) from e + except Exception as e: + error_msg = f"Error loading skill from {file}: {str(e)}" + raise ValueError(error_msg) from e + + # Process AgentSkills-style SKILL.md directories + for skill_md_path, dir_name in skill_md_files: + try: + skill = Skill.load( + skill_md_path, + skill_dir, + directory_name=dir_name, + validate_name=validate_names, + ) + add_skill(skill) + except SkillValidationError as e: + error_msg = f"Error loading skill from {skill_md_path}: {str(e)}" + raise SkillValidationError(error_msg) from e + except Exception as e: + error_msg = f"Error loading skill from {skill_md_path}: {str(e)}" + raise ValueError(error_msg) from e + + # Process regular .md files + for file in md_files: + try: + skill = Skill.load(file, skill_dir, validate_name=validate_names) + add_skill(skill) except SkillValidationError as e: - # For validation errors, include the original exception error_msg = f"Error loading skill from {file}: {str(e)}" raise SkillValidationError(error_msg) from e except Exception as e: - # For other errors, wrap in a ValueError with detailed message error_msg = f"Error loading skill from {file}: {str(e)}" raise ValueError(error_msg) from e diff --git a/tests/sdk/context/skill/test_skill_md_convention.py b/tests/sdk/context/skill/test_skill_md_convention.py new file mode 100644 index 0000000000..3e50dd9434 --- /dev/null +++ b/tests/sdk/context/skill/test_skill_md_convention.py @@ -0,0 +1,405 @@ +"""Tests for SKILL.md file convention and name validation (Issue #1475).""" + +from pathlib import Path + +import pytest + +from openhands.sdk.context.skills import ( + Skill, + SkillValidationError, + find_skill_md, + load_skills_from_dir, + validate_skill_name, +) + + +class TestFindSkillMd: + """Tests for find_skill_md() function.""" + + def test_finds_skill_md(self, tmp_path: Path) -> None: + """Should find SKILL.md file in directory.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text("# My Skill") + + result = find_skill_md(skill_dir) + assert result == skill_md + + def test_finds_skill_md_case_insensitive(self, tmp_path: Path) -> None: + """Should find skill.md with different casing.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "skill.MD" + skill_md.write_text("# My Skill") + + result = find_skill_md(skill_dir) + assert result == skill_md + + def test_returns_none_when_not_found(self, tmp_path: Path) -> None: + """Should return None when no SKILL.md exists.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "README.md").write_text("# README") + + result = find_skill_md(skill_dir) + assert result is None + + def test_returns_none_for_non_directory(self, tmp_path: Path) -> None: + """Should return None for non-directory path.""" + file_path = tmp_path / "not-a-dir.txt" + file_path.write_text("content") + + result = find_skill_md(file_path) + assert result is None + + +class TestValidateSkillName: + """Tests for validate_skill_name() function.""" + + def test_valid_simple_name(self) -> None: + """Should accept simple lowercase name.""" + errors = validate_skill_name("myskill") + assert errors == [] + + def test_valid_hyphenated_name(self) -> None: + """Should accept hyphenated name.""" + errors = validate_skill_name("my-skill") + assert errors == [] + + def test_valid_multi_hyphen_name(self) -> None: + """Should accept name with multiple hyphens.""" + errors = validate_skill_name("my-cool-skill") + assert errors == [] + + def test_valid_with_numbers(self) -> None: + """Should accept name with numbers.""" + errors = validate_skill_name("skill2") + assert errors == [] + + def test_invalid_uppercase(self) -> None: + """Should reject uppercase letters.""" + errors = validate_skill_name("MySkill") + assert len(errors) == 1 + assert "lowercase" in errors[0] + + def test_invalid_underscore(self) -> None: + """Should reject underscores.""" + errors = validate_skill_name("my_skill") + assert len(errors) == 1 + assert "lowercase" in errors[0] + + def test_invalid_starts_with_hyphen(self) -> None: + """Should reject name starting with hyphen.""" + errors = validate_skill_name("-myskill") + assert len(errors) == 1 + + def test_invalid_ends_with_hyphen(self) -> None: + """Should reject name ending with hyphen.""" + errors = validate_skill_name("myskill-") + assert len(errors) == 1 + + def test_invalid_consecutive_hyphens(self) -> None: + """Should reject consecutive hyphens.""" + errors = validate_skill_name("my--skill") + assert len(errors) == 1 + + def test_invalid_too_long(self) -> None: + """Should reject name exceeding 64 characters.""" + long_name = "a" * 65 + errors = validate_skill_name(long_name) + assert len(errors) == 1 + assert "64 characters" in errors[0] + + def test_invalid_empty(self) -> None: + """Should reject empty name.""" + errors = validate_skill_name("") + assert len(errors) == 1 + assert "empty" in errors[0] + + def test_directory_name_match(self) -> None: + """Should accept when name matches directory.""" + errors = validate_skill_name("my-skill", directory_name="my-skill") + assert errors == [] + + def test_directory_name_mismatch(self) -> None: + """Should reject when name doesn't match directory.""" + errors = validate_skill_name("my-skill", directory_name="other-skill") + assert len(errors) == 1 + assert "does not match directory" in errors[0] + + +class TestSkillLoadWithDirectoryName: + """Tests for Skill.load() with directory_name parameter.""" + + def test_load_skill_md_derives_name_from_directory(self, tmp_path: Path) -> None: + """Should derive skill name from directory when loading SKILL.md.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "pdf-tools" + my_skill_dir.mkdir() + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: + - pdf +--- +# PDF Tools + +Process PDF files. +""" + ) + + skill = Skill.load(skill_md, skill_dir, directory_name="pdf-tools") + assert skill.name == "pdf-tools" + + def test_load_skill_md_frontmatter_name_overrides(self, tmp_path: Path) -> None: + """Frontmatter name should override directory name.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "pdf-tools" + my_skill_dir.mkdir() + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +name: custom-name +triggers: + - pdf +--- +# PDF Tools +""" + ) + + skill = Skill.load(skill_md, skill_dir, directory_name="pdf-tools") + assert skill.name == "custom-name" + + def test_load_with_name_validation_valid(self, tmp_path: Path) -> None: + """Should pass validation for valid name.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "pdf-tools" + my_skill_dir.mkdir() + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: + - pdf +--- +# PDF Tools +""" + ) + + skill = Skill.load( + skill_md, + skill_dir, + directory_name="pdf-tools", + validate_name=True, + ) + assert skill.name == "pdf-tools" + + def test_load_with_name_validation_invalid(self, tmp_path: Path) -> None: + """Should raise error for invalid name when validation enabled.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "PDF_Tools" + my_skill_dir.mkdir() + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: + - pdf +--- +# PDF Tools +""" + ) + + with pytest.raises(SkillValidationError) as exc_info: + Skill.load( + skill_md, + skill_dir, + directory_name="PDF_Tools", + validate_name=True, + ) + assert "Invalid skill name" in str(exc_info.value) + + def test_load_with_name_mismatch_validation(self, tmp_path: Path) -> None: + """Should raise error when frontmatter name doesn't match directory.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "pdf-tools" + my_skill_dir.mkdir() + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +name: other-name +triggers: + - pdf +--- +# PDF Tools +""" + ) + + with pytest.raises(SkillValidationError) as exc_info: + Skill.load( + skill_md, + skill_dir, + directory_name="pdf-tools", + validate_name=True, + ) + assert "does not match directory" in str(exc_info.value) + + +class TestLoadSkillsFromDirWithSkillMd: + """Tests for load_skills_from_dir() with SKILL.md directories.""" + + def test_loads_skill_md_directories(self, tmp_path: Path) -> None: + """Should load skills from skill-name/SKILL.md directories.""" + # Create directory structure + repo_root = tmp_path / "repo" + repo_root.mkdir() + openhands_dir = repo_root / ".openhands" + openhands_dir.mkdir() + skills_dir = openhands_dir / "skills" + skills_dir.mkdir() + + # Create AgentSkills-style skill + pdf_skill_dir = skills_dir / "pdf-tools" + pdf_skill_dir.mkdir() + (pdf_skill_dir / "SKILL.md").write_text( + """--- +triggers: + - pdf +--- +# PDF Tools + +Process PDF files. +""" + ) + + repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) + assert "pdf-tools" in knowledge_skills + assert knowledge_skills["pdf-tools"].name == "pdf-tools" + + def test_loads_both_formats(self, tmp_path: Path) -> None: + """Should load both flat .md files and SKILL.md directories.""" + # Create directory structure + repo_root = tmp_path / "repo" + repo_root.mkdir() + openhands_dir = repo_root / ".openhands" + openhands_dir.mkdir() + skills_dir = openhands_dir / "skills" + skills_dir.mkdir() + + # Create flat .md skill + (skills_dir / "flat-skill.md").write_text( + """--- +triggers: + - flat +--- +# Flat Skill +""" + ) + + # Create AgentSkills-style skill + dir_skill = skills_dir / "dir-skill" + dir_skill.mkdir() + (dir_skill / "SKILL.md").write_text( + """--- +triggers: + - directory +--- +# Directory Skill +""" + ) + + repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) + assert "flat-skill" in knowledge_skills + assert "dir-skill" in knowledge_skills + + def test_skill_md_takes_precedence(self, tmp_path: Path) -> None: + """Files in SKILL.md directories should not be loaded separately.""" + # Create directory structure + repo_root = tmp_path / "repo" + repo_root.mkdir() + openhands_dir = repo_root / ".openhands" + openhands_dir.mkdir() + skills_dir = openhands_dir / "skills" + skills_dir.mkdir() + + # Create AgentSkills-style skill with extra .md file + pdf_skill_dir = skills_dir / "pdf-tools" + pdf_skill_dir.mkdir() + (pdf_skill_dir / "SKILL.md").write_text( + """--- +triggers: + - pdf +--- +# PDF Tools +""" + ) + # This file should NOT be loaded as a separate skill + (pdf_skill_dir / "extra.md").write_text( + """--- +triggers: + - extra +--- +# Extra +""" + ) + + repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) + # Should only have pdf-tools, not extra + assert "pdf-tools" in knowledge_skills + assert "extra" not in knowledge_skills + assert len(knowledge_skills) == 1 + + def test_validate_names_option(self, tmp_path: Path) -> None: + """Should validate names when validate_names=True.""" + # Create directory structure + repo_root = tmp_path / "repo" + repo_root.mkdir() + openhands_dir = repo_root / ".openhands" + openhands_dir.mkdir() + skills_dir = openhands_dir / "skills" + skills_dir.mkdir() + + # Create skill with invalid name + invalid_skill_dir = skills_dir / "Invalid_Name" + invalid_skill_dir.mkdir() + (invalid_skill_dir / "SKILL.md").write_text( + """--- +triggers: + - test +--- +# Invalid +""" + ) + + with pytest.raises(SkillValidationError) as exc_info: + load_skills_from_dir(skills_dir, validate_names=True) + assert "Invalid skill name" in str(exc_info.value) + + def test_case_insensitive_skill_md(self, tmp_path: Path) -> None: + """Should find skill.md with different casing.""" + # Create directory structure + repo_root = tmp_path / "repo" + repo_root.mkdir() + openhands_dir = repo_root / ".openhands" + openhands_dir.mkdir() + skills_dir = openhands_dir / "skills" + skills_dir.mkdir() + + # Create skill with lowercase skill.md + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir() + (skill_dir / "skill.md").write_text( + """--- +triggers: + - test +--- +# My Skill +""" + ) + + repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) + assert "my-skill" in knowledge_skills From 2fb77358ff8a308191f14a974310de350aef16db Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 16:10:51 +0000 Subject: [PATCH 4/7] feat: support .mcp.json for MCP server configuration - Add find_mcp_config() function to locate .mcp.json files - Add expand_mcp_variables() for variable expansion (, default) - Add load_mcp_config() to parse and validate .mcp.json files - Add mcp_config_path field to Skill model - Update Skill.load() to auto-load .mcp.json from SKILL.md directories - .mcp.json takes precedence over mcp_tools frontmatter - Support variable expansion - Export new functions from __init__.py - Add 19 unit tests for new functionality Closes #1476 Co-authored-by: openhands --- .../openhands/sdk/context/skills/__init__.py | 6 + .../openhands/sdk/context/skills/skill.py | 149 ++++++++- .../sdk/context/skill/test_mcp_json_config.py | 301 ++++++++++++++++++ 3 files changed, 453 insertions(+), 3 deletions(-) create mode 100644 tests/sdk/context/skill/test_mcp_json_config.py diff --git a/openhands-sdk/openhands/sdk/context/skills/__init__.py b/openhands-sdk/openhands/sdk/context/skills/__init__.py index 3e4b2b9fdf..46a6a51b38 100644 --- a/openhands-sdk/openhands/sdk/context/skills/__init__.py +++ b/openhands-sdk/openhands/sdk/context/skills/__init__.py @@ -1,7 +1,10 @@ from openhands.sdk.context.skills.exceptions import SkillValidationError from openhands.sdk.context.skills.skill import ( Skill, + expand_mcp_variables, + find_mcp_config, find_skill_md, + load_mcp_config, load_project_skills, load_public_skills, load_skills_from_dir, @@ -29,4 +32,7 @@ "SkillValidationError", "find_skill_md", "validate_skill_name", + "find_mcp_config", + "load_mcp_config", + "expand_mcp_variables", ] diff --git a/openhands-sdk/openhands/sdk/context/skills/skill.py b/openhands-sdk/openhands/sdk/context/skills/skill.py index 5ecc4c428d..2f7888f951 100644 --- a/openhands-sdk/openhands/sdk/context/skills/skill.py +++ b/openhands-sdk/openhands/sdk/context/skills/skill.py @@ -81,6 +81,115 @@ def validate_skill_name(name: str, directory_name: str | None = None) -> list[st return errors +def find_mcp_config(skill_dir: Path) -> Path | None: + """Find .mcp.json file in a skill directory. + + Args: + skill_dir: Path to the skill directory to search. + + Returns: + Path to .mcp.json if found, None otherwise. + """ + if not skill_dir.is_dir(): + return None + mcp_json = skill_dir / ".mcp.json" + if mcp_json.exists() and mcp_json.is_file(): + return mcp_json + return None + + +def expand_mcp_variables( + config: dict, + variables: dict[str, str], +) -> dict: + """Expand variables in MCP configuration. + + Supports variable expansion similar to Claude Code: + - ${VAR} - Environment variables or provided variables + - ${VAR:-default} - With default value + + Args: + config: MCP configuration dictionary. + variables: Dictionary of variable names to values. + + Returns: + Configuration with variables expanded. + """ + import json + import os + + # Convert to JSON string for easy replacement + config_str = json.dumps(config) + + # Pattern for ${VAR} or ${VAR:-default} + var_pattern = re.compile(r"\$\{([a-zA-Z_][a-zA-Z0-9_]*)(?::-([^}]*))?\}") + + def replace_var(match: re.Match) -> str: + var_name = match.group(1) + default_value = match.group(2) + + # Check provided variables first, then environment + if var_name in variables: + return variables[var_name] + if var_name in os.environ: + return os.environ[var_name] + if default_value is not None: + return default_value + # Return original if not found + return match.group(0) + + config_str = var_pattern.sub(replace_var, config_str) + return json.loads(config_str) + + +def load_mcp_config( + mcp_json_path: Path, + skill_root: Path | None = None, +) -> dict: + """Load and parse .mcp.json with variable expansion. + + Args: + mcp_json_path: Path to the .mcp.json file. + skill_root: Root directory of the skill (for ${SKILL_ROOT} expansion). + + Returns: + Parsed MCP configuration dictionary. + + Raises: + SkillValidationError: If the file cannot be parsed or is invalid. + """ + import json + + try: + with open(mcp_json_path) as f: + config = json.load(f) + except json.JSONDecodeError as e: + raise SkillValidationError(f"Invalid JSON in {mcp_json_path}: {e}") from e + except OSError as e: + raise SkillValidationError(f"Cannot read {mcp_json_path}: {e}") from e + + if not isinstance(config, dict): + raise SkillValidationError( + f"Invalid .mcp.json format: expected object, got {type(config).__name__}" + ) + + # Prepare variables for expansion + variables: dict[str, str] = {} + if skill_root: + variables["SKILL_ROOT"] = str(skill_root) + + # Expand variables + config = expand_mcp_variables(config, variables) + + # Validate using MCPConfig + try: + MCPConfig.model_validate(config) + except Exception as e: + raise SkillValidationError(f"Invalid MCP configuration: {e}") from e + + return config + + # Union type for all trigger types TriggerType = Annotated[ KeywordTrigger | TaskTrigger, @@ -168,6 +277,13 @@ class Skill(BaseModel): "AgentSkills standard field (parsed from space-delimited string)." ), ) + mcp_config_path: str | None = Field( + default=None, + description=( + "Path to .mcp.json file if MCP config was loaded from file. " + "Used to track the source of MCP configuration." + ), + ) PATH_TO_THIRD_PARTY_SKILL_NAME: ClassVar[dict[str, str]] = { ".cursorrules": "cursorrules", @@ -354,6 +470,31 @@ def load( # Parse AgentSkills standard fields agentskills_fields = cls._parse_agentskills_fields(metadata_dict) + # Load MCP configuration from .mcp.json if available (for SKILL.md directories) + mcp_tools: dict | None = None + mcp_config_path: str | None = None + + # Check for .mcp.json in skill directory (only for SKILL.md format) + if directory_name is not None: + skill_root = path.parent + mcp_json_path = find_mcp_config(skill_root) + if mcp_json_path: + mcp_tools = load_mcp_config(mcp_json_path, skill_root) + mcp_config_path = str(mcp_json_path) + # Log warning if both .mcp.json and mcp_tools frontmatter exist + if metadata_dict.get("mcp_tools"): + logger.warning( + f"Skill '{agent_name}' has both .mcp.json and mcp_tools " + "frontmatter. Using .mcp.json configuration." + ) + + # Fall back to mcp_tools from frontmatter if no .mcp.json + if mcp_tools is None: + frontmatter_mcp = metadata_dict.get("mcp_tools") + if frontmatter_mcp is not None and not isinstance(frontmatter_mcp, dict): + raise SkillValidationError("mcp_tools must be a dictionary or None") + mcp_tools = frontmatter_mcp + # Get trigger keywords from metadata keywords = metadata_dict.get("triggers", []) if not isinstance(keywords, list): @@ -380,6 +521,8 @@ def load( source=str(path), trigger=TaskTrigger(triggers=keywords), inputs=inputs, + mcp_tools=mcp_tools, + mcp_config_path=mcp_config_path, **agentskills_fields, ) @@ -389,19 +532,19 @@ def load( content=content, source=str(path), trigger=KeywordTrigger(keywords=keywords), + mcp_tools=mcp_tools, + mcp_config_path=mcp_config_path, **agentskills_fields, ) else: # No triggers, default to None (always active) - mcp_tools = metadata_dict.get("mcp_tools") - if not isinstance(mcp_tools, dict | None): - raise SkillValidationError("mcp_tools must be a dictionary or None") return Skill( name=agent_name, content=content, source=str(path), trigger=None, mcp_tools=mcp_tools, + mcp_config_path=mcp_config_path, **agentskills_fields, ) diff --git a/tests/sdk/context/skill/test_mcp_json_config.py b/tests/sdk/context/skill/test_mcp_json_config.py new file mode 100644 index 0000000000..576e079f6c --- /dev/null +++ b/tests/sdk/context/skill/test_mcp_json_config.py @@ -0,0 +1,301 @@ +"""Tests for .mcp.json configuration support (Issue #1476).""" + +import json +from pathlib import Path + +import pytest + +from openhands.sdk.context.skills import ( + Skill, + SkillValidationError, + expand_mcp_variables, + find_mcp_config, + load_mcp_config, +) + + +class TestFindMcpConfig: + """Tests for find_mcp_config() function.""" + + def test_finds_mcp_json(self, tmp_path: Path) -> None: + """Should find .mcp.json file in directory.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + mcp_json = skill_dir / ".mcp.json" + mcp_json.write_text('{"mcpServers": {}}') + + result = find_mcp_config(skill_dir) + assert result == mcp_json + + def test_returns_none_when_not_found(self, tmp_path: Path) -> None: + """Should return None when no .mcp.json exists.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("# Skill") + + result = find_mcp_config(skill_dir) + assert result is None + + def test_returns_none_for_non_directory(self, tmp_path: Path) -> None: + """Should return None for non-directory path.""" + file_path = tmp_path / "not-a-dir.txt" + file_path.write_text("content") + + result = find_mcp_config(file_path) + assert result is None + + +class TestExpandMcpVariables: + """Tests for expand_mcp_variables() function.""" + + def test_expands_simple_variable(self) -> None: + """Should expand ${VAR} with provided value.""" + config = {"command": "${MY_VAR}"} + result = expand_mcp_variables(config, {"MY_VAR": "value"}) + assert result["command"] == "value" + + def test_expands_env_variable(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Should expand ${VAR} from environment.""" + monkeypatch.setenv("TEST_ENV_VAR", "env_value") + config = {"command": "${TEST_ENV_VAR}"} + result = expand_mcp_variables(config, {}) + assert result["command"] == "env_value" + + def test_provided_takes_precedence(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Provided variables should take precedence over environment.""" + monkeypatch.setenv("MY_VAR", "env_value") + config = {"command": "${MY_VAR}"} + result = expand_mcp_variables(config, {"MY_VAR": "provided_value"}) + assert result["command"] == "provided_value" + + def test_expands_with_default(self) -> None: + """Should use default value when variable not found.""" + config = {"command": "${MISSING:-default_value}"} + result = expand_mcp_variables(config, {}) + assert result["command"] == "default_value" + + def test_keeps_original_when_not_found(self) -> None: + """Should keep original when variable not found and no default.""" + config = {"command": "${MISSING}"} + result = expand_mcp_variables(config, {}) + assert result["command"] == "${MISSING}" + + def test_expands_nested_values(self) -> None: + """Should expand variables in nested structures.""" + config = { + "mcpServers": { + "test": { + "command": "${CMD}", + "args": ["--path", "${PATH_VAR}"], + } + } + } + result = expand_mcp_variables(config, {"CMD": "python", "PATH_VAR": "/tmp"}) + assert result["mcpServers"]["test"]["command"] == "python" + assert result["mcpServers"]["test"]["args"][1] == "/tmp" + + def test_expands_skill_root(self) -> None: + """Should expand ${SKILL_ROOT} variable.""" + config = {"command": "${SKILL_ROOT}/scripts/run.sh"} + result = expand_mcp_variables(config, {"SKILL_ROOT": "/path/to/skill"}) + assert result["command"] == "/path/to/skill/scripts/run.sh" + + +class TestLoadMcpConfig: + """Tests for load_mcp_config() function.""" + + def test_loads_valid_config(self, tmp_path: Path) -> None: + """Should load valid .mcp.json file.""" + mcp_json = tmp_path / ".mcp.json" + config = { + "mcpServers": { + "test-server": { + "command": "python", + "args": ["-m", "test_server"], + } + } + } + mcp_json.write_text(json.dumps(config)) + + result = load_mcp_config(mcp_json) + assert "mcpServers" in result + assert "test-server" in result["mcpServers"] + + def test_expands_skill_root(self, tmp_path: Path) -> None: + """Should expand ${SKILL_ROOT} in config.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + mcp_json = skill_dir / ".mcp.json" + config = { + "mcpServers": { + "test": { + "command": "${SKILL_ROOT}/scripts/run.sh", + "args": [], + } + } + } + mcp_json.write_text(json.dumps(config)) + + result = load_mcp_config(mcp_json, skill_root=skill_dir) + assert result["mcpServers"]["test"]["command"] == f"{skill_dir}/scripts/run.sh" + + def test_raises_on_invalid_json(self, tmp_path: Path) -> None: + """Should raise error for invalid JSON.""" + mcp_json = tmp_path / ".mcp.json" + mcp_json.write_text("not valid json") + + with pytest.raises(SkillValidationError) as exc_info: + load_mcp_config(mcp_json) + assert "Invalid JSON" in str(exc_info.value) + + def test_raises_on_non_object(self, tmp_path: Path) -> None: + """Should raise error when JSON is not an object.""" + mcp_json = tmp_path / ".mcp.json" + mcp_json.write_text('["array", "not", "object"]') + + with pytest.raises(SkillValidationError) as exc_info: + load_mcp_config(mcp_json) + assert "expected object" in str(exc_info.value) + + +class TestSkillLoadWithMcpJson: + """Tests for Skill.load() with .mcp.json support.""" + + def test_loads_mcp_json_from_skill_directory(self, tmp_path: Path) -> None: + """Should load .mcp.json when loading SKILL.md.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + # Create SKILL.md + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: + - test +--- +# My Skill +""" + ) + + # Create .mcp.json + mcp_config = { + "mcpServers": { + "test-server": { + "command": "python", + "args": ["-m", "server"], + } + } + } + (my_skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.mcp_tools is not None + assert "mcpServers" in skill.mcp_tools + assert skill.mcp_config_path == str(my_skill_dir / ".mcp.json") + + def test_mcp_json_takes_precedence_over_frontmatter(self, tmp_path: Path) -> None: + """Should use .mcp.json over mcp_tools frontmatter.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + # Create SKILL.md with mcp_tools in frontmatter + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: + - test +mcp_tools: + mcpServers: + frontmatter-server: + command: old + args: [] +--- +# My Skill +""" + ) + + # Create .mcp.json with different config + mcp_config = { + "mcpServers": { + "file-server": { + "command": "new", + "args": [], + } + } + } + (my_skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.mcp_tools is not None + assert "file-server" in skill.mcp_tools["mcpServers"] + assert "frontmatter-server" not in skill.mcp_tools["mcpServers"] + + def test_falls_back_to_frontmatter_without_mcp_json(self, tmp_path: Path) -> None: + """Should use mcp_tools frontmatter when no .mcp.json exists.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + # Create SKILL.md with mcp_tools in frontmatter + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +mcp_tools: + mcpServers: + frontmatter-server: + command: test + args: [] +--- +# My Skill +""" + ) + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.mcp_tools is not None + assert "frontmatter-server" in skill.mcp_tools["mcpServers"] + assert skill.mcp_config_path is None + + def test_no_mcp_json_for_flat_skills(self, tmp_path: Path) -> None: + """Should not look for .mcp.json for flat .md files.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + + # Create flat skill file + skill_md = skill_dir / "flat-skill.md" + skill_md.write_text( + """--- +triggers: + - test +--- +# Flat Skill +""" + ) + + # Create .mcp.json in skills dir (should be ignored) + mcp_config = {"mcpServers": {"ignored": {"command": "x", "args": []}}} + (skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) + + skill = Skill.load(skill_md, skill_dir) + assert skill.mcp_tools is None + assert skill.mcp_config_path is None + + def test_mcp_config_path_is_set(self, tmp_path: Path) -> None: + """Should set mcp_config_path when loading from .mcp.json.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text("# My Skill") + + mcp_json = my_skill_dir / ".mcp.json" + mcp_json.write_text('{"mcpServers": {}}') + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.mcp_config_path == str(mcp_json) From f24f0162cca2e2c65fed030b9e918061a486c5f0 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 16:14:32 +0000 Subject: [PATCH 5/7] feat: support resource directories (scripts/, references/, assets/) - Add SkillResources model to track resource directories - Add discover_skill_resources() function to scan for resources - Add RESOURCE_DIRECTORIES constant for standard directory names - Add resources field to Skill model - Update Skill.load() to auto-discover resources from SKILL.md directories - Export new classes and functions from __init__.py - Add 21 unit tests for new functionality Closes #1477 Co-authored-by: openhands --- .../openhands/sdk/context/skills/__init__.py | 6 + .../openhands/sdk/context/skills/skill.py | 117 ++++++- .../skill/test_resource_directories.py | 300 ++++++++++++++++++ 3 files changed, 421 insertions(+), 2 deletions(-) create mode 100644 tests/sdk/context/skill/test_resource_directories.py diff --git a/openhands-sdk/openhands/sdk/context/skills/__init__.py b/openhands-sdk/openhands/sdk/context/skills/__init__.py index 46a6a51b38..a0fe48cd6a 100644 --- a/openhands-sdk/openhands/sdk/context/skills/__init__.py +++ b/openhands-sdk/openhands/sdk/context/skills/__init__.py @@ -1,6 +1,9 @@ from openhands.sdk.context.skills.exceptions import SkillValidationError from openhands.sdk.context.skills.skill import ( + RESOURCE_DIRECTORIES, Skill, + SkillResources, + discover_skill_resources, expand_mcp_variables, find_mcp_config, find_skill_md, @@ -21,6 +24,7 @@ __all__ = [ "Skill", + "SkillResources", "BaseTrigger", "KeywordTrigger", "TaskTrigger", @@ -35,4 +39,6 @@ "find_mcp_config", "load_mcp_config", "expand_mcp_variables", + "discover_skill_resources", + "RESOURCE_DIRECTORIES", ] diff --git a/openhands-sdk/openhands/sdk/context/skills/skill.py b/openhands-sdk/openhands/sdk/context/skills/skill.py index 2f7888f951..4ecf410711 100644 --- a/openhands-sdk/openhands/sdk/context/skills/skill.py +++ b/openhands-sdk/openhands/sdk/context/skills/skill.py @@ -32,6 +32,52 @@ # - Must not contain consecutive hyphens (--) SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*$") +# Standard resource directory names per AgentSkills spec +RESOURCE_DIRECTORIES = ("scripts", "references", "assets") + + +class SkillResources(BaseModel): + """Resource directories for a skill (AgentSkills standard). + + Per the AgentSkills specification, skills can include: + - scripts/: Executable scripts the agent can run + - references/: Reference documentation and examples + - assets/: Static assets (images, data files, etc.) + """ + + skill_root: str = Field(description="Root directory of the skill (absolute path)") + scripts: list[str] = Field( + default_factory=list, + description="List of script files in scripts/ directory (relative paths)", + ) + references: list[str] = Field( + default_factory=list, + description="List of reference files in references/ directory (relative paths)", + ) + assets: list[str] = Field( + default_factory=list, + description="List of asset files in assets/ directory (relative paths)", + ) + + def has_resources(self) -> bool: + """Check if any resources are available.""" + return bool(self.scripts or self.references or self.assets) + + def get_scripts_dir(self) -> Path | None: + """Get the scripts directory path if it exists.""" + scripts_dir = Path(self.skill_root) / "scripts" + return scripts_dir if scripts_dir.is_dir() else None + + def get_references_dir(self) -> Path | None: + """Get the references directory path if it exists.""" + refs_dir = Path(self.skill_root) / "references" + return refs_dir if refs_dir.is_dir() else None + + def get_assets_dir(self) -> Path | None: + """Get the assets directory path if it exists.""" + assets_dir = Path(self.skill_root) / "assets" + return assets_dir if assets_dir.is_dir() else None + def find_skill_md(skill_dir: Path) -> Path | None: """Find SKILL.md file in a directory (case-insensitive). @@ -50,6 +96,56 @@ def find_skill_md(skill_dir: Path) -> Path | None: return None +def discover_skill_resources(skill_dir: Path) -> SkillResources: + """Discover resource directories in a skill directory. + + Scans for standard AgentSkills resource directories: + - scripts/: Executable scripts + - references/: Reference documentation + - assets/: Static assets + + Args: + skill_dir: Path to the skill directory. + + Returns: + SkillResources with lists of files in each resource directory. + """ + resources = SkillResources(skill_root=str(skill_dir.resolve())) + + for resource_type in RESOURCE_DIRECTORIES: + resource_dir = skill_dir / resource_type + if resource_dir.is_dir(): + files = _list_resource_files(resource_dir, resource_type) + setattr(resources, resource_type, files) + + return resources + + +def _list_resource_files( + resource_dir: Path, + resource_type: str, +) -> list[str]: + """List files in a resource directory. + + Args: + resource_dir: Path to the resource directory. + resource_type: Type of resource (scripts, references, assets). + + Returns: + List of relative file paths within the resource directory. + """ + files: list[str] = [] + try: + for item in resource_dir.rglob("*"): + if item.is_file(): + # Store relative path from resource directory + rel_path = item.relative_to(resource_dir) + files.append(str(rel_path)) + except OSError as e: + logger.warning(f"Error listing {resource_type} directory: {e}") + return sorted(files) + + def validate_skill_name(name: str, directory_name: str | None = None) -> list[str]: """Validate skill name according to AgentSkills spec. @@ -284,6 +380,13 @@ class Skill(BaseModel): "Used to track the source of MCP configuration." ), ) + resources: SkillResources | None = Field( + default=None, + description=( + "Resource directories for the skill (scripts/, references/, assets/). " + "AgentSkills standard field. Only populated for SKILL.md directory format." + ), + ) PATH_TO_THIRD_PARTY_SKILL_NAME: ClassVar[dict[str, str]] = { ".cursorrules": "cursorrules", @@ -470,11 +573,12 @@ def load( # Parse AgentSkills standard fields agentskills_fields = cls._parse_agentskills_fields(metadata_dict) - # Load MCP configuration from .mcp.json if available (for SKILL.md directories) + # Load MCP configuration and resources (for SKILL.md directories) mcp_tools: dict | None = None mcp_config_path: str | None = None + resources: SkillResources | None = None - # Check for .mcp.json in skill directory (only for SKILL.md format) + # Check for .mcp.json and resources in skill directory (SKILL.md format only) if directory_name is not None: skill_root = path.parent mcp_json_path = find_mcp_config(skill_root) @@ -488,6 +592,12 @@ def load( "frontmatter. Using .mcp.json configuration." ) + # Discover resource directories + resources = discover_skill_resources(skill_root) + # Only include resources if any exist + if not resources.has_resources(): + resources = None + # Fall back to mcp_tools from frontmatter if no .mcp.json if mcp_tools is None: frontmatter_mcp = metadata_dict.get("mcp_tools") @@ -523,6 +633,7 @@ def load( inputs=inputs, mcp_tools=mcp_tools, mcp_config_path=mcp_config_path, + resources=resources, **agentskills_fields, ) @@ -534,6 +645,7 @@ def load( trigger=KeywordTrigger(keywords=keywords), mcp_tools=mcp_tools, mcp_config_path=mcp_config_path, + resources=resources, **agentskills_fields, ) else: @@ -545,6 +657,7 @@ def load( trigger=None, mcp_tools=mcp_tools, mcp_config_path=mcp_config_path, + resources=resources, **agentskills_fields, ) diff --git a/tests/sdk/context/skill/test_resource_directories.py b/tests/sdk/context/skill/test_resource_directories.py new file mode 100644 index 0000000000..61d58152c2 --- /dev/null +++ b/tests/sdk/context/skill/test_resource_directories.py @@ -0,0 +1,300 @@ +"""Tests for resource directories support (Issue #1477).""" + +from pathlib import Path + +from openhands.sdk.context.skills import ( + RESOURCE_DIRECTORIES, + Skill, + SkillResources, + discover_skill_resources, +) + + +class TestSkillResources: + """Tests for SkillResources model.""" + + def test_has_resources_empty(self) -> None: + """Should return False when no resources.""" + resources = SkillResources(skill_root="/path/to/skill") + assert not resources.has_resources() + + def test_has_resources_with_scripts(self) -> None: + """Should return True when scripts exist.""" + resources = SkillResources( + skill_root="/path/to/skill", + scripts=["run.sh"], + ) + assert resources.has_resources() + + def test_has_resources_with_references(self) -> None: + """Should return True when references exist.""" + resources = SkillResources( + skill_root="/path/to/skill", + references=["guide.md"], + ) + assert resources.has_resources() + + def test_has_resources_with_assets(self) -> None: + """Should return True when assets exist.""" + resources = SkillResources( + skill_root="/path/to/skill", + assets=["logo.png"], + ) + assert resources.has_resources() + + def test_get_scripts_dir_exists(self, tmp_path: Path) -> None: + """Should return scripts directory path when it exists.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + + resources = SkillResources(skill_root=str(skill_dir)) + assert resources.get_scripts_dir() == scripts_dir + + def test_get_scripts_dir_not_exists(self, tmp_path: Path) -> None: + """Should return None when scripts directory doesn't exist.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + resources = SkillResources(skill_root=str(skill_dir)) + assert resources.get_scripts_dir() is None + + def test_get_references_dir_exists(self, tmp_path: Path) -> None: + """Should return references directory path when it exists.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + refs_dir = skill_dir / "references" + refs_dir.mkdir() + + resources = SkillResources(skill_root=str(skill_dir)) + assert resources.get_references_dir() == refs_dir + + def test_get_assets_dir_exists(self, tmp_path: Path) -> None: + """Should return assets directory path when it exists.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + assets_dir = skill_dir / "assets" + assets_dir.mkdir() + + resources = SkillResources(skill_root=str(skill_dir)) + assert resources.get_assets_dir() == assets_dir + + +class TestDiscoverSkillResources: + """Tests for discover_skill_resources() function.""" + + def test_discovers_scripts(self, tmp_path: Path) -> None: + """Should discover files in scripts/ directory.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text("#!/bin/bash") + (scripts_dir / "setup.py").write_text("# setup") + + resources = discover_skill_resources(skill_dir) + assert "run.sh" in resources.scripts + assert "setup.py" in resources.scripts + assert len(resources.scripts) == 2 + + def test_discovers_references(self, tmp_path: Path) -> None: + """Should discover files in references/ directory.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + refs_dir = skill_dir / "references" + refs_dir.mkdir() + (refs_dir / "guide.md").write_text("# Guide") + (refs_dir / "api.md").write_text("# API") + + resources = discover_skill_resources(skill_dir) + assert "guide.md" in resources.references + assert "api.md" in resources.references + assert len(resources.references) == 2 + + def test_discovers_assets(self, tmp_path: Path) -> None: + """Should discover files in assets/ directory.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + assets_dir = skill_dir / "assets" + assets_dir.mkdir() + (assets_dir / "logo.png").write_bytes(b"PNG") + (assets_dir / "data.json").write_text("{}") + + resources = discover_skill_resources(skill_dir) + assert "logo.png" in resources.assets + assert "data.json" in resources.assets + assert len(resources.assets) == 2 + + def test_discovers_nested_files(self, tmp_path: Path) -> None: + """Should discover files in nested directories.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + subdir = scripts_dir / "utils" + subdir.mkdir() + (subdir / "helper.py").write_text("# helper") + + resources = discover_skill_resources(skill_dir) + # Should include relative path from scripts/ + assert "utils/helper.py" in resources.scripts + + def test_empty_when_no_resources(self, tmp_path: Path) -> None: + """Should return empty lists when no resource directories.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + resources = discover_skill_resources(skill_dir) + assert resources.scripts == [] + assert resources.references == [] + assert resources.assets == [] + assert not resources.has_resources() + + def test_sets_skill_root(self, tmp_path: Path) -> None: + """Should set skill_root to absolute path.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + resources = discover_skill_resources(skill_dir) + assert resources.skill_root == str(skill_dir.resolve()) + + def test_files_are_sorted(self, tmp_path: Path) -> None: + """Should return files in sorted order.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "z_last.sh").write_text("") + (scripts_dir / "a_first.sh").write_text("") + (scripts_dir / "m_middle.sh").write_text("") + + resources = discover_skill_resources(skill_dir) + assert resources.scripts == ["a_first.sh", "m_middle.sh", "z_last.sh"] + + +class TestResourceDirectoriesConstant: + """Tests for RESOURCE_DIRECTORIES constant.""" + + def test_contains_expected_directories(self) -> None: + """Should contain scripts, references, and assets.""" + assert "scripts" in RESOURCE_DIRECTORIES + assert "references" in RESOURCE_DIRECTORIES + assert "assets" in RESOURCE_DIRECTORIES + assert len(RESOURCE_DIRECTORIES) == 3 + + +class TestSkillLoadWithResources: + """Tests for Skill.load() with resource directories.""" + + def test_loads_resources_from_skill_directory(self, tmp_path: Path) -> None: + """Should discover resources when loading SKILL.md.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + # Create SKILL.md + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: + - test +--- +# My Skill +""" + ) + + # Create resource directories + scripts_dir = my_skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text("#!/bin/bash") + + refs_dir = my_skill_dir / "references" + refs_dir.mkdir() + (refs_dir / "guide.md").write_text("# Guide") + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.resources is not None + assert "run.sh" in skill.resources.scripts + assert "guide.md" in skill.resources.references + + def test_no_resources_for_flat_skills(self, tmp_path: Path) -> None: + """Should not discover resources for flat .md files.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + + # Create flat skill file + skill_md = skill_dir / "flat-skill.md" + skill_md.write_text( + """--- +triggers: + - test +--- +# Flat Skill +""" + ) + + # Create scripts dir in skills dir (should be ignored) + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text("#!/bin/bash") + + skill = Skill.load(skill_md, skill_dir) + assert skill.resources is None + + def test_resources_none_when_empty(self, tmp_path: Path) -> None: + """Should set resources to None when no resource directories exist.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text("# My Skill") + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.resources is None + + def test_resources_with_all_types(self, tmp_path: Path) -> None: + """Should discover all resource types.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text("# My Skill") + + # Create all resource directories + for dir_name in RESOURCE_DIRECTORIES: + res_dir = my_skill_dir / dir_name + res_dir.mkdir() + (res_dir / "file.txt").write_text("content") + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + assert skill.resources is not None + assert skill.resources.scripts == ["file.txt"] + assert skill.resources.references == ["file.txt"] + assert skill.resources.assets == ["file.txt"] + + def test_resources_serialization(self, tmp_path: Path) -> None: + """Should serialize resources correctly.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text("# My Skill") + + scripts_dir = my_skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text("#!/bin/bash") + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + data = skill.model_dump() + + assert "resources" in data + assert data["resources"]["scripts"] == ["run.sh"] + assert data["resources"]["skill_root"] == str(my_skill_dir.resolve()) From e8430f0e04e4316459beb5cd1b42c36e40c77dbb Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 16:18:39 +0000 Subject: [PATCH 6/7] feat: add validation and prompt generation utilities - Enhance SkillValidationError with errors list - Add validate_skill() function for skill directory validation - Add to_prompt() function for XML prompt generation - Export new functions from __init__.py - Add 24 unit tests for new functionality Closes #1478 Co-authored-by: openhands --- .../openhands/sdk/context/skills/__init__.py | 4 + .../sdk/context/skills/exceptions.py | 12 +- .../openhands/sdk/context/skills/skill.py | 133 +++++++ .../context/skill/test_validation_prompt.py | 332 ++++++++++++++++++ 4 files changed, 480 insertions(+), 1 deletion(-) create mode 100644 tests/sdk/context/skill/test_validation_prompt.py diff --git a/openhands-sdk/openhands/sdk/context/skills/__init__.py b/openhands-sdk/openhands/sdk/context/skills/__init__.py index a0fe48cd6a..9d7d280ea0 100644 --- a/openhands-sdk/openhands/sdk/context/skills/__init__.py +++ b/openhands-sdk/openhands/sdk/context/skills/__init__.py @@ -12,6 +12,8 @@ load_public_skills, load_skills_from_dir, load_user_skills, + to_prompt, + validate_skill, validate_skill_name, ) from openhands.sdk.context.skills.trigger import ( @@ -36,9 +38,11 @@ "SkillValidationError", "find_skill_md", "validate_skill_name", + "validate_skill", "find_mcp_config", "load_mcp_config", "expand_mcp_variables", "discover_skill_resources", "RESOURCE_DIRECTORIES", + "to_prompt", ] diff --git a/openhands-sdk/openhands/sdk/context/skills/exceptions.py b/openhands-sdk/openhands/sdk/context/skills/exceptions.py index a94d6eb6cc..2a73cd9745 100644 --- a/openhands-sdk/openhands/sdk/context/skills/exceptions.py +++ b/openhands-sdk/openhands/sdk/context/skills/exceptions.py @@ -7,5 +7,15 @@ class SkillError(Exception): class SkillValidationError(SkillError): """Raised when there's a validation error in skill metadata.""" - def __init__(self, message: str = "Skill validation failed") -> None: + def __init__( + self, + message: str = "Skill validation failed", + errors: list[str] | None = None, + ) -> None: super().__init__(message) + self.errors = errors or [] + + def __str__(self) -> str: + if self.errors: + return f"{self.args[0]}: {'; '.join(self.errors)}" + return str(self.args[0]) diff --git a/openhands-sdk/openhands/sdk/context/skills/skill.py b/openhands-sdk/openhands/sdk/context/skills/skill.py index 4ecf410711..6c058bd79c 100644 --- a/openhands-sdk/openhands/sdk/context/skills/skill.py +++ b/openhands-sdk/openhands/sdk/context/skills/skill.py @@ -286,6 +286,139 @@ def load_mcp_config( return config +def validate_skill(skill_dir: str | Path) -> list[str]: + """Validate a skill directory according to AgentSkills spec. + + Performs basic validation of skill structure and metadata: + - Checks for SKILL.md file + - Validates skill name format + - Validates frontmatter structure + + Args: + skill_dir: Path to the skill directory containing SKILL.md + + Returns: + List of validation error messages (empty if valid) + """ + skill_path = Path(skill_dir) + errors: list[str] = [] + + # Check directory exists + if not skill_path.is_dir(): + errors.append(f"Skill directory does not exist: {skill_path}") + return errors + + # Check for SKILL.md + skill_md = find_skill_md(skill_path) + if not skill_md: + errors.append("Missing SKILL.md file") + return errors + + # Validate skill name (directory name) + dir_name = skill_path.name + name_errors = validate_skill_name(dir_name, dir_name) + errors.extend(name_errors) + + # Parse and validate frontmatter + try: + content = skill_md.read_text(encoding="utf-8") + parsed = frontmatter.loads(content) + metadata = dict(parsed.metadata) + + # Check for recommended fields + if not parsed.content.strip(): + errors.append("SKILL.md has no content (body is empty)") + + # Validate description length if present + description = metadata.get("description") + if isinstance(description, str) and len(description) > 1024: + errors.append( + f"Description exceeds 1024 characters ({len(description)} chars)" + ) + + # Validate mcp_tools if present + mcp_tools = metadata.get("mcp_tools") + if mcp_tools is not None and not isinstance(mcp_tools, dict): + errors.append("mcp_tools must be a dictionary") + + # Validate triggers if present + triggers = metadata.get("triggers") + if triggers is not None and not isinstance(triggers, list): + errors.append("triggers must be a list") + + # Validate inputs if present + inputs = metadata.get("inputs") + if inputs is not None and not isinstance(inputs, list): + errors.append("inputs must be a list") + + except Exception as e: + errors.append(f"Failed to parse SKILL.md: {e}") + + # Check for .mcp.json validity if present + mcp_json = find_mcp_config(skill_path) + if mcp_json: + try: + load_mcp_config(mcp_json, skill_path) + except SkillValidationError as e: + errors.append(f"Invalid .mcp.json: {e}") + + return errors + + +def to_prompt(skills: list["Skill"]) -> str: + """Generate XML prompt block for available skills. + + Creates an `` XML block suitable for inclusion + in system prompts, following the AgentSkills format. + + Args: + skills: List of skills to include in the prompt + + Returns: + XML string in AgentSkills format + + Example: + >>> skills = [Skill(name="pdf-tools", content="...", description="...")] + >>> print(to_prompt(skills)) + + Extract text from PDF files. + + """ # noqa: E501 + if not skills: + return "\n" + + lines = [""] + for skill in skills: + # Use description if available, otherwise use first line of content + description = skill.description + if not description: + # Extract first non-empty line from content as fallback + for line in skill.content.split("\n"): + line = line.strip() + # Skip markdown headers and empty lines + if line and not line.startswith("#"): + description = line[:200] # Limit to 200 chars + break + description = description or "" + # Escape XML special characters + description = _escape_xml(description) + name = _escape_xml(skill.name) + lines.append(f' {description}') + lines.append("") + return "\n".join(lines) + + +def _escape_xml(text: str) -> str: + """Escape XML special characters.""" + return ( + text.replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace('"', """) + .replace("'", "'") + ) + + # Union type for all trigger types TriggerType = Annotated[ KeywordTrigger | TaskTrigger, diff --git a/tests/sdk/context/skill/test_validation_prompt.py b/tests/sdk/context/skill/test_validation_prompt.py new file mode 100644 index 0000000000..cbf381c88a --- /dev/null +++ b/tests/sdk/context/skill/test_validation_prompt.py @@ -0,0 +1,332 @@ +"""Tests for validation and prompt generation utilities (Issue #1478).""" + +from pathlib import Path + +from openhands.sdk.context.skills import ( + Skill, + SkillValidationError, + to_prompt, + validate_skill, +) + + +class TestSkillValidationError: + """Tests for enhanced SkillValidationError.""" + + def test_error_without_errors_list(self) -> None: + """Should work without errors list.""" + error = SkillValidationError("Test error") + assert str(error) == "Test error" + assert error.errors == [] + + def test_error_with_errors_list(self) -> None: + """Should include errors in string representation.""" + error = SkillValidationError( + "Validation failed", + errors=["Error 1", "Error 2"], + ) + assert "Error 1" in str(error) + assert "Error 2" in str(error) + assert error.errors == ["Error 1", "Error 2"] + + def test_error_with_empty_errors_list(self) -> None: + """Should handle empty errors list.""" + error = SkillValidationError("Test error", errors=[]) + assert str(error) == "Test error" + assert error.errors == [] + + +class TestValidateSkill: + """Tests for validate_skill() function.""" + + def test_validates_valid_skill(self, tmp_path: Path) -> None: + """Should return empty list for valid skill.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + """--- +description: A test skill +--- +# My Skill + +This is a valid skill. +""" + ) + + errors = validate_skill(skill_dir) + assert errors == [] + + def test_error_for_nonexistent_directory(self, tmp_path: Path) -> None: + """Should return error for nonexistent directory.""" + skill_dir = tmp_path / "nonexistent" + errors = validate_skill(skill_dir) + assert len(errors) == 1 + assert "does not exist" in errors[0] + + def test_error_for_missing_skill_md(self, tmp_path: Path) -> None: + """Should return error for missing SKILL.md.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + errors = validate_skill(skill_dir) + assert len(errors) == 1 + assert "Missing SKILL.md" in errors[0] + + def test_error_for_invalid_skill_name(self, tmp_path: Path) -> None: + """Should return error for invalid skill name.""" + skill_dir = tmp_path / "Invalid_Name" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text("# Invalid Name Skill\n\nContent here.") + + errors = validate_skill(skill_dir) + # Should have error about lowercase alphanumeric requirement + assert any("lowercase" in e.lower() for e in errors) + + def test_error_for_empty_content(self, tmp_path: Path) -> None: + """Should return error for empty SKILL.md content.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + """--- +description: A test skill +--- +""" + ) + + errors = validate_skill(skill_dir) + assert any("no content" in e.lower() for e in errors) + + def test_error_for_long_description(self, tmp_path: Path) -> None: + """Should return error for description over 1024 chars.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + long_desc = "x" * 1025 + skill_md.write_text( + f"""--- +description: {long_desc} +--- +# My Skill + +Content here. +""" + ) + + errors = validate_skill(skill_dir) + assert any("1024 characters" in e for e in errors) + + def test_error_for_invalid_mcp_tools(self, tmp_path: Path) -> None: + """Should return error for invalid mcp_tools type.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + """--- +mcp_tools: "not a dict" +--- +# My Skill + +Content here. +""" + ) + + errors = validate_skill(skill_dir) + assert any("mcp_tools must be a dictionary" in e for e in errors) + + def test_error_for_invalid_triggers(self, tmp_path: Path) -> None: + """Should return error for invalid triggers type.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: "not a list" +--- +# My Skill + +Content here. +""" + ) + + errors = validate_skill(skill_dir) + assert any("triggers must be a list" in e for e in errors) + + def test_error_for_invalid_inputs(self, tmp_path: Path) -> None: + """Should return error for invalid inputs type.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + """--- +inputs: "not a list" +--- +# My Skill + +Content here. +""" + ) + + errors = validate_skill(skill_dir) + assert any("inputs must be a list" in e for e in errors) + + def test_validates_mcp_json(self, tmp_path: Path) -> None: + """Should validate .mcp.json if present.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text("# My Skill\n\nContent here.") + mcp_json = skill_dir / ".mcp.json" + mcp_json.write_text("invalid json") + + errors = validate_skill(skill_dir) + assert any(".mcp.json" in e for e in errors) + + def test_multiple_errors(self, tmp_path: Path) -> None: + """Should return multiple errors.""" + skill_dir = tmp_path / "Invalid_Name" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + """--- +triggers: "not a list" +--- +""" + ) + + errors = validate_skill(skill_dir) + # Should have errors for: invalid name, empty content, invalid triggers + assert len(errors) >= 2 + + +class TestToPrompt: + """Tests for to_prompt() function.""" + + def test_empty_skills_list(self) -> None: + """Should return empty available_skills block.""" + result = to_prompt([]) + assert result == "\n" + + def test_single_skill_with_description(self) -> None: + """Should generate XML for skill with description.""" + skill = Skill( + name="pdf-tools", + content="# PDF Tools\n\nProcess PDF files.", + description="Extract text and tables from PDF files.", + ) + result = to_prompt([skill]) + assert '' in result + assert "Extract text and tables from PDF files." in result + assert "" in result + assert "" in result + + def test_skill_without_description_uses_content(self) -> None: + """Should use first content line when no description.""" + skill = Skill( + name="code-review", + content="# Code Review\n\nReview code for bugs and style issues.", + ) + result = to_prompt([skill]) + assert '' in result + assert "Review code for bugs and style issues." in result + + def test_multiple_skills(self) -> None: + """Should generate XML for multiple skills.""" + skills = [ + Skill( + name="pdf-tools", + content="# PDF Tools", + description="Process PDF files.", + ), + Skill( + name="code-review", + content="# Code Review", + description="Review code.", + ), + ] + result = to_prompt(skills) + assert '' in result + assert '' in result + assert result.count(" None: + """Should escape XML special characters.""" + skill = Skill( + name="test-skill", + content="# Test", + description='Handle & "quotes" safely.', + ) + result = to_prompt([skill]) + assert "<tags>" in result + assert "&" in result + assert ""quotes"" in result + + def test_escapes_skill_name(self) -> None: + """Should escape skill name.""" + skill = Skill( + name="test&skill", + content="# Test", + description="Test skill.", + ) + result = to_prompt([skill]) + assert 'name="test&skill"' in result + + def test_truncates_long_content_fallback(self) -> None: + """Should truncate long content when used as fallback.""" + long_content = "x" * 300 + skill = Skill( + name="test-skill", + content=f"# Test\n\n{long_content}", + ) + result = to_prompt([skill]) + # Should be truncated to 200 chars + assert len(result) < 400 + + def test_skips_markdown_headers_in_content(self) -> None: + """Should skip markdown headers when extracting description.""" + skill = Skill( + name="test-skill", + content="# Header\n## Subheader\n\nActual content here.", + ) + result = to_prompt([skill]) + assert "Actual content here." in result + assert "# Header" not in result + + def test_handles_empty_description_and_content(self) -> None: + """Should handle skill with empty description and content.""" + skill = Skill( + name="empty-skill", + content="", + ) + result = to_prompt([skill]) + assert '' in result + + +class TestToPromptIntegration: + """Integration tests for to_prompt() with loaded skills.""" + + def test_with_loaded_skill(self, tmp_path: Path) -> None: + """Should work with skills loaded from files.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + skill_md = my_skill_dir / "SKILL.md" + skill_md.write_text( + """--- +description: A test skill for processing data. +--- +# My Skill + +This skill processes data efficiently. +""" + ) + + skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") + result = to_prompt([skill]) + + assert '' in result + assert "A test skill for processing data." in result From 3427e4c844f892c891fd42c48980ccadeeff6f25 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 22 Dec 2025 16:25:07 +0000 Subject: [PATCH 7/7] refactor: consolidate AgentSkills tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduce test code by 74% while maintaining essential coverage: - test_agentskills_fields.py: 299 → 73 lines - test_skill_md_convention.py: 405 → 86 lines - test_mcp_json_config.py: 301 → 89 lines - test_resource_directories.py: 300 → 83 lines - test_validation_prompt.py: 332 → 101 lines Total: 1637 → 432 lines Co-authored-by: openhands --- .../context/skill/test_agentskills_fields.py | 312 ++---------- .../sdk/context/skill/test_mcp_json_config.py | 358 +++----------- .../skill/test_resource_directories.py | 359 +++----------- .../context/skill/test_skill_md_convention.py | 450 +++--------------- .../context/skill/test_validation_prompt.py | 411 ++++------------ 5 files changed, 352 insertions(+), 1538 deletions(-) diff --git a/tests/sdk/context/skill/test_agentskills_fields.py b/tests/sdk/context/skill/test_agentskills_fields.py index a61226735a..4fb9acd340 100644 --- a/tests/sdk/context/skill/test_agentskills_fields.py +++ b/tests/sdk/context/skill/test_agentskills_fields.py @@ -7,293 +7,79 @@ from openhands.sdk.context.skills import Skill, SkillValidationError -def test_skill_with_all_agentskills_fields(): - """Test loading a skill with all AgentSkills standard fields.""" +def test_skill_with_agentskills_fields() -> None: + """Skill should support AgentSkills standard fields.""" skill_content = """--- name: pdf-processing -description: Extract text and tables from PDF files, fill forms, merge documents. +description: Extract text from PDF files. license: Apache-2.0 -compatibility: Requires poppler-utils and ghostscript +compatibility: Requires poppler-utils metadata: author: example-org version: "1.0" allowed-tools: Bash(pdftotext:*) Read Write +triggers: + - pdf --- - -# PDF Processing Skill - -Instructions for processing PDF files. +# PDF Processing """ - skill = Skill.load(Path("pdf-processing.md"), file_content=skill_content) + skill = Skill.load(Path("pdf.md"), file_content=skill_content) assert skill.name == "pdf-processing" - assert skill.description == ( - "Extract text and tables from PDF files, fill forms, merge documents." - ) + assert skill.description == "Extract text from PDF files." assert skill.license == "Apache-2.0" - assert skill.compatibility == "Requires poppler-utils and ghostscript" + assert skill.compatibility == "Requires poppler-utils" assert skill.metadata == {"author": "example-org", "version": "1.0"} assert skill.allowed_tools == ["Bash(pdftotext:*)", "Read", "Write"] - assert skill.trigger is None # No triggers = always active - - -def test_skill_with_description_only(): - """Test loading a skill with only description field.""" - skill_content = """--- -name: simple-skill -description: A simple skill for testing. ---- - -# Simple Skill - -Content here. -""" - skill = Skill.load(Path("simple.md"), file_content=skill_content) - - assert skill.name == "simple-skill" - assert skill.description == "A simple skill for testing." - assert skill.license is None - assert skill.compatibility is None - assert skill.metadata is None - assert skill.allowed_tools is None - - -def test_skill_with_allowed_tools_as_list(): - """Test loading a skill with allowed-tools as a YAML list.""" - skill_content = """--- -name: list-tools-skill -description: Skill with tools as list. -allowed-tools: - - Bash(git:*) - - Read - - Write ---- - -# List Tools Skill - -Content here. -""" - skill = Skill.load(Path("list-tools.md"), file_content=skill_content) - - assert skill.allowed_tools == ["Bash(git:*)", "Read", "Write"] - - -def test_skill_with_allowed_tools_underscore(): - """Test loading a skill with allowed_tools (underscore variant).""" - skill_content = """--- -name: underscore-tools-skill -description: Skill with underscore variant. -allowed_tools: Bash Read Write ---- - -# Underscore Tools Skill - -Content here. -""" - skill = Skill.load(Path("underscore-tools.md"), file_content=skill_content) - - assert skill.allowed_tools == ["Bash", "Read", "Write"] - - -def test_skill_with_metadata_numeric_values(): - """Test that metadata values are converted to strings.""" - skill_content = """--- -name: numeric-metadata-skill -description: Skill with numeric metadata values. -metadata: - version: 2 - priority: 1.5 - enabled: true ---- - -# Numeric Metadata Skill - -Content here. -""" - skill = Skill.load(Path("numeric-metadata.md"), file_content=skill_content) - - # All values should be converted to strings - assert skill.metadata == { - "version": "2", - "priority": "1.5", - "enabled": "True", - } - - -def test_skill_agentskills_fields_with_triggers(): - """Test AgentSkills fields work with keyword triggers.""" - skill_content = """--- -name: triggered-skill -description: A skill that activates on keywords. -license: MIT -triggers: - - pdf - - document ---- - -# Triggered Skill - -Content here. -""" - skill = Skill.load(Path("triggered.md"), file_content=skill_content) - - assert skill.name == "triggered-skill" - assert skill.description == "A skill that activates on keywords." - assert skill.license == "MIT" - assert skill.trigger is not None - assert skill.match_trigger("process this pdf") == "pdf" + assert skill.match_trigger("process pdf") == "pdf" -def test_skill_agentskills_fields_with_task_trigger(): - """Test AgentSkills fields work with task triggers.""" - skill_content = """--- -name: task-skill -description: A task skill with inputs. -compatibility: Requires Python 3.12+ -inputs: - - name: filename - description: The file to process ---- - -# Task Skill - -Process ${filename} here. -""" - skill = Skill.load(Path("task.md"), file_content=skill_content) - - assert skill.name == "task-skill" - assert skill.description == "A task skill with inputs." - assert skill.compatibility == "Requires Python 3.12+" - assert len(skill.inputs) == 1 - assert skill.inputs[0].name == "filename" - - -def test_skill_invalid_description_type(): - """Test that non-string description raises error.""" - skill_content = """--- -name: invalid-desc -description: - - not - - a - - string ---- - -Content. -""" - with pytest.raises(SkillValidationError) as excinfo: - Skill.load(Path("invalid.md"), file_content=skill_content) - - assert "description must be a string" in str(excinfo.value) - - -def test_skill_invalid_license_type(): - """Test that non-string license raises error.""" - skill_content = """--- -name: invalid-license -license: 123 ---- - -Content. -""" - with pytest.raises(SkillValidationError) as excinfo: - Skill.load(Path("invalid.md"), file_content=skill_content) - - assert "license must be a string" in str(excinfo.value) - - -def test_skill_invalid_compatibility_type(): - """Test that non-string compatibility raises error.""" - skill_content = """--- -name: invalid-compat -compatibility: - requires: git ---- - -Content. -""" - with pytest.raises(SkillValidationError) as excinfo: - Skill.load(Path("invalid.md"), file_content=skill_content) - - assert "compatibility must be a string" in str(excinfo.value) - - -def test_skill_invalid_metadata_type(): - """Test that non-dict metadata raises error.""" - skill_content = """--- -name: invalid-meta -metadata: not-a-dict ---- - -Content. -""" - with pytest.raises(SkillValidationError) as excinfo: - Skill.load(Path("invalid.md"), file_content=skill_content) - - assert "metadata must be a dictionary" in str(excinfo.value) - - -def test_skill_invalid_allowed_tools_type(): - """Test that invalid allowed-tools type raises error.""" - skill_content = """--- -name: invalid-tools -allowed-tools: 123 ---- - -Content. -""" - with pytest.raises(SkillValidationError) as excinfo: - Skill.load(Path("invalid.md"), file_content=skill_content) - - assert "allowed-tools must be a string or list" in str(excinfo.value) - +def test_skill_allowed_tools_formats() -> None: + """allowed-tools should accept string or list format.""" + # String format + skill = Skill.load( + Path("s.md"), file_content="---\nname: s\nallowed-tools: A B\n---\n#" + ) + assert skill.allowed_tools == ["A", "B"] -def test_skill_serialization_with_agentskills_fields(): - """Test that AgentSkills fields serialize and deserialize correctly.""" - skill = Skill( - name="test-skill", - content="Test content", - description="Test description", - license="MIT", - compatibility="Python 3.12+", - metadata={"author": "test", "version": "1.0"}, - allowed_tools=["Read", "Write"], + # List format + skill = Skill.load( + Path("s.md"), file_content="---\nname: s\nallowed-tools:\n - A\n - B\n---\n#" ) + assert skill.allowed_tools == ["A", "B"] - # Serialize - serialized = skill.model_dump() - assert serialized["description"] == "Test description" - assert serialized["license"] == "MIT" - assert serialized["compatibility"] == "Python 3.12+" - assert serialized["metadata"] == {"author": "test", "version": "1.0"} - assert serialized["allowed_tools"] == ["Read", "Write"] + # Underscore variant + skill = Skill.load( + Path("s.md"), file_content="---\nname: s\nallowed_tools: A B\n---\n#" + ) + assert skill.allowed_tools == ["A", "B"] - # Deserialize - deserialized = Skill.model_validate(serialized) - assert deserialized.description == "Test description" - assert deserialized.license == "MIT" - assert deserialized.compatibility == "Python 3.12+" - assert deserialized.metadata == {"author": "test", "version": "1.0"} - assert deserialized.allowed_tools == ["Read", "Write"] +def test_skill_invalid_field_types() -> None: + """Skill should reject invalid field types.""" + # Invalid description + with pytest.raises(SkillValidationError, match="description must be a string"): + Skill.load( + Path("s.md"), file_content="---\nname: s\ndescription:\n - list\n---\n#" + ) -def test_skill_backward_compatibility(): - """Test that existing skills without AgentSkills fields still work.""" - skill_content = """--- -name: legacy-skill -triggers: - - legacy ---- + # Invalid metadata + with pytest.raises(SkillValidationError, match="metadata must be a dictionary"): + Skill.load(Path("s.md"), file_content="---\nname: s\nmetadata: string\n---\n#") -# Legacy Skill + # Invalid allowed-tools + with pytest.raises(SkillValidationError, match="allowed-tools must be"): + Skill.load( + Path("s.md"), file_content="---\nname: s\nallowed-tools: 123\n---\n#" + ) -This skill has no AgentSkills fields. -""" - skill = Skill.load(Path("legacy.md"), file_content=skill_content) - assert skill.name == "legacy-skill" +def test_skill_backward_compatibility() -> None: + """Skills without AgentSkills fields should still work.""" + skill = Skill.load( + Path("s.md"), file_content="---\nname: legacy\ntriggers:\n - test\n---\n#" + ) + assert skill.name == "legacy" assert skill.description is None assert skill.license is None - assert skill.compatibility is None - assert skill.metadata is None - assert skill.allowed_tools is None - assert skill.match_trigger("use legacy feature") == "legacy" + assert skill.match_trigger("test") == "test" diff --git a/tests/sdk/context/skill/test_mcp_json_config.py b/tests/sdk/context/skill/test_mcp_json_config.py index 576e079f6c..857c812c23 100644 --- a/tests/sdk/context/skill/test_mcp_json_config.py +++ b/tests/sdk/context/skill/test_mcp_json_config.py @@ -14,288 +14,76 @@ ) -class TestFindMcpConfig: - """Tests for find_mcp_config() function.""" - - def test_finds_mcp_json(self, tmp_path: Path) -> None: - """Should find .mcp.json file in directory.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - mcp_json = skill_dir / ".mcp.json" - mcp_json.write_text('{"mcpServers": {}}') - - result = find_mcp_config(skill_dir) - assert result == mcp_json - - def test_returns_none_when_not_found(self, tmp_path: Path) -> None: - """Should return None when no .mcp.json exists.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - (skill_dir / "SKILL.md").write_text("# Skill") - - result = find_mcp_config(skill_dir) - assert result is None - - def test_returns_none_for_non_directory(self, tmp_path: Path) -> None: - """Should return None for non-directory path.""" - file_path = tmp_path / "not-a-dir.txt" - file_path.write_text("content") - - result = find_mcp_config(file_path) - assert result is None - - -class TestExpandMcpVariables: - """Tests for expand_mcp_variables() function.""" - - def test_expands_simple_variable(self) -> None: - """Should expand ${VAR} with provided value.""" - config = {"command": "${MY_VAR}"} - result = expand_mcp_variables(config, {"MY_VAR": "value"}) - assert result["command"] == "value" - - def test_expands_env_variable(self, monkeypatch: pytest.MonkeyPatch) -> None: - """Should expand ${VAR} from environment.""" - monkeypatch.setenv("TEST_ENV_VAR", "env_value") - config = {"command": "${TEST_ENV_VAR}"} - result = expand_mcp_variables(config, {}) - assert result["command"] == "env_value" - - def test_provided_takes_precedence(self, monkeypatch: pytest.MonkeyPatch) -> None: - """Provided variables should take precedence over environment.""" - monkeypatch.setenv("MY_VAR", "env_value") - config = {"command": "${MY_VAR}"} - result = expand_mcp_variables(config, {"MY_VAR": "provided_value"}) - assert result["command"] == "provided_value" - - def test_expands_with_default(self) -> None: - """Should use default value when variable not found.""" - config = {"command": "${MISSING:-default_value}"} - result = expand_mcp_variables(config, {}) - assert result["command"] == "default_value" - - def test_keeps_original_when_not_found(self) -> None: - """Should keep original when variable not found and no default.""" - config = {"command": "${MISSING}"} - result = expand_mcp_variables(config, {}) - assert result["command"] == "${MISSING}" - - def test_expands_nested_values(self) -> None: - """Should expand variables in nested structures.""" - config = { - "mcpServers": { - "test": { - "command": "${CMD}", - "args": ["--path", "${PATH_VAR}"], - } - } - } - result = expand_mcp_variables(config, {"CMD": "python", "PATH_VAR": "/tmp"}) - assert result["mcpServers"]["test"]["command"] == "python" - assert result["mcpServers"]["test"]["args"][1] == "/tmp" - - def test_expands_skill_root(self) -> None: - """Should expand ${SKILL_ROOT} variable.""" - config = {"command": "${SKILL_ROOT}/scripts/run.sh"} - result = expand_mcp_variables(config, {"SKILL_ROOT": "/path/to/skill"}) - assert result["command"] == "/path/to/skill/scripts/run.sh" - - -class TestLoadMcpConfig: - """Tests for load_mcp_config() function.""" - - def test_loads_valid_config(self, tmp_path: Path) -> None: - """Should load valid .mcp.json file.""" - mcp_json = tmp_path / ".mcp.json" - config = { - "mcpServers": { - "test-server": { - "command": "python", - "args": ["-m", "test_server"], - } - } - } - mcp_json.write_text(json.dumps(config)) - - result = load_mcp_config(mcp_json) - assert "mcpServers" in result - assert "test-server" in result["mcpServers"] - - def test_expands_skill_root(self, tmp_path: Path) -> None: - """Should expand ${SKILL_ROOT} in config.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - mcp_json = skill_dir / ".mcp.json" - config = { - "mcpServers": { - "test": { - "command": "${SKILL_ROOT}/scripts/run.sh", - "args": [], - } - } - } - mcp_json.write_text(json.dumps(config)) - - result = load_mcp_config(mcp_json, skill_root=skill_dir) - assert result["mcpServers"]["test"]["command"] == f"{skill_dir}/scripts/run.sh" - - def test_raises_on_invalid_json(self, tmp_path: Path) -> None: - """Should raise error for invalid JSON.""" - mcp_json = tmp_path / ".mcp.json" - mcp_json.write_text("not valid json") - - with pytest.raises(SkillValidationError) as exc_info: - load_mcp_config(mcp_json) - assert "Invalid JSON" in str(exc_info.value) - - def test_raises_on_non_object(self, tmp_path: Path) -> None: - """Should raise error when JSON is not an object.""" - mcp_json = tmp_path / ".mcp.json" - mcp_json.write_text('["array", "not", "object"]') - - with pytest.raises(SkillValidationError) as exc_info: - load_mcp_config(mcp_json) - assert "expected object" in str(exc_info.value) - - -class TestSkillLoadWithMcpJson: - """Tests for Skill.load() with .mcp.json support.""" - - def test_loads_mcp_json_from_skill_directory(self, tmp_path: Path) -> None: - """Should load .mcp.json when loading SKILL.md.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - # Create SKILL.md - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: - - test ---- -# My Skill -""" - ) - - # Create .mcp.json - mcp_config = { - "mcpServers": { - "test-server": { - "command": "python", - "args": ["-m", "server"], - } - } - } - (my_skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.mcp_tools is not None - assert "mcpServers" in skill.mcp_tools - assert skill.mcp_config_path == str(my_skill_dir / ".mcp.json") - - def test_mcp_json_takes_precedence_over_frontmatter(self, tmp_path: Path) -> None: - """Should use .mcp.json over mcp_tools frontmatter.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - # Create SKILL.md with mcp_tools in frontmatter - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: - - test -mcp_tools: - mcpServers: - frontmatter-server: - command: old - args: [] ---- -# My Skill -""" - ) - - # Create .mcp.json with different config - mcp_config = { - "mcpServers": { - "file-server": { - "command": "new", - "args": [], - } - } - } - (my_skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.mcp_tools is not None - assert "file-server" in skill.mcp_tools["mcpServers"] - assert "frontmatter-server" not in skill.mcp_tools["mcpServers"] - - def test_falls_back_to_frontmatter_without_mcp_json(self, tmp_path: Path) -> None: - """Should use mcp_tools frontmatter when no .mcp.json exists.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - # Create SKILL.md with mcp_tools in frontmatter - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -mcp_tools: - mcpServers: - frontmatter-server: - command: test - args: [] ---- -# My Skill -""" - ) - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.mcp_tools is not None - assert "frontmatter-server" in skill.mcp_tools["mcpServers"] - assert skill.mcp_config_path is None - - def test_no_mcp_json_for_flat_skills(self, tmp_path: Path) -> None: - """Should not look for .mcp.json for flat .md files.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - - # Create flat skill file - skill_md = skill_dir / "flat-skill.md" - skill_md.write_text( - """--- -triggers: - - test ---- -# Flat Skill -""" - ) - - # Create .mcp.json in skills dir (should be ignored) - mcp_config = {"mcpServers": {"ignored": {"command": "x", "args": []}}} - (skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) - - skill = Skill.load(skill_md, skill_dir) - assert skill.mcp_tools is None - assert skill.mcp_config_path is None - - def test_mcp_config_path_is_set(self, tmp_path: Path) -> None: - """Should set mcp_config_path when loading from .mcp.json.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text("# My Skill") - - mcp_json = my_skill_dir / ".mcp.json" - mcp_json.write_text('{"mcpServers": {}}') - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.mcp_config_path == str(mcp_json) +def test_find_mcp_config(tmp_path: Path) -> None: + """find_mcp_config() should locate .mcp.json files.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + # Not found + assert find_mcp_config(skill_dir) is None + + # Found + mcp_json = skill_dir / ".mcp.json" + mcp_json.write_text('{"mcpServers": {}}') + assert find_mcp_config(skill_dir) == mcp_json + + +def test_expand_mcp_variables(monkeypatch: pytest.MonkeyPatch) -> None: + """expand_mcp_variables() should expand ${VAR} patterns.""" + # Simple expansion + result = expand_mcp_variables({"cmd": "${MY_VAR}"}, {"MY_VAR": "value"}) + assert result["cmd"] == "value" + + # Environment variable + monkeypatch.setenv("TEST_VAR", "env_value") + result = expand_mcp_variables({"cmd": "${TEST_VAR}"}, {}) + assert result["cmd"] == "env_value" + + # Default value + result = expand_mcp_variables({"cmd": "${MISSING:-default}"}, {}) + assert result["cmd"] == "default" + + # Nested structures + config = {"mcpServers": {"test": {"command": "${CMD}", "args": ["${ARG}"]}}} + result = expand_mcp_variables(config, {"CMD": "python", "ARG": "--help"}) + assert result["mcpServers"]["test"]["command"] == "python" + assert result["mcpServers"]["test"]["args"][0] == "--help" + + +def test_load_mcp_config(tmp_path: Path) -> None: + """load_mcp_config() should load and validate .mcp.json files.""" + mcp_json = tmp_path / ".mcp.json" + + # Valid config + config = {"mcpServers": {"test": {"command": "python", "args": []}}} + mcp_json.write_text(json.dumps(config)) + result = load_mcp_config(mcp_json) + assert "test" in result["mcpServers"] + + # Invalid JSON + mcp_json.write_text("not valid json") + with pytest.raises(SkillValidationError, match="Invalid JSON"): + load_mcp_config(mcp_json) + + +def test_skill_load_with_mcp_json(tmp_path: Path) -> None: + """Skill.load() should load .mcp.json for SKILL.md directories.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + (my_skill_dir / "SKILL.md").write_text("# My Skill") + mcp_config = {"mcpServers": {"test": {"command": "python", "args": []}}} + (my_skill_dir / ".mcp.json").write_text(json.dumps(mcp_config)) + + skill = Skill.load(my_skill_dir / "SKILL.md", skill_dir, directory_name="my-skill") + assert skill.mcp_tools is not None + assert "test" in skill.mcp_tools["mcpServers"] + assert skill.mcp_config_path == str(my_skill_dir / ".mcp.json") + + # Flat files should not load .mcp.json + flat_skill = skill_dir / "flat.md" + flat_skill.write_text("# Flat") + skill = Skill.load(flat_skill, skill_dir) + assert skill.mcp_config_path is None diff --git a/tests/sdk/context/skill/test_resource_directories.py b/tests/sdk/context/skill/test_resource_directories.py index 61d58152c2..7ea637e61d 100644 --- a/tests/sdk/context/skill/test_resource_directories.py +++ b/tests/sdk/context/skill/test_resource_directories.py @@ -10,291 +10,74 @@ ) -class TestSkillResources: - """Tests for SkillResources model.""" - - def test_has_resources_empty(self) -> None: - """Should return False when no resources.""" - resources = SkillResources(skill_root="/path/to/skill") - assert not resources.has_resources() - - def test_has_resources_with_scripts(self) -> None: - """Should return True when scripts exist.""" - resources = SkillResources( - skill_root="/path/to/skill", - scripts=["run.sh"], - ) - assert resources.has_resources() - - def test_has_resources_with_references(self) -> None: - """Should return True when references exist.""" - resources = SkillResources( - skill_root="/path/to/skill", - references=["guide.md"], - ) - assert resources.has_resources() - - def test_has_resources_with_assets(self) -> None: - """Should return True when assets exist.""" - resources = SkillResources( - skill_root="/path/to/skill", - assets=["logo.png"], - ) - assert resources.has_resources() - - def test_get_scripts_dir_exists(self, tmp_path: Path) -> None: - """Should return scripts directory path when it exists.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - scripts_dir = skill_dir / "scripts" - scripts_dir.mkdir() - - resources = SkillResources(skill_root=str(skill_dir)) - assert resources.get_scripts_dir() == scripts_dir - - def test_get_scripts_dir_not_exists(self, tmp_path: Path) -> None: - """Should return None when scripts directory doesn't exist.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - - resources = SkillResources(skill_root=str(skill_dir)) - assert resources.get_scripts_dir() is None - - def test_get_references_dir_exists(self, tmp_path: Path) -> None: - """Should return references directory path when it exists.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - refs_dir = skill_dir / "references" - refs_dir.mkdir() - - resources = SkillResources(skill_root=str(skill_dir)) - assert resources.get_references_dir() == refs_dir - - def test_get_assets_dir_exists(self, tmp_path: Path) -> None: - """Should return assets directory path when it exists.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - assets_dir = skill_dir / "assets" - assets_dir.mkdir() - - resources = SkillResources(skill_root=str(skill_dir)) - assert resources.get_assets_dir() == assets_dir - - -class TestDiscoverSkillResources: - """Tests for discover_skill_resources() function.""" - - def test_discovers_scripts(self, tmp_path: Path) -> None: - """Should discover files in scripts/ directory.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - scripts_dir = skill_dir / "scripts" - scripts_dir.mkdir() - (scripts_dir / "run.sh").write_text("#!/bin/bash") - (scripts_dir / "setup.py").write_text("# setup") - - resources = discover_skill_resources(skill_dir) - assert "run.sh" in resources.scripts - assert "setup.py" in resources.scripts - assert len(resources.scripts) == 2 - - def test_discovers_references(self, tmp_path: Path) -> None: - """Should discover files in references/ directory.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - refs_dir = skill_dir / "references" - refs_dir.mkdir() - (refs_dir / "guide.md").write_text("# Guide") - (refs_dir / "api.md").write_text("# API") - - resources = discover_skill_resources(skill_dir) - assert "guide.md" in resources.references - assert "api.md" in resources.references - assert len(resources.references) == 2 - - def test_discovers_assets(self, tmp_path: Path) -> None: - """Should discover files in assets/ directory.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - assets_dir = skill_dir / "assets" - assets_dir.mkdir() - (assets_dir / "logo.png").write_bytes(b"PNG") - (assets_dir / "data.json").write_text("{}") - - resources = discover_skill_resources(skill_dir) - assert "logo.png" in resources.assets - assert "data.json" in resources.assets - assert len(resources.assets) == 2 - - def test_discovers_nested_files(self, tmp_path: Path) -> None: - """Should discover files in nested directories.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - scripts_dir = skill_dir / "scripts" - scripts_dir.mkdir() - subdir = scripts_dir / "utils" - subdir.mkdir() - (subdir / "helper.py").write_text("# helper") - - resources = discover_skill_resources(skill_dir) - # Should include relative path from scripts/ - assert "utils/helper.py" in resources.scripts - - def test_empty_when_no_resources(self, tmp_path: Path) -> None: - """Should return empty lists when no resource directories.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - - resources = discover_skill_resources(skill_dir) - assert resources.scripts == [] - assert resources.references == [] - assert resources.assets == [] - assert not resources.has_resources() - - def test_sets_skill_root(self, tmp_path: Path) -> None: - """Should set skill_root to absolute path.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - - resources = discover_skill_resources(skill_dir) - assert resources.skill_root == str(skill_dir.resolve()) - - def test_files_are_sorted(self, tmp_path: Path) -> None: - """Should return files in sorted order.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - scripts_dir = skill_dir / "scripts" - scripts_dir.mkdir() - (scripts_dir / "z_last.sh").write_text("") - (scripts_dir / "a_first.sh").write_text("") - (scripts_dir / "m_middle.sh").write_text("") - - resources = discover_skill_resources(skill_dir) - assert resources.scripts == ["a_first.sh", "m_middle.sh", "z_last.sh"] - - -class TestResourceDirectoriesConstant: - """Tests for RESOURCE_DIRECTORIES constant.""" - - def test_contains_expected_directories(self) -> None: - """Should contain scripts, references, and assets.""" - assert "scripts" in RESOURCE_DIRECTORIES - assert "references" in RESOURCE_DIRECTORIES - assert "assets" in RESOURCE_DIRECTORIES - assert len(RESOURCE_DIRECTORIES) == 3 - - -class TestSkillLoadWithResources: - """Tests for Skill.load() with resource directories.""" - - def test_loads_resources_from_skill_directory(self, tmp_path: Path) -> None: - """Should discover resources when loading SKILL.md.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - # Create SKILL.md - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: - - test ---- -# My Skill -""" - ) - - # Create resource directories - scripts_dir = my_skill_dir / "scripts" - scripts_dir.mkdir() - (scripts_dir / "run.sh").write_text("#!/bin/bash") - - refs_dir = my_skill_dir / "references" - refs_dir.mkdir() - (refs_dir / "guide.md").write_text("# Guide") - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.resources is not None - assert "run.sh" in skill.resources.scripts - assert "guide.md" in skill.resources.references - - def test_no_resources_for_flat_skills(self, tmp_path: Path) -> None: - """Should not discover resources for flat .md files.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - - # Create flat skill file - skill_md = skill_dir / "flat-skill.md" - skill_md.write_text( - """--- -triggers: - - test ---- -# Flat Skill -""" - ) - - # Create scripts dir in skills dir (should be ignored) - scripts_dir = skill_dir / "scripts" - scripts_dir.mkdir() - (scripts_dir / "run.sh").write_text("#!/bin/bash") - - skill = Skill.load(skill_md, skill_dir) - assert skill.resources is None - - def test_resources_none_when_empty(self, tmp_path: Path) -> None: - """Should set resources to None when no resource directories exist.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text("# My Skill") - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.resources is None - - def test_resources_with_all_types(self, tmp_path: Path) -> None: - """Should discover all resource types.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text("# My Skill") - - # Create all resource directories - for dir_name in RESOURCE_DIRECTORIES: - res_dir = my_skill_dir / dir_name - res_dir.mkdir() - (res_dir / "file.txt").write_text("content") - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - assert skill.resources is not None - assert skill.resources.scripts == ["file.txt"] - assert skill.resources.references == ["file.txt"] - assert skill.resources.assets == ["file.txt"] - - def test_resources_serialization(self, tmp_path: Path) -> None: - """Should serialize resources correctly.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text("# My Skill") - - scripts_dir = my_skill_dir / "scripts" - scripts_dir.mkdir() - (scripts_dir / "run.sh").write_text("#!/bin/bash") - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - data = skill.model_dump() - - assert "resources" in data - assert data["resources"]["scripts"] == ["run.sh"] - assert data["resources"]["skill_root"] == str(my_skill_dir.resolve()) +def test_skill_resources_model(tmp_path: Path) -> None: + """SkillResources should track resources and provide directory paths.""" + # Empty resources + resources = SkillResources(skill_root="/path/to/skill") + assert not resources.has_resources() + + # With resources + resources = SkillResources(skill_root="/path", scripts=["run.sh"]) + assert resources.has_resources() + + # Directory path getters + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "scripts").mkdir() + resources = SkillResources(skill_root=str(skill_dir)) + assert resources.get_scripts_dir() == skill_dir / "scripts" + assert resources.get_references_dir() is None # Doesn't exist + + +def test_discover_skill_resources(tmp_path: Path) -> None: + """discover_skill_resources() should find files in resource directories.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + # Create resource directories with files + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text("#!/bin/bash") + subdir = scripts_dir / "utils" + subdir.mkdir() + (subdir / "helper.py").write_text("# helper") + + refs_dir = skill_dir / "references" + refs_dir.mkdir() + (refs_dir / "guide.md").write_text("# Guide") + + resources = discover_skill_resources(skill_dir) + assert "run.sh" in resources.scripts + assert "utils/helper.py" in resources.scripts # Nested files + assert "guide.md" in resources.references + assert resources.assets == [] # No assets dir + assert resources.skill_root == str(skill_dir.resolve()) + + +def test_resource_directories_constant() -> None: + """RESOURCE_DIRECTORIES should contain standard directory names.""" + assert set(RESOURCE_DIRECTORIES) == {"scripts", "references", "assets"} + + +def test_skill_load_with_resources(tmp_path: Path) -> None: + """Skill.load() should discover resources for SKILL.md directories.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "my-skill" + my_skill_dir.mkdir() + + (my_skill_dir / "SKILL.md").write_text("# My Skill") + scripts_dir = my_skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text("#!/bin/bash") + + # SKILL.md directory format - should have resources + skill = Skill.load(my_skill_dir / "SKILL.md", skill_dir, directory_name="my-skill") + assert skill.resources is not None + assert "run.sh" in skill.resources.scripts + + # Flat file format - should not have resources + flat_skill = skill_dir / "flat.md" + flat_skill.write_text("# Flat Skill") + skill = Skill.load(flat_skill, skill_dir) + assert skill.resources is None diff --git a/tests/sdk/context/skill/test_skill_md_convention.py b/tests/sdk/context/skill/test_skill_md_convention.py index 3e50dd9434..5e04dc5849 100644 --- a/tests/sdk/context/skill/test_skill_md_convention.py +++ b/tests/sdk/context/skill/test_skill_md_convention.py @@ -13,393 +13,79 @@ ) -class TestFindSkillMd: - """Tests for find_skill_md() function.""" - - def test_finds_skill_md(self, tmp_path: Path) -> None: - """Should find SKILL.md file in directory.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text("# My Skill") - - result = find_skill_md(skill_dir) - assert result == skill_md - - def test_finds_skill_md_case_insensitive(self, tmp_path: Path) -> None: - """Should find skill.md with different casing.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "skill.MD" - skill_md.write_text("# My Skill") - - result = find_skill_md(skill_dir) - assert result == skill_md - - def test_returns_none_when_not_found(self, tmp_path: Path) -> None: - """Should return None when no SKILL.md exists.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - (skill_dir / "README.md").write_text("# README") - - result = find_skill_md(skill_dir) - assert result is None - - def test_returns_none_for_non_directory(self, tmp_path: Path) -> None: - """Should return None for non-directory path.""" - file_path = tmp_path / "not-a-dir.txt" - file_path.write_text("content") - - result = find_skill_md(file_path) - assert result is None - - -class TestValidateSkillName: - """Tests for validate_skill_name() function.""" - - def test_valid_simple_name(self) -> None: - """Should accept simple lowercase name.""" - errors = validate_skill_name("myskill") - assert errors == [] - - def test_valid_hyphenated_name(self) -> None: - """Should accept hyphenated name.""" - errors = validate_skill_name("my-skill") - assert errors == [] - - def test_valid_multi_hyphen_name(self) -> None: - """Should accept name with multiple hyphens.""" - errors = validate_skill_name("my-cool-skill") - assert errors == [] - - def test_valid_with_numbers(self) -> None: - """Should accept name with numbers.""" - errors = validate_skill_name("skill2") - assert errors == [] - - def test_invalid_uppercase(self) -> None: - """Should reject uppercase letters.""" - errors = validate_skill_name("MySkill") - assert len(errors) == 1 - assert "lowercase" in errors[0] - - def test_invalid_underscore(self) -> None: - """Should reject underscores.""" - errors = validate_skill_name("my_skill") - assert len(errors) == 1 - assert "lowercase" in errors[0] - - def test_invalid_starts_with_hyphen(self) -> None: - """Should reject name starting with hyphen.""" - errors = validate_skill_name("-myskill") - assert len(errors) == 1 - - def test_invalid_ends_with_hyphen(self) -> None: - """Should reject name ending with hyphen.""" - errors = validate_skill_name("myskill-") - assert len(errors) == 1 - - def test_invalid_consecutive_hyphens(self) -> None: - """Should reject consecutive hyphens.""" - errors = validate_skill_name("my--skill") - assert len(errors) == 1 - - def test_invalid_too_long(self) -> None: - """Should reject name exceeding 64 characters.""" - long_name = "a" * 65 - errors = validate_skill_name(long_name) - assert len(errors) == 1 - assert "64 characters" in errors[0] - - def test_invalid_empty(self) -> None: - """Should reject empty name.""" - errors = validate_skill_name("") - assert len(errors) == 1 - assert "empty" in errors[0] - - def test_directory_name_match(self) -> None: - """Should accept when name matches directory.""" - errors = validate_skill_name("my-skill", directory_name="my-skill") - assert errors == [] - - def test_directory_name_mismatch(self) -> None: - """Should reject when name doesn't match directory.""" - errors = validate_skill_name("my-skill", directory_name="other-skill") - assert len(errors) == 1 - assert "does not match directory" in errors[0] - - -class TestSkillLoadWithDirectoryName: - """Tests for Skill.load() with directory_name parameter.""" - - def test_load_skill_md_derives_name_from_directory(self, tmp_path: Path) -> None: - """Should derive skill name from directory when loading SKILL.md.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "pdf-tools" - my_skill_dir.mkdir() - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: - - pdf ---- -# PDF Tools - -Process PDF files. -""" - ) - - skill = Skill.load(skill_md, skill_dir, directory_name="pdf-tools") - assert skill.name == "pdf-tools" - - def test_load_skill_md_frontmatter_name_overrides(self, tmp_path: Path) -> None: - """Frontmatter name should override directory name.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "pdf-tools" - my_skill_dir.mkdir() - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -name: custom-name -triggers: - - pdf ---- -# PDF Tools -""" - ) - - skill = Skill.load(skill_md, skill_dir, directory_name="pdf-tools") - assert skill.name == "custom-name" - - def test_load_with_name_validation_valid(self, tmp_path: Path) -> None: - """Should pass validation for valid name.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "pdf-tools" - my_skill_dir.mkdir() - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: - - pdf ---- -# PDF Tools -""" - ) - - skill = Skill.load( - skill_md, +def test_find_skill_md(tmp_path: Path) -> None: + """find_skill_md() should locate SKILL.md files case-insensitively.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + + # Not found + assert find_skill_md(skill_dir) is None + + # Found (case-insensitive) + skill_md = skill_dir / "skill.MD" + skill_md.write_text("# My Skill") + assert find_skill_md(skill_dir) == skill_md + + +def test_validate_skill_name() -> None: + """validate_skill_name() should enforce AgentSkills naming rules.""" + # Valid names + assert validate_skill_name("my-skill") == [] + assert validate_skill_name("skill2") == [] + assert validate_skill_name("my-cool-skill") == [] + + # Invalid names + assert len(validate_skill_name("MySkill")) == 1 # Uppercase + assert len(validate_skill_name("my_skill")) == 1 # Underscore + assert len(validate_skill_name("-myskill")) == 1 # Starts with hyphen + assert len(validate_skill_name("my--skill")) == 1 # Consecutive hyphens + assert len(validate_skill_name("a" * 65)) == 1 # Too long + assert len(validate_skill_name("")) == 1 # Empty + + # Directory name mismatch + errors = validate_skill_name("my-skill", directory_name="other-skill") + assert "does not match directory" in errors[0] + + +def test_skill_load_with_directory_name(tmp_path: Path) -> None: + """Skill.load() should use directory_name for SKILL.md format.""" + skill_dir = tmp_path / "skills" + skill_dir.mkdir() + my_skill_dir = skill_dir / "pdf-tools" + my_skill_dir.mkdir() + (my_skill_dir / "SKILL.md").write_text("---\ntriggers:\n - pdf\n---\n# PDF Tools") + + # Uses directory name + skill = Skill.load(my_skill_dir / "SKILL.md", skill_dir, directory_name="pdf-tools") + assert skill.name == "pdf-tools" + + # Validates name when requested + bad_dir = skill_dir / "Bad_Name" + bad_dir.mkdir() + (bad_dir / "SKILL.md").write_text("# Bad") + with pytest.raises(SkillValidationError, match="Invalid skill name"): + Skill.load( + bad_dir / "SKILL.md", skill_dir, - directory_name="pdf-tools", + directory_name="Bad_Name", validate_name=True, ) - assert skill.name == "pdf-tools" - - def test_load_with_name_validation_invalid(self, tmp_path: Path) -> None: - """Should raise error for invalid name when validation enabled.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "PDF_Tools" - my_skill_dir.mkdir() - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: - - pdf ---- -# PDF Tools -""" - ) - - with pytest.raises(SkillValidationError) as exc_info: - Skill.load( - skill_md, - skill_dir, - directory_name="PDF_Tools", - validate_name=True, - ) - assert "Invalid skill name" in str(exc_info.value) - - def test_load_with_name_mismatch_validation(self, tmp_path: Path) -> None: - """Should raise error when frontmatter name doesn't match directory.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "pdf-tools" - my_skill_dir.mkdir() - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -name: other-name -triggers: - - pdf ---- -# PDF Tools -""" - ) - - with pytest.raises(SkillValidationError) as exc_info: - Skill.load( - skill_md, - skill_dir, - directory_name="pdf-tools", - validate_name=True, - ) - assert "does not match directory" in str(exc_info.value) - - -class TestLoadSkillsFromDirWithSkillMd: - """Tests for load_skills_from_dir() with SKILL.md directories.""" - def test_loads_skill_md_directories(self, tmp_path: Path) -> None: - """Should load skills from skill-name/SKILL.md directories.""" - # Create directory structure - repo_root = tmp_path / "repo" - repo_root.mkdir() - openhands_dir = repo_root / ".openhands" - openhands_dir.mkdir() - skills_dir = openhands_dir / "skills" - skills_dir.mkdir() - # Create AgentSkills-style skill - pdf_skill_dir = skills_dir / "pdf-tools" - pdf_skill_dir.mkdir() - (pdf_skill_dir / "SKILL.md").write_text( - """--- -triggers: - - pdf ---- -# PDF Tools +def test_load_skills_from_dir_with_skill_md(tmp_path: Path) -> None: + """load_skills_from_dir() should discover SKILL.md directories.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir() -Process PDF files. -""" - ) - - repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) - assert "pdf-tools" in knowledge_skills - assert knowledge_skills["pdf-tools"].name == "pdf-tools" + # Flat skill + (skills_dir / "flat-skill.md").write_text("---\ntriggers:\n - flat\n---\n# Flat") - def test_loads_both_formats(self, tmp_path: Path) -> None: - """Should load both flat .md files and SKILL.md directories.""" - # Create directory structure - repo_root = tmp_path / "repo" - repo_root.mkdir() - openhands_dir = repo_root / ".openhands" - openhands_dir.mkdir() - skills_dir = openhands_dir / "skills" - skills_dir.mkdir() - - # Create flat .md skill - (skills_dir / "flat-skill.md").write_text( - """--- -triggers: - - flat ---- -# Flat Skill -""" - ) - - # Create AgentSkills-style skill - dir_skill = skills_dir / "dir-skill" - dir_skill.mkdir() - (dir_skill / "SKILL.md").write_text( - """--- -triggers: - - directory ---- -# Directory Skill -""" - ) - - repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) - assert "flat-skill" in knowledge_skills - assert "dir-skill" in knowledge_skills - - def test_skill_md_takes_precedence(self, tmp_path: Path) -> None: - """Files in SKILL.md directories should not be loaded separately.""" - # Create directory structure - repo_root = tmp_path / "repo" - repo_root.mkdir() - openhands_dir = repo_root / ".openhands" - openhands_dir.mkdir() - skills_dir = openhands_dir / "skills" - skills_dir.mkdir() - - # Create AgentSkills-style skill with extra .md file - pdf_skill_dir = skills_dir / "pdf-tools" - pdf_skill_dir.mkdir() - (pdf_skill_dir / "SKILL.md").write_text( - """--- -triggers: - - pdf ---- -# PDF Tools -""" - ) - # This file should NOT be loaded as a separate skill - (pdf_skill_dir / "extra.md").write_text( - """--- -triggers: - - extra ---- -# Extra -""" - ) - - repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) - # Should only have pdf-tools, not extra - assert "pdf-tools" in knowledge_skills - assert "extra" not in knowledge_skills - assert len(knowledge_skills) == 1 - - def test_validate_names_option(self, tmp_path: Path) -> None: - """Should validate names when validate_names=True.""" - # Create directory structure - repo_root = tmp_path / "repo" - repo_root.mkdir() - openhands_dir = repo_root / ".openhands" - openhands_dir.mkdir() - skills_dir = openhands_dir / "skills" - skills_dir.mkdir() - - # Create skill with invalid name - invalid_skill_dir = skills_dir / "Invalid_Name" - invalid_skill_dir.mkdir() - (invalid_skill_dir / "SKILL.md").write_text( - """--- -triggers: - - test ---- -# Invalid -""" - ) - - with pytest.raises(SkillValidationError) as exc_info: - load_skills_from_dir(skills_dir, validate_names=True) - assert "Invalid skill name" in str(exc_info.value) - - def test_case_insensitive_skill_md(self, tmp_path: Path) -> None: - """Should find skill.md with different casing.""" - # Create directory structure - repo_root = tmp_path / "repo" - repo_root.mkdir() - openhands_dir = repo_root / ".openhands" - openhands_dir.mkdir() - skills_dir = openhands_dir / "skills" - skills_dir.mkdir() - - # Create skill with lowercase skill.md - skill_dir = skills_dir / "my-skill" - skill_dir.mkdir() - (skill_dir / "skill.md").write_text( - """--- -triggers: - - test ---- -# My Skill -""" - ) + # SKILL.md directory + dir_skill = skills_dir / "dir-skill" + dir_skill.mkdir() + (dir_skill / "SKILL.md").write_text("---\ntriggers:\n - dir\n---\n# Dir") - repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) - assert "my-skill" in knowledge_skills + repo_skills, knowledge_skills = load_skills_from_dir(skills_dir) + assert "flat-skill" in knowledge_skills + assert "dir-skill" in knowledge_skills + assert knowledge_skills["dir-skill"].name == "dir-skill" diff --git a/tests/sdk/context/skill/test_validation_prompt.py b/tests/sdk/context/skill/test_validation_prompt.py index cbf381c88a..c01cc19625 100644 --- a/tests/sdk/context/skill/test_validation_prompt.py +++ b/tests/sdk/context/skill/test_validation_prompt.py @@ -10,323 +10,94 @@ ) -class TestSkillValidationError: - """Tests for enhanced SkillValidationError.""" - - def test_error_without_errors_list(self) -> None: - """Should work without errors list.""" - error = SkillValidationError("Test error") - assert str(error) == "Test error" - assert error.errors == [] - - def test_error_with_errors_list(self) -> None: - """Should include errors in string representation.""" - error = SkillValidationError( - "Validation failed", - errors=["Error 1", "Error 2"], - ) - assert "Error 1" in str(error) - assert "Error 2" in str(error) - assert error.errors == ["Error 1", "Error 2"] - - def test_error_with_empty_errors_list(self) -> None: - """Should handle empty errors list.""" - error = SkillValidationError("Test error", errors=[]) - assert str(error) == "Test error" - assert error.errors == [] - - -class TestValidateSkill: - """Tests for validate_skill() function.""" - - def test_validates_valid_skill(self, tmp_path: Path) -> None: - """Should return empty list for valid skill.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text( - """--- -description: A test skill ---- -# My Skill - -This is a valid skill. -""" - ) - - errors = validate_skill(skill_dir) - assert errors == [] - - def test_error_for_nonexistent_directory(self, tmp_path: Path) -> None: - """Should return error for nonexistent directory.""" - skill_dir = tmp_path / "nonexistent" - errors = validate_skill(skill_dir) - assert len(errors) == 1 - assert "does not exist" in errors[0] - - def test_error_for_missing_skill_md(self, tmp_path: Path) -> None: - """Should return error for missing SKILL.md.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - - errors = validate_skill(skill_dir) - assert len(errors) == 1 - assert "Missing SKILL.md" in errors[0] - - def test_error_for_invalid_skill_name(self, tmp_path: Path) -> None: - """Should return error for invalid skill name.""" - skill_dir = tmp_path / "Invalid_Name" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text("# Invalid Name Skill\n\nContent here.") - - errors = validate_skill(skill_dir) - # Should have error about lowercase alphanumeric requirement - assert any("lowercase" in e.lower() for e in errors) - - def test_error_for_empty_content(self, tmp_path: Path) -> None: - """Should return error for empty SKILL.md content.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text( - """--- -description: A test skill ---- -""" - ) - - errors = validate_skill(skill_dir) - assert any("no content" in e.lower() for e in errors) - - def test_error_for_long_description(self, tmp_path: Path) -> None: - """Should return error for description over 1024 chars.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - long_desc = "x" * 1025 - skill_md.write_text( - f"""--- -description: {long_desc} ---- -# My Skill - -Content here. -""" - ) - - errors = validate_skill(skill_dir) - assert any("1024 characters" in e for e in errors) - - def test_error_for_invalid_mcp_tools(self, tmp_path: Path) -> None: - """Should return error for invalid mcp_tools type.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text( - """--- -mcp_tools: "not a dict" ---- -# My Skill - -Content here. -""" - ) - - errors = validate_skill(skill_dir) - assert any("mcp_tools must be a dictionary" in e for e in errors) - - def test_error_for_invalid_triggers(self, tmp_path: Path) -> None: - """Should return error for invalid triggers type.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: "not a list" ---- -# My Skill - -Content here. -""" - ) - - errors = validate_skill(skill_dir) - assert any("triggers must be a list" in e for e in errors) - - def test_error_for_invalid_inputs(self, tmp_path: Path) -> None: - """Should return error for invalid inputs type.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text( - """--- -inputs: "not a list" ---- -# My Skill - -Content here. -""" - ) - - errors = validate_skill(skill_dir) - assert any("inputs must be a list" in e for e in errors) - - def test_validates_mcp_json(self, tmp_path: Path) -> None: - """Should validate .mcp.json if present.""" - skill_dir = tmp_path / "my-skill" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text("# My Skill\n\nContent here.") - mcp_json = skill_dir / ".mcp.json" - mcp_json.write_text("invalid json") - - errors = validate_skill(skill_dir) - assert any(".mcp.json" in e for e in errors) - - def test_multiple_errors(self, tmp_path: Path) -> None: - """Should return multiple errors.""" - skill_dir = tmp_path / "Invalid_Name" - skill_dir.mkdir() - skill_md = skill_dir / "SKILL.md" - skill_md.write_text( - """--- -triggers: "not a list" ---- -""" - ) - - errors = validate_skill(skill_dir) - # Should have errors for: invalid name, empty content, invalid triggers - assert len(errors) >= 2 - - -class TestToPrompt: - """Tests for to_prompt() function.""" - - def test_empty_skills_list(self) -> None: - """Should return empty available_skills block.""" - result = to_prompt([]) - assert result == "\n" - - def test_single_skill_with_description(self) -> None: - """Should generate XML for skill with description.""" - skill = Skill( - name="pdf-tools", - content="# PDF Tools\n\nProcess PDF files.", - description="Extract text and tables from PDF files.", - ) - result = to_prompt([skill]) - assert '' in result - assert "Extract text and tables from PDF files." in result - assert "" in result - assert "" in result - - def test_skill_without_description_uses_content(self) -> None: - """Should use first content line when no description.""" - skill = Skill( - name="code-review", - content="# Code Review\n\nReview code for bugs and style issues.", - ) - result = to_prompt([skill]) - assert '' in result - assert "Review code for bugs and style issues." in result - - def test_multiple_skills(self) -> None: - """Should generate XML for multiple skills.""" - skills = [ - Skill( - name="pdf-tools", - content="# PDF Tools", - description="Process PDF files.", - ), - Skill( - name="code-review", - content="# Code Review", - description="Review code.", - ), - ] - result = to_prompt(skills) - assert '' in result - assert '' in result - assert result.count(" None: - """Should escape XML special characters.""" - skill = Skill( - name="test-skill", - content="# Test", - description='Handle & "quotes" safely.', - ) - result = to_prompt([skill]) - assert "<tags>" in result - assert "&" in result - assert ""quotes"" in result - - def test_escapes_skill_name(self) -> None: - """Should escape skill name.""" - skill = Skill( - name="test&skill", - content="# Test", - description="Test skill.", - ) - result = to_prompt([skill]) - assert 'name="test&skill"' in result - - def test_truncates_long_content_fallback(self) -> None: - """Should truncate long content when used as fallback.""" - long_content = "x" * 300 - skill = Skill( - name="test-skill", - content=f"# Test\n\n{long_content}", - ) - result = to_prompt([skill]) - # Should be truncated to 200 chars - assert len(result) < 400 - - def test_skips_markdown_headers_in_content(self) -> None: - """Should skip markdown headers when extracting description.""" - skill = Skill( - name="test-skill", - content="# Header\n## Subheader\n\nActual content here.", - ) - result = to_prompt([skill]) - assert "Actual content here." in result - assert "# Header" not in result - - def test_handles_empty_description_and_content(self) -> None: - """Should handle skill with empty description and content.""" - skill = Skill( - name="empty-skill", - content="", - ) - result = to_prompt([skill]) - assert '' in result - - -class TestToPromptIntegration: - """Integration tests for to_prompt() with loaded skills.""" - - def test_with_loaded_skill(self, tmp_path: Path) -> None: - """Should work with skills loaded from files.""" - skill_dir = tmp_path / "skills" - skill_dir.mkdir() - my_skill_dir = skill_dir / "my-skill" - my_skill_dir.mkdir() - - skill_md = my_skill_dir / "SKILL.md" - skill_md.write_text( - """--- -description: A test skill for processing data. ---- -# My Skill - -This skill processes data efficiently. -""" - ) - - skill = Skill.load(skill_md, skill_dir, directory_name="my-skill") - result = to_prompt([skill]) - - assert '' in result - assert "A test skill for processing data." in result +def test_skill_validation_error_with_errors_list() -> None: + """SkillValidationError should include errors in string representation.""" + # Without errors + error = SkillValidationError("Test error") + assert str(error) == "Test error" + assert error.errors == [] + + # With errors + error = SkillValidationError("Validation failed", errors=["Error 1", "Error 2"]) + assert "Error 1" in str(error) + assert "Error 2" in str(error) + assert error.errors == ["Error 1", "Error 2"] + + +def test_validate_skill_valid_directory(tmp_path: Path) -> None: + """validate_skill() should return empty list for valid skill.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\ndescription: Test\n---\n# My Skill\n\nContent." + ) + + errors = validate_skill(skill_dir) + assert errors == [] + + +def test_validate_skill_errors(tmp_path: Path) -> None: + """validate_skill() should return errors for invalid skills.""" + # Nonexistent directory + errors = validate_skill(tmp_path / "nonexistent") + assert any("does not exist" in e for e in errors) + + # Missing SKILL.md + empty_dir = tmp_path / "empty" + empty_dir.mkdir() + errors = validate_skill(empty_dir) + assert any("Missing SKILL.md" in e for e in errors) + + # Invalid skill name + bad_name = tmp_path / "Invalid_Name" + bad_name.mkdir() + (bad_name / "SKILL.md").write_text("# Test\n\nContent.") + errors = validate_skill(bad_name) + assert any("lowercase" in e.lower() for e in errors) + + # Empty content + empty_content = tmp_path / "empty-content" + empty_content.mkdir() + (empty_content / "SKILL.md").write_text("---\ndescription: Test\n---\n") + errors = validate_skill(empty_content) + assert any("no content" in e.lower() for e in errors) + + +def test_to_prompt_generates_xml() -> None: + """to_prompt() should generate valid XML for skills.""" + # Empty list + assert to_prompt([]) == "\n" + + # Single skill with description + skill = Skill(name="pdf-tools", content="# PDF", description="Process PDFs.") + result = to_prompt([skill]) + assert '' in result + assert "Process PDFs." in result + assert "" in result + + # Multiple skills + skills = [ + Skill(name="pdf-tools", content="# PDF", description="Process PDFs."), + Skill(name="code-review", content="# Code", description="Review code."), + ] + result = to_prompt(skills) + assert result.count(" None: + """to_prompt() should escape XML special characters.""" + skill = Skill( + name="test", content="# Test", description='Handle & "quotes".' + ) + result = to_prompt([skill]) + assert "<tags>" in result + assert "&" in result + assert ""quotes"" in result + + +def test_to_prompt_uses_content_fallback() -> None: + """to_prompt() should use content when no description.""" + skill = Skill(name="test", content="# Header\n\nActual content here.") + result = to_prompt([skill]) + assert "Actual content here." in result + assert "# Header" not in result