From e54653efcc2bd21e6a0a79c44daa70599e6347d8 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 26 May 2026 15:10:59 -0500 Subject: [PATCH] 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. --- src/specify_cli/__init__.py | 51 ++++++++++++++++++++++++++++++++ src/specify_cli/extensions.py | 42 +++++++++----------------- src/specify_cli/presets.py | 41 +++++++++----------------- src/specify_cli/shared_infra.py | 20 ++++++++----- tests/test_extension_skills.py | 52 +++++++++++++++++++++++++++++---- 5 files changed, 138 insertions(+), 68 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index ee75b925d..71579bef3 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -317,6 +317,57 @@ def _get_skills_dir(project_path: Path, selected_ai: str) -> Path: return project_path / ".agents" / "skills" +def resolve_active_skills_dir(project_root: Path) -> Path | None: + """Return the active skills directory, creating it on demand when enabled. + + Reads ``.specify/init-options.json`` to determine whether skills are + enabled and which agent was selected. When ``ai_skills`` is true the + directory is created safely (symlink/containment checks); when false + only Kimi's native-skills fallback is honoured (directory must already + exist). + + Returns: + The skills directory ``Path``, or ``None`` if skills are not active. + + Raises: + ValueError: If the resolved skills path escapes the project root, + a parent component is a symlink, or a path component exists + but is not a directory. + OSError: If the directory cannot be created (e.g. permission denied). + """ + from .shared_infra import _ensure_safe_shared_directory + + opts = load_init_options(project_root) + if not isinstance(opts, dict): + opts = {} + + agent = opts.get("ai") + if not isinstance(agent, str) or not agent: + return None + + ai_skills_enabled = bool(opts.get("ai_skills")) + if not ai_skills_enabled and agent != "kimi": + return None + + skills_dir = _get_skills_dir(project_root, agent) + + if not ai_skills_enabled: + # Kimi native-skills fallback: use the directory only if it exists. + if not skills_dir.is_dir(): + return None + _ensure_safe_shared_directory( + project_root, skills_dir, + create=False, context="agent skills directory", + ) + return skills_dir + + # ai_skills is explicitly enabled — create the directory safely. + _ensure_safe_shared_directory( + project_root, skills_dir, context="agent skills directory", + ) + return skills_dir + + def _cli_error_detail(exc: BaseException) -> str: """Return a compact one-line exception detail for CLI output.""" detail = str(exc).replace("\n", " ").strip() diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 871503f0a..20ed91412 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -801,38 +801,24 @@ class ExtensionManager: def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for extension skill registration. - 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. + Delegates to :func:`resolve_active_skills_dir` which reads + init-options, applies the Kimi native-skills fallback, and + safely creates the directory when ``ai_skills`` is enabled. - Kimi is treated as a native-skills agent: if ``ai == "kimi"`` and - ``.kimi/skills`` exists, extension installs should still propagate - command skills even when ``ai_skills`` is false. - - Returns: - The skills directory ``Path``, or ``None`` if skills were not - enabled and no native-skills fallback applies. + Returns ``None`` (instead of raising) when the directory cannot + be created due to symlink, containment, or permission issues so + that callers can fall back gracefully. """ - from . import load_init_options, _get_skills_dir as resolve_skills_dir - - opts = load_init_options(self.project_root) - if not isinstance(opts, dict): - opts = {} - - agent = opts.get("ai") - if not isinstance(agent, str) or not agent: + from . import resolve_active_skills_dir, _print_cli_warning + try: + return resolve_active_skills_dir(self.project_root) + except (ValueError, OSError) as exc: + _print_cli_warning( + "resolve", "skills directory", None, exc, + continuing="Continuing without skill registration.", + ) return None - ai_skills_enabled = bool(opts.get("ai_skills")) - if not ai_skills_enabled and agent != "kimi": - return None - - skills_dir = resolve_skills_dir(self.project_root, agent) - if not skills_dir.is_dir(): - return None - - return skills_dir - def _register_extension_skills( self, manifest: ExtensionManifest, diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index c6e75ae79..79b991f5a 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -1097,37 +1097,24 @@ class PresetManager: def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for preset skill overrides. - 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. + Delegates to :func:`resolve_active_skills_dir` which reads + init-options, applies the Kimi native-skills fallback, and + safely creates the directory when ``ai_skills`` is enabled. - Kimi is treated as a native-skills agent: if ``ai == "kimi"`` and - ``.kimi/skills`` exists, presets should still propagate command - overrides to skills even when ``ai_skills`` is false. - - Returns: - The skills directory ``Path``, or ``None`` if skills were not - enabled and no native-skills fallback applies. + Returns ``None`` (instead of raising) when the directory cannot + be created due to symlink, containment, or permission issues so + that callers can fall back gracefully. """ - from . import load_init_options, _get_skills_dir - - opts = load_init_options(self.project_root) - if not isinstance(opts, dict): - opts = {} - agent = opts.get("ai") - if not isinstance(agent, str) or not agent: + from . import resolve_active_skills_dir, _print_cli_warning + try: + return resolve_active_skills_dir(self.project_root) + except (ValueError, OSError) as exc: + _print_cli_warning( + "resolve", "skills directory", None, exc, + continuing="Continuing without skill registration.", + ) return None - ai_skills_enabled = bool(opts.get("ai_skills")) - if not ai_skills_enabled and agent != "kimi": - return None - - skills_dir = _get_skills_dir(self.project_root, agent) - if not skills_dir.is_dir(): - return None - - return skills_dir - @staticmethod def _skill_names_for_command(cmd_name: str) -> tuple[str, str]: """Return the modern and legacy skill directory names for a command.""" diff --git a/src/specify_cli/shared_infra.py b/src/specify_cli/shared_infra.py index 35bf02e64..32ce7e8c0 100644 --- a/src/specify_cli/shared_infra.py +++ b/src/specify_cli/shared_infra.py @@ -88,7 +88,13 @@ def _shared_relative_path(project_path: Path, dest: Path) -> Path: return rel -def _ensure_safe_shared_directory(project_path: Path, directory: Path, *, create: bool = True) -> None: +def _ensure_safe_shared_directory( + project_path: Path, + directory: Path, + *, + create: bool = True, + context: str = "shared infrastructure directory", +) -> None: """Create a shared infra directory without following symlinked parents.""" root = project_path.resolve() rel = _shared_relative_path(project_path, directory) @@ -98,24 +104,24 @@ def _ensure_safe_shared_directory(project_path: Path, directory: Path, *, create current = current / part label = _shared_destination_label(project_path, current) if current.is_symlink(): - raise SymlinkedSharedPathError(f"Refusing to use symlinked shared infrastructure directory: {label}") + raise SymlinkedSharedPathError(f"Refusing to use symlinked {context}: {label}") if current.exists(): if not current.is_dir(): - raise ValueError(f"Shared infrastructure directory path is not a directory: {label}") + raise ValueError(f"{context.capitalize()} path is not a directory: {label}") try: current.resolve().relative_to(root) except (OSError, ValueError): - raise ValueError(f"Shared infrastructure directory escapes project root: {label}") from None + raise ValueError(f"{context.capitalize()} escapes project root: {label}") from None continue if not create: - raise ValueError(f"Shared infrastructure directory does not exist: {label}") + raise ValueError(f"{context.capitalize()} does not exist: {label}") current.mkdir() if current.is_symlink(): - raise SymlinkedSharedPathError(f"Refusing to use symlinked shared infrastructure directory: {label}") + raise SymlinkedSharedPathError(f"Refusing to use symlinked {context}: {label}") try: current.resolve().relative_to(root) except (OSError, ValueError): - raise ValueError(f"Shared infrastructure directory escapes project root: {label}") from None + raise ValueError(f"{context.capitalize()} escapes project root: {label}") from None def _validate_safe_shared_directory(project_path: Path, directory: Path) -> None: diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 89e8b4a8b..fb4bf92d9 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -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 =====