diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index df76828cb..e80375033 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -16,7 +16,7 @@ import zipfile import shutil from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Optional, Dict, List, Any +from typing import TYPE_CHECKING, Optional, Dict, List, Any, Union if TYPE_CHECKING: from ..agents import CommandRegistrar @@ -774,15 +774,14 @@ class PresetManager: updates["registered_commands"] = merged_commands registered_skills = self._register_skills(manifest, pack_dir) - if registered_skills: - existing_skills = metadata.get("registered_skills", []) - if not isinstance(existing_skills, list): - existing_skills = [] - merged_skills = list( - dict.fromkeys(existing_skills + registered_skills) - ) - if merged_skills != existing_skills: - updates["registered_skills"] = merged_skills + existing_skills = self._normalize_registered_skills( + metadata.get("registered_skills"), fallback_agent=agent_name + ) + merged_skills = copy.deepcopy(existing_skills) + if registered_skills.get(agent_name): + merged_skills[agent_name] = registered_skills[agent_name] + if merged_skills != existing_skills: + updates["registered_skills"] = merged_skills if updates: self.registry.update(pack_id, updates) @@ -1159,7 +1158,16 @@ class PresetManager: for _pid, meta in presets_by_priority: if not isinstance(meta, dict): continue - if skill_name in meta.get("registered_skills", []): + recorded = meta.get("registered_skills", []) + if isinstance(recorded, dict): + in_any_agent = any( + skill_name in names + for names in recorded.values() + if isinstance(names, list) + ) + else: + in_any_agent = skill_name in recorded + if in_any_agent: was_managed = True break if was_managed: @@ -1373,7 +1381,7 @@ class PresetManager: self, manifest: "PresetManifest", preset_dir: Path, - ) -> List[str]: + ) -> Dict[str, List[str]]: """Generate SKILL.md files for preset command overrides. For every command template in the preset, checks whether a @@ -1388,13 +1396,16 @@ class PresetManager: preset_dir: Installed preset directory. Returns: - List of skill names that were written (for registry storage). + ``{agent_name: [skill_name, ...]}`` for the single active + agent skills were written for (empty if none were written), + matching the shape ``registered_commands`` already uses so the + two can be tracked/restored consistently (#2948). """ command_templates = [ t for t in manifest.templates if t.get("type") == "command" ] if not command_templates: - return [] + return {} # Filter out extension command overrides if the extension isn't installed, # matching the same logic used by _register_commands(). @@ -1409,11 +1420,11 @@ class PresetManager: filtered.append(cmd) if not filtered: - return [] + return {} skills_dir = self._get_skills_dir() if not skills_dir: - return [] + return {} from .. import SKILL_DESCRIPTIONS, load_init_options from ..agents import CommandRegistrar @@ -1423,8 +1434,8 @@ class PresetManager: if not isinstance(init_opts, dict): init_opts = {} selected_ai = init_opts.get("ai") - if not isinstance(selected_ai, str): - return [] + if not isinstance(selected_ai, str) or not selected_ai: + return {} ai_skills_enabled = is_ai_skills_enabled(init_opts) registrar = CommandRegistrar() integration = get_integration(selected_ai) @@ -1525,68 +1536,115 @@ class PresetManager: skill_file.write_text(skill_content, encoding="utf-8") written.append(target_skill_name) - return written + return {selected_ai: written} if written else {} - def _tracked_skill_agent_dirs(self) -> List[tuple]: - """Return (skills_dir, agent_name) pairs for every skill-mode - integration directory that currently exists under the project root. + @staticmethod + def _normalize_registered_skills( + value: Any, fallback_agent: Optional[str] = None + ) -> Dict[str, List[str]]: + """Normalize a ``registered_skills`` registry value to per-agent form. - ``registered_skills`` only tracks skill *names*, not which agent - directories they were written under, so a preset used first under - one skill-mode agent and later switched to another can have live - overrides in both directories at removal time. Restoring every - existing skill-mode directory (instead of only the currently active - one) ensures none of them are left permanently orphaned. + The registry stores ``registered_skills`` as ``Dict[str, List[str]]`` + (agent name -> skill names actually written for that agent), + mirroring ``registered_commands``. Older registries predate that + provenance and stored a flat ``List[str]`` with no record of which + agent directory the names were written under; since that can't be + recovered, ``fallback_agent`` (when given) attributes the legacy + list to the agent currently being processed so the format + self-migrates on the next write. Without a fallback agent, legacy + lists are dropped rather than guessed at. + """ + if isinstance(value, dict): + return { + agent: list(names) + for agent, names in value.items() + if isinstance(agent, str) and isinstance(names, list) + } + if isinstance(value, list) and value and fallback_agent: + return {fallback_agent: [n for n in value if isinstance(n, str)]} + return {} - Multiple integration keys can share the same physical directory - (e.g. ``agy``/``codex``/``zed`` all use ``.agents/skills``); only one - representative agent name is kept per unique resolved directory so - each physical directory is processed exactly once. + def _safe_skills_dir_for_agent(self, agent_name: str) -> Optional[Path]: + """Resolve ``agent_name``'s skills directory, validated for safety. + + Unlike :meth:`_get_skills_dir` (which resolves only the *currently + active* integration via init-options), this resolves an arbitrary + agent's directory from persisted provenance so a preset's skill + registrations can be restored/cleaned up under an agent that isn't + currently active. The candidate directory is validated through the + project's shared symlink/containment guard before any file in it is + touched; directories that don't exist or fail validation are + skipped rather than raising. """ from .. import _get_skills_dir as _resolve_skills_dir - from ..integrations import INTEGRATION_REGISTRY - from ..integrations.base import SkillsIntegration + from ..shared_infra import _ensure_safe_shared_directory - seen: Dict[Path, str] = {} - for key in sorted(INTEGRATION_REGISTRY): - integration = INTEGRATION_REGISTRY[key] - if not ( - isinstance(integration, SkillsIntegration) - or getattr(integration, "_skills_mode", False) - ): - continue - skills_dir = _resolve_skills_dir(self.project_root, key) - if not skills_dir.is_dir(): - continue - try: - resolved = skills_dir.resolve() - except OSError: - continue - seen.setdefault(resolved, key) + skills_dir = _resolve_skills_dir(self.project_root, agent_name) + try: + _ensure_safe_shared_directory( + self.project_root, skills_dir, + create=False, context="preset skills directory", + ) + except (ValueError, OSError): + return None + return skills_dir - return [(path, agent) for path, agent in seen.items()] - - def _unregister_skills(self, skill_names: List[str], preset_dir: Path) -> None: + def _unregister_skills( + self, + registered_skills: Union[Dict[str, List[str]], List[str]], + preset_dir: Path, + ) -> None: """Restore original SKILL.md files after a preset is removed. For each skill that was overridden by the preset, attempts to regenerate the skill from the core command template. If no core template exists, the skill directory is removed. - Restores across every existing skill-mode agent directory (see - :meth:`_tracked_skill_agent_dirs`), not just the currently active - integration, so switching integrations before removal can't leave a - preset override behind permanently. + ``registered_skills`` records exactly which agent directories this + preset actually wrote to (see :meth:`_register_skills`), so removal + restores precisely those directories rather than guessing at every + skill-mode agent that happens to exist on disk. Each directory is + re-resolved and safety-validated at removal time (see + :meth:`_safe_skills_dir_for_agent`) since it may belong to an agent + that isn't currently active. Args: - skill_names: List of skill names written by the preset. + registered_skills: Per-agent skill names written by the preset + (``{agent_name: [skill_name, ...]}``), or a legacy flat + ``List[str]`` from a registry written before this + provenance tracking existed. preset_dir: The preset's installed directory (may already be deleted). """ - if not skill_names: + if not registered_skills: return - for skills_dir, agent_name in self._tracked_skill_agent_dirs(): - self._unregister_skills_in_dir(skill_names, skills_dir, agent_name) + if isinstance(registered_skills, dict): + for agent_name, skill_names in registered_skills.items(): + if not skill_names: + continue + skills_dir = self._safe_skills_dir_for_agent(agent_name) + if skills_dir is None: + continue + self._unregister_skills_in_dir(skill_names, skills_dir, agent_name) + return + + # Legacy flat-list format: no record of which agent directory these + # names were written under, so best-effort restore is limited to the + # currently active agent's directory (the pre-provenance behaviour). + skills_dir = self._get_skills_dir() + if not skills_dir: + return + from .. import load_init_options + + init_opts = load_init_options(self.project_root) + if not isinstance(init_opts, dict): + init_opts = {} + selected_ai = init_opts.get("ai") + self._unregister_skills_in_dir( + registered_skills, + skills_dir, + selected_ai if isinstance(selected_ai, str) else None, + ) def _unregister_skills_in_dir( self, skill_names: List[str], skills_dir: Path, selected_ai: Optional[str] @@ -1760,11 +1818,11 @@ class PresetManager: "enabled": True, "priority": priority, "registered_commands": {}, - "registered_skills": [], + "registered_skills": {}, }) registered_commands: Dict[str, List[str]] = {} - registered_skills: List[str] = [] + registered_skills: Dict[str, List[str]] = {} try: # Register command overrides with AI agents and persist the result # immediately so cleanup can recover even if installation stops diff --git a/tests/integrations/test_integration_claude.py b/tests/integrations/test_integration_claude.py index 1b1b2308d..7916fdeba 100644 --- a/tests/integrations/test_integration_claude.py +++ b/tests/integrations/test_integration_claude.py @@ -303,7 +303,7 @@ class TestClaudeIntegration: assert "disable-model-invocation: false" in content metadata = manager.registry.get("claude-skill-command") - assert "speckit-research" in metadata.get("registered_skills", []) + assert "speckit-research" in metadata.get("registered_skills", {}).get("claude", []) class TestClaudeArgumentHints: diff --git a/tests/test_presets.py b/tests/test_presets.py index 6c468e444..bf09b8744 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3186,9 +3186,9 @@ class TestPresetSkills: assert "preset:self-test" in content, "Skill should reference preset source" assert "disable-model-invocation: false" in content - # Verify it was recorded in registry + # Verify it was recorded in registry, keyed by the active agent metadata = manager.registry.get("self-test") - assert "speckit-specify" in metadata.get("registered_skills", []) + assert "speckit-specify" in metadata.get("registered_skills", {}).get("claude", []) def _install_arg_hint_preset(self, project_dir, temp_dir, ai, skills_dir, description, arg_hint): """Install a preset whose command declares argument-hint; return the SKILL.md path.""" @@ -3645,7 +3645,7 @@ class TestPresetSkills: assert (skills_dir / "speckit-specify").is_file() metadata = manager.registry.get("self-test") - assert "speckit-specify" not in metadata.get("registered_skills", []) + assert "speckit-specify" not in metadata.get("registered_skills", {}).get("qwen", []) def test_no_skills_registered_when_no_skill_dir_exists(self, project_dir, temp_dir): """Skills should not be created when no existing skill dir is found.""" @@ -3656,7 +3656,7 @@ class TestPresetSkills: install_self_test_preset(manager) metadata = manager.registry.get("self-test") - assert metadata.get("registered_skills", []) == [] + assert metadata.get("registered_skills", {}) == {} def test_extension_skill_override_matches_hyphenated_multisegment_name(self, project_dir, temp_dir): """Preset overrides for speckit.. should target speckit-- skills.""" @@ -3704,7 +3704,7 @@ class TestPresetSkills: assert "# Speckit Fakeext Cmd Skill" in content metadata = manager.registry.get("ext-skill-override") - assert "speckit-fakeext-cmd" in metadata.get("registered_skills", []) + assert "speckit-fakeext-cmd" in metadata.get("registered_skills", {}).get("codex", []) def test_extension_skill_restored_on_preset_remove(self, project_dir, temp_dir): """Preset removal should restore an extension-backed skill instead of deleting it.""" @@ -3867,7 +3867,7 @@ class TestPresetSkills: assert "name: speckit.specify" in content metadata = manager.registry.get("self-test") - assert "speckit.specify" in metadata.get("registered_skills", []) + assert "speckit.specify" in metadata.get("registered_skills", {}).get("kimi", []) def test_kimi_skill_updated_even_when_ai_skills_disabled(self, project_dir, temp_dir): """Kimi presets should still propagate command overrides to existing skills.""" @@ -3887,7 +3887,7 @@ class TestPresetSkills: assert "name: speckit-specify" in content metadata = manager.registry.get("self-test") - assert "speckit-specify" in metadata.get("registered_skills", []) + assert "speckit-specify" in metadata.get("registered_skills", {}).get("kimi", []) def test_kimi_new_skill_created_even_when_ai_skills_disabled(self, project_dir, temp_dir): """Kimi native skills should still receive brand-new preset commands.""" @@ -3936,7 +3936,7 @@ class TestPresetSkills: assert "name: speckit-research" in content metadata = manager.registry.get("kimi-new-skill") - assert "speckit-research" in metadata.get("registered_skills", []) + assert "speckit-research" in metadata.get("registered_skills", {}).get("kimi", []) def test_kimi_preset_skill_override_resolves_script_placeholders(self, project_dir, temp_dir): """Kimi preset skill overrides should resolve placeholders and rewrite project paths.""" @@ -4192,12 +4192,14 @@ class TestPresetSkills: must restore both agents' directories, not just the currently active one. - ``registered_skills`` is a flat list of skill names shared across - agents, so it can't tell which agent directories a preset actually - touched. Before the fix, ``_unregister_skills`` only restored the - currently active agent's skills directory; a preset used first - under Claude and later switched to Codex would have its Claude - override left behind permanently on removal (#2948). + ``registered_skills`` records exactly which agent directories the + preset wrote to (``{agent_name: [skill_name, ...]}``); switching to + codex and re-registering adds a "codex" entry alongside the + original "claude" entry, so removal restores both. Before the + provenance fix, ``_unregister_skills`` only restored the currently + active agent's skills directory; a preset used first under Claude + and later switched to Codex would have its Claude override left + behind permanently on removal (#2948). """ self._write_init_options(project_dir, ai="claude", ai_skills=True) @@ -4243,6 +4245,13 @@ class TestPresetSkills: "sanity: the previous agent's registration is preserved on switch" ) + metadata = manager.registry.get("multi-skill-agent-preset") + registered_skills = metadata.get("registered_skills", {}) + assert set(registered_skills) == {"claude", "codex"}, ( + "registered_skills must record both agent directories this " + "preset actually wrote to (#2948)" + ) + assert manager.remove("multi-skill-agent-preset") is True for skill_file, label in ((claude_skill, "claude"), (codex_skill, "codex")): @@ -4254,6 +4263,160 @@ class TestPresetSkills: ) assert "Core specify body" in content + def test_symlinked_skills_dir_rejected_on_removal(self, project_dir, temp_dir): + """Removal must validate a recorded skill directory before touching it. + + If an agent's skills directory is replaced with a symlink escaping + the project root between install and removal, restoration must + refuse to write/rmtree through it rather than trusting the + recorded agent name blindly. The unsafe directory is skipped + best-effort; removal still succeeds and doesn't crash (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + self._create_skill(claude_skills_dir, "speckit-specify") + + preset_dir = self._create_command_preset( + temp_dir, "symlink-guard-preset", "speckit.specify", + "Symlink guard test", "preset body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + metadata = manager.registry.get("symlink-guard-preset") + assert "speckit-specify" in metadata.get("registered_skills", {}).get("claude", []) + + # Simulate the claude skills directory being replaced with a symlink + # that escapes the project root, containing an external + # "speckit-specify" directory that must not be touched. + outside_target = temp_dir / "outside-claude-skills" + outside_skill_dir = outside_target / "speckit-specify" + outside_skill_dir.mkdir(parents=True) + sentinel = outside_skill_dir / "SKILL.md" + sentinel.write_text("do-not-touch") + shutil.rmtree(claude_skills_dir) + claude_skills_dir.symlink_to(outside_target, target_is_directory=True) + + assert manager.remove("symlink-guard-preset") is True + + assert sentinel.read_text() == "do-not-touch", ( + "removal must not follow a symlinked skills directory outside " + "the project root (#2948)" + ) + assert outside_skill_dir.is_dir(), ( + "the external directory must not be rmtree'd through a " + "symlinked skills path" + ) + assert claude_skills_dir.is_symlink(), ( + "the symlink itself should be left alone, not rmtree'd through" + ) + + def test_preset_removal_does_not_touch_other_presets_skill_dir( + self, project_dir, temp_dir + ): + """Removing a preset must only touch directories it actually wrote to. + + Preset A is installed while Claude is active and preset B is + installed while Codex is active; both override the same command + name, so both materialize a ``speckit-specify`` skill, but in + *different* agent directories. Before the provenance fix, removing + B enumerated every existing skill-mode directory (including + Claude's) and restored/overwrote anything named ``speckit-specify`` + found there, corrupting A's override even though B never touched + Claude's directory (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + self._create_skill(claude_skills_dir, "speckit-specify") + + preset_a_dir = self._create_command_preset( + temp_dir, "preset-a", "speckit.specify", "Preset A", "preset A body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_a_dir, "0.1.5") + + claude_skill_file = claude_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:preset-a" in claude_skill_file.read_text() + + # Switch to codex and install a second preset overriding the same + # command; codex's skills directory is entirely separate. + self._write_init_options(project_dir, ai="codex", ai_skills=True) + codex_skills_dir = project_dir / ".agents" / "skills" + self._create_skill(codex_skills_dir, "speckit-specify") + + preset_b_dir = self._create_command_preset( + temp_dir, "preset-b", "speckit.specify", "Preset B", "preset B body", + ) + manager.install_from_directory(preset_b_dir, "0.1.5") + + metadata_b = manager.registry.get("preset-b") + assert "claude" not in metadata_b.get("registered_skills", {}), ( + "preset B never wrote to claude's skills directory and must " + "not record it as touched" + ) + + assert manager.remove("preset-b") is True + + assert "preset:preset-a" in claude_skill_file.read_text(), ( + "removing preset B must not disturb preset A's Claude override (#2948)" + ) + + def test_copilot_skills_registration_restored_after_process_restart( + self, project_dir, temp_dir + ): + """Copilot skills-mode registrations must restore even when the + transient ``_skills_mode`` integration attribute has been reset, + simulating a fresh CLI process. + + ``_skills_mode`` is set during ``setup()`` and is never persisted; + after switching the active agent and running ``preset remove`` in + a brand-new process, a naive "is this integration currently in + skills mode" check would be False even though Copilot's + ``.github/skills`` directory holds a live override this preset + wrote. Restoration must rely on the persisted per-agent provenance + recorded at write time, not on runtime integration state (#2948). + """ + self._write_init_options(project_dir, ai="copilot", ai_skills=True) + 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", + ) + copilot_skills_dir = project_dir / ".github" / "skills" + self._create_skill(copilot_skills_dir, "speckit-specify") + + preset_dir = self._create_command_preset( + temp_dir, "copilot-fresh-process-preset", "speckit.specify", + "Copilot fresh process test", "preset body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + skill_file = copilot_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:copilot-fresh-process-preset" in skill_file.read_text() + + metadata = manager.registry.get("copilot-fresh-process-preset") + assert "copilot" in metadata.get("registered_skills", {}) + + # Switch the active agent away from copilot, then simulate a fresh + # CLI process (a brand-new PresetManager, so any transient + # `_skills_mode` state set during a prior setup() call is gone) + # removing the preset. + self._write_init_options(project_dir, ai="claude", ai_skills=True) + fresh_manager = PresetManager(project_dir) + + assert fresh_manager.remove("copilot-fresh-process-preset") is True + + assert "preset:copilot-fresh-process-preset" not in skill_file.read_text(), ( + "removal must restore copilot's .github/skills override even " + "when copilot's transient skills-mode state isn't set in this " + "process (#2948)" + ) + assert "Core specify body" in skill_file.read_text() + class TestPresetSetPriority: """Test preset set-priority CLI command."""