From ac1f20dfedc1baf528522f08bd329f9846df8b4b Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Fri, 13 Mar 2026 19:29:46 -0400 Subject: [PATCH 1/8] feat: Auto-register ai-skills for extensions whenever applicable --- README.md | 2 +- extensions/EXTENSION-USER-GUIDE.md | 15 + src/specify_cli/__init__.py | 12 +- src/specify_cli/extensions.py | 192 ++++++++- tests/test_extension_skills.py | 640 +++++++++++++++++++++++++++++ 5 files changed, 857 insertions(+), 4 deletions(-) create mode 100644 tests/test_extension_skills.py diff --git a/README.md b/README.md index 1b866380ac..4d83483036 100644 --- a/README.md +++ b/README.md @@ -292,7 +292,7 @@ The `specify` command supports the following options: | `--skip-tls` | Flag | Skip SSL/TLS verification (not recommended) | | `--debug` | Flag | Enable detailed debug output for troubleshooting | | `--github-token` | Option | GitHub token for API requests (or set GH_TOKEN/GITHUB_TOKEN env variable) | -| `--ai-skills` | Flag | Install Prompt.MD templates as agent skills in agent-specific `skills/` directory (requires `--ai`) | +| `--ai-skills` | Flag | Install Prompt.MD templates as agent skills in agent-specific `skills/` directory (requires `--ai`). Extension commands are also auto-registered as skills when extensions are added later. | | `--branch-numbering` | Option | Branch numbering strategy: `sequential` (default — `001`, `002`, `003`) or `timestamp` (`YYYYMMDD-HHMMSS`). Timestamp mode is useful for distributed teams to avoid numbering conflicts | ### Examples diff --git a/extensions/EXTENSION-USER-GUIDE.md b/extensions/EXTENSION-USER-GUIDE.md index 21313c0aca..2fd28191ca 100644 --- a/extensions/EXTENSION-USER-GUIDE.md +++ b/extensions/EXTENSION-USER-GUIDE.md @@ -187,6 +187,21 @@ Provided commands: Check: .specify/extensions/jira/ ``` +### Automatic Agent Skill Registration + +If your project was initialized with `--ai-skills`, extension commands are **automatically registered as agent skills** during installation. This ensures that extensions are discoverable by agents that use the [agentskills.io](https://agentskills.io) skill specification. + +```text +✓ Extension installed successfully! + +Jira Integration (v1.0.0) + ... + +✓ 3 agent skill(s) auto-registered +``` + +When an extension is removed, its corresponding skills are also cleaned up automatically. Pre-existing skills that were manually customized are never overwritten. + --- ## Using Extensions diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 08af651078..8ac22504ff 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3594,6 +3594,12 @@ def extension_add( for cmd in manifest.commands: console.print(f" • {cmd['name']} - {cmd.get('description', '')}") + # Report agent skills registration + reg_meta = manager.registry.get(manifest.id) + reg_skills = reg_meta.get("registered_skills", []) if reg_meta else [] + if reg_skills: + console.print(f"\n[green]✓[/green] {len(reg_skills)} agent skill(s) auto-registered") + console.print("\n[yellow]⚠[/yellow] Configuration may be required") console.print(f" Check: .specify/extensions/{manifest.id}/") @@ -3632,14 +3638,18 @@ def extension_remove( installed = manager.list_installed() extension_id, display_name = _resolve_installed_extension(extension, installed, "remove") - # Get extension info for command count + # Get extension info for command and skill counts ext_manifest = manager.get_extension(extension_id) cmd_count = len(ext_manifest.commands) if ext_manifest else 0 + reg_meta = manager.registry.get(extension_id) + skill_count = len(reg_meta.get("registered_skills", [])) if reg_meta else 0 # Confirm removal if not force: console.print("\n[yellow]⚠ This will remove:[/yellow]") console.print(f" • {cmd_count} commands from AI agent") + if skill_count: + console.print(f" • {skill_count} agent skill(s)") console.print(f" • Extension directory: .specify/extensions/{extension_id}/") if not keep_config: console.print(" • Config files (will be backed up)") diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index b26b1e9310..66ef3e159f 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -510,6 +510,185 @@ def _ignore(directory: str, entries: List[str]) -> Set[str]: return _ignore + def _get_skills_dir(self) -> Optional[Path]: + """Return the skills directory if ``--ai-skills`` was used during init. + + Reads ``.specify/init-options.json`` to determine whether skills + are enabled and which agent was selected, then delegates to + the module-level ``_get_skills_dir()`` helper for the concrete path. + + Returns: + The skills directory ``Path``, or ``None`` if skills were not + enabled or the init-options file is missing. + """ + from . import load_init_options, _get_skills_dir + + opts = load_init_options(self.project_root) + if not opts.get("ai_skills"): + return None + + agent = opts.get("ai") + if not agent: + return None + + skills_dir = _get_skills_dir(self.project_root, agent) + if not skills_dir.is_dir(): + return None + + return skills_dir + + def _register_extension_skills( + self, + manifest: ExtensionManifest, + extension_dir: Path, + ) -> List[str]: + """Generate SKILL.md files for extension commands as agent skills. + + For every command in the extension manifest, creates a SKILL.md + file in the agent's skills directory following the agentskills.io + specification. This is only done when ``--ai-skills`` was used + during project initialisation. + + Args: + manifest: Extension manifest. + extension_dir: Installed extension directory. + + Returns: + List of skill names that were created (for registry storage). + """ + skills_dir = self._get_skills_dir() + if not skills_dir: + return [] + + from . import load_init_options + import yaml + + opts = load_init_options(self.project_root) + selected_ai = opts.get("ai", "") + + written: List[str] = [] + + for cmd_info in manifest.commands: + cmd_name = cmd_info["name"] + cmd_file_rel = cmd_info["file"] + source_file = extension_dir / cmd_file_rel + if not source_file.exists(): + continue + + # Derive skill name from command name + # e.g. "speckit.jira.create" -> "speckit-jira-create" (or dot for kimi) + if selected_ai == "kimi": + skill_name = cmd_name # Keep dot notation for kimi + else: + skill_name = cmd_name.replace(".", "-") + + # Check if skill already exists before creating the directory + skill_subdir = skills_dir / skill_name + skill_file = skill_subdir / "SKILL.md" + if skill_file.exists(): + # Do not overwrite user-customized skills + continue + + # Create skill directory only when we're going to write to it + skill_subdir.mkdir(parents=True, exist_ok=True) + + # Parse the command file + content = source_file.read_text(encoding="utf-8") + if content.startswith("---"): + parts = content.split("---", 2) + if len(parts) >= 3: + try: + frontmatter = yaml.safe_load(parts[1]) + except yaml.YAMLError: + frontmatter = {} + if not isinstance(frontmatter, dict): + frontmatter = {} + body = parts[2].strip() + else: + frontmatter = {} + body = content + else: + frontmatter = {} + body = content + + original_desc = frontmatter.get("description", "") + description = original_desc or f"Extension command: {cmd_name}" + + frontmatter_data = { + "name": skill_name, + "description": description, + "compatibility": "Requires spec-kit project structure with .specify/ directory", + "metadata": { + "author": "github-spec-kit", + "source": f"extension:{manifest.id}", + }, + } + frontmatter_text = yaml.safe_dump(frontmatter_data, sort_keys=False).strip() + + # Derive a human-friendly title from the command name + short_name = cmd_name + if short_name.startswith("speckit."): + short_name = short_name[len("speckit."):] + + skill_content = ( + f"---\n" + f"{frontmatter_text}\n" + f"---\n\n" + f"# {skill_name.replace('-', ' ').replace('.', ' ').title()} Skill\n\n" + f"{body}\n" + ) + + skill_file.write_text(skill_content, encoding="utf-8") + written.append(skill_name) + + return written + + def _unregister_extension_skills(self, skill_names: List[str]) -> None: + """Remove SKILL.md directories for extension skills. + + Called during extension removal to clean up skill files that + were created by ``_register_extension_skills()``. + + If ``_get_skills_dir()`` returns ``None`` (e.g. the user removed + init-options.json or toggled ai_skills after installation), we + fall back to scanning all known agent skills directories so that + orphaned skill directories are still cleaned up. + + Args: + skill_names: List of skill names to remove. + """ + if not skill_names: + return + + skills_dir = self._get_skills_dir() + + if skills_dir: + # Fast path: we know the exact skills directory + for skill_name in skill_names: + skill_subdir = skills_dir / skill_name + if skill_subdir.exists(): + shutil.rmtree(skill_subdir) + else: + # Fallback: scan all possible agent skills directories + from . import AGENT_CONFIG, AGENT_SKILLS_DIR_OVERRIDES, DEFAULT_SKILLS_DIR + + candidate_dirs: set[Path] = set() + for override_path in AGENT_SKILLS_DIR_OVERRIDES.values(): + candidate_dirs.add(self.project_root / override_path) + for cfg in AGENT_CONFIG.values(): + folder = cfg.get("folder", "") + if folder: + candidate_dirs.add(self.project_root / folder.rstrip("/") / "skills") + candidate_dirs.add(self.project_root / DEFAULT_SKILLS_DIR) + + for skills_candidate in candidate_dirs: + if not skills_candidate.is_dir(): + continue + for skill_name in skill_names: + skill_subdir = skills_candidate / skill_name + if skill_subdir.exists(): + shutil.rmtree(skill_subdir) + def check_compatibility( self, manifest: ExtensionManifest, @@ -601,6 +780,10 @@ def install_from_directory( manifest, dest_dir, self.project_root ) + # Auto-register extension commands as agent skills when --ai-skills + # was used during project initialisation (feature parity). + registered_skills = self._register_extension_skills(manifest, dest_dir) + # Register hooks hook_executor = HookExecutor(self.project_root) hook_executor.register_hooks(manifest) @@ -612,7 +795,8 @@ def install_from_directory( "manifest_hash": manifest.get_hash(), "enabled": True, "priority": priority, - "registered_commands": registered_commands + "registered_commands": registered_commands, + "registered_skills": registered_skills, }) return manifest @@ -690,9 +874,10 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if not self.registry.is_installed(extension_id): return False - # Get registered commands before removal + # Get registered commands and skills before removal metadata = self.registry.get(extension_id) registered_commands = metadata.get("registered_commands", {}) if metadata else {} + registered_skills = metadata.get("registered_skills", []) if metadata else [] extension_dir = self.extensions_dir / extension_id @@ -701,6 +886,9 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: registrar = CommandRegistrar() registrar.unregister_commands(registered_commands, self.project_root) + # Unregister agent skills + self._unregister_extension_skills(registered_skills) + if keep_config: # Preserve config files, only remove non-config files if extension_dir.exists(): diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py new file mode 100644 index 0000000000..f011c059f9 --- /dev/null +++ b/tests/test_extension_skills.py @@ -0,0 +1,640 @@ +""" +Unit tests for extension skill auto-registration. + +Tests cover: +- SKILL.md generation when --ai-skills was used during init +- No skills created when ai_skills not active +- SKILL.md content correctness +- Existing user-modified skills not overwritten +- Skill cleanup on extension removal +- Registry metadata includes registered_skills +""" + +import json +import pytest +import tempfile +import shutil +import yaml +from pathlib import Path + +from specify_cli.extensions import ( + ExtensionManifest, + ExtensionManager, + ExtensionError, +) + + +# ===== Helpers ===== + +def _create_init_options(project_root: Path, ai: str = "claude", ai_skills: bool = True): + """Write a .specify/init-options.json file.""" + opts_dir = project_root / ".specify" + opts_dir.mkdir(parents=True, exist_ok=True) + opts_file = opts_dir / "init-options.json" + opts_file.write_text(json.dumps({ + "ai": ai, + "ai_skills": ai_skills, + "script": "sh", + })) + + +def _create_skills_dir(project_root: Path, ai: str = "claude") -> Path: + """Create and return the expected skills directory for the given agent.""" + # Match the logic in _get_skills_dir() from specify_cli + from specify_cli import AGENT_CONFIG, AGENT_SKILLS_DIR_OVERRIDES, DEFAULT_SKILLS_DIR + + if ai in AGENT_SKILLS_DIR_OVERRIDES: + skills_dir = project_root / AGENT_SKILLS_DIR_OVERRIDES[ai] + else: + agent_config = AGENT_CONFIG.get(ai, {}) + agent_folder = agent_config.get("folder", "") + if agent_folder: + skills_dir = project_root / agent_folder.rstrip("/") / "skills" + else: + skills_dir = project_root / DEFAULT_SKILLS_DIR + + skills_dir.mkdir(parents=True, exist_ok=True) + return skills_dir + + +def _create_extension_dir(temp_dir: Path, ext_id: str = "test-ext") -> Path: + """Create a complete extension directory with manifest and command files.""" + ext_dir = temp_dir / ext_id + ext_dir.mkdir() + + manifest_data = { + "schema_version": "1.0", + "extension": { + "id": ext_id, + "name": "Test Extension", + "version": "1.0.0", + "description": "A test extension for skill registration", + }, + "requires": { + "speckit_version": ">=0.1.0", + }, + "provides": { + "commands": [ + { + "name": f"speckit.{ext_id}.hello", + "file": "commands/hello.md", + "description": "Test hello command", + }, + { + "name": f"speckit.{ext_id}.world", + "file": "commands/world.md", + "description": "Test world command", + }, + ] + }, + } + + with open(ext_dir / "extension.yml", "w") as f: + yaml.dump(manifest_data, f) + + commands_dir = ext_dir / "commands" + commands_dir.mkdir() + + (commands_dir / "hello.md").write_text( + "---\n" + "description: \"Test hello command\"\n" + "---\n" + "\n" + "# Hello Command\n" + "\n" + "Run this to say hello.\n" + "$ARGUMENTS\n" + ) + + (commands_dir / "world.md").write_text( + "---\n" + "description: \"Test world command\"\n" + "---\n" + "\n" + "# World Command\n" + "\n" + "Run this to greet the world.\n" + ) + + return ext_dir + + +# ===== Fixtures ===== + +@pytest.fixture +def temp_dir(): + """Create a temporary directory for tests.""" + tmpdir = tempfile.mkdtemp() + yield Path(tmpdir) + shutil.rmtree(tmpdir) + + +@pytest.fixture +def project_dir(temp_dir): + """Create a mock spec-kit project directory.""" + proj_dir = temp_dir / "project" + proj_dir.mkdir() + + # Create .specify directory + specify_dir = proj_dir / ".specify" + specify_dir.mkdir() + + return proj_dir + + +@pytest.fixture +def extension_dir(temp_dir): + """Create a complete extension directory.""" + return _create_extension_dir(temp_dir) + + +@pytest.fixture +def skills_project(project_dir): + """Create a project with --ai-skills enabled and skills directory.""" + _create_init_options(project_dir, ai="claude", ai_skills=True) + skills_dir = _create_skills_dir(project_dir, ai="claude") + return project_dir, skills_dir + + +@pytest.fixture +def no_skills_project(project_dir): + """Create a project without --ai-skills.""" + _create_init_options(project_dir, ai="claude", ai_skills=False) + return project_dir + + +# ===== ExtensionManager._get_skills_dir Tests ===== + +class TestExtensionManagerGetSkillsDir: + """Test _get_skills_dir() on ExtensionManager.""" + + def test_returns_skills_dir_when_active(self, skills_project): + """Should return skills dir when ai_skills is true and dir exists.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + result = manager._get_skills_dir() + assert result == skills_dir + + def test_returns_none_when_no_ai_skills(self, no_skills_project): + """Should return None when ai_skills is false.""" + manager = ExtensionManager(no_skills_project) + result = manager._get_skills_dir() + assert result is None + + def test_returns_none_when_no_init_options(self, project_dir): + """Should return None when init-options.json is missing.""" + manager = ExtensionManager(project_dir) + result = manager._get_skills_dir() + assert result is None + + def test_returns_none_when_skills_dir_missing(self, project_dir): + """Should return None when skills dir doesn't exist on disk.""" + _create_init_options(project_dir, ai="claude", ai_skills=True) + # Don't create the skills directory + manager = ExtensionManager(project_dir) + result = manager._get_skills_dir() + assert result is None + + +# ===== Extension Skill Registration Tests ===== + +class TestExtensionSkillRegistration: + """Test _register_extension_skills() on ExtensionManager.""" + + def test_skills_created_when_ai_skills_active(self, skills_project, extension_dir): + """Skills should be created when ai_skills is enabled.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Check that skill directories were created + skill_dirs = sorted([d.name for d in skills_dir.iterdir() if d.is_dir()]) + assert "speckit-test-ext-hello" in skill_dirs + assert "speckit-test-ext-world" in skill_dirs + + def test_skill_md_content_correct(self, skills_project, extension_dir): + """SKILL.md should have correct agentskills.io structure.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md" + assert skill_file.exists() + content = skill_file.read_text() + + # Check structure + assert content.startswith("---\n") + assert "name: speckit-test-ext-hello" in content + assert "description:" in content + assert "Test hello command" in content + assert "source: extension:test-ext" in content + assert "author: github-spec-kit" in content + assert "compatibility:" in content + assert "Run this to say hello." in content + + def test_skill_md_has_parseable_yaml(self, skills_project, extension_dir): + """Generated SKILL.md should contain valid, parseable YAML frontmatter.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md" + content = skill_file.read_text() + + assert content.startswith("---\n") + parts = content.split("---", 2) + assert len(parts) >= 3 + parsed = yaml.safe_load(parts[1]) + assert isinstance(parsed, dict) + assert parsed["name"] == "speckit-test-ext-hello" + assert "description" in parsed + + def test_no_skills_when_ai_skills_disabled(self, no_skills_project, extension_dir): + """No skills should be created when ai_skills is false.""" + manager = ExtensionManager(no_skills_project) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Verify registry + metadata = manager.registry.get(manifest.id) + assert metadata["registered_skills"] == [] + + def test_no_skills_when_init_options_missing(self, project_dir, extension_dir): + """No skills should be created when init-options.json is absent.""" + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + metadata = manager.registry.get(manifest.id) + assert metadata["registered_skills"] == [] + + def test_existing_skill_not_overwritten(self, skills_project, extension_dir): + """Pre-existing SKILL.md should not be overwritten.""" + project_dir, skills_dir = skills_project + + # Pre-create a custom skill + custom_dir = skills_dir / "speckit-test-ext-hello" + custom_dir.mkdir(parents=True) + custom_content = "# My Custom Hello Skill\nUser-modified content\n" + (custom_dir / "SKILL.md").write_text(custom_content) + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Custom skill should be untouched + assert (custom_dir / "SKILL.md").read_text() == custom_content + + # But the other skill should still be created + metadata = manager.registry.get(manifest.id) + assert "speckit-test-ext-world" in metadata["registered_skills"] + # The pre-existing one should NOT be in registered_skills (it was skipped) + assert "speckit-test-ext-hello" not in metadata["registered_skills"] + + def test_registered_skills_in_registry(self, skills_project, extension_dir): + """Registry should contain registered_skills list.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + metadata = manager.registry.get(manifest.id) + assert "registered_skills" in metadata + assert len(metadata["registered_skills"]) == 2 + assert "speckit-test-ext-hello" in metadata["registered_skills"] + assert "speckit-test-ext-world" in metadata["registered_skills"] + + def test_kimi_uses_dot_notation(self, project_dir, temp_dir): + """Kimi agent should use dot notation for skill names.""" + _create_init_options(project_dir, ai="kimi", ai_skills=True) + _create_skills_dir(project_dir, ai="kimi") + ext_dir = _create_extension_dir(temp_dir, ext_id="test-ext") + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + metadata = manager.registry.get(manifest.id) + # Kimi should use dots, not hyphens + assert "speckit.test-ext.hello" in metadata["registered_skills"] + assert "speckit.test-ext.world" in metadata["registered_skills"] + + def test_missing_command_file_skipped(self, skills_project, temp_dir): + """Commands with missing source files should be skipped gracefully.""" + project_dir, skills_dir = skills_project + + ext_dir = temp_dir / "missing-cmd-ext" + ext_dir.mkdir() + manifest_data = { + "schema_version": "1.0", + "extension": { + "id": "missing-cmd-ext", + "name": "Missing Cmd Extension", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.missing-cmd-ext.exists", + "file": "commands/exists.md", + "description": "Exists", + }, + { + "name": "speckit.missing-cmd-ext.ghost", + "file": "commands/ghost.md", + "description": "Does not exist", + }, + ] + }, + } + with open(ext_dir / "extension.yml", "w") as f: + yaml.dump(manifest_data, f) + + (ext_dir / "commands").mkdir() + (ext_dir / "commands" / "exists.md").write_text( + "---\ndescription: Exists\n---\n\n# Exists\n\nBody.\n" + ) + # Intentionally do NOT create ghost.md + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + metadata = manager.registry.get(manifest.id) + assert "speckit-missing-cmd-ext-exists" in metadata["registered_skills"] + assert "speckit-missing-cmd-ext-ghost" not in metadata["registered_skills"] + + +# ===== Extension Skill Unregistration Tests ===== + +class TestExtensionSkillUnregistration: + """Test _unregister_extension_skills() on ExtensionManager.""" + + def test_skills_removed_on_extension_remove(self, skills_project, extension_dir): + """Removing an extension should clean up its skill directories.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Verify skills exist + assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext-world" / "SKILL.md").exists() + + # Remove extension + result = manager.remove(manifest.id, keep_config=False) + assert result is True + + # Skills should be gone + assert not (skills_dir / "speckit-test-ext-hello").exists() + assert not (skills_dir / "speckit-test-ext-world").exists() + + def test_other_skills_preserved_on_remove(self, skills_project, extension_dir): + """Non-extension skills should not be affected by extension removal.""" + project_dir, skills_dir = skills_project + + # Pre-create a custom skill + custom_dir = skills_dir / "my-custom-skill" + custom_dir.mkdir(parents=True) + (custom_dir / "SKILL.md").write_text("# My Custom Skill\n") + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + manager.remove(manifest.id, keep_config=False) + + # Custom skill should still exist + assert (custom_dir / "SKILL.md").exists() + assert (custom_dir / "SKILL.md").read_text() == "# My Custom Skill\n" + + def test_remove_handles_already_deleted_skills(self, skills_project, extension_dir): + """Gracefully handle case where skill dirs were already deleted.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Manually delete skill dirs before calling remove + shutil.rmtree(skills_dir / "speckit-test-ext-hello") + shutil.rmtree(skills_dir / "speckit-test-ext-world") + + # Should not raise + result = manager.remove(manifest.id, keep_config=False) + assert result is True + + def test_remove_no_skills_when_not_active(self, no_skills_project, extension_dir): + """Removal without active skills should not attempt skill cleanup.""" + manager = ExtensionManager(no_skills_project) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Should not raise even though no skills exist + result = manager.remove(manifest.id, keep_config=False) + assert result is True + + +# ===== Command File Without Frontmatter ===== + +class TestExtensionSkillEdgeCases: + """Test edge cases in extension skill registration.""" + + def test_command_without_frontmatter(self, skills_project, temp_dir): + """Commands without YAML frontmatter should still produce valid skills.""" + project_dir, skills_dir = skills_project + + ext_dir = temp_dir / "nofm-ext" + ext_dir.mkdir() + manifest_data = { + "schema_version": "1.0", + "extension": { + "id": "nofm-ext", + "name": "No Frontmatter Extension", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.nofm-ext.plain", + "file": "commands/plain.md", + "description": "Plain command", + } + ] + }, + } + with open(ext_dir / "extension.yml", "w") as f: + yaml.dump(manifest_data, f) + + (ext_dir / "commands").mkdir() + (ext_dir / "commands" / "plain.md").write_text( + "# Plain Command\n\nBody without frontmatter.\n" + ) + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + skill_file = skills_dir / "speckit-nofm-ext-plain" / "SKILL.md" + assert skill_file.exists() + content = skill_file.read_text() + assert "name: speckit-nofm-ext-plain" in content + # Fallback description when no frontmatter description + assert "Extension command: speckit.nofm-ext.plain" in content + assert "Body without frontmatter." in content + + def test_gemini_agent_skills(self, project_dir, temp_dir): + """Gemini agent should use .gemini/skills/ for skill directory.""" + _create_init_options(project_dir, ai="gemini", ai_skills=True) + _create_skills_dir(project_dir, ai="gemini") + ext_dir = _create_extension_dir(temp_dir, ext_id="test-ext") + + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + skills_dir = project_dir / ".gemini" / "skills" + assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext-world" / "SKILL.md").exists() + + def test_multiple_extensions_independent_skills(self, skills_project, temp_dir): + """Installing and removing different extensions should be independent.""" + project_dir, skills_dir = skills_project + + ext_dir_a = _create_extension_dir(temp_dir, ext_id="ext-a") + ext_dir_b = _create_extension_dir(temp_dir, ext_id="ext-b") + + manager = ExtensionManager(project_dir) + manifest_a = manager.install_from_directory( + ext_dir_a, "0.1.0", register_commands=False + ) + manifest_b = manager.install_from_directory( + ext_dir_b, "0.1.0", register_commands=False + ) + + # Both should have skills + assert (skills_dir / "speckit-ext-a-hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-ext-b-hello" / "SKILL.md").exists() + + # Remove ext-a + manager.remove("ext-a", keep_config=False) + + # ext-a skills gone, ext-b skills preserved + assert not (skills_dir / "speckit-ext-a-hello").exists() + assert (skills_dir / "speckit-ext-b-hello" / "SKILL.md").exists() + + def test_malformed_frontmatter_handled(self, skills_project, temp_dir): + """Commands with invalid YAML frontmatter should still produce valid skills.""" + project_dir, skills_dir = skills_project + + ext_dir = temp_dir / "badfm-ext" + ext_dir.mkdir() + manifest_data = { + "schema_version": "1.0", + "extension": { + "id": "badfm-ext", + "name": "Bad Frontmatter Extension", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": "speckit.badfm-ext.broken", + "file": "commands/broken.md", + "description": "Broken frontmatter", + } + ] + }, + } + with open(ext_dir / "extension.yml", "w") as f: + yaml.dump(manifest_data, f) + + (ext_dir / "commands").mkdir() + # Malformed YAML: invalid key-value syntax + (ext_dir / "commands" / "broken.md").write_text( + "---\n" + "description: [invalid yaml\n" + " unclosed: bracket\n" + "---\n" + "\n" + "# Broken Command\n" + "\n" + "This body should still be used.\n" + ) + + manager = ExtensionManager(project_dir) + # Should not raise + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + skill_file = skills_dir / "speckit-badfm-ext-broken" / "SKILL.md" + assert skill_file.exists() + content = skill_file.read_text() + # Fallback description since frontmatter was invalid + assert "Extension command: speckit.badfm-ext.broken" in content + assert "This body should still be used." in content + + def test_remove_cleans_up_when_init_options_deleted(self, skills_project, extension_dir): + """Skills should be cleaned up even if init-options.json is deleted after install.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Verify skills exist + assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() + + # Delete init-options.json to simulate user change + init_opts = project_dir / ".specify" / "init-options.json" + init_opts.unlink() + + # Remove should still clean up via fallback scan + result = manager.remove(manifest.id, keep_config=False) + assert result is True + assert not (skills_dir / "speckit-test-ext-hello").exists() + assert not (skills_dir / "speckit-test-ext-world").exists() + + def test_remove_cleans_up_when_ai_skills_toggled(self, skills_project, extension_dir): + """Skills should be cleaned up even if ai_skills is toggled to false after install.""" + project_dir, skills_dir = skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Verify skills exist + assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() + + # Toggle ai_skills to false + _create_init_options(project_dir, ai="claude", ai_skills=False) + + # Remove should still clean up via fallback scan + result = manager.remove(manifest.id, keep_config=False) + assert result is True + assert not (skills_dir / "speckit-test-ext-hello").exists() + assert not (skills_dir / "speckit-test-ext-world").exists() From 9ecbd6a56cf2c5e39541beebd5bd04634c7f5b8e Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 17:11:51 -0400 Subject: [PATCH 2/8] fix: failing test --- tests/test_presets.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_presets.py b/tests/test_presets.py index 50f6502161..95dca41224 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -1170,10 +1170,12 @@ def test_clear_cache(self, project_dir): assert not catalog.cache_file.exists() assert not catalog.cache_metadata_file.exists() - def test_search_with_cached_data(self, project_dir): + def test_search_with_cached_data(self, project_dir, monkeypatch): """Test search with cached catalog data.""" from unittest.mock import patch + # Only use the default catalog to prevent fetching the community catalog from the network + monkeypatch.setenv("SPECKIT_PRESET_CATALOG_URL", PresetCatalog.DEFAULT_CATALOG_URL) catalog = PresetCatalog(project_dir) catalog.cache_dir.mkdir(parents=True, exist_ok=True) From 7c2a81240d3d7496ee9059a6f9a939ccba9830ae Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 21:14:45 -0400 Subject: [PATCH 3/8] =?UTF-8?q?fix:=20address=20copilot=20review=20comment?= =?UTF-8?q?s=20=E2=80=93=20path=20traversal=20guard=20and=20use=20short=5F?= =?UTF-8?q?name=20in=20title?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/specify_cli/extensions.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 66ef3e159f..b12e555ec2 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -571,7 +571,19 @@ def _register_extension_skills( for cmd_info in manifest.commands: cmd_name = cmd_info["name"] cmd_file_rel = cmd_info["file"] - source_file = extension_dir / cmd_file_rel + + # Guard against path traversal: reject absolute paths and ensure + # the resolved file stays within the extension directory. + cmd_path = Path(cmd_file_rel) + if cmd_path.is_absolute(): + continue + try: + ext_root = extension_dir.resolve() + source_file = (ext_root / cmd_path).resolve() + source_file.relative_to(ext_root) # raises ValueError if outside + except (OSError, ValueError): + continue + if not source_file.exists(): continue @@ -629,12 +641,13 @@ def _register_extension_skills( short_name = cmd_name if short_name.startswith("speckit."): short_name = short_name[len("speckit."):] + title_name = short_name.replace(".", " ").replace("-", " ").title() skill_content = ( f"---\n" f"{frontmatter_text}\n" f"---\n\n" - f"# {skill_name.replace('-', ' ').replace('.', ' ').title()} Skill\n\n" + f"# {title_name} Skill\n\n" f"{body}\n" ) From a33537120f95f842d3086b739ea998a05dc9a554 Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 21:23:10 -0400 Subject: [PATCH 4/8] =?UTF-8?q?fix:=20address=20remaining=20copilot=20revi?= =?UTF-8?q?ew=20comments=20=E2=80=93=20is=5Ffile=20guard,=20skills=20type-?= =?UTF-8?q?validation,=20and=20exact=20extension=20ownership=20check=20on?= =?UTF-8?q?=20fallback=20rmtree?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/specify_cli/extensions.py | 57 +++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index b12e555ec2..f6bdcd152f 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -584,7 +584,7 @@ def _register_extension_skills( except (OSError, ValueError): continue - if not source_file.exists(): + if not source_file.is_file(): continue # Derive skill name from command name @@ -604,8 +604,12 @@ def _register_extension_skills( # Create skill directory only when we're going to write to it skill_subdir.mkdir(parents=True, exist_ok=True) - # Parse the command file - content = source_file.read_text(encoding="utf-8") + # Parse the command file — guard against IsADirectoryError / decode errors + try: + content = source_file.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + skill_subdir.rmdir() # undo the mkdir above before skipping + continue if content.startswith("---"): parts = content.split("---", 2) if len(parts) >= 3: @@ -656,7 +660,7 @@ def _register_extension_skills( return written - def _unregister_extension_skills(self, skill_names: List[str]) -> None: + def _unregister_extension_skills(self, skill_names: List[str], extension_id: str) -> None: """Remove SKILL.md directories for extension skills. Called during extension removal to clean up skill files that @@ -665,10 +669,15 @@ def _unregister_extension_skills(self, skill_names: List[str]) -> None: If ``_get_skills_dir()`` returns ``None`` (e.g. the user removed init-options.json or toggled ai_skills after installation), we fall back to scanning all known agent skills directories so that - orphaned skill directories are still cleaned up. + orphaned skill directories are still cleaned up. In that case + each candidate directory is verified against the SKILL.md + ``metadata.source`` field before removal to avoid accidentally + deleting user-created skills with the same name. Args: skill_names: List of skill names to remove. + extension_id: Extension ID used to verify ownership during + fallback candidate scanning. """ if not skill_names: return @@ -699,8 +708,33 @@ def _unregister_extension_skills(self, skill_names: List[str]) -> None: continue for skill_name in skill_names: skill_subdir = skills_candidate / skill_name - if skill_subdir.exists(): - shutil.rmtree(skill_subdir) + if not skill_subdir.is_dir(): + continue + # Safety check: only delete if SKILL.md metadata.source matches + # this extension, to avoid wiping user-created skills with the + # same name under a different agent dir. + skill_md = skill_subdir / "SKILL.md" + if skill_md.is_file(): + try: + import yaml as _yaml + raw = skill_md.read_text(encoding="utf-8") + source = "" + if raw.startswith("---"): + parts = raw.split("---", 2) + if len(parts) >= 3: + fm = _yaml.safe_load(parts[1]) or {} + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) + # Only remove skills explicitly created by this extension + if source != f"extension:{extension_id}": + continue + except (OSError, UnicodeDecodeError, Exception): + # If we can't verify, skip to avoid accidental deletion + continue + shutil.rmtree(skill_subdir) def check_compatibility( self, @@ -890,7 +924,12 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: # Get registered commands and skills before removal metadata = self.registry.get(extension_id) registered_commands = metadata.get("registered_commands", {}) if metadata else {} - registered_skills = metadata.get("registered_skills", []) if metadata else [] + raw_skills = metadata.get("registered_skills", []) if metadata else [] + # Normalize: must be a list of plain strings to avoid corrupted-registry errors + if isinstance(raw_skills, list): + registered_skills = [s for s in raw_skills if isinstance(s, str)] + else: + registered_skills = [] extension_dir = self.extensions_dir / extension_id @@ -900,7 +939,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: registrar.unregister_commands(registered_commands, self.project_root) # Unregister agent skills - self._unregister_extension_skills(registered_skills) + self._unregister_extension_skills(registered_skills, extension_id) if keep_config: # Preserve config files, only remove non-config files From de65357bbb143208aa3768e22f58338a8800a9c3 Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 21:47:50 -0400 Subject: [PATCH 5/8] =?UTF-8?q?fix:=20address=20copilot=20round-3=20commen?= =?UTF-8?q?ts=20=E2=80=93=20align=20skill=20naming=20with=20presets.py=20c?= =?UTF-8?q?onvention,=20safe=20rmdir=20on=20fail,=20require=20SKILL.md=20f?= =?UTF-8?q?or=20fallback=20rmtree,=20normalize=20skill=5Fcount=20in=20CLI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/specify_cli/__init__.py | 6 ++- src/specify_cli/extensions.py | 69 ++++++++++++++++++++-------------- tests/test_extension_skills.py | 66 ++++++++++++++++---------------- 3 files changed, 79 insertions(+), 62 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 8ac22504ff..d78609ad66 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3597,6 +3597,9 @@ def extension_add( # Report agent skills registration reg_meta = manager.registry.get(manifest.id) reg_skills = reg_meta.get("registered_skills", []) if reg_meta else [] + # Normalize to guard against corrupted registry entries + if not isinstance(reg_skills, list): + reg_skills = [] if reg_skills: console.print(f"\n[green]✓[/green] {len(reg_skills)} agent skill(s) auto-registered") @@ -3642,7 +3645,8 @@ def extension_remove( ext_manifest = manager.get_extension(extension_id) cmd_count = len(ext_manifest.commands) if ext_manifest else 0 reg_meta = manager.registry.get(extension_id) - skill_count = len(reg_meta.get("registered_skills", [])) if reg_meta else 0 + raw_skills = reg_meta.get("registered_skills") if reg_meta else None + skill_count = len(raw_skills) if isinstance(raw_skills, list) else 0 # Confirm removal if not force: diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index f6bdcd152f..0de2a3fc1a 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -587,12 +587,17 @@ def _register_extension_skills( if not source_file.is_file(): continue - # Derive skill name from command name - # e.g. "speckit.jira.create" -> "speckit-jira-create" (or dot for kimi) + # Derive skill name from command name, matching the convention used by + # presets.py: strip the leading "speckit." prefix, then form: + # Kimi → "speckit.{short_name}" (dot preserved for Kimi agent) + # other → "speckit-{short_name}" (hyphen separator) + short_name_raw = cmd_name + if short_name_raw.startswith("speckit."): + short_name_raw = short_name_raw[len("speckit."):] if selected_ai == "kimi": - skill_name = cmd_name # Keep dot notation for kimi + skill_name = f"speckit.{short_name_raw}" else: - skill_name = cmd_name.replace(".", "-") + skill_name = f"speckit-{short_name_raw}" # Check if skill already exists before creating the directory skill_subdir = skills_dir / skill_name @@ -601,14 +606,20 @@ def _register_extension_skills( # Do not overwrite user-customized skills continue - # Create skill directory only when we're going to write to it + # Create skill directory; track whether we created it so we can clean + # up safely if reading the source file subsequently fails. + created_now = not skill_subdir.exists() skill_subdir.mkdir(parents=True, exist_ok=True) # Parse the command file — guard against IsADirectoryError / decode errors try: content = source_file.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): - skill_subdir.rmdir() # undo the mkdir above before skipping + if created_now: + try: + skill_subdir.rmdir() # undo the mkdir; dir is empty at this point + except OSError: + pass # best-effort cleanup continue if content.startswith("---"): parts = content.split("---", 2) @@ -710,30 +721,32 @@ def _unregister_extension_skills(self, skill_names: List[str], extension_id: str skill_subdir = skills_candidate / skill_name if not skill_subdir.is_dir(): continue - # Safety check: only delete if SKILL.md metadata.source matches - # this extension, to avoid wiping user-created skills with the - # same name under a different agent dir. + # Safety check: only delete if SKILL.md exists and its + # metadata.source matches exactly this extension. If the + # file is missing or unreadable we skip to avoid deleting + # unrelated user-created directories. skill_md = skill_subdir / "SKILL.md" - if skill_md.is_file(): - try: - import yaml as _yaml - raw = skill_md.read_text(encoding="utf-8") - source = "" - if raw.startswith("---"): - parts = raw.split("---", 2) - if len(parts) >= 3: - fm = _yaml.safe_load(parts[1]) or {} - source = ( - fm.get("metadata", {}).get("source", "") - if isinstance(fm, dict) - else "" - ) - # Only remove skills explicitly created by this extension - if source != f"extension:{extension_id}": - continue - except (OSError, UnicodeDecodeError, Exception): - # If we can't verify, skip to avoid accidental deletion + if not skill_md.is_file(): + continue + try: + import yaml as _yaml + raw = skill_md.read_text(encoding="utf-8") + source = "" + if raw.startswith("---"): + parts = raw.split("---", 2) + if len(parts) >= 3: + fm = _yaml.safe_load(parts[1]) or {} + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) + # Only remove skills explicitly created by this extension + if source != f"extension:{extension_id}": continue + except (OSError, UnicodeDecodeError, Exception): + # If we can't verify, skip to avoid accidental deletion + continue shutil.rmtree(skill_subdir) def check_compatibility( diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index f011c059f9..0bf5686761 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -211,8 +211,8 @@ def test_skills_created_when_ai_skills_active(self, skills_project, extension_di # Check that skill directories were created skill_dirs = sorted([d.name for d in skills_dir.iterdir() if d.is_dir()]) - assert "speckit-test-ext-hello" in skill_dirs - assert "speckit-test-ext-world" in skill_dirs + assert "speckit-test-ext.hello" in skill_dirs + assert "speckit-test-ext.world" in skill_dirs def test_skill_md_content_correct(self, skills_project, extension_dir): """SKILL.md should have correct agentskills.io structure.""" @@ -222,13 +222,13 @@ def test_skill_md_content_correct(self, skills_project, extension_dir): extension_dir, "0.1.0", register_commands=False ) - skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md" + skill_file = skills_dir / "speckit-test-ext.hello" / "SKILL.md" assert skill_file.exists() content = skill_file.read_text() # Check structure assert content.startswith("---\n") - assert "name: speckit-test-ext-hello" in content + assert "name: speckit-test-ext.hello" in content assert "description:" in content assert "Test hello command" in content assert "source: extension:test-ext" in content @@ -244,7 +244,7 @@ def test_skill_md_has_parseable_yaml(self, skills_project, extension_dir): extension_dir, "0.1.0", register_commands=False ) - skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md" + skill_file = skills_dir / "speckit-test-ext.hello" / "SKILL.md" content = skill_file.read_text() assert content.startswith("---\n") @@ -252,7 +252,7 @@ def test_skill_md_has_parseable_yaml(self, skills_project, extension_dir): assert len(parts) >= 3 parsed = yaml.safe_load(parts[1]) assert isinstance(parsed, dict) - assert parsed["name"] == "speckit-test-ext-hello" + assert parsed["name"] == "speckit-test-ext.hello" assert "description" in parsed def test_no_skills_when_ai_skills_disabled(self, no_skills_project, extension_dir): @@ -281,7 +281,7 @@ def test_existing_skill_not_overwritten(self, skills_project, extension_dir): project_dir, skills_dir = skills_project # Pre-create a custom skill - custom_dir = skills_dir / "speckit-test-ext-hello" + custom_dir = skills_dir / "speckit-test-ext.hello" custom_dir.mkdir(parents=True) custom_content = "# My Custom Hello Skill\nUser-modified content\n" (custom_dir / "SKILL.md").write_text(custom_content) @@ -296,9 +296,9 @@ def test_existing_skill_not_overwritten(self, skills_project, extension_dir): # But the other skill should still be created metadata = manager.registry.get(manifest.id) - assert "speckit-test-ext-world" in metadata["registered_skills"] + assert "speckit-test-ext.world" in metadata["registered_skills"] # The pre-existing one should NOT be in registered_skills (it was skipped) - assert "speckit-test-ext-hello" not in metadata["registered_skills"] + assert "speckit-test-ext.hello" not in metadata["registered_skills"] def test_registered_skills_in_registry(self, skills_project, extension_dir): """Registry should contain registered_skills list.""" @@ -311,8 +311,8 @@ def test_registered_skills_in_registry(self, skills_project, extension_dir): metadata = manager.registry.get(manifest.id) assert "registered_skills" in metadata assert len(metadata["registered_skills"]) == 2 - assert "speckit-test-ext-hello" in metadata["registered_skills"] - assert "speckit-test-ext-world" in metadata["registered_skills"] + assert "speckit-test-ext.hello" in metadata["registered_skills"] + assert "speckit-test-ext.world" in metadata["registered_skills"] def test_kimi_uses_dot_notation(self, project_dir, temp_dir): """Kimi agent should use dot notation for skill names.""" @@ -375,7 +375,7 @@ def test_missing_command_file_skipped(self, skills_project, temp_dir): ) metadata = manager.registry.get(manifest.id) - assert "speckit-missing-cmd-ext-exists" in metadata["registered_skills"] + assert "speckit-missing-cmd-ext.exists" in metadata["registered_skills"] assert "speckit-missing-cmd-ext-ghost" not in metadata["registered_skills"] @@ -393,16 +393,16 @@ def test_skills_removed_on_extension_remove(self, skills_project, extension_dir) ) # Verify skills exist - assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() - assert (skills_dir / "speckit-test-ext-world" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext.world" / "SKILL.md").exists() # Remove extension result = manager.remove(manifest.id, keep_config=False) assert result is True # Skills should be gone - assert not (skills_dir / "speckit-test-ext-hello").exists() - assert not (skills_dir / "speckit-test-ext-world").exists() + assert not (skills_dir / "speckit-test-ext.hello").exists() + assert not (skills_dir / "speckit-test-ext.world").exists() def test_other_skills_preserved_on_remove(self, skills_project, extension_dir): """Non-extension skills should not be affected by extension removal.""" @@ -433,8 +433,8 @@ def test_remove_handles_already_deleted_skills(self, skills_project, extension_d ) # Manually delete skill dirs before calling remove - shutil.rmtree(skills_dir / "speckit-test-ext-hello") - shutil.rmtree(skills_dir / "speckit-test-ext-world") + shutil.rmtree(skills_dir / "speckit-test-ext.hello") + shutil.rmtree(skills_dir / "speckit-test-ext.world") # Should not raise result = manager.remove(manifest.id, keep_config=False) @@ -495,10 +495,10 @@ def test_command_without_frontmatter(self, skills_project, temp_dir): ext_dir, "0.1.0", register_commands=False ) - skill_file = skills_dir / "speckit-nofm-ext-plain" / "SKILL.md" + skill_file = skills_dir / "speckit-nofm-ext.plain" / "SKILL.md" assert skill_file.exists() content = skill_file.read_text() - assert "name: speckit-nofm-ext-plain" in content + assert "name: speckit-nofm-ext.plain" in content # Fallback description when no frontmatter description assert "Extension command: speckit.nofm-ext.plain" in content assert "Body without frontmatter." in content @@ -515,8 +515,8 @@ def test_gemini_agent_skills(self, project_dir, temp_dir): ) skills_dir = project_dir / ".gemini" / "skills" - assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() - assert (skills_dir / "speckit-test-ext-world" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext.world" / "SKILL.md").exists() def test_multiple_extensions_independent_skills(self, skills_project, temp_dir): """Installing and removing different extensions should be independent.""" @@ -534,15 +534,15 @@ def test_multiple_extensions_independent_skills(self, skills_project, temp_dir): ) # Both should have skills - assert (skills_dir / "speckit-ext-a-hello" / "SKILL.md").exists() - assert (skills_dir / "speckit-ext-b-hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-ext-a.hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-ext-b.hello" / "SKILL.md").exists() # Remove ext-a manager.remove("ext-a", keep_config=False) # ext-a skills gone, ext-b skills preserved - assert not (skills_dir / "speckit-ext-a-hello").exists() - assert (skills_dir / "speckit-ext-b-hello" / "SKILL.md").exists() + assert not (skills_dir / "speckit-ext-a.hello").exists() + assert (skills_dir / "speckit-ext-b.hello" / "SKILL.md").exists() def test_malformed_frontmatter_handled(self, skills_project, temp_dir): """Commands with invalid YAML frontmatter should still produce valid skills.""" @@ -591,7 +591,7 @@ def test_malformed_frontmatter_handled(self, skills_project, temp_dir): ext_dir, "0.1.0", register_commands=False ) - skill_file = skills_dir / "speckit-badfm-ext-broken" / "SKILL.md" + skill_file = skills_dir / "speckit-badfm-ext.broken" / "SKILL.md" assert skill_file.exists() content = skill_file.read_text() # Fallback description since frontmatter was invalid @@ -607,7 +607,7 @@ def test_remove_cleans_up_when_init_options_deleted(self, skills_project, extens ) # Verify skills exist - assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists() # Delete init-options.json to simulate user change init_opts = project_dir / ".specify" / "init-options.json" @@ -616,8 +616,8 @@ def test_remove_cleans_up_when_init_options_deleted(self, skills_project, extens # Remove should still clean up via fallback scan result = manager.remove(manifest.id, keep_config=False) assert result is True - assert not (skills_dir / "speckit-test-ext-hello").exists() - assert not (skills_dir / "speckit-test-ext-world").exists() + assert not (skills_dir / "speckit-test-ext.hello").exists() + assert not (skills_dir / "speckit-test-ext.world").exists() def test_remove_cleans_up_when_ai_skills_toggled(self, skills_project, extension_dir): """Skills should be cleaned up even if ai_skills is toggled to false after install.""" @@ -628,7 +628,7 @@ def test_remove_cleans_up_when_ai_skills_toggled(self, skills_project, extension ) # Verify skills exist - assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists() + assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists() # Toggle ai_skills to false _create_init_options(project_dir, ai="claude", ai_skills=False) @@ -636,5 +636,5 @@ def test_remove_cleans_up_when_ai_skills_toggled(self, skills_project, extension # Remove should still clean up via fallback scan result = manager.remove(manifest.id, keep_config=False) assert result is True - assert not (skills_dir / "speckit-test-ext-hello").exists() - assert not (skills_dir / "speckit-test-ext-world").exists() + assert not (skills_dir / "speckit-test-ext.hello").exists() + assert not (skills_dir / "speckit-test-ext.world").exists() From 9c41de61243f458a764e80f820dac107d741a213 Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 22:05:35 -0400 Subject: [PATCH 6/8] fix: is_dir() guard in fast-path rmtree and fix ghost-skill assertion naming --- src/specify_cli/extensions.py | 2 +- tests/test_extension_skills.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 0de2a3fc1a..91c4c64c8e 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -699,7 +699,7 @@ def _unregister_extension_skills(self, skill_names: List[str], extension_id: str # Fast path: we know the exact skills directory for skill_name in skill_names: skill_subdir = skills_dir / skill_name - if skill_subdir.exists(): + if skill_subdir.is_dir(): shutil.rmtree(skill_subdir) else: # Fallback: scan all possible agent skills directories diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 0bf5686761..b8d5202f52 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -376,7 +376,7 @@ def test_missing_command_file_skipped(self, skills_project, temp_dir): metadata = manager.registry.get(manifest.id) assert "speckit-missing-cmd-ext.exists" in metadata["registered_skills"] - assert "speckit-missing-cmd-ext-ghost" not in metadata["registered_skills"] + assert "speckit-missing-cmd-ext.ghost" not in metadata["registered_skills"] # ===== Extension Skill Unregistration Tests ===== From b24815745af63e9e1e962b4622fa9c052b9544e6 Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 22:18:59 -0400 Subject: [PATCH 7/8] fix: path-traversal guard on skill_name in both rmtree paths of _unregister_extension_skills --- src/specify_cli/extensions.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 91c4c64c8e..0c89740140 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -698,7 +698,17 @@ def _unregister_extension_skills(self, skill_names: List[str], extension_id: str if skills_dir: # Fast path: we know the exact skills directory for skill_name in skill_names: - skill_subdir = skills_dir / skill_name + # Guard against path traversal from a corrupted registry entry: + # reject names that are absolute, contain path separators, or + # resolve to a path outside the skills directory. + sn_path = Path(skill_name) + if sn_path.is_absolute() or len(sn_path.parts) != 1: + continue + try: + skill_subdir = (skills_dir / skill_name).resolve() + skill_subdir.relative_to(skills_dir.resolve()) # raises if outside + except (OSError, ValueError): + continue if skill_subdir.is_dir(): shutil.rmtree(skill_subdir) else: @@ -718,7 +728,15 @@ def _unregister_extension_skills(self, skill_names: List[str], extension_id: str if not skills_candidate.is_dir(): continue for skill_name in skill_names: - skill_subdir = skills_candidate / skill_name + # Same path-traversal guard as the fast path above + sn_path = Path(skill_name) + if sn_path.is_absolute() or len(sn_path.parts) != 1: + continue + try: + skill_subdir = (skills_candidate / skill_name).resolve() + skill_subdir.relative_to(skills_candidate.resolve()) # raises if outside + except (OSError, ValueError): + continue if not skill_subdir.is_dir(): continue # Safety check: only delete if SKILL.md exists and its From c689aa56bfcf2b5cb0a52b86c01aa0742a855a8b Mon Sep 17 00:00:00 2001 From: dhilipkumars Date: Tue, 24 Mar 2026 22:28:19 -0400 Subject: [PATCH 8/8] fix: add SKILL.md ownership check to fast-path rmtree and alias shadowed _get_skills_dir import --- src/specify_cli/extensions.py | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 0c89740140..d71480ac47 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -521,7 +521,7 @@ def _get_skills_dir(self) -> Optional[Path]: The skills directory ``Path``, or ``None`` if skills were not enabled or the init-options file is missing. """ - from . import load_init_options, _get_skills_dir + from . import load_init_options, _get_skills_dir as resolve_skills_dir opts = load_init_options(self.project_root) if not opts.get("ai_skills"): @@ -531,7 +531,7 @@ def _get_skills_dir(self) -> Optional[Path]: if not agent: return None - skills_dir = _get_skills_dir(self.project_root, agent) + skills_dir = resolve_skills_dir(self.project_root, agent) if not skills_dir.is_dir(): return None @@ -709,8 +709,33 @@ def _unregister_extension_skills(self, skill_names: List[str], extension_id: str skill_subdir.relative_to(skills_dir.resolve()) # raises if outside except (OSError, ValueError): continue - if skill_subdir.is_dir(): - shutil.rmtree(skill_subdir) + if not skill_subdir.is_dir(): + continue + # Safety check: only delete if SKILL.md exists and its + # metadata.source matches exactly this extension — mirroring + # the fallback branch — so a corrupted registry entry cannot + # delete an unrelated user skill. + skill_md = skill_subdir / "SKILL.md" + if not skill_md.is_file(): + continue + try: + import yaml as _yaml + raw = skill_md.read_text(encoding="utf-8") + source = "" + if raw.startswith("---"): + parts = raw.split("---", 2) + if len(parts) >= 3: + fm = _yaml.safe_load(parts[1]) or {} + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) + if source != f"extension:{extension_id}": + continue + except (OSError, UnicodeDecodeError, Exception): + continue + shutil.rmtree(skill_subdir) else: # Fallback: scan all possible agent skills directories from . import AGENT_CONFIG, AGENT_SKILLS_DIR_OVERRIDES, DEFAULT_SKILLS_DIR