diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 5a4a4fb739..b170f4ecdc 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -151,6 +151,7 @@ def _reconcile_skills( # directory-independent, so it's computed once and then applied to # every affected directory below. preset_cmds: Dict[str, List[str]] = {} + composed_winners: Dict[str, str] = {} non_preset_skills: List[tuple] = [] managed_skill_names: set = set() reconciled_skill_commands: set[str] = set() @@ -195,6 +196,8 @@ def _reconcile_skills( pack_dir = self.presets_dir / pack_id if top_path.is_relative_to(pack_dir): preset_cmds.setdefault(pack_id, []).append(cmd_name) + if layers[0]["strategy"] != "replace": + composed_winners[cmd_name] = pack_id found_preset = True break if not found_preset: @@ -362,7 +365,12 @@ def apply_to_dir( # Preserve exact prior behaviour for the currently # active directory (including the ability to create # brand-new skill subdirectories when ai_skills is on). - written = self._register_skills(filtered_manifest, pack_dir) + # On removal, write only the skill names this directory + # may manage, so a user-owned modern/legacy sibling of a + # recomposed skill is left alone. + written = self._register_skills( + filtered_manifest, pack_dir, only_skill_names=managed_names, + ) else: written = self._register_skills( filtered_manifest, pack_dir, @@ -394,16 +402,21 @@ def apply_to_dir( extra_dirs = extra_skills_dirs or {} if active_skills_dir: active_provenance = extra_dirs.get(active_skills_dir) - if extra_skills_dirs is None or active_provenance: + active_names: Optional[Set[str]] = None + if extra_skills_dirs is not None: + # Removal restores only skills the removed preset still owned, + # but a surviving preset composing over a removed lower layer + # owns its skill: recompose it while it keeps that marker. + active_names = set(active_provenance[1]) if active_provenance else set() + active_names |= self._skills_owned_by_preset( + active_skills_dir, composed_winners + ) + if active_names is None or active_names: apply_to_dir( active_skills_dir, active_ai, is_active=True, - managed_names=( - set(active_provenance[1]) - if active_provenance - else None - ), + managed_names=active_names, ) for extra_dir, (extra_agent, extra_names) in extra_dirs.items(): @@ -418,6 +431,31 @@ def apply_to_dir( return reconciled_skill_commands + def _skills_owned_by_preset( + self, skills_dir: Path, owners: Dict[str, str] + ) -> Set[str]: + """Return skills in ``skills_dir`` whose source marker names their preset. + + ``owners`` maps command names to the expected owning preset id. + """ + registrar = self._command_registrar() + owned: Set[str] = set() + for cmd_name, pack_id in owners.items(): + for skill_name in self._skill_names_for_command(cmd_name): + skill_file = skills_dir / skill_name / "SKILL.md" + if not self._validate_skill_subdir( + skill_file.parent, create=False, skills_root=skills_dir + ) or not skill_file.is_file(): + continue + try: + content = skill_file.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + metadata = registrar.parse_frontmatter(content)[0].get("metadata") + if isinstance(metadata, dict) and metadata.get("source") == f"preset:{pack_id}": + owned.add(skill_name) + return owned + def _resolve_agent_skills_dir(self, agent_name: str) -> Path: """Resolve the real skill output directory for an integration.""" from .. import _get_skills_dir as _project_skills_dir @@ -626,6 +664,7 @@ def _register_skills( *, target_dir: Optional[Path] = None, target_agent: Optional[str] = None, + only_skill_names: Optional[Set[str]] = None, ) -> Dict[str, List[str]]: """Generate SKILL.md files for preset command overrides. @@ -651,6 +690,9 @@ def _register_skills( subdirectories are never created for a non-active, explicitly targeted directory (that creation path is only meaningful for the currently active agent). + only_skill_names: When given, write only these skill + directory names; any other existing modern or legacy + skill for the same command is left untouched. Returns: ``{agent_name: [skill_name, ...]}`` for the single active @@ -736,6 +778,10 @@ def _register_skills( missing_skill_dir = skills_dir / skill_name if not missing_skill_dir.exists(): target_skill_names.append(skill_name) + if only_skill_names is not None: + target_skill_names = [ + name for name in target_skill_names if name in only_skill_names + ] if not target_skill_names: continue diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 7ae833bfb9..9c89ade670 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1897,6 +1897,122 @@ def test_remove_preserves_non_owned_skill_during_reconciliation( "---\nname: speckit-specify\n---\n\nUser-owned body\n" ) + def _install_composed_specify_stack(self, project_dir, temp_dir, layers): + """Install ``(preset_id, body, strategy, priority)`` layers for speckit.specify.""" + self._write_init_options(project_dir, ai="claude", ai_skills=True) + self._create_skill(project_dir / ".claude" / "skills", "speckit-specify") + (project_dir / ".specify" / "templates" / "commands" / "specify.md").write_text( + "---\ndescription: Core specify\n---\n\nCore specify body\n", + encoding="utf-8", + ) + manager = PresetManager(project_dir) + for preset_id, body, strategy, priority in layers: + preset_dir = self._create_command_preset( + temp_dir, preset_id, "speckit.specify", preset_id, body + ) + if strategy != "replace": + manifest_path = preset_dir / "preset.yml" + manifest = yaml.safe_load(manifest_path.read_text()) + manifest["provides"]["templates"][0]["strategy"] = strategy + manifest_path.write_text(yaml.dump(manifest)) + manager.install_from_directory(preset_dir, "0.1.5", priority=priority) + return manager, project_dir / ".claude" / "skills" / "speckit-specify" / "SKILL.md" + + def test_remove_base_preset_recomposes_surviving_composed_skill( + self, project_dir, temp_dir + ): + """Removing a composition's base layer must recompose the surviving skill. + + The appending preset owns the composed SKILL.md, so removing the + replace preset beneath it restored nothing in the active skills + directory and reconciliation skipped it: the skill kept the removed + preset's body while command-file agents were recomposed onto core. + """ + manager, skill_file = self._install_composed_specify_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("append-preset", "Appended body", "append", 5), + ], + ) + content = skill_file.read_text(encoding="utf-8") + assert "Base preset body" in content and "Appended body" in content + + assert manager.remove("base-preset") is True + + content = skill_file.read_text(encoding="utf-8") + assert "Base preset body" not in content + assert "Core specify body" in content + assert "Appended body" in content + assert "preset:append-preset" in content + + def test_remove_middle_preset_drops_its_layer_from_composed_skill( + self, project_dir, temp_dir + ): + """Removing a middle composing layer must drop its fragment from the skill.""" + manager, skill_file = self._install_composed_specify_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("middle-preset", "Middle body", "append", 7), + ("top-preset", "Top body", "append", 5), + ], + ) + assert "Middle body" in skill_file.read_text(encoding="utf-8") + + assert manager.remove("middle-preset") is True + + content = skill_file.read_text(encoding="utf-8") + assert "Middle body" not in content + assert "Base preset body" in content + assert "Top body" in content + + def test_remove_base_preset_preserves_hand_edited_composed_skill( + self, project_dir, temp_dir + ): + """Recomposition after removal must not overwrite a hand-edited skill.""" + manager, skill_file = self._install_composed_specify_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("append-preset", "Appended body", "append", 5), + ], + ) + edited = "---\nname: speckit-specify\n---\n\nUser-owned body\n" + skill_file.write_text(edited, encoding="utf-8") + + assert manager.remove("base-preset") is True + + assert skill_file.read_text(encoding="utf-8") == edited + + def test_remove_base_preset_preserves_user_owned_legacy_skill( + self, project_dir, temp_dir + ): + """Recomposition writes only the skill name that still carries the + winning preset's marker, not a user-owned legacy sibling.""" + manager, skill_file = self._install_composed_specify_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("append-preset", "Appended body", "append", 5), + ], + ) + legacy_file = skill_file.parent.parent / "speckit.specify" / "SKILL.md" + legacy_file.parent.mkdir(parents=True, exist_ok=True) + legacy = "---\nname: speckit.specify\n---\n\nUser-owned legacy body\n" + legacy_file.write_text(legacy, encoding="utf-8") + + assert manager.remove("base-preset") is True + + assert legacy_file.read_text(encoding="utf-8") == legacy + content = skill_file.read_text(encoding="utf-8") + assert "Base preset body" not in content + assert "Appended body" in content + def test_shared_skills_dir_restored_once_using_active_agent( self, project_dir, temp_dir ):