mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
marcelsafinandCopilot
ab6c28cd81
Fixes 3 findings from the Copilot review on HEAD 31c9b97 (#2948):
1. presets/__init__.py: remove()'s command reconciliation only recreated
the surviving preset's content for the currently active agent, even
though the removed preset's registered_commands could span multiple
historical (now-inactive) agents recorded via prior rescaffolds. Now
remove() captures every historical agent registered_commands actually
targeted (before mutation) and passes it as extra_agents through
_reconcile_composed_commands -> _register_for_non_skill_agents /
_register_command_from_path -> registrar.register_commands_for_non_
skill_agents, so the active-only restriction for install/use is
preserved while post-removal reconciliation restores every touched
directory.
2. presets/__init__.py: the analogous gap existed for skills. _unregister_
skills() now returns {skills_dir: renderer_agent} for every directory it
actually restored, and _reconcile_skills() accepts extra_skills_dirs to
reconcile each of those directories (via a new apply_to_dir() helper),
not only the currently active skills directory. _register_skills() gained
optional target_dir/target_agent overrides (forcing
create_missing_skills off for non-active directories) so a historical
directory is only ever restored, never seeded with brand-new skills.
3. extensions/__init__.py: _extension_owned_skill_names() and both the
fast and fallback paths of _unregister_extension_skills() validated only
the parent skills_dir for symlink escape, then resolved
skills_dir / skill_name and checked containment relative to that
already-resolved parent. A per-skill child that is itself a symlink to
a different, legitimate skill directory within the same (safe) root
passed that containment check, so deleting/attributing through the
symlink name could destroy or misattribute an unrelated skill reached
only via the alias. All three call sites now run the shared
_validate_safe_shared_directory() component-wise check against the full
skills_dir / skill_name path (not just the parent) before any read or
delete, rejecting a symlinked child outright rather than following it,
even when its resolved target remains in-bounds.
Regression tests added (all confirmed red against pre-fix code, green
after):
- test_remove_reconciles_command_for_every_historical_agent
- test_remove_reconciles_skill_for_every_historical_agent
- test_extension_owned_skill_names_rejects_symlinked_child_skill_dir
- test_unregister_extension_skills_explicit_dir_rejects_symlinked_child
- test_unregister_extension_skills_fallback_rejects_symlinked_child
Tests: tests/test_presets.py (361), tests/test_extension_skills.py (69),
tests/test_extensions.py (338) all pass; tests/integrations (1768 passed,
1 skipped) pass; full suite 3902 passed / 74 skipped (90 pre-existing,
environment-only git-signing tests deselected — confirmed failing
identically on the pre-change baseline due to local 1Password SSH-agent
signing, unrelated to this change). ruff check clean on all changed files.
Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>