From b7ecc94c9b85aadc79e0d6598ea42ad7dbaa15f1 Mon Sep 17 00:00:00 2001 From: Re-bin Date: Tue, 10 Mar 2026 09:16:23 +0000 Subject: [PATCH] fix(skill-creator): restore validation and align packaging docs --- nanobot/skills/skill-creator/SKILL.md | 10 +- .../skill-creator/scripts/package_skill.py | 77 ++++--- .../skill-creator/scripts/quick_validate.py | 213 ++++++++++++++++++ tests/test_skill_creator_scripts.py | 127 +++++++++++ 4 files changed, 392 insertions(+), 35 deletions(-) create mode 100644 nanobot/skills/skill-creator/scripts/quick_validate.py create mode 100644 tests/test_skill_creator_scripts.py diff --git a/nanobot/skills/skill-creator/SKILL.md b/nanobot/skills/skill-creator/SKILL.md index f4d6e0b..ea53abe 100644 --- a/nanobot/skills/skill-creator/SKILL.md +++ b/nanobot/skills/skill-creator/SKILL.md @@ -268,6 +268,8 @@ Skip this step only if the skill being developed already exists, and iteration o When creating a new skill from scratch, always run the `init_skill.py` script. The script conveniently generates a new template skill directory that automatically includes everything a skill requires, making the skill creation process much more efficient and reliable. +For `nanobot`, custom skills should live under the active workspace `skills/` directory so they can be discovered automatically at runtime (for example, `/skills/my-skill/SKILL.md`). + Usage: ```bash @@ -277,9 +279,9 @@ scripts/init_skill.py --path [--resources script Examples: ```bash -scripts/init_skill.py my-skill --path skills/public -scripts/init_skill.py my-skill --path skills/public --resources scripts,references -scripts/init_skill.py my-skill --path skills/public --resources scripts --examples +scripts/init_skill.py my-skill --path ./workspace/skills +scripts/init_skill.py my-skill --path ./workspace/skills --resources scripts,references +scripts/init_skill.py my-skill --path ./workspace/skills --resources scripts --examples ``` The script: @@ -326,7 +328,7 @@ Write the YAML frontmatter with `name` and `description`: - Include all "when to use" information here - Not in the body. The body is only loaded after triggering, so "When to Use This Skill" sections in the body are not helpful to the agent. - Example description for a `docx` skill: "Comprehensive document creation, editing, and analysis with support for tracked changes, comments, formatting preservation, and text extraction. Use when the agent needs to work with professional documents (.docx files) for: (1) Creating new documents, (2) Modifying or editing content, (3) Working with tracked changes, (4) Adding comments, or any other document tasks" -Do not include any other fields in YAML frontmatter. +Keep frontmatter minimal. In `nanobot`, `metadata` and `always` are also supported when needed, but avoid adding extra fields unless they are actually required. ##### Body diff --git a/nanobot/skills/skill-creator/scripts/package_skill.py b/nanobot/skills/skill-creator/scripts/package_skill.py index aa4de89..48fcbbe 100755 --- a/nanobot/skills/skill-creator/scripts/package_skill.py +++ b/nanobot/skills/skill-creator/scripts/package_skill.py @@ -3,11 +3,11 @@ Skill Packager - Creates a distributable .skill file of a skill folder Usage: - python utils/package_skill.py [output-directory] + python package_skill.py [output-directory] Example: - python utils/package_skill.py skills/public/my-skill - python utils/package_skill.py skills/public/my-skill ./dist + python package_skill.py skills/public/my-skill + python package_skill.py skills/public/my-skill ./dist """ import sys @@ -25,6 +25,14 @@ def _is_within(path: Path, root: Path) -> bool: return False +def _cleanup_partial_archive(skill_filename: Path) -> None: + try: + if skill_filename.exists(): + skill_filename.unlink() + except OSError: + pass + + def package_skill(skill_path, output_dir=None): """ Package a skill folder into a .skill file. @@ -74,49 +82,56 @@ def package_skill(skill_path, output_dir=None): EXCLUDED_DIRS = {".git", ".svn", ".hg", "__pycache__", "node_modules"} + files_to_package = [] + resolved_archive = skill_filename.resolve() + + for file_path in skill_path.rglob("*"): + # Fail closed on symlinks so the packaged contents are explicit and predictable. + if file_path.is_symlink(): + print(f"[ERROR] Symlink not allowed in packaged skill: {file_path}") + _cleanup_partial_archive(skill_filename) + return None + + rel_parts = file_path.relative_to(skill_path).parts + if any(part in EXCLUDED_DIRS for part in rel_parts): + continue + + if file_path.is_file(): + resolved_file = file_path.resolve() + if not _is_within(resolved_file, skill_path): + print(f"[ERROR] File escapes skill root: {file_path}") + _cleanup_partial_archive(skill_filename) + return None + # If output lives under skill_path, avoid writing archive into itself. + if resolved_file == resolved_archive: + print(f"[WARN] Skipping output archive: {file_path}") + continue + files_to_package.append(file_path) + # Create the .skill file (zip format) try: with zipfile.ZipFile(skill_filename, "w", zipfile.ZIP_DEFLATED) as zipf: - # Walk through the skill directory - for file_path in skill_path.rglob("*"): - # Security: never follow or package symlinks. - if file_path.is_symlink(): - print(f"[WARN] Skipping symlink: {file_path}") - continue - - rel_parts = file_path.relative_to(skill_path).parts - if any(part in EXCLUDED_DIRS for part in rel_parts): - continue - - if file_path.is_file(): - resolved_file = file_path.resolve() - if not _is_within(resolved_file, skill_path): - print(f"[ERROR] File escapes skill root: {file_path}") - return None - # If output lives under skill_path, avoid writing archive into itself. - if resolved_file == skill_filename.resolve(): - print(f"[WARN] Skipping output archive: {file_path}") - continue - - # Calculate the relative path within the zip. - arcname = Path(skill_name) / file_path.relative_to(skill_path) - zipf.write(file_path, arcname) - print(f" Added: {arcname}") + for file_path in files_to_package: + # Calculate the relative path within the zip. + arcname = Path(skill_name) / file_path.relative_to(skill_path) + zipf.write(file_path, arcname) + print(f" Added: {arcname}") print(f"\n[OK] Successfully packaged skill to: {skill_filename}") return skill_filename except Exception as e: + _cleanup_partial_archive(skill_filename) print(f"[ERROR] Error creating .skill file: {e}") return None def main(): if len(sys.argv) < 2: - print("Usage: python utils/package_skill.py [output-directory]") + print("Usage: python package_skill.py [output-directory]") print("\nExample:") - print(" python utils/package_skill.py skills/public/my-skill") - print(" python utils/package_skill.py skills/public/my-skill ./dist") + print(" python package_skill.py skills/public/my-skill") + print(" python package_skill.py skills/public/my-skill ./dist") sys.exit(1) skill_path = sys.argv[1] diff --git a/nanobot/skills/skill-creator/scripts/quick_validate.py b/nanobot/skills/skill-creator/scripts/quick_validate.py new file mode 100644 index 0000000..03d246d --- /dev/null +++ b/nanobot/skills/skill-creator/scripts/quick_validate.py @@ -0,0 +1,213 @@ +#!/usr/bin/env python3 +""" +Minimal validator for nanobot skill folders. +""" + +import re +import sys +from pathlib import Path +from typing import Optional + +try: + import yaml +except ModuleNotFoundError: + yaml = None + +MAX_SKILL_NAME_LENGTH = 64 +ALLOWED_FRONTMATTER_KEYS = { + "name", + "description", + "metadata", + "always", + "license", + "allowed-tools", +} +ALLOWED_RESOURCE_DIRS = {"scripts", "references", "assets"} +PLACEHOLDER_MARKERS = ("[todo", "todo:") + + +def _extract_frontmatter(content: str) -> Optional[str]: + lines = content.splitlines() + if not lines or lines[0].strip() != "---": + return None + for i in range(1, len(lines)): + if lines[i].strip() == "---": + return "\n".join(lines[1:i]) + return None + + +def _parse_simple_frontmatter(frontmatter_text: str) -> Optional[dict[str, str]]: + """Fallback parser for simple frontmatter when PyYAML is unavailable.""" + parsed: dict[str, str] = {} + current_key: Optional[str] = None + multiline_key: Optional[str] = None + + for raw_line in frontmatter_text.splitlines(): + stripped = raw_line.strip() + if not stripped or stripped.startswith("#"): + continue + + is_indented = raw_line[:1].isspace() + if is_indented: + if current_key is None: + return None + current_value = parsed[current_key] + parsed[current_key] = f"{current_value}\n{stripped}" if current_value else stripped + continue + + if ":" not in stripped: + return None + + key, value = stripped.split(":", 1) + key = key.strip() + value = value.strip() + if not key: + return None + + if value in {"|", ">"}: + parsed[key] = "" + current_key = key + multiline_key = key + continue + + if (value.startswith('"') and value.endswith('"')) or ( + value.startswith("'") and value.endswith("'") + ): + value = value[1:-1] + parsed[key] = value + current_key = key + multiline_key = None + + if multiline_key is not None and multiline_key not in parsed: + return None + return parsed + + +def _load_frontmatter(frontmatter_text: str) -> tuple[Optional[dict], Optional[str]]: + if yaml is not None: + try: + frontmatter = yaml.safe_load(frontmatter_text) + except yaml.YAMLError as exc: + return None, f"Invalid YAML in frontmatter: {exc}" + if not isinstance(frontmatter, dict): + return None, "Frontmatter must be a YAML dictionary" + return frontmatter, None + + frontmatter = _parse_simple_frontmatter(frontmatter_text) + if frontmatter is None: + return None, "Invalid YAML in frontmatter: unsupported syntax without PyYAML installed" + return frontmatter, None + + +def _validate_skill_name(name: str, folder_name: str) -> Optional[str]: + if not re.fullmatch(r"[a-z0-9]+(?:-[a-z0-9]+)*", name): + return ( + f"Name '{name}' should be hyphen-case " + "(lowercase letters, digits, and single hyphens only)" + ) + if len(name) > MAX_SKILL_NAME_LENGTH: + return ( + f"Name is too long ({len(name)} characters). " + f"Maximum is {MAX_SKILL_NAME_LENGTH} characters." + ) + if name != folder_name: + return f"Skill name '{name}' must match directory name '{folder_name}'" + return None + + +def _validate_description(description: str) -> Optional[str]: + trimmed = description.strip() + if not trimmed: + return "Description cannot be empty" + lowered = trimmed.lower() + if any(marker in lowered for marker in PLACEHOLDER_MARKERS): + return "Description still contains TODO placeholder text" + if "<" in trimmed or ">" in trimmed: + return "Description cannot contain angle brackets (< or >)" + if len(trimmed) > 1024: + return f"Description is too long ({len(trimmed)} characters). Maximum is 1024 characters." + return None + + +def validate_skill(skill_path): + """Validate a skill folder structure and required frontmatter.""" + skill_path = Path(skill_path).resolve() + + if not skill_path.exists(): + return False, f"Skill folder not found: {skill_path}" + if not skill_path.is_dir(): + return False, f"Path is not a directory: {skill_path}" + + skill_md = skill_path / "SKILL.md" + if not skill_md.exists(): + return False, "SKILL.md not found" + + try: + content = skill_md.read_text(encoding="utf-8") + except OSError as exc: + return False, f"Could not read SKILL.md: {exc}" + + frontmatter_text = _extract_frontmatter(content) + if frontmatter_text is None: + return False, "Invalid frontmatter format" + + frontmatter, error = _load_frontmatter(frontmatter_text) + if error: + return False, error + + unexpected_keys = sorted(set(frontmatter.keys()) - ALLOWED_FRONTMATTER_KEYS) + if unexpected_keys: + allowed = ", ".join(sorted(ALLOWED_FRONTMATTER_KEYS)) + unexpected = ", ".join(unexpected_keys) + return ( + False, + f"Unexpected key(s) in SKILL.md frontmatter: {unexpected}. Allowed properties are: {allowed}", + ) + + if "name" not in frontmatter: + return False, "Missing 'name' in frontmatter" + if "description" not in frontmatter: + return False, "Missing 'description' in frontmatter" + + name = frontmatter["name"] + if not isinstance(name, str): + return False, f"Name must be a string, got {type(name).__name__}" + name_error = _validate_skill_name(name.strip(), skill_path.name) + if name_error: + return False, name_error + + description = frontmatter["description"] + if not isinstance(description, str): + return False, f"Description must be a string, got {type(description).__name__}" + description_error = _validate_description(description) + if description_error: + return False, description_error + + always = frontmatter.get("always") + if always is not None and not isinstance(always, bool): + return False, f"'always' must be a boolean, got {type(always).__name__}" + + for child in skill_path.iterdir(): + if child.name == "SKILL.md": + continue + if child.is_dir() and child.name in ALLOWED_RESOURCE_DIRS: + continue + if child.is_symlink(): + continue + return ( + False, + f"Unexpected file or directory in skill root: {child.name}. " + "Only SKILL.md, scripts/, references/, and assets/ are allowed.", + ) + + return True, "Skill is valid!" + + +if __name__ == "__main__": + if len(sys.argv) != 2: + print("Usage: python quick_validate.py ") + sys.exit(1) + + valid, message = validate_skill(sys.argv[1]) + print(message) + sys.exit(0 if valid else 1) diff --git a/tests/test_skill_creator_scripts.py b/tests/test_skill_creator_scripts.py new file mode 100644 index 0000000..4207c6f --- /dev/null +++ b/tests/test_skill_creator_scripts.py @@ -0,0 +1,127 @@ +import importlib +import shutil +import sys +import zipfile +from pathlib import Path + + +SCRIPT_DIR = Path("nanobot/skills/skill-creator/scripts").resolve() +if str(SCRIPT_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPT_DIR)) + +init_skill = importlib.import_module("init_skill") +package_skill = importlib.import_module("package_skill") +quick_validate = importlib.import_module("quick_validate") + + +def test_init_skill_creates_expected_files(tmp_path: Path) -> None: + skill_dir = init_skill.init_skill( + "demo-skill", + tmp_path, + ["scripts", "references", "assets"], + include_examples=True, + ) + + assert skill_dir == tmp_path / "demo-skill" + assert (skill_dir / "SKILL.md").exists() + assert (skill_dir / "scripts" / "example.py").exists() + assert (skill_dir / "references" / "api_reference.md").exists() + assert (skill_dir / "assets" / "example_asset.txt").exists() + + +def test_validate_skill_accepts_existing_skill_creator() -> None: + valid, message = quick_validate.validate_skill( + Path("nanobot/skills/skill-creator").resolve() + ) + + assert valid, message + + +def test_validate_skill_rejects_placeholder_description(tmp_path: Path) -> None: + skill_dir = tmp_path / "placeholder-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\n" + "name: placeholder-skill\n" + 'description: "[TODO: fill me in]"\n' + "---\n" + "# Placeholder\n", + encoding="utf-8", + ) + + valid, message = quick_validate.validate_skill(skill_dir) + + assert not valid + assert "TODO placeholder" in message + + +def test_validate_skill_rejects_root_files_outside_allowed_dirs(tmp_path: Path) -> None: + skill_dir = tmp_path / "bad-root-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\n" + "name: bad-root-skill\n" + "description: Valid description\n" + "---\n" + "# Skill\n", + encoding="utf-8", + ) + (skill_dir / "README.md").write_text("extra\n", encoding="utf-8") + + valid, message = quick_validate.validate_skill(skill_dir) + + assert not valid + assert "Unexpected file or directory in skill root" in message + + +def test_package_skill_creates_archive(tmp_path: Path) -> None: + skill_dir = tmp_path / "package-me" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\n" + "name: package-me\n" + "description: Package this skill.\n" + "---\n" + "# Skill\n", + encoding="utf-8", + ) + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "helper.py").write_text("print('ok')\n", encoding="utf-8") + + archive_path = package_skill.package_skill(skill_dir, tmp_path / "dist") + + assert archive_path == (tmp_path / "dist" / "package-me.skill") + assert archive_path.exists() + with zipfile.ZipFile(archive_path, "r") as archive: + names = set(archive.namelist()) + assert "package-me/SKILL.md" in names + assert "package-me/scripts/helper.py" in names + + +def test_package_skill_rejects_symlink(tmp_path: Path) -> None: + skill_dir = tmp_path / "symlink-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\n" + "name: symlink-skill\n" + "description: Reject symlinks during packaging.\n" + "---\n" + "# Skill\n", + encoding="utf-8", + ) + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + target = tmp_path / "outside.txt" + target.write_text("secret\n", encoding="utf-8") + link = scripts_dir / "outside.txt" + + try: + link.symlink_to(target) + except (OSError, NotImplementedError): + return + + archive_path = package_skill.package_skill(skill_dir, tmp_path / "dist") + + assert archive_path is None + assert not (tmp_path / "dist" / "symlink-skill.skill").exists()