mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix(presets): restore core skills instead of deleting them on preset remove (#3929)
* fix(presets): restore core skills instead of deleting them on preset remove 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 * fix(tests): use explicit utf-8 encoding reading restored skill content read_text() defaults to the platform locale encoding, which is cp1252 ("charmap") on Windows. The bundled specify.md core template contains a UTF-8 multi-byte emoji whose bytes aren't valid cp1252, so the Windows CI job failed decoding the restored SKILL.md with UnicodeDecodeError. * fix(presets): keep extension restore priority over bundled-core fallback The bundled-core fallback added for #3928 ran before the extension_restore_index lookup, so a skill an installed extension owns could be silently replaced by lower-priority bundled core content on preset removal instead of preserving the extension's winning layer.
This commit is contained in:
@@ -2186,7 +2186,10 @@ class PresetManager:
|
||||
]
|
||||
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:
|
||||
@@ -3021,6 +3024,7 @@ class PresetManager:
|
||||
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.
|
||||
|
||||
@@ -3028,6 +3032,17 @@ class PresetManager:
|
||||
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
|
||||
@@ -3101,6 +3116,7 @@ class PresetManager:
|
||||
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] = (
|
||||
@@ -3133,6 +3149,7 @@ class PresetManager:
|
||||
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)}
|
||||
@@ -3206,6 +3223,7 @@ class PresetManager:
|
||||
*,
|
||||
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.
|
||||
|
||||
@@ -3216,6 +3234,7 @@ class PresetManager:
|
||||
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.
|
||||
@@ -3291,9 +3310,34 @@ class PresetManager:
|
||||
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():
|
||||
extension_restore = extension_restore_index.get(skill_name)
|
||||
|
||||
# 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). An
|
||||
# installed extension providing a core-named command resolves
|
||||
# ahead of bundled core elsewhere, so skip the bundled fallback
|
||||
# when an extension restore exists — otherwise it would win
|
||||
# over the higher-priority extension layer below.
|
||||
core_file = core_templates_dir / f"{short_name}.md"
|
||||
if (
|
||||
not core_file.exists()
|
||||
and restore_from_bundled_core
|
||||
and extension_restore is None
|
||||
):
|
||||
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:
|
||||
@@ -3338,7 +3382,6 @@ class PresetManager:
|
||||
mutated_names.append(skill_name)
|
||||
continue
|
||||
|
||||
extension_restore = extension_restore_index.get(skill_name)
|
||||
if extension_restore:
|
||||
content = extension_restore["source_file"].read_text(encoding="utf-8")
|
||||
frontmatter, body = registrar.parse_frontmatter(content)
|
||||
@@ -3477,7 +3520,9 @@ class PresetManager:
|
||||
"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)
|
||||
@@ -3815,6 +3860,7 @@ class PresetManager:
|
||||
restorable_skills,
|
||||
pack_dir,
|
||||
additional_owned_sources=override_sources,
|
||||
restore_from_bundled_core=True,
|
||||
)
|
||||
try:
|
||||
from ..agents import CommandRegistrar
|
||||
|
||||
Reference in New Issue
Block a user