From 7563d60d655f2a18ea3dd4aa38ab185032ff900a Mon Sep 17 00:00:00 2001 From: chelsealong Date: Fri, 31 Jul 2026 18:08:02 +0000 Subject: [PATCH] fix(presets): restore core skills instead of deleting them on preset remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skill restoration looked for core command templates under .specify/templates/commands, a directory specify init never populates in real projects. Since that lookup always missed, presets overriding a core command (e.g. speckit.plan) had their skill deleted outright on removal instead of restored — the actual core templates live in the bundled core_pack (wheel install) or the repo-root templates/ tree. Restoration now falls back to that bundled location, gated behind a restore_from_bundled_core flag so the existing "retire a stale skill superseded by a command-mode winner" path keeps deleting rather than resurrecting a duplicate skill. Fixes #3928 --- src/specify_cli/presets/__init__.py | 47 ++++++++++++++++++++++++++--- tests/test_presets.py | 32 ++++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 2f32d162b4..d99d4f4401 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2157,7 +2157,10 @@ def apply_to_dir( ] if dir_core_ext_names: self._unregister_skills_in_dir( - dir_core_ext_names, skills_dir, dir_agent + dir_core_ext_names, + skills_dir, + dir_agent, + restore_from_bundled_core=True, ) for _skill_name, cmd_name, top_layer in override_skills: @@ -2992,6 +2995,7 @@ def _unregister_skills( preset_dir: Union[Path, str], *, additional_owned_sources: Optional[Dict[str, str]] = None, + restore_from_bundled_core: bool = False, ) -> Dict[Path, tuple[Optional[str], List[str]]]: """Restore original SKILL.md files after a preset is removed. @@ -2999,6 +3003,17 @@ def _unregister_skills( regenerate the skill from the core command template. If no core template exists, the skill directory is removed. + Args: + restore_from_bundled_core: When True, a missing project-local + core template (the common case — ``specify init`` never + populates ``.specify/templates/commands``) falls back to + the bundled core_pack/repo-root templates so the skill is + restored instead of deleted (#3928). Callers that are + retiring a skill because its command now renders elsewhere + (a command file superseding it) must leave this False so + the skill is removed rather than resurrected with core + content that would duplicate the winning command. + ``registered_skills`` records exactly which agent directories this preset actually wrote to (see :meth:`_register_skills`), so removal restores precisely those directories rather than guessing at every @@ -3072,6 +3087,7 @@ def _unregister_skills( renderer_agent, pack_id=pack_id, additional_owned_sources=additional_owned_sources, + restore_from_bundled_core=restore_from_bundled_core, ) if mutated_names: restored[skills_dir] = ( @@ -3104,6 +3120,7 @@ def _unregister_skills( selected_ai, pack_id=pack_id, additional_owned_sources=additional_owned_sources, + restore_from_bundled_core=restore_from_bundled_core, ) return ( {skills_dir: (selected_ai, mutated_names)} @@ -3177,6 +3194,7 @@ def _unregister_skills_in_dir( *, pack_id: Optional[str] = None, additional_owned_sources: Optional[Dict[str, str]] = None, + restore_from_bundled_core: bool = False, ) -> List[str]: """Restore original SKILL.md files within a single skills directory. @@ -3187,6 +3205,7 @@ def _unregister_skills_in_dir( placeholder resolution and argument-hint formatting. additional_owned_sources: Generated non-preset source markers accepted as owned for specific skill names. + restore_from_bundled_core: See ``_unregister_skills``. Returns: Skill names whose files were restored or removed. @@ -3262,9 +3281,24 @@ def _unregister_skills_in_dir( if current_source not in owned_sources: continue - # Try to find the core command template - core_file = core_templates_dir / f"{short_name}.md" if core_templates_dir.exists() else None - if core_file and not core_file.exists(): + # Try to find the core command template. Project-local overrides + # in core_templates_dir take precedence, but that directory is + # rarely populated — the real core commands ship in the bundled + # core_pack (wheel install) or the repo-root templates/ tree + # (source checkout). Callers that want a genuine restore (a + # preset was removed outright, not superseded by another + # renderer) opt into that fallback via restore_from_bundled_core + # so the skill is restored instead of deleted (#3928). + core_file = core_templates_dir / f"{short_name}.md" + if not core_file.exists() and restore_from_bundled_core: + from .. import _locate_core_pack, _repo_root + + _core_pack = _locate_core_pack() + if _core_pack is not None: + core_file = _core_pack / "commands" / f"{short_name}.md" + else: + core_file = _repo_root() / "templates" / "commands" / f"{short_name}.md" + if not core_file.exists(): core_file = None if core_file: @@ -3448,7 +3482,9 @@ def install_from_directory( "registered_skills", registered_skills ) if persisted_skills: - self._unregister_skills(persisted_skills, dest_dir) + self._unregister_skills( + persisted_skills, dest_dir, restore_from_bundled_core=True + ) try: if dest_dir.exists(): shutil.rmtree(dest_dir) @@ -3786,6 +3822,7 @@ def remove(self, pack_id: str) -> bool: restorable_skills, pack_dir, additional_owned_sources=override_sources, + restore_from_bundled_core=True, ) try: from ..agents import CommandRegistrar diff --git a/tests/test_presets.py b/tests/test_presets.py index d4c964c838..015b982efb 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4925,6 +4925,38 @@ def test_skill_restored_on_preset_remove(self, project_dir, temp_dir): assert "templates/commands/specify.md" in content, "Should reference core template" assert "disable-model-invocation: false" in content + def test_skill_restored_on_preset_remove_without_project_core_templates(self, project_dir): + """Removing a preset must restore core skills even when the project + has no ``.specify/templates/commands`` directory of its own — which + is the normal case, since ``specify init`` never populates it. The + real core commands live in the bundled core_pack/repo-root templates + tree, and restoration must fall back there instead of deleting the + skill outright (#3928). + """ + self._write_init_options(project_dir, ai="claude") + skills_dir = project_dir / ".claude" / "skills" + self._create_skill(skills_dir, "speckit-specify") + + # The project_dir fixture's commands dir is empty, matching a real + # project — specify init never populates project-local overrides + # for unmodified core commands. + core_cmds = project_dir / ".specify" / "templates" / "commands" + assert core_cmds.exists() and not any(core_cmds.iterdir()) + + manager = PresetManager(project_dir) + install_self_test_preset(manager) + + skill_file = skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:self-test" in skill_file.read_text() + + manager.remove("self-test") + + assert skill_file.exists(), "Core skill must be restored, not deleted" + content = skill_file.read_text() + assert "preset:self-test" not in content + assert "templates/commands/specify.md" in content + assert "Create or update the feature specification" in content + def test_skill_restored_on_remove_resolves_script_placeholders(self, project_dir): """Core restore should resolve {SCRIPT}/{ARGS} placeholders like other skill paths.""" self._write_init_options(project_dir, ai="claude", ai_skills=True, script="sh")