From 1239cbeefcb52dc436daa37e3ccee57236581597 Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Sun, 18 Oct 2026 00:20:17 +0530 Subject: [PATCH 1/8] fix(presets): recompose surviving skills when a lower preset layer is removed Removing a preset that is the base (or a middle layer) of another preset's prepend/append/wrap composition left the removed preset's content in the active agent's SKILL.md (Claude, Codex, Copilot with --skills, ...), while command-file agents such as Gemini were recomposed onto the new stack. The composed skill is owned by the surviving preset at the top of the chain, so _unregister_skills() restores nothing for the removed preset, and _reconcile_skills() only revisited the active skills directory when that restore had touched it. The recomposed .composed/.md that _reconcile_composed_commands() writes was therefore never rendered into the skill, and `specify preset resolve` disagreed with what the agent ran. After a removal, also reconcile active-directory skills for commands whose winning layer composes, as long as the skill still carries the winning preset's source marker. A hand-edited skill is left alone, matching the existing protection for non-owned skills during removal. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 45 +++++++-- .../presets/test_manager_skills.py | 91 +++++++++++++++++++ 2 files changed, 130 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 5a4a4fb739..f58beef2af 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: @@ -394,16 +397,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 +426,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 diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 7ae833bfb9..31387b80bf 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1897,6 +1897,97 @@ 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_shared_skills_dir_restored_once_using_active_agent( self, project_dir, temp_dir ): From 8c0328f1132f275351c575e94f6cbeb35dac3485 Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:53:32 +0530 Subject: [PATCH 2/8] fix(presets): limit removal recomposition to owned skill names When both the modern (speckit-specify) and legacy (speckit.specify) skill directories exist, the active-directory recomposition added for lower-layer removal selected the command because the modern skill kept the winning preset's marker, and _register_skills() then rewrote both directories, overwriting a user-owned legacy skill. Give _register_skills() an optional only_skill_names filter and pass the directory's managed names on removal, so only names that are still preset-owned are written. Install and rescaffold calls pass None and keep writing every existing directory, as before. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 15 ++++++++++- .../presets/test_manager_skills.py | 25 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index f58beef2af..b170f4ecdc 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -365,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, @@ -659,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. @@ -684,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 @@ -769,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 31387b80bf..9c89ade670 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1988,6 +1988,31 @@ def test_remove_base_preset_preserves_hand_edited_composed_skill( 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 ): From 57bdfc9b9d8cd250c76569d6163af180957591f5 Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Mon, 19 Oct 2026 19:48:40 +0530 Subject: [PATCH 3/8] fix(presets): retire a composed skill whose only base was removed When the removed preset was the only replace base of a composing command, with no core or extension fallback, resolution returns None. Command-file agents unregister that command, but the survivor's stale .composed/.md stayed behind, and the removal recomposition rendered it back into the skill, so the removed preset's instructions stayed active. Track composing winners whose stack no longer resolves, delete their stale composed cache, and on removal delete the active skill only while it still carries the winning preset's source marker. Hand-edited skills are kept, matching the existing ownership protection. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 23 ++++++++- .../presets/test_manager_skills.py | 49 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index b170f4ecdc..ea4d23720a 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -152,6 +152,9 @@ def _reconcile_skills( # every affected directory below. preset_cmds: Dict[str, List[str]] = {} composed_winners: Dict[str, str] = {} + # Composing winners whose stack no longer has a base (resolution + # returns None): their .composed cache and skill are stale. + unresolved_winners: Dict[str, str] = {} non_preset_skills: List[tuple] = [] managed_skill_names: set = set() reconciled_skill_commands: set[str] = set() @@ -197,7 +200,15 @@ def _reconcile_skills( 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 + if resolver.resolve_content(cmd_name, "command") is None: + # Nothing left to compose onto: drop the stale + # cache so it can't be rendered back into a skill. + unresolved_winners[cmd_name] = pack_id + stale = pack_dir / ".composed" / f"{cmd_name}.md" + if stale.is_file() and not stale.is_symlink(): + stale.unlink() + else: + composed_winners[cmd_name] = pack_id found_preset = True break if not found_preset: @@ -411,6 +422,16 @@ def apply_to_dir( active_names |= self._skills_owned_by_preset( active_skills_dir, composed_winners ) + # Command-file agents unregister an uncomposable command; + # retire its skill too while it still carries the marker. + for stale_skill in self._skills_owned_by_preset( + active_skills_dir, unresolved_winners + ): + stale_dir = active_skills_dir / stale_skill + if self._validate_skill_subdir( + stale_dir, create=False, skills_root=active_skills_dir + ): + shutil.rmtree(stale_dir) if active_names is None or active_names: apply_to_dir( active_skills_dir, diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 9c89ade670..01601459c1 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1918,6 +1918,55 @@ def _install_composed_specify_stack(self, project_dir, temp_dir, layers): manager.install_from_directory(preset_dir, "0.1.5", priority=priority) return manager, project_dir / ".claude" / "skills" / "speckit-specify" / "SKILL.md" + def _install_unbacked_custom_stack(self, project_dir, temp_dir): + """Base + append presets for ``speckit.custom``, which has no core template.""" + self._write_init_options(project_dir, ai="claude", ai_skills=True) + self._create_skill(project_dir / ".claude" / "skills", "speckit-custom") + manager = PresetManager(project_dir) + for preset_id, body, strategy, priority in [ + ("base-preset", "Base preset body", "replace", 10), + ("append-preset", "Appended body", "append", 5), + ]: + preset_dir = self._create_command_preset( + temp_dir, preset_id, "speckit.custom", 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) + skill_file = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" + assert "Base preset body" in skill_file.read_text(encoding="utf-8") + return manager, skill_file + + def test_remove_sole_base_preset_retires_stale_composed_skill( + self, project_dir, temp_dir + ): + """With no core or extension fallback, removing the only base leaves + nothing to compose onto: the composed skill and its cache must not + keep serving the removed preset's instructions.""" + manager, skill_file = self._install_unbacked_custom_stack(project_dir, temp_dir) + cache = manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + + assert not skill_file.exists() + assert not cache.exists() + + def test_remove_sole_base_preset_keeps_hand_edited_skill( + self, project_dir, temp_dir + ): + manager, skill_file = self._install_unbacked_custom_stack(project_dir, temp_dir) + edited = "---\nname: speckit-custom\n---\n\nUser-owned body\n" + skill_file.write_text(edited, encoding="utf-8") + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + + assert skill_file.read_text(encoding="utf-8") == edited + def test_remove_base_preset_recomposes_surviving_composed_skill( self, project_dir, temp_dir ): From c184e1a5345f388e1a2204873ccd64cf49b1bd20 Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Mon, 19 Oct 2026 20:28:29 +0530 Subject: [PATCH 4/8] fix(presets): guard, widen and journal stale composed-cache deletion Round-3 review of the no-base removal fix: - Delete a cache only when no path component below the project root is a symlink, so a symlinked .composed directory can't redirect the deletion to a file outside the project. - Drop every surviving preset's .composed/.md for an uncomposable command, not only the winner's: a middle preset's stale cache could be rendered back into the skill by a later rescaffold. - Delete through the journal-aware unlink helper, as the cache writes already do, so a failing integration lifecycle can restore the cache. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 38 +++++++++- .../presets/test_manager_skills.py | 70 ++++++++++++++++++- 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index ea4d23720a..d7a2ec323f 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -204,9 +204,9 @@ def _reconcile_skills( # Nothing left to compose onto: drop the stale # cache so it can't be rendered back into a skill. unresolved_winners[cmd_name] = pack_id - stale = pack_dir / ".composed" / f"{cmd_name}.md" - if stale.is_file() and not stale.is_symlink(): - stale.unlink() + self._drop_stale_composed_caches( + cmd_name, presets_by_priority + ) else: composed_winners[cmd_name] = pack_id found_preset = True @@ -452,6 +452,38 @@ def apply_to_dir( return reconciled_skill_commands + def _drop_stale_composed_caches( + self, cmd_name: str, presets_by_priority: List[Any] + ) -> None: + """Delete every preset's ``.composed/.md`` for an uncomposable command. + + With no base left, any preset's cached composition of ``cmd_name`` + still holds the removed layer and could be rendered back by a later + registration. A cache reached through a symlinked ancestor below the + project root is skipped, so nothing outside the project is deleted; + the deletion is journaled like the cache writes. + """ + from ..integrations._file_changes import unlink as _unlink_file + + for pack_id, _meta in presets_by_priority: + stale = self.presets_dir / pack_id / ".composed" / f"{cmd_name}.md" + if not stale.is_file() or not self._has_no_symlink_below_project(stale): + continue + _unlink_file(stale) + + def _has_no_symlink_below_project(self, path: Path) -> bool: + """True if ``path`` is under the project root with no symlinked component.""" + try: + path.relative_to(self.project_root) + except ValueError: + return False + current = path + while current != self.project_root: + if current.is_symlink(): + return False + current = current.parent + return True + def _skills_owned_by_preset( self, skills_dir: Path, owners: Dict[str, str] ) -> Set[str]: diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 01601459c1..86dea134ff 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1918,12 +1918,12 @@ def _install_composed_specify_stack(self, project_dir, temp_dir, layers): manager.install_from_directory(preset_dir, "0.1.5", priority=priority) return manager, project_dir / ".claude" / "skills" / "speckit-specify" / "SKILL.md" - def _install_unbacked_custom_stack(self, project_dir, temp_dir): - """Base + append presets for ``speckit.custom``, which has no core template.""" + def _install_unbacked_custom_stack(self, project_dir, temp_dir, layers=None): + """Presets for ``speckit.custom``, which has no core template.""" self._write_init_options(project_dir, ai="claude", ai_skills=True) self._create_skill(project_dir / ".claude" / "skills", "speckit-custom") manager = PresetManager(project_dir) - for preset_id, body, strategy, priority in [ + for preset_id, body, strategy, priority in layers or [ ("base-preset", "Base preset body", "replace", 10), ("append-preset", "Appended body", "append", 5), ]: @@ -1967,6 +1967,70 @@ def test_remove_sole_base_preset_keeps_hand_edited_skill( assert skill_file.read_text(encoding="utf-8") == edited + def test_remove_sole_base_preset_keeps_cache_behind_symlinked_dir( + self, project_dir, temp_dir + ): + """A ``.composed`` directory that links outside the project is not + followed when stale caches are deleted.""" + manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) + composed = manager.presets_dir / "append-preset" / ".composed" + external = temp_dir / "external-composed" + shutil.move(str(composed), str(external)) + composed.symlink_to(external, target_is_directory=True) + target = external / "speckit.custom.md" + assert target.is_file() + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + + assert target.is_file() + + def test_remove_sole_base_under_two_composers_survives_rescaffold( + self, project_dir, temp_dir + ): + """Every surviving preset's stale cache is dropped, so a later + rescaffold cannot render the removed base back into the skill.""" + manager, skill_file = self._install_unbacked_custom_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("middle-preset", "Middle body", "append", 7), + ("top-preset", "Top body", "append", 5), + ], + ) + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + manager.register_enabled_presets_for_agent("claude") + + for preset_id in ("middle-preset", "top-preset"): + assert not ( + manager.presets_dir / preset_id / ".composed" / "speckit.custom.md" + ).exists() + assert not skill_file.exists() or "Base preset body" not in skill_file.read_text( + encoding="utf-8" + ) + + def test_stale_cache_deletion_is_journaled(self, project_dir, temp_dir): + """The cache deletion is reported to the lifecycle file-change journal.""" + from specify_cli.integrations._file_changes import file_change_observer + + manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) + cache = manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" + events = [] + token = file_change_observer.set( + lambda path, before, removal: events.append((path, before, removal)) + ) + try: + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + finally: + file_change_observer.reset(token) + + assert (cache, True, True) in events + assert not cache.exists() + def test_remove_base_preset_recomposes_surviving_composed_skill( self, project_dir, temp_dir ): From b9a2c359ccac538fef80b8a60e635f39ef60f76d Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Mon, 19 Oct 2026 22:52:52 +0530 Subject: [PATCH 5/8] fix(presets): harden no-base cleanup against unsafe keys and read failures Round-4 review of the no-base removal fix: - Skip registry keys (and command names) that are not a single plain path segment, so a malformed key such as "../../../outside" cannot steer cache cleanup outside the project. - Invalidate stale caches for every installed preset, disabled ones included: re-enabling a disabled composer and rescaffolding could otherwise render its stale cache back. - Retire output only when the stack has no replace layer left. resolve_content() also returns None when a surviving layer can't be read; that keeps the existing cache and skill instead of deleting them. - Journal the stale skill-directory removal like the other skill deletions, so a failing lifecycle can restore it. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 53 +++++++++---- .../presets/test_manager_skills.py | 78 ++++++++++++++++++- 2 files changed, 115 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index d7a2ec323f..7c408468db 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -20,6 +20,16 @@ from ._resolver import PresetResolver +def _is_plain_path_segment(name: Any) -> bool: + """True for a single, non-traversing path component.""" + return ( + isinstance(name, str) + and name not in ("", ".", "..") + and "/" not in name + and "\\" not in name + ) + + class _PresetSkillMethods: """Skill artifact methods shared through PresetManager's lifecycle state.""" @@ -200,15 +210,17 @@ def _reconcile_skills( if top_path.is_relative_to(pack_dir): preset_cmds.setdefault(pack_id, []).append(cmd_name) if layers[0]["strategy"] != "replace": - if resolver.resolve_content(cmd_name, "command") is None: + if resolver.resolve_content(cmd_name, "command") is not None: + composed_winners[cmd_name] = pack_id + elif not any( + layer["strategy"] == "replace" for layer in layers + ): # Nothing left to compose onto: drop the stale - # cache so it can't be rendered back into a skill. + # caches so they can't be rendered back into a + # skill. A None from an unreadable layer, with a + # base still present, keeps existing output. unresolved_winners[cmd_name] = pack_id - self._drop_stale_composed_caches( - cmd_name, presets_by_priority - ) - else: - composed_winners[cmd_name] = pack_id + self._drop_stale_composed_caches(cmd_name) found_preset = True break if not found_preset: @@ -431,7 +443,14 @@ def apply_to_dir( if self._validate_skill_subdir( stale_dir, create=False, skills_root=active_skills_dir ): + from ..integrations._file_changes import ( + after_file_change, + before_file_change, + ) + + before_file_change(stale_dir, removal=True) shutil.rmtree(stale_dir) + after_file_change(stale_dir) if active_names is None or active_names: apply_to_dir( active_skills_dir, @@ -452,20 +471,24 @@ def apply_to_dir( return reconciled_skill_commands - def _drop_stale_composed_caches( - self, cmd_name: str, presets_by_priority: List[Any] - ) -> None: - """Delete every preset's ``.composed/.md`` for an uncomposable command. + def _drop_stale_composed_caches(self, cmd_name: str) -> None: + """Delete every installed preset's ``.composed/.md`` for an + uncomposable command, disabled presets included. With no base left, any preset's cached composition of ``cmd_name`` still holds the removed layer and could be rendered back by a later - registration. A cache reached through a symlinked ancestor below the - project root is skipped, so nothing outside the project is deleted; - the deletion is journaled like the cache writes. + registration. A registry key or command name with a path separator or + ``..`` component is skipped, as is a cache reached through a symlinked + ancestor below the project root, so nothing outside the project is + deleted; the deletion is journaled like the cache writes. """ from ..integrations._file_changes import unlink as _unlink_file - for pack_id, _meta in presets_by_priority: + for pack_id in self.registry.list(): + if not _is_plain_path_segment(pack_id) or not _is_plain_path_segment( + f"{cmd_name}.md" + ): + continue stale = self.presets_dir / pack_id / ".composed" / f"{cmd_name}.md" if not stale.is_file() or not self._has_no_symlink_below_project(stale): continue diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 86dea134ff..098d19ce9a 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1937,7 +1937,8 @@ def _install_unbacked_custom_stack(self, project_dir, temp_dir, layers=None): manifest_path.write_text(yaml.dump(manifest)) manager.install_from_directory(preset_dir, "0.1.5", priority=priority) skill_file = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" - assert "Base preset body" in skill_file.read_text(encoding="utf-8") + if layers is None: + assert "Base preset body" in skill_file.read_text(encoding="utf-8") return manager, skill_file def test_remove_sole_base_preset_retires_stale_composed_skill( @@ -2030,6 +2031,81 @@ def test_stale_cache_deletion_is_journaled(self, project_dir, temp_dir): assert (cache, True, True) in events assert not cache.exists() + skill_dir = project_dir / ".claude" / "skills" / "speckit-custom" + assert (skill_dir, True, True) in events + assert not skill_dir.exists() + + def test_stale_cache_cleanup_ignores_traversing_registry_key( + self, project_dir, temp_dir + ): + """A malformed registry key with ``..`` must not reach outside the project.""" + manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) + outside = (manager.presets_dir / "../../../outside").resolve() + target = outside / ".composed" / "speckit.custom.md" + target.parent.mkdir(parents=True) + target.write_text("external\n", encoding="utf-8") + manager.registry.add("../../../outside", {"version": "1.0.0", "enabled": True, "priority": 20}) + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + + assert target.read_text(encoding="utf-8") == "external\n" + + def test_remove_sole_base_drops_disabled_preset_cache( + self, project_dir, temp_dir + ): + """A disabled composer's stale cache is dropped too, so re-enabling it + and rescaffolding cannot render the removed base back.""" + manager, skill_file = self._install_unbacked_custom_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("middle-preset", "Middle body", "append", 7), + ("top-preset", "Top body", "append", 5), + ], + ) + manager.registry.update("middle-preset", {"enabled": False}) + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + manager.registry.update("middle-preset", {"enabled": True}) + manager.register_enabled_presets_for_agent("claude") + + assert not ( + manager.presets_dir / "middle-preset" / ".composed" / "speckit.custom.md" + ).exists() + assert not skill_file.exists() or "Base preset body" not in skill_file.read_text( + encoding="utf-8" + ) + + def test_unreadable_surviving_base_keeps_composed_output( + self, project_dir, temp_dir + ): + """Resolution also fails when a surviving base can't be decoded; that + must not be treated as a missing base that retires the output.""" + manager, skill_file = self._install_unbacked_custom_stack( + project_dir, + temp_dir, + [ + ("base-preset", "Base preset body", "replace", 10), + ("other-base", "Other base body", "replace", 8), + ("append-preset", "Appended body", "append", 5), + ], + ) + cache = manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" + skill_before = skill_file.read_text(encoding="utf-8") + assert cache.is_file() and "Other base body" in skill_before + manifest = yaml.safe_load( + (manager.presets_dir / "other-base" / "preset.yml").read_text() + ) + command_file = manager.presets_dir / "other-base" / manifest["provides"]["templates"][0]["file"] + command_file.write_bytes(b"---\ndescription: x\n---\n\xff\xfe broken\n") + + manager.remove("base-preset") + + assert cache.is_file() + assert skill_file.read_text(encoding="utf-8") == skill_before def test_remove_base_preset_recomposes_surviving_composed_skill( self, project_dir, temp_dir From b38ba5317d85852f1d61a75fac21fccf0e61f49f Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Mon, 19 Oct 2026 23:39:48 +0530 Subject: [PATCH 6/8] fix(presets): reject junctions in cache cleanup, filter historical writes Round-5 review of the removal reconciliation: - Treat Windows reparse points (directory junctions, which Path.is_symlink() misses) like symlinks when checking a stale cache's ancestors, mirroring FileHelper._entry_mode. - Pass the directory's managed names to _register_skills() for historical (inactive) agent directories too, so a user-owned modern/legacy sibling there is not overwritten. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 19 +++++- .../presets/test_manager_skills.py | 64 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 7c408468db..982242453a 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -3,6 +3,7 @@ import copy import os import shutil +import stat from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Union @@ -20,6 +21,21 @@ from ._resolver import PresetResolver +def _is_link_or_reparse_point(path: Path) -> bool: + """True for a symlink or a Windows reparse point such as a directory junction. + + ``Path.is_symlink()`` does not report junctions; mirror the check in + ``FileHelper._entry_mode``. + """ + try: + entry = path.lstat() + except OSError: + return False + return stat.S_ISLNK(entry.st_mode) or bool( + getattr(entry, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT + ) + + def _is_plain_path_segment(name: Any) -> bool: """True for a single, non-traversing path component.""" return ( @@ -398,6 +414,7 @@ def apply_to_dir( written = self._register_skills( filtered_manifest, pack_dir, target_dir=skills_dir, target_agent=dir_agent or "", + only_skill_names=managed_names, ) if target_agent is None: written_names = { @@ -502,7 +519,7 @@ def _has_no_symlink_below_project(self, path: Path) -> bool: return False current = path while current != self.project_root: - if current.is_symlink(): + if _is_link_or_reparse_point(current): return False current = current.parent return True diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 098d19ce9a..dd2385b046 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -2035,6 +2035,34 @@ def test_stale_cache_deletion_is_journaled(self, project_dir, temp_dir): assert (skill_dir, True, True) in events assert not skill_dir.exists() + def test_stale_cache_cleanup_rejects_junctioned_composed_dir( + self, project_dir, temp_dir, monkeypatch + ): + """A ``.composed`` directory reported as a Windows reparse point (a + junction, which ``is_symlink()`` misses) is not followed.""" + from types import SimpleNamespace + + manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) + composed = manager.presets_dir / "append-preset" / ".composed" + cache = composed / "speckit.custom.md" + lstat = Path.lstat + + def junction_lstat(path, **kwargs): + result = lstat(path, **kwargs) + if path == composed: + return SimpleNamespace( + st_mode=result.st_mode, + st_file_attributes=0x400, + st_reparse_tag=0xA0000003, + ) + return result + + monkeypatch.setattr(Path, "lstat", junction_lstat) + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + + assert cache.is_file() + def test_stale_cache_cleanup_ignores_traversing_registry_key( self, project_dir, temp_dir ): @@ -2408,6 +2436,42 @@ def test_remove_reconciles_skill_for_every_historical_agent( "the currently active agent" ) + def test_remove_keeps_user_owned_legacy_skill_in_historical_dir( + self, project_dir, temp_dir + ): + """Reconciling an inactive agent's directory writes only the managed + skill name, not a user-owned legacy sibling beside it.""" + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + core_cmds = project_dir / ".specify" / "templates" / "commands" + core_cmds.mkdir(parents=True, exist_ok=True) + (core_cmds / "specify.md").write_text( + "---\ndescription: Core specify command\n---\n\nCore specify body\n", + encoding="utf-8", + ) + preset_b_dir = self._create_command_preset( + temp_dir, "hist-legacy-b", "speckit.specify", "Preset B", "preset B body", + ) + preset_a_dir = self._create_command_preset( + temp_dir, "hist-legacy-a", "speckit.specify", "Preset A", "preset A body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_b_dir, "0.1.5", priority=10) + manager.install_from_directory(preset_a_dir, "0.1.5", priority=1) + self._write_init_options(project_dir, ai="codex", ai_skills=True) + manager.register_enabled_presets_for_agent("codex") + legacy_file = claude_skills_dir / "speckit.specify" / "SKILL.md" + legacy_file.parent.mkdir(parents=True) + legacy = "---\nname: speckit.specify\n---\n\nUser-owned legacy body\n" + legacy_file.write_text(legacy, encoding="utf-8") + + assert manager.remove("hist-legacy-a") is True + + assert legacy_file.read_text(encoding="utf-8") == legacy + assert "preset:hist-legacy-b" in ( + claude_skills_dir / "speckit-specify" / "SKILL.md" + ).read_text() + def test_skill_reconciliation_preserves_per_directory_names( self, project_dir, temp_dir ): From 3738814214c781fc750172d343f8f0899503d9ed Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Tue, 20 Oct 2026 00:56:39 +0530 Subject: [PATCH 7/8] fix(presets): reconcile composed skills on every path, historical dirs and aliases Round-6 review ("previously missed" items): - Never render a cached composition whose stack has no base, even when the cache file exists: with the sole composer disabled during removal its stale cache survived, and re-enabling it rendered the removed base. Retirement of still-owned skills for an uncomposable command now runs on every reconciliation (activation and rescaffold), not only removal. - On removal, also recompose or retire a surviving composer's still-owned skills in its recorded historical (inactive) agent directories, resolved through _safe_skills_dir_for_agent(). - Treat a command's manifest aliases as part of its skills: ownership accepts the registrar's ":" alias marker as well as "preset:", targeted reconciliation re-renders owned alias skills, and retirement removes them. Hand-edited aliases are kept. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 131 ++++++++++++++---- .../presets/test_manager_skills.py | 107 ++++++++++++++ 2 files changed, 214 insertions(+), 24 deletions(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 982242453a..0ffc8d4d22 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -366,7 +366,7 @@ def apply_to_dir( for cmd in cmds if any( name in dir_managed_names - for name in self._skill_names_for_command(cmd) + for name in self._command_skill_names(cmd, pack_id) ) ] if not dir_cmds: @@ -390,7 +390,7 @@ def apply_to_dir( # the same symlink guard — or the surviving preset's # override would be silently skipped (#2948). for cmd_name in dir_cmds: - for skill_name in self._skill_names_for_command(cmd_name): + for skill_name in self._command_skill_names(cmd_name, pack_id): if skill_name not in dir_managed_names: continue skill_subdir = skills_dir / skill_name @@ -439,8 +439,24 @@ def apply_to_dir( # reconciliation just wrote to on its behalf (#2948). self._merge_pack_registered_skills(pack_id, written) - extra_dirs = extra_skills_dirs or {} + extra_dirs = dict(extra_skills_dirs or {}) + if extra_skills_dirs is not None: + # Removal restores only what the removed preset still owned. A + # surviving composer's skills in historical (inactive) agent + # directories are its own, so recompose or retire them there too. + for dir_path, agent, owned_composed in self._historical_owned_skills( + composed_winners, unresolved_winners, active_ai, active_skills_dir + ): + prior_agent, prior_names = extra_dirs.get(dir_path, (agent, [])) + extra_dirs[dir_path] = ( + prior_agent, list(dict.fromkeys([*prior_names, *owned_composed])) + ) if active_skills_dir: + # Command-file agents unregister an uncomposable command; retire + # its skills too while they still carry the composer's marker. + # This runs on every reconciliation (activation and rescaffold + # too), so a stale skill can't outlive its base. + self._retire_owned_skills(active_skills_dir, unresolved_winners) active_provenance = extra_dirs.get(active_skills_dir) active_names: Optional[Set[str]] = None if extra_skills_dirs is not None: @@ -451,23 +467,6 @@ def apply_to_dir( active_names |= self._skills_owned_by_preset( active_skills_dir, composed_winners ) - # Command-file agents unregister an uncomposable command; - # retire its skill too while it still carries the marker. - for stale_skill in self._skills_owned_by_preset( - active_skills_dir, unresolved_winners - ): - stale_dir = active_skills_dir / stale_skill - if self._validate_skill_subdir( - stale_dir, create=False, skills_root=active_skills_dir - ): - from ..integrations._file_changes import ( - after_file_change, - before_file_change, - ) - - before_file_change(stale_dir, removal=True) - shutil.rmtree(stale_dir) - after_file_change(stale_dir) if active_names is None or active_names: apply_to_dir( active_skills_dir, @@ -524,17 +523,85 @@ def _has_no_symlink_below_project(self, path: Path) -> bool: current = current.parent return True + def _command_skill_names(self, cmd_name: str, pack_id: str) -> List[str]: + """Modern and legacy skill names for ``cmd_name`` and its aliases in ``pack_id``.""" + names = list(self._skill_names_for_command(cmd_name)) + manifest_path = self.presets_dir / pack_id / "preset.yml" + try: + manifest = PresetManifest(manifest_path) + except (PresetValidationError, OSError): + return names + for tmpl in manifest.templates: + if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": + for alias in tmpl.get("aliases") or []: + if isinstance(alias, str): + names.extend(self._skill_names_for_command(alias)) + break + return list(dict.fromkeys(names)) + + def _retire_owned_skills(self, skills_dir: Path, owners: Dict[str, str]) -> None: + """Delete skills in ``skills_dir`` still owned by their composer (journaled).""" + from ..integrations._file_changes import after_file_change, before_file_change + + for stale_skill in self._skills_owned_by_preset(skills_dir, owners): + stale_dir = skills_dir / stale_skill + if self._validate_skill_subdir( + stale_dir, create=False, skills_root=skills_dir + ): + before_file_change(stale_dir, removal=True) + shutil.rmtree(stale_dir) + after_file_change(stale_dir) + + def _historical_owned_skills( + self, + composed_winners: Dict[str, str], + unresolved_winners: Dict[str, str], + active_ai: Optional[str], + active_skills_dir: Optional[Path], + ) -> List[tuple]: + """Recompose targets in surviving composers' historical skill directories. + + Retires the composers' still-owned skills for uncomposable commands in + those directories, and returns ``(dir, agent, owned_names)`` for each + directory holding still-owned skills of composable commands. + """ + targets: List[tuple] = [] + for pack_id in dict.fromkeys( + [*composed_winners.values(), *unresolved_winners.values()] + ): + meta = self.registry.get(pack_id) or {} + recorded = meta.get("registered_skills") + if not isinstance(recorded, dict): + continue + for agent in recorded: + if not isinstance(agent, str) or agent == active_ai: + continue + skills_dir = self._safe_skills_dir_for_agent(agent) + if skills_dir is None or skills_dir == active_skills_dir: + continue + self._retire_owned_skills(skills_dir, { + cmd: pid for cmd, pid in unresolved_winners.items() if pid == pack_id + }) + owned = self._skills_owned_by_preset(skills_dir, { + cmd: pid for cmd, pid in composed_winners.items() if pid == pack_id + }) + if owned: + targets.append((skills_dir, agent, sorted(owned))) + return targets + 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. + ``owners`` maps command names to the expected owning preset id. Alias + skills count too; the registrar marks those ``:`` rather + than ``preset:``. """ 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): + for skill_name in self._command_skill_names(cmd_name, pack_id): skill_file = skills_dir / skill_name / "SKILL.md" if not self._validate_skill_subdir( skill_file.parent, create=False, skills_root=skills_dir @@ -545,7 +612,10 @@ def _skills_owned_by_preset( except (OSError, UnicodeDecodeError): continue metadata = registrar.parse_frontmatter(content)[0].get("metadata") - if isinstance(metadata, dict) and metadata.get("source") == f"preset:{pack_id}": + source = metadata.get("source") if isinstance(metadata, dict) else None + if isinstance(source, str) and ( + source == f"preset:{pack_id}" or source.startswith(f"{pack_id}:") + ): owned.add(skill_name) return owned @@ -872,6 +942,17 @@ def _register_skills( if not missing_skill_dir.exists(): target_skill_names.append(skill_name) if only_skill_names is not None: + # Targeted reconciliation also refreshes the command's alias + # skills it may manage, so they don't keep a removed layer. + for alias in cmd_tmpl.get("aliases") or []: + if not isinstance(alias, str): + continue + for alias_skill in self._skill_names_for_command(alias): + if ( + alias_skill not in target_skill_names + and (skills_dir / alias_skill).is_dir() + ): + target_skill_names.append(alias_skill) target_skill_names = [ name for name in target_skill_names if name in only_skill_names ] @@ -898,9 +979,11 @@ def _register_skills( or frontmatter.get("strategy") or "replace" ) + # A cached composition is not proof of a base either: it can be a + # stale leftover of a removed base (e.g. while its composer was + # disabled), so check resolution even when the cache exists. if ( effective_strategy != "replace" - and not composed_file.exists() and resolver.resolve_content(cmd_name, "command") is None ): continue diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index dd2385b046..34f7aed758 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -2063,6 +2063,113 @@ def junction_lstat(path, **kwargs): assert cache.is_file() + def _install_custom_layers(self, project_dir, temp_dir, *, core=False, alias=None): + """Base (replace) + append presets for ``speckit.custom`` under Claude.""" + self._write_init_options(project_dir, ai="claude", ai_skills=True) + self._create_skill(project_dir / ".claude" / "skills", "speckit-custom") + if core: + core_cmds = project_dir / ".specify" / "templates" / "commands" + core_cmds.mkdir(parents=True, exist_ok=True) + (core_cmds / "custom.md").write_text( + "---\ndescription: Core custom\n---\n\nCore custom body\n", + encoding="utf-8", + ) + manager = PresetManager(project_dir) + for preset_id, body, strategy, priority in [ + ("base-preset", "Base preset body", "replace", 10), + ("append-preset", "Appended body", "append", 5), + ]: + preset_dir = self._create_command_preset( + temp_dir, preset_id, "speckit.custom", preset_id, body + ) + manifest_path = preset_dir / "preset.yml" + manifest = yaml.safe_load(manifest_path.read_text()) + manifest["provides"]["templates"][0]["strategy"] = strategy + if alias and strategy != "replace": + manifest["provides"]["templates"][0]["aliases"] = [alias] + manifest_path.write_text(yaml.dump(manifest)) + manager.install_from_directory(preset_dir, "0.1.5", priority=priority) + return manager + + def test_reenabled_sole_composer_does_not_render_removed_base( + self, project_dir, temp_dir + ): + """The only composer is disabled while its base is removed; re-enabling + and rescaffolding must not render its stale cache, and the old skill is + retired once nothing is left to compose onto.""" + manager = self._install_custom_layers(project_dir, temp_dir) + skill = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" + manager.registry.update("append-preset", {"enabled": False}) + manager.remove("base-preset") + manager.registry.update("append-preset", {"enabled": True}) + + with pytest.warns(UserWarning, match="no base layer"): + manager.register_enabled_presets_for_agent("claude") + + assert not skill.exists() or "Base preset body" not in skill.read_text( + encoding="utf-8" + ) + assert not ( + manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" + ).exists() + + @pytest.mark.parametrize("core", [False, True], ids=["no-fallback", "core-fallback"]) + def test_remove_base_reconciles_historical_agent_skill( + self, project_dir, temp_dir, core + ): + """A composer's skill in a historical (inactive) agent directory is + recomposed onto the fallback, or retired when there is none.""" + manager = self._install_custom_layers(project_dir, temp_dir, core=core) + claude_skill = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" + self._write_init_options(project_dir, ai="codex", ai_skills=True) + manager.register_enabled_presets_for_agent("codex") + + if core: + assert manager.remove("base-preset") is True + content = claude_skill.read_text(encoding="utf-8") + assert "Base preset body" not in content + assert "Core custom body" in content and "Appended body" in content + else: + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + assert not claude_skill.exists() + + @pytest.mark.parametrize("core", [False, True], ids=["no-fallback", "core-fallback"]) + def test_remove_base_reconciles_native_alias_skill( + self, project_dir, temp_dir, core + ): + """A composer's alias skill is recomposed or retired with the primary.""" + manager = self._install_custom_layers( + project_dir, temp_dir, core=core, alias="speckit.custom-alias" + ) + alias_skill = project_dir / ".claude" / "skills" / "speckit-custom-alias" / "SKILL.md" + assert "Base preset body" in alias_skill.read_text(encoding="utf-8") + + if core: + assert manager.remove("base-preset") is True + content = alias_skill.read_text(encoding="utf-8") + assert "Base preset body" not in content + assert "Core custom body" in content and "Appended body" in content + else: + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + assert not alias_skill.exists() + + def test_remove_sole_base_keeps_hand_edited_alias_skill( + self, project_dir, temp_dir + ): + manager = self._install_custom_layers( + project_dir, temp_dir, alias="speckit.custom-alias" + ) + alias_skill = project_dir / ".claude" / "skills" / "speckit-custom-alias" / "SKILL.md" + edited = "---\nname: speckit-custom-alias\n---\n\nUser-owned alias\n" + alias_skill.write_text(edited, encoding="utf-8") + + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + + assert alias_skill.read_text(encoding="utf-8") == edited + def test_stale_cache_cleanup_ignores_traversing_registry_key( self, project_dir, temp_dir ): From a9a200021188d2c2b9e921bba8533bdbbdac4718 Mon Sep 17 00:00:00 2001 From: yxshee <84263676+yxshee@users.noreply.github.com> Date: Tue, 20 Oct 2026 19:09:35 +0530 Subject: [PATCH 8/8] refactor(presets): rework removal reconciliation around registration records Rework the fix as requested in review: reconcile the destinations the registry already tracks instead of adding removal-specific cases. - On removal, _unregister_skills() only restores what the removed preset owned. Also collect the affected commands' skill destinations from the surviving presets' registered_skills (inferring per-agent provenance for legacy flat lists), keep those whose SKILL.md still carries that preset's marker, and pass them through the existing extra_skills_dirs reconciliation, so active and historical directories are refreshed from the remaining stack. - When a composing winner's stack has no replace base left, nothing resolves: remove its preset-owned output (journaled) instead of rendering it. Ownership accepts both preset: and the registrar's : marker; anything else is preserved. - _register_skills() lets the resolver decide even when a .composed cache exists, so a stale cache is never rendered. This replaces the cache deletion and its path guards. - Keep the only_skill_names filter so a user-owned modern/legacy sibling is not overwritten. - Drop the alias handling added in an earlier round, which introduced the alias regressions; aliases behave as on main. Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/presets/_manager_skills.py | 323 +++++++----------- .../presets/test_manager_skills.py | 211 +++++------- 2 files changed, 196 insertions(+), 338 deletions(-) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index 0ffc8d4d22..cc4d516be3 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -3,7 +3,6 @@ import copy import os import shutil -import stat from pathlib import Path from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Union @@ -21,31 +20,6 @@ from ._resolver import PresetResolver -def _is_link_or_reparse_point(path: Path) -> bool: - """True for a symlink or a Windows reparse point such as a directory junction. - - ``Path.is_symlink()`` does not report junctions; mirror the check in - ``FileHelper._entry_mode``. - """ - try: - entry = path.lstat() - except OSError: - return False - return stat.S_ISLNK(entry.st_mode) or bool( - getattr(entry, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT - ) - - -def _is_plain_path_segment(name: Any) -> bool: - """True for a single, non-traversing path component.""" - return ( - isinstance(name, str) - and name not in ("", ".", "..") - and "/" not in name - and "\\" not in name - ) - - class _PresetSkillMethods: """Skill artifact methods shared through PresetManager's lifecycle state.""" @@ -177,10 +151,9 @@ 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] = {} - # Composing winners whose stack no longer has a base (resolution - # returns None): their .composed cache and skill are stale. - unresolved_winners: Dict[str, str] = {} + # Commands won by a composing preset whose stack no longer has any + # replace base: nothing resolves, so their owned output is removed. + unresolvable_cmds: Set[str] = set() non_preset_skills: List[tuple] = [] managed_skill_names: set = set() reconciled_skill_commands: set[str] = set() @@ -225,18 +198,11 @@ 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": - if resolver.resolve_content(cmd_name, "command") is not None: - composed_winners[cmd_name] = pack_id - elif not any( - layer["strategy"] == "replace" for layer in layers - ): - # Nothing left to compose onto: drop the stale - # caches so they can't be rendered back into a - # skill. A None from an unreadable layer, with a - # base still present, keeps existing output. - unresolved_winners[cmd_name] = pack_id - self._drop_stale_composed_caches(cmd_name) + if ( + all(layer["strategy"] != "replace" for layer in layers) + and resolver.resolve_content(cmd_name, "command") is None + ): + unresolvable_cmds.add(cmd_name) found_preset = True break if not found_preset: @@ -366,9 +332,21 @@ def apply_to_dir( for cmd in cmds if any( name in dir_managed_names - for name in self._command_skill_names(cmd, pack_id) + for name in self._skill_names_for_command(cmd) ) ] + stale_cmds = [cmd for cmd in dir_cmds if cmd in unresolvable_cmds] + if stale_cmds: + self._remove_owned_skills( + skills_dir, + [ + name + for cmd in stale_cmds + for name in self._skill_names_for_command(cmd) + if name in dir_managed_names + ], + ) + dir_cmds = [cmd for cmd in dir_cmds if cmd not in unresolvable_cmds] if not dir_cmds: continue pack_dir = self.presets_dir / pack_id @@ -390,7 +368,7 @@ def apply_to_dir( # the same symlink guard — or the surviving preset's # override would be silently skipped (#2948). for cmd_name in dir_cmds: - for skill_name in self._command_skill_names(cmd_name, pack_id): + for skill_name in self._skill_names_for_command(cmd_name): if skill_name not in dir_managed_names: continue skill_subdir = skills_dir / skill_name @@ -404,9 +382,6 @@ 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). - # 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, ) @@ -441,38 +416,28 @@ def apply_to_dir( extra_dirs = dict(extra_skills_dirs or {}) if extra_skills_dirs is not None: - # Removal restores only what the removed preset still owned. A - # surviving composer's skills in historical (inactive) agent - # directories are its own, so recompose or retire them there too. - for dir_path, agent, owned_composed in self._historical_owned_skills( - composed_winners, unresolved_winners, active_ai, active_skills_dir - ): - prior_agent, prior_names = extra_dirs.get(dir_path, (agent, [])) - extra_dirs[dir_path] = ( - prior_agent, list(dict.fromkeys([*prior_names, *owned_composed])) + # _unregister_skills only restored what the removed preset owned. + # Skills a surviving preset rendered on top of it are its own, so + # refresh those recorded destinations from the remaining stack too. + for skills_dir, (agent, names) in self._surviving_owned_skills( + command_names, presets_by_priority, active_ai, active_skills_dir + ).items(): + prior_agent, prior_names = extra_dirs.get(skills_dir, (agent, [])) + extra_dirs[skills_dir] = ( + prior_agent, list(dict.fromkeys([*prior_names, *names])) ) if active_skills_dir: - # Command-file agents unregister an uncomposable command; retire - # its skills too while they still carry the composer's marker. - # This runs on every reconciliation (activation and rescaffold - # too), so a stale skill can't outlive its base. - self._retire_owned_skills(active_skills_dir, unresolved_winners) active_provenance = extra_dirs.get(active_skills_dir) - 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: + if extra_skills_dirs is None or active_provenance: apply_to_dir( active_skills_dir, active_ai, is_active=True, - managed_names=active_names, + managed_names=( + set(active_provenance[1]) + if active_provenance + else None + ), ) for extra_dir, (extra_agent, extra_names) in extra_dirs.items(): @@ -487,137 +452,96 @@ def apply_to_dir( return reconciled_skill_commands - def _drop_stale_composed_caches(self, cmd_name: str) -> None: - """Delete every installed preset's ``.composed/.md`` for an - uncomposable command, disabled presets included. - - With no base left, any preset's cached composition of ``cmd_name`` - still holds the removed layer and could be rendered back by a later - registration. A registry key or command name with a path separator or - ``..`` component is skipped, as is a cache reached through a symlinked - ancestor below the project root, so nothing outside the project is - deleted; the deletion is journaled like the cache writes. - """ - from ..integrations._file_changes import unlink as _unlink_file - - for pack_id in self.registry.list(): - if not _is_plain_path_segment(pack_id) or not _is_plain_path_segment( - f"{cmd_name}.md" - ): - continue - stale = self.presets_dir / pack_id / ".composed" / f"{cmd_name}.md" - if not stale.is_file() or not self._has_no_symlink_below_project(stale): - continue - _unlink_file(stale) - - def _has_no_symlink_below_project(self, path: Path) -> bool: - """True if ``path`` is under the project root with no symlinked component.""" - try: - path.relative_to(self.project_root) - except ValueError: - return False - current = path - while current != self.project_root: - if _is_link_or_reparse_point(current): - return False - current = current.parent - return True - - def _command_skill_names(self, cmd_name: str, pack_id: str) -> List[str]: - """Modern and legacy skill names for ``cmd_name`` and its aliases in ``pack_id``.""" - names = list(self._skill_names_for_command(cmd_name)) - manifest_path = self.presets_dir / pack_id / "preset.yml" - try: - manifest = PresetManifest(manifest_path) - except (PresetValidationError, OSError): - return names - for tmpl in manifest.templates: - if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": - for alias in tmpl.get("aliases") or []: - if isinstance(alias, str): - names.extend(self._skill_names_for_command(alias)) - break - return list(dict.fromkeys(names)) - - def _retire_owned_skills(self, skills_dir: Path, owners: Dict[str, str]) -> None: - """Delete skills in ``skills_dir`` still owned by their composer (journaled).""" - from ..integrations._file_changes import after_file_change, before_file_change - - for stale_skill in self._skills_owned_by_preset(skills_dir, owners): - stale_dir = skills_dir / stale_skill - if self._validate_skill_subdir( - stale_dir, create=False, skills_root=skills_dir - ): - before_file_change(stale_dir, removal=True) - shutil.rmtree(stale_dir) - after_file_change(stale_dir) - - def _historical_owned_skills( + def _surviving_owned_skills( self, - composed_winners: Dict[str, str], - unresolved_winners: Dict[str, str], + command_names: List[str], + presets_by_priority: List[Any], active_ai: Optional[str], active_skills_dir: Optional[Path], - ) -> List[tuple]: - """Recompose targets in surviving composers' historical skill directories. + ) -> Dict[Path, tuple[Optional[str], List[str]]]: + """Recorded skill destinations of ``command_names`` still owned by a survivor. - Retires the composers' still-owned skills for uncomposable commands in - those directories, and returns ``(dir, agent, owned_names)`` for each - directory holding still-owned skills of composable commands. + Reads each remaining preset's ``registered_skills`` (inferring per-agent + provenance for legacy flat lists) and keeps the names whose ``SKILL.md`` + still carries that preset's ``preset:`` marker. """ - targets: List[tuple] = [] - for pack_id in dict.fromkeys( - [*composed_winners.values(), *unresolved_winners.values()] - ): - meta = self.registry.get(pack_id) or {} + registrar = self._command_registrar() + affected = { + name + for cmd_name in command_names + for name in self._skill_names_for_command(cmd_name) + } + found: Dict[Path, tuple[Optional[str], List[str]]] = {} + for pack_id, meta in presets_by_priority: + if not isinstance(meta, dict): + continue recorded = meta.get("registered_skills") + if isinstance(recorded, list) and active_ai: + recorded = self._infer_legacy_skill_provenance( + [n for n in recorded if isinstance(n, str)], pack_id, active_ai + ) if not isinstance(recorded, dict): continue - for agent in recorded: - if not isinstance(agent, str) or agent == active_ai: + for agent, names in recorded.items(): + if not isinstance(agent, str) or not isinstance(names, list): continue - skills_dir = self._safe_skills_dir_for_agent(agent) - if skills_dir is None or skills_dir == active_skills_dir: + skills_dir = ( + active_skills_dir + if agent == active_ai + else self._safe_skills_dir_for_agent(agent) + ) + if skills_dir is None: continue - self._retire_owned_skills(skills_dir, { - cmd: pid for cmd, pid in unresolved_winners.items() if pid == pack_id - }) - owned = self._skills_owned_by_preset(skills_dir, { - cmd: pid for cmd, pid in composed_winners.items() if pid == pack_id - }) - if owned: - targets.append((skills_dir, agent, sorted(owned))) - return targets - - 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. Alias - skills count too; the registrar marks those ``:`` rather - than ``preset:``. + for name in names: + if name not in affected: + continue + skill_file = skills_dir / 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}": + dir_agent, dir_names = found.get(skills_dir, (agent, [])) + found[skills_dir] = (dir_agent, [*dir_names, name]) + return found + + def _remove_owned_skills(self, skills_dir: Path, skill_names: List[str]) -> None: + """Delete preset-owned skills among ``skill_names`` (journaled). + + A skill is preset-owned when its ``SKILL.md`` source marker names an + installed preset, either as ``preset:`` (skill registration) or + ``:`` (command registration for native skill agents); + anything else is left untouched. """ + from ..integrations._file_changes import after_file_change, before_file_change + registrar = self._command_registrar() - owned: Set[str] = set() - for cmd_name, pack_id in owners.items(): - for skill_name in self._command_skill_names(cmd_name, pack_id): - 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") - source = metadata.get("source") if isinstance(metadata, dict) else None - if isinstance(source, str) and ( - source == f"preset:{pack_id}" or source.startswith(f"{pack_id}:") - ): - owned.add(skill_name) - return owned + pack_ids = list(self.registry.list()) + for name in dict.fromkeys(skill_names): + skill_dir = skills_dir / name + skill_file = skill_dir / "SKILL.md" + if not self._validate_skill_subdir( + skill_dir, 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") + source = metadata.get("source") if isinstance(metadata, dict) else None + if isinstance(source, str) and any( + source == f"preset:{pack_id}" or source.startswith(f"{pack_id}:") + for pack_id in pack_ids + ): + before_file_change(skill_dir, removal=True) + shutil.rmtree(skill_dir) + after_file_change(skill_dir) def _resolve_agent_skills_dir(self, agent_name: str) -> Path: """Resolve the real skill output directory for an integration.""" @@ -942,17 +866,6 @@ def _register_skills( if not missing_skill_dir.exists(): target_skill_names.append(skill_name) if only_skill_names is not None: - # Targeted reconciliation also refreshes the command's alias - # skills it may manage, so they don't keep a removed layer. - for alias in cmd_tmpl.get("aliases") or []: - if not isinstance(alias, str): - continue - for alias_skill in self._skill_names_for_command(alias): - if ( - alias_skill not in target_skill_names - and (skills_dir / alias_skill).is_dir() - ): - target_skill_names.append(alias_skill) target_skill_names = [ name for name in target_skill_names if name in only_skill_names ] @@ -979,9 +892,9 @@ def _register_skills( or frontmatter.get("strategy") or "replace" ) - # A cached composition is not proof of a base either: it can be a - # stale leftover of a removed base (e.g. while its composer was - # disabled), so check resolution even when the cache exists. + # A cached composition can outlive its base (e.g. one removed + # while its composer was disabled), so the resolver decides even + # when the cache exists. if ( effective_strategy != "replace" and resolver.resolve_content(cmd_name, "command") is None diff --git a/tests/specify_cli/presets/test_manager_skills.py b/tests/specify_cli/presets/test_manager_skills.py index 34f7aed758..647895ee45 100644 --- a/tests/specify_cli/presets/test_manager_skills.py +++ b/tests/specify_cli/presets/test_manager_skills.py @@ -1945,16 +1945,15 @@ def test_remove_sole_base_preset_retires_stale_composed_skill( self, project_dir, temp_dir ): """With no core or extension fallback, removing the only base leaves - nothing to compose onto: the composed skill and its cache must not - keep serving the removed preset's instructions.""" + nothing to compose onto: the composer's skill is removed, and a later + rescaffold does not render its stale cached composition back.""" manager, skill_file = self._install_unbacked_custom_stack(project_dir, temp_dir) - cache = manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" with pytest.warns(UserWarning, match="no base layer"): assert manager.remove("base-preset") is True + manager.register_enabled_presets_for_agent("claude") assert not skill_file.exists() - assert not cache.exists() def test_remove_sole_base_preset_keeps_hand_edited_skill( self, project_dir, temp_dir @@ -1968,29 +1967,11 @@ def test_remove_sole_base_preset_keeps_hand_edited_skill( assert skill_file.read_text(encoding="utf-8") == edited - def test_remove_sole_base_preset_keeps_cache_behind_symlinked_dir( - self, project_dir, temp_dir - ): - """A ``.composed`` directory that links outside the project is not - followed when stale caches are deleted.""" - manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) - composed = manager.presets_dir / "append-preset" / ".composed" - external = temp_dir / "external-composed" - shutil.move(str(composed), str(external)) - composed.symlink_to(external, target_is_directory=True) - target = external / "speckit.custom.md" - assert target.is_file() - - with pytest.warns(UserWarning, match="no base layer"): - assert manager.remove("base-preset") is True - - assert target.is_file() - def test_remove_sole_base_under_two_composers_survives_rescaffold( self, project_dir, temp_dir ): - """Every surviving preset's stale cache is dropped, so a later - rescaffold cannot render the removed base back into the skill.""" + """With two composers and no base left, the skill is removed and a + rescaffold does not render either composer's stale cache back.""" manager, skill_file = self._install_unbacked_custom_stack( project_dir, temp_dir, @@ -2005,20 +1986,13 @@ def test_remove_sole_base_under_two_composers_survives_rescaffold( assert manager.remove("base-preset") is True manager.register_enabled_presets_for_agent("claude") - for preset_id in ("middle-preset", "top-preset"): - assert not ( - manager.presets_dir / preset_id / ".composed" / "speckit.custom.md" - ).exists() - assert not skill_file.exists() or "Base preset body" not in skill_file.read_text( - encoding="utf-8" - ) + assert not skill_file.exists() - def test_stale_cache_deletion_is_journaled(self, project_dir, temp_dir): - """The cache deletion is reported to the lifecycle file-change journal.""" + def test_stale_skill_removal_is_journaled(self, project_dir, temp_dir): + """Removing the composer's stale skill is reported to the lifecycle journal.""" from specify_cli.integrations._file_changes import file_change_observer manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) - cache = manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" events = [] token = file_change_observer.set( lambda path, before, removal: events.append((path, before, removal)) @@ -2029,40 +2003,10 @@ def test_stale_cache_deletion_is_journaled(self, project_dir, temp_dir): finally: file_change_observer.reset(token) - assert (cache, True, True) in events - assert not cache.exists() skill_dir = project_dir / ".claude" / "skills" / "speckit-custom" assert (skill_dir, True, True) in events assert not skill_dir.exists() - def test_stale_cache_cleanup_rejects_junctioned_composed_dir( - self, project_dir, temp_dir, monkeypatch - ): - """A ``.composed`` directory reported as a Windows reparse point (a - junction, which ``is_symlink()`` misses) is not followed.""" - from types import SimpleNamespace - - manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) - composed = manager.presets_dir / "append-preset" / ".composed" - cache = composed / "speckit.custom.md" - lstat = Path.lstat - - def junction_lstat(path, **kwargs): - result = lstat(path, **kwargs) - if path == composed: - return SimpleNamespace( - st_mode=result.st_mode, - st_file_attributes=0x400, - st_reparse_tag=0xA0000003, - ) - return result - - monkeypatch.setattr(Path, "lstat", junction_lstat) - with pytest.warns(UserWarning, match="no base layer"): - assert manager.remove("base-preset") is True - - assert cache.is_file() - def _install_custom_layers(self, project_dir, temp_dir, *, core=False, alias=None): """Base (replace) + append presets for ``speckit.custom`` under Claude.""" self._write_init_options(project_dir, ai="claude", ai_skills=True) @@ -2091,12 +2035,75 @@ def _install_custom_layers(self, project_dir, temp_dir, *, core=False, alias=Non manager.install_from_directory(preset_dir, "0.1.5", priority=priority) return manager + @pytest.mark.parametrize("core", [False, True], ids=["no-fallback", "core-fallback"]) + def test_remove_base_reconciles_skill_owned_by_lower_survivor_in_historical_dir( + self, project_dir, temp_dir, core + ): + """Claude's skill was rendered by ``middle``; ``top`` was installed later + under Codex only. Removing the base must still refresh Claude's skill, + whose recorded owner is ``middle``, not the current winner ``top``.""" + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skill = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" + self._create_skill(project_dir / ".claude" / "skills", "speckit-custom") + if core: + core_cmds = project_dir / ".specify" / "templates" / "commands" + core_cmds.mkdir(parents=True, exist_ok=True) + (core_cmds / "custom.md").write_text( + "---\ndescription: Core custom\n---\n\nCore custom body\n", + encoding="utf-8", + ) + manager = PresetManager(project_dir) + + def install(preset_id, body, strategy, priority): + preset_dir = self._create_command_preset( + temp_dir, preset_id, "speckit.custom", preset_id, body + ) + 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) + + install("base-preset", "Base preset body", "replace", 10) + install("middle-preset", "Middle body", "append", 7) + assert "preset:middle-preset" in claude_skill.read_text(encoding="utf-8") + self._write_init_options(project_dir, ai="codex", ai_skills=True) + manager.register_enabled_presets_for_agent("codex") + install("top-preset", "Top body", "append", 5) + + if core: + assert manager.remove("base-preset") is True + content = claude_skill.read_text(encoding="utf-8") + assert "Base preset body" not in content + assert "Core custom body" in content and "Middle body" in content + else: + with pytest.warns(UserWarning, match="no base layer"): + assert manager.remove("base-preset") is True + assert not claude_skill.exists() + + def test_remove_base_reconciles_survivor_with_legacy_flat_skill_record( + self, project_dir, temp_dir + ): + """A surviving composer whose ``registered_skills`` is still a legacy + flat list gets its inactive agent's skill refreshed too.""" + manager = self._install_custom_layers(project_dir, temp_dir, core=True) + claude_skill = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" + self._write_init_options(project_dir, ai="codex", ai_skills=True) + manager.register_enabled_presets_for_agent("codex") + manager.registry.update("append-preset", {"registered_skills": ["speckit-custom"]}) + + assert manager.remove("base-preset") is True + + content = claude_skill.read_text(encoding="utf-8") + assert "Base preset body" not in content + assert "Core custom body" in content and "Appended body" in content + def test_reenabled_sole_composer_does_not_render_removed_base( self, project_dir, temp_dir ): """The only composer is disabled while its base is removed; re-enabling - and rescaffolding must not render its stale cache, and the old skill is - retired once nothing is left to compose onto.""" + and rescaffolding neither renders its stale cache nor keeps its old + skill, since nothing is left to compose onto.""" manager = self._install_custom_layers(project_dir, temp_dir) skill = project_dir / ".claude" / "skills" / "speckit-custom" / "SKILL.md" manager.registry.update("append-preset", {"enabled": False}) @@ -2106,12 +2113,7 @@ def test_reenabled_sole_composer_does_not_render_removed_base( with pytest.warns(UserWarning, match="no base layer"): manager.register_enabled_presets_for_agent("claude") - assert not skill.exists() or "Base preset body" not in skill.read_text( - encoding="utf-8" - ) - assert not ( - manager.presets_dir / "append-preset" / ".composed" / "speckit.custom.md" - ).exists() + assert not skill.exists() @pytest.mark.parametrize("core", [False, True], ids=["no-fallback", "core-fallback"]) def test_remove_base_reconciles_historical_agent_skill( @@ -2134,63 +2136,11 @@ def test_remove_base_reconciles_historical_agent_skill( assert manager.remove("base-preset") is True assert not claude_skill.exists() - @pytest.mark.parametrize("core", [False, True], ids=["no-fallback", "core-fallback"]) - def test_remove_base_reconciles_native_alias_skill( - self, project_dir, temp_dir, core - ): - """A composer's alias skill is recomposed or retired with the primary.""" - manager = self._install_custom_layers( - project_dir, temp_dir, core=core, alias="speckit.custom-alias" - ) - alias_skill = project_dir / ".claude" / "skills" / "speckit-custom-alias" / "SKILL.md" - assert "Base preset body" in alias_skill.read_text(encoding="utf-8") - - if core: - assert manager.remove("base-preset") is True - content = alias_skill.read_text(encoding="utf-8") - assert "Base preset body" not in content - assert "Core custom body" in content and "Appended body" in content - else: - with pytest.warns(UserWarning, match="no base layer"): - assert manager.remove("base-preset") is True - assert not alias_skill.exists() - - def test_remove_sole_base_keeps_hand_edited_alias_skill( + def test_remove_sole_base_with_disabled_composer_survives_rescaffold( self, project_dir, temp_dir ): - manager = self._install_custom_layers( - project_dir, temp_dir, alias="speckit.custom-alias" - ) - alias_skill = project_dir / ".claude" / "skills" / "speckit-custom-alias" / "SKILL.md" - edited = "---\nname: speckit-custom-alias\n---\n\nUser-owned alias\n" - alias_skill.write_text(edited, encoding="utf-8") - - with pytest.warns(UserWarning, match="no base layer"): - assert manager.remove("base-preset") is True - - assert alias_skill.read_text(encoding="utf-8") == edited - - def test_stale_cache_cleanup_ignores_traversing_registry_key( - self, project_dir, temp_dir - ): - """A malformed registry key with ``..`` must not reach outside the project.""" - manager, _skill = self._install_unbacked_custom_stack(project_dir, temp_dir) - outside = (manager.presets_dir / "../../../outside").resolve() - target = outside / ".composed" / "speckit.custom.md" - target.parent.mkdir(parents=True) - target.write_text("external\n", encoding="utf-8") - manager.registry.add("../../../outside", {"version": "1.0.0", "enabled": True, "priority": 20}) - - with pytest.warns(UserWarning, match="no base layer"): - assert manager.remove("base-preset") is True - - assert target.read_text(encoding="utf-8") == "external\n" - - def test_remove_sole_base_drops_disabled_preset_cache( - self, project_dir, temp_dir - ): - """A disabled composer's stale cache is dropped too, so re-enabling it - and rescaffolding cannot render the removed base back.""" + """A composer disabled during the removal can't bring the removed base + back through its stale cache once it is re-enabled and rescaffolded.""" manager, skill_file = self._install_unbacked_custom_stack( project_dir, temp_dir, @@ -2207,12 +2157,7 @@ def test_remove_sole_base_drops_disabled_preset_cache( manager.registry.update("middle-preset", {"enabled": True}) manager.register_enabled_presets_for_agent("claude") - assert not ( - manager.presets_dir / "middle-preset" / ".composed" / "speckit.custom.md" - ).exists() - assert not skill_file.exists() or "Base preset body" not in skill_file.read_text( - encoding="utf-8" - ) + assert not skill_file.exists() def test_unreadable_surviving_base_keeps_composed_output( self, project_dir, temp_dir