mirror of
https://github.com/github/spec-kit.git
synced 2026-07-08 15:06:10 +08:00
fix: create skills directory on demand during extension/preset install (#2711)
* fix: create skills directory on demand during extension/preset install _get_skills_dir() in both extensions.py and presets.py returned None when the skills directory did not yet exist on disk, even though skills were enabled in init-options. This caused extension skill registration to silently produce an empty registered_skills list and skip writing SKILL.md files. Replace the is_dir() bail-out with mkdir(parents=True, exist_ok=True) so the directory is created on demand when ai_skills is enabled. Update the existing test expectation and add a parametrized regression test (claude + codex) that installs an extension before the skills directory exists and asserts SKILL.md files and registry entries are created. Fixes #2682 * test: assert skills dir is NOT created when skills are disabled Strengthen negative tests to verify _get_skills_dir does not create the directory on disk when ai_skills is false or init-options.json is absent. * fix: add symlink/containment check and preserve Kimi existence gate Address PR review feedback: - Use _ensure_safe_shared_directory() instead of raw mkdir() to prevent symlink-following writes outside the project root. - Restore the is_dir() existence gate for the Kimi native-skills fallback (ai_skills=false): only create the directory on demand when ai_skills is explicitly enabled. - Update docstrings to reflect the on-demand vs existence-gate behavior. - Reuse resolve_skills_dir helper in tests instead of manually reconstructing paths from AGENT_CONFIG. * refactor: extract resolve_active_skills_dir shared helper Deduplicate the _get_skills_dir logic that was nearly identical in ExtensionManager and PresetManager into a single module-level resolve_active_skills_dir() function in __init__.py. The shared helper wraps _ensure_safe_shared_directory errors with skills-specific messages so users see 'agent skills directory' instead of 'shared infrastructure directory' in error output. Both class methods now delegate to the shared helper. * fix: preserve original error reason in skills dir safety check Include the original exception message from _ensure_safe_shared_directory in the re-raised ValueError so the user sees the specific reason (symlink, not-a-directory, path escape, etc.) instead of a generic message. * fix: handle skills dir safety errors gracefully during install Catch ValueError/OSError from _get_skills_dir() inside _register_extension_skills() so a symlink or permission error logs a warning and returns [] instead of aborting mid-install and leaving a partially-installed extension without a registry entry. Also document OSError in resolve_active_skills_dir() docstring. * fix: catch errors in _get_skills_dir and use _print_cli_warning Move the ValueError/OSError catch from _register_extension_skills into _get_skills_dir itself so all callers (install, uninstall, reconcile) are protected from unsafe-path exceptions. Replace logging.getLogger().warning with _print_cli_warning for consistent Rich-formatted user output. * fix: use context-aware error messages for skills directory safety Add a 'context' parameter to _ensure_safe_shared_directory (defaults to 'shared infrastructure directory' for backward compat). The skills dir caller passes context='agent skills directory' so error messages say e.g. 'Refusing to use symlinked agent skills directory' instead of 'Refusing to use symlinked shared infrastructure directory'. Simplify resolve_active_skills_dir by removing the now-unnecessary try/except wrapper. * fix: validate Kimi native-skills directory for symlink/containment The Kimi fallback path (ai_skills=false) used is_dir() which follows symlinks, so a symlinked .kimi/skills could cause writes outside the project root. Now validates with _ensure_safe_shared_directory(create= False) before returning the directory.
This commit is contained in:
@@ -173,24 +173,32 @@ class TestExtensionManagerGetSkillsDir:
|
||||
assert result == skills_dir
|
||||
|
||||
def test_returns_none_when_no_ai_skills(self, no_skills_project):
|
||||
"""Should return None when ai_skills is false."""
|
||||
"""Should return None when ai_skills is false and not create the dir."""
|
||||
manager = ExtensionManager(no_skills_project)
|
||||
result = manager._get_skills_dir()
|
||||
assert result is None
|
||||
# Ensure the directory was NOT created on disk
|
||||
from specify_cli import _get_skills_dir as resolve_skills_dir
|
||||
skills_path = resolve_skills_dir(no_skills_project, "claude")
|
||||
assert not skills_path.exists()
|
||||
|
||||
def test_returns_none_when_no_init_options(self, project_dir):
|
||||
"""Should return None when init-options.json is missing."""
|
||||
"""Should return None when init-options.json is missing and not create any dir."""
|
||||
manager = ExtensionManager(project_dir)
|
||||
result = manager._get_skills_dir()
|
||||
assert result is None
|
||||
# No agent skills directory should have been created
|
||||
assert not (project_dir / ".claude" / "skills").exists()
|
||||
assert not (project_dir / ".agents" / "skills").exists()
|
||||
|
||||
def test_returns_none_when_skills_dir_missing(self, project_dir):
|
||||
"""Should return None when skills dir doesn't exist on disk."""
|
||||
def test_creates_skills_dir_on_demand(self, project_dir):
|
||||
"""Should create skills dir when ai_skills is enabled but dir is missing."""
|
||||
_create_init_options(project_dir, ai="claude", ai_skills=True)
|
||||
# Don't create the skills directory
|
||||
# Don't create the skills directory — _get_skills_dir should do it
|
||||
manager = ExtensionManager(project_dir)
|
||||
result = manager._get_skills_dir()
|
||||
assert result is None
|
||||
assert result is not None
|
||||
assert result.is_dir()
|
||||
|
||||
def test_returns_kimi_skills_dir_when_ai_skills_disabled(self, project_dir):
|
||||
"""Kimi should still use its native skills dir when ai_skills is false."""
|
||||
@@ -460,6 +468,38 @@ class TestExtensionSkillRegistration:
|
||||
assert "speckit-missing-cmd-ext-exists" in metadata["registered_skills"]
|
||||
assert "speckit-missing-cmd-ext-ghost" not in metadata["registered_skills"]
|
||||
|
||||
@pytest.mark.parametrize("ai", ["claude", "codex"])
|
||||
def test_skills_registered_when_dir_missing(self, project_dir, temp_dir, ai):
|
||||
"""Extension add should create skills dir on demand and register skills.
|
||||
|
||||
Regression test for https://github.com/github/spec-kit/issues/2682:
|
||||
when an extension is installed before the agent skills directory exists,
|
||||
skills must still be materialized (the directory is created on demand).
|
||||
"""
|
||||
_create_init_options(project_dir, ai=ai, ai_skills=True)
|
||||
# Deliberately do NOT create the skills directory
|
||||
ext_dir = _create_extension_dir(temp_dir, ext_id="early-ext")
|
||||
|
||||
manager = ExtensionManager(project_dir)
|
||||
manifest = manager.install_from_directory(
|
||||
ext_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
# Skills dir should have been created automatically
|
||||
from specify_cli import _get_skills_dir as resolve_skills_dir
|
||||
skills_dir = resolve_skills_dir(project_dir, ai)
|
||||
assert skills_dir.is_dir()
|
||||
|
||||
# SKILL.md files should exist
|
||||
assert (skills_dir / "speckit-early-ext-hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-early-ext-world" / "SKILL.md").exists()
|
||||
|
||||
# Registry should record them
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
assert len(metadata["registered_skills"]) == 2
|
||||
assert "speckit-early-ext-hello" in metadata["registered_skills"]
|
||||
assert "speckit-early-ext-world" in metadata["registered_skills"]
|
||||
|
||||
|
||||
# ===== Extension Skill Unregistration Tests =====
|
||||
|
||||
|
||||
Reference in New Issue
Block a user