* 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.
* fix(integration): refresh shared infra on integration switch
* fix(integration): address Copilot review on switch shared-infra refresh
- Clarify install_shared_infra docstring: force overwrites regular files
but always preserves symlinks (safe-destination check refuses to follow).
- Print refresh_hint only for preserved_user_files; skipped_files keeps
the generic remediation. Avoids misleading guidance when files were
merely skipped (not detected as customized).
- Catch ValueError from the safe-destination check and bucket the path
under a new symlinked_files warning instead of aborting the switch.
- Restore templates/constitution-template.md to upstream (drop accidental
leading blank lines).
* fix(integration): narrow symlink bucketing to dedicated exception
Address Copilot feedback on shared_infra.py:305 — _safe_dest_or_bucket
caught any ValueError as 'symlinked', which masked genuine safety errors
(path escape, parent-not-a-directory).
- Introduce SymlinkedSharedPathError(ValueError) raised only by the
symlink-specific branches in _ensure_safe_shared_*().
- _safe_dest_or_bucket() now catches only SymlinkedSharedPathError;
other ValueErrors propagate so the operation aborts with the real
cause instead of being silently bucketed.
- Wrap top-level dest_scripts/dest_variant/dest_templates mkdir calls
in the same bucket helper so a symlinked .specify/scripts or
.specify/templates is preserved with a warning rather than aborting
the switch (matches the documented 'preserve customizations' behavior).
- Update tests to expect the new bucket+warn behavior for leaf-level
symlinked destinations.
* fix(integration): tailor shared-infra warnings and rename preflight test
Address Copilot review on PR #2375:
- skipped_files hint now uses refresh_hint when refresh_managed=True
so integration switch suggests --refresh-shared-infra instead of the
generic init/upgrade flags.
- symlinked-files warning header says "path(s)" rather than "file(s)"
since symlinked directories (e.g. .specify/scripts/bash) are also
bucketed there.
- Rename test_shared_infra_install_preflights_before_writing to
test_shared_infra_install_buckets_unsafe_destinations_and_continues
to match the new bucket-and-continue semantics.
* test: rename symlink bucketing tests to reflect bucket-and-continue behavior
The two file-bucketing tests at line 300/320 were named *_refuses_*, but
the new behavior buckets symlinked file destinations with a warning while
safe destinations in the same install still complete. Rename to
*_buckets_* and update docstrings to match.
The remaining *_refuses_* tests (line 342/362/381) genuinely raise on
symlinked dirs/manifests and keep their names.
---------
Co-authored-by: Quratulain-bilal <quratulain.bilal@users.noreply.github.com>
