fix: create skills directory on demand during extension/preset install by mnriem · Pull Request #2711 · github/spec-kit

mnriem · 2026-05-26T18:28:58Z

Summary

_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.json. This caused extension skill registration to silently produce an empty registered_skills list and skip writing SKILL.md files — with no warning at any stage.

Changes

src/specify_cli/extensions.py — Replace the is_dir() bail-out in _get_skills_dir() with mkdir(parents=True, exist_ok=True) so the directory is created on demand when ai_skills is enabled.
src/specify_cli/presets.py — Same fix for the parallel _get_skills_dir() in PresetManager.
tests/test_extension_skills.py — Update existing test expectation; add parametrized regression test (claude + codex) that installs an extension before the skills directory exists and asserts SKILL.md files and registry entries are created. Strengthen negative tests to assert the directory is NOT created when skills are disabled.

Root Cause

When specify extension add runs before the agent's skills directory exists (e.g. .claude/skills/), _get_skills_dir() checked skills_dir.is_dir() and returned None, causing _register_extension_skills() to return []. The extension was recorded without any skills and hooks bound to those skills silently never fired.

Fixes #2682

_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 github#2682

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.

Copilot

Pull request overview

This PR fixes a silent failure mode where extension/preset skill registration could be skipped when the agent skills directory didn’t exist yet, even though ai_skills was enabled in .specify/init-options.json. The change ensures the skills directory is created on demand so SKILL.md files and registry entries are generated consistently (regression for #2682).

Changes:

Create the agent skills directory on demand in ExtensionManager._get_skills_dir() so extension installs can always materialize skills when enabled.
Apply the same on-demand directory creation in PresetManager._get_skills_dir() for preset skill overrides.
Update/add tests to assert (a) the dir is not created when skills are disabled and (b) skills registration works when installing before the directory exists (claude + codex).

Show a summary per file

File	Description
src/specify_cli/extensions.py	Create skills directory during extension skill registration instead of bailing when missing.
src/specify_cli/presets.py	Create skills directory during preset skill override registration instead of bailing when missing.
tests/test_extension_skills.py	Add regression coverage for “dir missing” installs; strengthen negative assertions around not creating dirs when disabled.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 3/3 changed files
Comments generated: 5

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.

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 3

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.

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 1

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.

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 2

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.

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 2

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.

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 1

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.

Copilot

Copilot's findings

Files reviewed: 5/5 changed files
Comments generated: 1

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.

Copilot

Copilot's findings

Comments suppressed due to low confidence (1)

src/specify_cli/shared_infra.py:101

_ensure_safe_shared_directory() now accepts a context string for error messages, but if _shared_relative_path() raises (e.g. resolved directory is outside project root), the exception text still says "Shared infrastructure path escapes project root". When this helper is reused for agent skills (context="agent skills directory"), that message becomes misleading in user-facing warnings. Consider threading context into _shared_relative_path() (or making its message context-neutral) so diagnostics stay accurate.

    """Create a shared infra directory without following symlinked parents."""
    root = project_path.resolve()
    rel = _shared_relative_path(project_path, directory)
    current = project_path

Files reviewed: 5/5 changed files
Comments generated: 0 new

mnriem added 2 commits May 26, 2026 13:13

test: assert skills dir is NOT created when skills are disabled

3407187

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.

Copilot AI review requested due to automatic review settings May 26, 2026 18:28

Copilot started reviewing on behalf of mnriem May 26, 2026 18:29 View session