diff --git a/openhands-sdk/openhands/sdk/context/skills/__init__.py b/openhands-sdk/openhands/sdk/context/skills/__init__.py index fc667f89ab..9d7d280ea0 100644 --- a/openhands-sdk/openhands/sdk/context/skills/__init__.py +++ b/openhands-sdk/openhands/sdk/context/skills/__init__.py @@ -1,10 +1,20 @@ 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, + load_mcp_config, load_project_skills, load_public_skills, load_skills_from_dir, load_user_skills, + to_prompt, + validate_skill, + validate_skill_name, ) from openhands.sdk.context.skills.trigger import ( BaseTrigger, @@ -16,6 +26,7 @@ __all__ = [ "Skill", + "SkillResources", "BaseTrigger", "KeywordTrigger", "TaskTrigger", @@ -25,4 +36,13 @@ "load_project_skills", "load_public_skills", "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 b845c91451..78a1b8be47 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,400 @@ # 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]+)*$") + +# 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). + + 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 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. + + 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 + + +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 + + +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, @@ -113,6 +506,20 @@ 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." + ), + ) + 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." + ), + ) @field_validator("allowed_tools", mode="before") @classmethod @@ -187,19 +594,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() @@ -227,6 +650,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)}" + ) + # Extract AgentSkills standard fields (Pydantic validators handle # transformation). Handle "allowed-tools" to "allowed_tools" key mapping. allowed_tools_value = metadata_dict.get( @@ -244,6 +675,38 @@ def load( k: v for k, v in agentskills_fields.items() if v is not None } + # 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 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) + 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." + ) + + # 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") + 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): @@ -270,6 +733,9 @@ def load( source=str(path), trigger=TaskTrigger(triggers=keywords), inputs=inputs, + mcp_tools=mcp_tools, + mcp_config_path=mcp_config_path, + resources=resources, **agentskills_fields, ) @@ -279,19 +745,21 @@ def load( content=content, source=str(path), trigger=KeywordTrigger(keywords=keywords), + mcp_tools=mcp_tools, + mcp_config_path=mcp_config_path, + resources=resources, **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, + resources=resources, **agentskills_fields, ) @@ -370,13 +838,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. @@ -386,14 +861,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 @@ -403,26 +878,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(): + 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(): - md_files = [f for f in skill_dir.rglob("*.md") if f.name != "README.md"] + 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_mcp_json_config.py b/tests/sdk/context/skill/test_mcp_json_config.py new file mode 100644 index 0000000000..857c812c23 --- /dev/null +++ b/tests/sdk/context/skill/test_mcp_json_config.py @@ -0,0 +1,89 @@ +"""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, +) + + +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 new file mode 100644 index 0000000000..7ea637e61d --- /dev/null +++ b/tests/sdk/context/skill/test_resource_directories.py @@ -0,0 +1,83 @@ +"""Tests for resource directories support (Issue #1477).""" + +from pathlib import Path + +from openhands.sdk.context.skills import ( + RESOURCE_DIRECTORIES, + Skill, + SkillResources, + discover_skill_resources, +) + + +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 new file mode 100644 index 0000000000..5e04dc5849 --- /dev/null +++ b/tests/sdk/context/skill/test_skill_md_convention.py @@ -0,0 +1,91 @@ +"""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, +) + + +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="Bad_Name", + validate_name=True, + ) + + +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() + + # Flat skill + (skills_dir / "flat-skill.md").write_text("---\ntriggers:\n - flat\n---\n# Flat") + + # 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 "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 new file mode 100644 index 0000000000..c01cc19625 --- /dev/null +++ b/tests/sdk/context/skill/test_validation_prompt.py @@ -0,0 +1,103 @@ +"""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, +) + + +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