diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 735032039..b97677a9a 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -11,7 +11,7 @@ import platform import re from copy import deepcopy from pathlib import Path -from typing import Any, Dict, List, Optional +from typing import Any, Dict, Iterable, List, Optional import yaml @@ -1083,6 +1083,7 @@ class CommandRegistrar: link_outputs: bool = False, extension_id: Optional[str] = None, only_agent: Optional[str] = None, + extra_agents: Optional[Iterable[str]] = None, ) -> Dict[str, List[str]]: """Register commands for all non-skill agents in the project. @@ -1102,14 +1103,25 @@ class CommandRegistrar: only_agent: If set, restrict registration to this single agent (#2948). An agent name that matches no configured agent (e.g. an empty string) yields no registrations at all. + extra_agents: Additional agent names to register for besides + ``only_agent``. Used by post-removal reconciliation to also + restore surviving content into historical agent directories + a just-removed preset actually wrote to, not only the + currently active agent (#2948). Ignored when ``only_agent`` + is ``None`` (already unrestricted). Returns: Dictionary mapping agent names to list of registered commands """ results = {} self._ensure_configs() + extra_agents_set = frozenset(extra_agents) if extra_agents else frozenset() for agent_name, agent_config in self.AGENT_CONFIGS.items(): - if only_agent is not None and agent_name != only_agent: + if ( + only_agent is not None + and agent_name != only_agent + and agent_name not in extra_agents_set + ): continue if agent_config.get("extension") == "/SKILL.md": continue diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 76afb57ee..96f96f6c4 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1290,10 +1290,19 @@ class ExtensionManager: sn_path = Path(skill_name) if sn_path.is_absolute() or len(sn_path.parts) != 1: continue + skill_subdir = skills_dir / skill_name + # Validate every path component down to the skill's own + # subdirectory, not just the already-validated parent + # skills_dir: a per-skill child can itself be a symlink to + # another directory whose *resolved* target still lands + # inside this same (safe) skills root, which the previous + # resolve()+relative_to() containment check alone would + # not catch. Reject the symlink outright rather than + # following it, even when the target is otherwise + # in-bounds (#2948). try: - skill_subdir = (skills_dir / skill_name).resolve() - skill_subdir.relative_to(skills_dir.resolve()) # raises if outside - except (OSError, ValueError): + _validate_safe_shared_directory(self.project_root, skill_subdir) + except (ValueError, OSError): continue if not skill_subdir.is_dir(): continue @@ -1354,12 +1363,19 @@ class ExtensionManager: sn_path = Path(skill_name) if sn_path.is_absolute() or len(sn_path.parts) != 1: continue + skill_subdir = skills_candidate / skill_name + # Validate every path component down to the skill's + # own subdirectory, not just the already-validated + # candidate parent: a per-skill child can itself be a + # symlink to another directory whose resolved target + # still lands inside this same candidate, which the + # previous resolve()+relative_to() containment check + # alone would not catch (#2948). try: - skill_subdir = (skills_candidate / skill_name).resolve() - skill_subdir.relative_to( - skills_candidate.resolve() - ) # raises if outside - except (OSError, ValueError): + _validate_safe_shared_directory( + self.project_root, skill_subdir + ) + except (ValueError, OSError): continue if not skill_subdir.is_dir(): continue @@ -1450,20 +1466,23 @@ class ExtensionManager: _validate_safe_shared_directory(self.project_root, skills_candidate) except (ValueError, OSError): continue - try: - resolved_candidate = skills_candidate.resolve() - except OSError: - continue for skill_name in skill_names: if skill_name in owned: continue sn_path = Path(skill_name) if sn_path.is_absolute() or len(sn_path.parts) != 1: continue + skill_subdir = skills_candidate / skill_name + # Validate every path component down to the skill's own + # subdirectory, not just the already-validated candidate + # parent: a per-skill child can itself be a symlink to + # another directory whose resolved target still lands + # inside this same candidate, which the previous + # resolve()+relative_to() containment check alone would + # not catch (#2948). try: - skill_subdir = (skills_candidate / skill_name).resolve() - skill_subdir.relative_to(resolved_candidate) # raises if outside - except (OSError, ValueError): + _validate_safe_shared_directory(self.project_root, skill_subdir) + except (ValueError, OSError): continue if not skill_subdir.is_dir(): continue diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 7852eb5d1..69021dcd3 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, Union +from typing import TYPE_CHECKING, Optional, Dict, List, Any, Union, Set if TYPE_CHECKING: from ..agents import CommandRegistrar @@ -910,7 +910,9 @@ class PresetManager: registrar = CommandRegistrar() registrar.unregister_commands(registered_commands, self.project_root) - def _reconcile_composed_commands(self, command_names: List[str]) -> None: + def _reconcile_composed_commands( + self, command_names: List[str], extra_agents: Optional[Set[str]] = None + ) -> None: """Re-resolve and re-register composed commands from the full stack. After install or remove, recompute the effective content for each @@ -930,6 +932,13 @@ class PresetManager: Args: command_names: List of command names to reconcile + extra_agents: Additional agent names to also reconcile besides + the currently active one. Populated by ``remove()`` with the + historical agents a just-removed preset's + ``registered_commands`` actually targeted, so a surviving + lower-priority preset's content is restored there too — not + only for the currently active agent (#2948). Install/use + callers omit this, preserving pure active-only behavior. """ if not command_names: return @@ -996,7 +1005,7 @@ class PresetManager: if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": self._register_for_non_skill_agents( registrar, [tmpl], manifest.id, pack_dir, - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) registered = True break @@ -1024,7 +1033,7 @@ class PresetManager: matching_cmds, ext_id, ext_dir, self.project_root, context_note=f"\n\n\n", - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) registered = True except Exception: @@ -1036,7 +1045,7 @@ class PresetManager: self._register_command_from_path( registrar, cmd_name, top_path, source_id=source_id, - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) else: # Composed command — resolve from full stack @@ -1073,7 +1082,11 @@ class PresetManager: agent: cmd_names_to_unregister for agent in registrar.AGENT_CONFIGS if registrar.AGENT_CONFIGS[agent].get("extension") != "/SKILL.md" - and (only_agent is None or agent == only_agent) + and ( + only_agent is None + or agent == only_agent + or agent in (extra_agents or ()) + ) }, self.project_root, ) @@ -1096,7 +1109,7 @@ class PresetManager: registrar, [{**tmpl, "file": f".composed/{cmd_name}.md"}], manifest.id, pack_dir, - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) registered = True break @@ -1118,7 +1131,7 @@ class PresetManager: self._register_command_from_path( registrar, cmd_name, composed_file, source_id=source_id, - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) def _register_command_from_path( @@ -1128,6 +1141,7 @@ class PresetManager: cmd_path: Path, source_id: str = "reconciled", only_agent: Optional[str] = None, + extra_agents: Optional[Set[str]] = None, ) -> None: """Register a single command from a file path (non-preset source). @@ -1140,6 +1154,8 @@ class PresetManager: cmd_path: Path to the command file source_id: Source attribution for rendered output only_agent: If set, restrict registration to this single agent (#2948). + extra_agents: Additional agent names to register for besides + ``only_agent`` (post-removal reconciliation only, #2948). """ if not cmd_path.exists(): return @@ -1170,7 +1186,7 @@ class PresetManager: pass # best-effort alias loading self._register_for_non_skill_agents( registrar, [cmd_tmpl], source_id, cmd_path.parent, - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) def _register_for_non_skill_agents( @@ -1180,6 +1196,7 @@ class PresetManager: source_id: str, source_dir: Path, only_agent: Optional[str] = None, + extra_agents: Optional[Set[str]] = None, ) -> None: """Register commands for non-skill agents during reconciliation. @@ -1198,10 +1215,14 @@ class PresetManager: only_agent: If set, restrict registration to this single agent, matching the active-only rule applied by ``_register_commands`` (#2948). + extra_agents: Additional agent names to register for besides + ``only_agent``. Used by post-removal reconciliation to also + restore surviving content into historical agent directories + a just-removed preset actually wrote to (#2948). """ registrar.register_commands_for_non_skill_agents( commands, source_id, source_dir, self.project_root, - only_agent=only_agent, + only_agent=only_agent, extra_agents=extra_agents, ) class _FilteredManifest: @@ -1225,7 +1246,11 @@ class PresetManager: if t.get("name") in self._cmd_names ] - def _reconcile_skills(self, command_names: List[str]) -> None: + def _reconcile_skills( + self, + command_names: List[str], + extra_skills_dirs: Optional[Dict[Path, Optional[str]]] = None, + ) -> None: """Re-register skills for commands whose winning layer changed. After a preset is removed, finds the next preset in the priority @@ -1234,52 +1259,66 @@ class PresetManager: Args: command_names: List of command names to reconcile skills for + extra_skills_dirs: Additional ``{skills_dir: renderer_agent}`` + pairs to reconcile besides the currently active skills + directory. Populated by ``remove()`` from the directories + ``_unregister_skills`` just restored to core/extension + content: a surviving lower-priority preset's override must + be re-applied to every one of those directories too, not + only the currently active agent's directory, or an + inactive integration is left with stale/missing content + even though the removed preset no longer wins there + either (#2948). """ if not command_names: return resolver = PresetResolver(self.project_root) - skills_dir = self._get_skills_dir() + active_skills_dir = self._get_skills_dir() + + from .. import load_init_options + + init_opts = load_init_options(self.project_root) + active_ai = init_opts.get("ai") if isinstance(init_opts, dict) else None + if not isinstance(active_ai, str) or not active_ai: + active_ai = None # Cache registry once to avoid repeated filesystem reads presets_by_priority = list(self.registry.list_by_priority()) # Group command names by winning preset to batch _register_skills calls - # while only registering skills for the specific commands being reconciled. + # while only registering skills for the specific commands being + # reconciled. This resolution (which preset/content wins) is + # directory-independent, so it's computed once and then applied to + # every affected directory below. preset_cmds: Dict[str, List[str]] = {} non_preset_skills: List[tuple] = [] + managed_skill_names: set = set() for cmd_name in command_names: layers = resolver.collect_all_layers(cmd_name, "command") if not layers: continue - # Re-create the skill directory only if it was previously managed - # (i.e., listed in some preset's registered_skills). This avoids - # creating new skill dirs that _register_skills would normally skip. - if skills_dir: - skill_name, _ = self._skill_names_for_command(cmd_name) - skill_subdir = skills_dir / skill_name - if not skill_subdir.exists(): - # Check if any preset previously registered this skill - was_managed = False - for _pid, meta in presets_by_priority: - if not isinstance(meta, dict): - continue - 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: - skill_subdir.mkdir(parents=True, exist_ok=True) + skill_name, _ = self._skill_names_for_command(cmd_name) + # Track whether any preset previously registered this skill + # (i.e., it was actively managed), so a not-yet-existing skill + # dir can be re-created per affected directory below. + for _pid, meta in presets_by_priority: + if not isinstance(meta, dict): + continue + 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: + managed_skill_names.add(skill_name) + break top_path = layers[0]["path"] # Find the preset that owns the winning layer @@ -1293,39 +1332,44 @@ class PresetManager: if not found_preset: # Winner is a non-preset source (core/extension/override). # Track the winning layer path for skill restoration. - skill_name, _ = self._skill_names_for_command(cmd_name) non_preset_skills.append((skill_name, cmd_name, layers[0])) - # Restore skills for commands whose winner is non-preset. - if non_preset_skills and skills_dir: - # Separate override-backed skills from core/extension-backed ones. - # _unregister_skills can rmtree the skill dir, so overrides must - # be handled directly (create dir + write) without that call. - core_ext_skills = [] - override_skills = [] - for item in non_preset_skills: - if item[2]["source"] == "project override": - override_skills.append(item) - else: - core_ext_skills.append(item) + core_ext_skills = [s for s in non_preset_skills if s[2]["source"] != "project override"] + override_skills = [s for s in non_preset_skills if s[2]["source"] == "project override"] + def apply_to_dir( + skills_dir: Path, dir_agent: Optional[str], *, is_active: bool + ) -> None: + # Re-create the skill directory only if it was previously + # managed (i.e., listed in some preset's registered_skills). + # This avoids creating new skill dirs that _register_skills + # would normally skip. + for skill_name in managed_skill_names: + skill_subdir = skills_dir / skill_name + if not skill_subdir.exists(): + skill_subdir.mkdir(parents=True, exist_ok=True) + + # Restore skills for commands whose winner is non-preset. + # _unregister_skills_in_dir can rmtree the skill dir, so + # overrides must be handled directly (create dir + write) + # without that call. if core_ext_skills: - self._unregister_skills( - [s[0] for s in core_ext_skills], self.presets_dir + self._unregister_skills_in_dir( + [s[0] for s in core_ext_skills], skills_dir, dir_agent ) for skill_name, cmd_name, top_layer in override_skills: skill_subdir = skills_dir / skill_name # Same symlink guard as _register_skills's registration path - # (#2948): mkdir(exist_ok=True) alone would silently follow an - # existing symlinked subdirectory before writing SKILL.md + # (#2948): mkdir(exist_ok=True) alone would silently follow + # an existing symlinked subdirectory before writing SKILL.md # through it. if not self._validate_skill_subdir(skill_subdir, create=True): continue skill_file = skill_subdir / "SKILL.md" try: from ..agents import CommandRegistrar - from .. import SKILL_DESCRIPTIONS, load_init_options + from .. import SKILL_DESCRIPTIONS registrar = CommandRegistrar() content = top_layer["path"].read_text(encoding="utf-8") fm, body = registrar.parse_frontmatter(content) @@ -1336,9 +1380,8 @@ class PresetManager: short_name.replace(".", "-"), f"Command: {short_name}", ) - init_opts = load_init_options(self.project_root) - selected_ai = init_opts.get("ai") if isinstance(init_opts, dict) else "" - if isinstance(selected_ai, str): + selected_ai = dir_agent if isinstance(dir_agent, str) else "" + if selected_ai: body = registrar.resolve_skill_placeholders( selected_ai, fm, body, self.project_root ) @@ -1346,10 +1389,9 @@ class PresetManager: body, registrar, selected_ai ) from ..integrations import get_integration - integration = get_integration(selected_ai) if isinstance(selected_ai, str) else None + integration = get_integration(selected_ai) if selected_ai else None fm_data = registrar.build_skill_frontmatter( - selected_ai if isinstance(selected_ai, str) else "", - skill_name, desc, + selected_ai, skill_name, desc, f"override:{cmd_name}", ) registrar.apply_argument_hint(fm, fm_data, integration) @@ -1366,21 +1408,38 @@ class PresetManager: except Exception: pass # best-effort override skill restoration - # Register skills only for the specific commands being reconciled, - # not all commands in each winning preset's manifest. - for pack_id, cmds in preset_cmds.items(): - pack_dir = self.presets_dir / pack_id - manifest_path = pack_dir / "preset.yml" - if not manifest_path.exists(): - continue - try: - manifest = PresetManifest(manifest_path) - except PresetValidationError: - continue - # Filter manifest to only the commands being reconciled - cmds_set = set(cmds) - filtered_manifest = self._FilteredManifest(manifest, cmds_set) - self._register_skills(filtered_manifest, pack_dir) + # Register skills only for the specific commands being + # reconciled, not all commands in each winning preset's + # manifest. + for pack_id, cmds in preset_cmds.items(): + pack_dir = self.presets_dir / pack_id + manifest_path = pack_dir / "preset.yml" + if not manifest_path.exists(): + continue + try: + manifest = PresetManifest(manifest_path) + except PresetValidationError: + continue + cmds_set = set(cmds) + filtered_manifest = self._FilteredManifest(manifest, cmds_set) + if is_active: + # Preserve exact prior behaviour for the currently + # active directory (including the ability to create + # brand-new skill subdirectories when ai_skills is on). + self._register_skills(filtered_manifest, pack_dir) + else: + self._register_skills( + filtered_manifest, pack_dir, + target_dir=skills_dir, target_agent=dir_agent or "", + ) + + if active_skills_dir: + apply_to_dir(active_skills_dir, active_ai, is_active=True) + + for extra_dir, extra_agent in (extra_skills_dirs or {}).items(): + if extra_dir == active_skills_dir: + continue # already reconciled above as the active directory + apply_to_dir(extra_dir, extra_agent, is_active=False) def _get_skills_dir(self) -> Optional[Path]: """Return the active skills directory for preset skill overrides. @@ -1494,6 +1553,9 @@ class PresetManager: self, manifest: "PresetManifest", preset_dir: Path, + *, + target_dir: Optional[Path] = None, + target_agent: Optional[str] = None, ) -> Dict[str, List[str]]: """Generate SKILL.md files for preset command overrides. @@ -1507,6 +1569,18 @@ class PresetManager: Args: manifest: Preset manifest. preset_dir: Installed preset directory. + target_dir: Explicit skills directory to render into, instead + of resolving the currently active one. Used by + ``_reconcile_skills`` to restore a surviving preset's + override into a historical (currently inactive) agent's + directory that removal of a higher-priority preset just + reverted (#2948). + target_agent: Explicit agent name to render for, paired with + ``target_dir``. When set, skills are only ever restored + into already-tracked directories/names — brand-new skill + subdirectories are never created for a non-active, + explicitly targeted directory (that creation path is only + meaningful for the currently active agent). Returns: ``{agent_name: [skill_name, ...]}`` for the single active @@ -1535,7 +1609,7 @@ class PresetManager: if not filtered: return {} - skills_dir = self._get_skills_dir() + skills_dir = target_dir if target_dir is not None else self._get_skills_dir() if not skills_dir: return {} @@ -1546,10 +1620,16 @@ class PresetManager: init_opts = load_init_options(self.project_root) if not isinstance(init_opts, dict): init_opts = {} - selected_ai = init_opts.get("ai") + selected_ai = target_agent if target_agent is not None else init_opts.get("ai") if not isinstance(selected_ai, str) or not selected_ai: return {} - ai_skills_enabled = is_ai_skills_enabled(init_opts) + # A target_dir/target_agent call reconciles an explicitly-known, + # already-tracked directory (see _reconcile_skills) rather than the + # currently active agent, so ai_skills_enabled must not be derived + # from the *current* project-wide toggle for that other agent — it + # only controls whether brand-new skill subdirectories may be + # created below, which is only meaningful for the active agent. + ai_skills_enabled = target_agent is None and is_ai_skills_enabled(init_opts) registrar = CommandRegistrar() integration = get_integration(selected_ai) agent_config = registrar.AGENT_CONFIGS.get(selected_ai, {}) @@ -1846,7 +1926,7 @@ class PresetManager: self, registered_skills: Union[Dict[str, List[str]], List[str]], preset_dir: Path, - ) -> None: + ) -> Dict[Path, Optional[str]]: """Restore original SKILL.md files after a preset is removed. For each skill that was overridden by the preset, attempts to @@ -1867,9 +1947,17 @@ class PresetManager: ``List[str]`` from a registry written before this provenance tracking existed. preset_dir: The preset's installed directory (may already be deleted). + + Returns: + ``{skills_dir: renderer_agent}`` for every directory actually + restored, so callers (e.g. ``remove()``) can hand these same + directories to :meth:`_reconcile_skills` — a surviving + lower-priority preset's override must be re-applied to every + directory this removal touched, not only the currently active + agent's directory (#2948). """ if not registered_skills: - return + return {} if isinstance(registered_skills, dict): from .. import load_init_options @@ -1909,25 +1997,32 @@ class PresetManager: active_agent if active_agent in agents else sorted(agents)[0] ) self._unregister_skills_in_dir(group["names"], skills_dir, renderer_agent) - return + return { + skills_dir: ( + active_agent if active_agent in group["agents"] else sorted(group["agents"])[0] + ) + for skills_dir, group in groups.items() + } # 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 + 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") + selected_ai = selected_ai if isinstance(selected_ai, str) else None self._unregister_skills_in_dir( registered_skills, skills_dir, - selected_ai if isinstance(selected_ai, str) else None, + selected_ai, ) + return {skills_dir: selected_ai} def _unregister_skills_in_dir( self, skill_names: List[str], skills_dir: Path, selected_ai: Optional[str] @@ -2274,6 +2369,27 @@ class PresetManager: registered_commands = metadata.get("registered_commands", {}) if metadata else {} pack_dir = self.presets_dir / pack_id + # Record which historical agents this preset's registered_commands + # actually targeted, *before* any filtering below, so post-removal + # reconciliation can restore a surviving lower-priority preset's + # override into every one of those directories too — not only the + # currently active agent's. Without this, removing a preset that + # was rendered under a previously-active (now inactive) agent + # deletes that agent's command file via _unregister_commands below, + # but active-only reconciliation would only recreate the surviving + # winner for the current agent, leaving the inactive integration + # with a missing/stale file (#2948). + try: + from ..agents import CommandRegistrar as _CommandRegistrarForScope + except ImportError: + _CommandRegistrarForScope = None + affected_command_agents = { + agent_name + for agent_name in registered_commands + if _CommandRegistrarForScope is None + or _CommandRegistrarForScope.AGENT_CONFIGS.get(agent_name, {}).get("extension") != "/SKILL.md" + } + # Collect ALL command names before filtering for reconciliation, # so commands registered only for skill-based agents are also # reconciled. Every command-type template's primary name is added @@ -2304,8 +2420,9 @@ class PresetManager: # names from registered_commands are still unregistered. pass + affected_skill_dirs: Dict[Path, Optional[str]] = {} if registered_skills: - self._unregister_skills(registered_skills, pack_dir) + affected_skill_dirs = self._unregister_skills(registered_skills, pack_dir) try: from ..agents import CommandRegistrar except ImportError: @@ -2330,8 +2447,12 @@ class PresetManager: # re-resolve from the remaining stack so the next layer takes effect. if removed_cmd_names: try: - self._reconcile_composed_commands(list(removed_cmd_names)) - self._reconcile_skills(list(removed_cmd_names)) + self._reconcile_composed_commands( + list(removed_cmd_names), extra_agents=affected_command_agents + ) + self._reconcile_skills( + list(removed_cmd_names), extra_skills_dirs=affected_skill_dirs + ) except Exception as exc: import warnings warnings.warn( diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 36fdad657..33e8b175e 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1824,6 +1824,149 @@ class TestExtensionSkillRegistration: "removed via an explicit symlinked directory argument" ) + def test_extension_owned_skill_names_rejects_symlinked_child_skill_dir( + self, project_dir, temp_dir + ): + """Provenance probing must reject a per-skill child directory that + is itself a symlink, even when its resolved target stays inside + the (real, non-symlinked) skills root. + + Both ``_extension_owned_skill_names`` and + ``_unregister_extension_skills`` previously only validated the + *parent* ``skills_dir`` for symlink escape, then resolved + ``skills_dir / skill_name`` and checked containment relative to + the already-resolved parent. A child symlink whose target + resolves inside that same root passes that containment check, so + a corrupted or attacker-controlled registry entry naming a + symlink alias could cause a legitimate, unrelated skill directory + to be falsely attributed as extension-owned via the alias. + """ + skills_dir = project_dir / ".claude" / "skills" + skills_dir.mkdir(parents=True) + + # A real, legitimately marker-matching skill directory under its + # own name — this represents genuine extension-owned content. + real_skill_dir = skills_dir / "speckit-child-sym-real" + real_skill_dir.mkdir() + (real_skill_dir / "SKILL.md").write_text( + "---\n" + "name: speckit-child-sym-real\n" + "description: real skill\n" + "metadata:\n" + " source: extension:child-sym-ext\n" + "---\n\n" + "real body\n", + encoding="utf-8", + ) + + if not _can_create_symlink(temp_dir): + pytest.skip("Current platform/user cannot create symlinks") + + # A *different* registered name that is merely a symlink alias + # pointing at the real skill directory above — both still live + # inside the same, non-symlinked skills root. + alias_name = "speckit-child-sym-alias" + os.symlink(str(real_skill_dir), str(skills_dir / alias_name)) + + manager = ExtensionManager(project_dir) + owned = manager._extension_owned_skill_names( + [alias_name], "child-sym-ext" + ) + + assert owned == [], ( + "a per-skill child directory that is itself a symlink must " + "never be followed for provenance attribution, even when its " + "resolved target remains inside the skills root" + ) + + def test_unregister_extension_skills_explicit_dir_rejects_symlinked_child( + self, project_dir, temp_dir + ): + """Fast (explicit ``skills_dir``) removal path must refuse to + delete through a per-skill child directory that is itself a + symlink, even when the resolved target stays inside the skills + root — deleting the resolved target would destroy a legitimate, + differently-named skill directory via the alias. + """ + skills_dir = project_dir / ".claude" / "skills" + skills_dir.mkdir(parents=True) + + precious_skill_dir = skills_dir / "speckit-child-sym-precious" + precious_skill_dir.mkdir() + precious_skill_md = precious_skill_dir / "SKILL.md" + precious_skill_md.write_text( + "---\n" + "name: speckit-child-sym-precious\n" + "description: precious skill\n" + "metadata:\n" + " source: extension:child-sym-ext2\n" + "---\n\n" + "precious body\n", + encoding="utf-8", + ) + + if not _can_create_symlink(temp_dir): + pytest.skip("Current platform/user cannot create symlinks") + + alias_name = "speckit-child-sym-alias2" + os.symlink(str(precious_skill_dir), str(skills_dir / alias_name)) + + manager = ExtensionManager(project_dir) + manager._unregister_extension_skills( + [alias_name], "child-sym-ext2", skills_dir=skills_dir, + ) + + assert precious_skill_dir.exists(), ( + "the real skill directory reached only through a symlink " + "alias must survive removal of the alias name (#2948)" + ) + assert precious_skill_md.exists(), ( + "the real skill directory's SKILL.md must not be deleted via " + "a differently-named symlink alias" + ) + + def test_unregister_extension_skills_fallback_rejects_symlinked_child( + self, project_dir, temp_dir + ): + """Fallback (unscoped, ``skills_dir=None``) removal scan must also + refuse to delete through a per-skill child symlink, mirroring the + explicit-dir fast path. + """ + skills_dir = project_dir / ".claude" / "skills" + skills_dir.mkdir(parents=True) + + precious_skill_dir = skills_dir / "speckit-child-sym-precious3" + precious_skill_dir.mkdir() + precious_skill_md = precious_skill_dir / "SKILL.md" + precious_skill_md.write_text( + "---\n" + "name: speckit-child-sym-precious3\n" + "description: precious skill\n" + "metadata:\n" + " source: extension:child-sym-ext3\n" + "---\n\n" + "precious body\n", + encoding="utf-8", + ) + + if not _can_create_symlink(temp_dir): + pytest.skip("Current platform/user cannot create symlinks") + + alias_name = "speckit-child-sym-alias3" + os.symlink(str(precious_skill_dir), str(skills_dir / alias_name)) + + manager = ExtensionManager(project_dir) + manager._unregister_extension_skills([alias_name], "child-sym-ext3") + + assert precious_skill_dir.exists(), ( + "the real skill directory reached only through a symlink " + "alias must survive the unscoped fallback removal scan (#2948)" + ) + assert precious_skill_md.exists(), ( + "the real skill directory's SKILL.md must not be deleted via " + "a differently-named symlink alias during fallback removal" + ) + def test_existing_agent_command_path_file_is_not_detected( self, project_dir, temp_dir ): diff --git a/tests/test_presets.py b/tests/test_presets.py index fcbcf3171..4a7bc8b16 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -5295,6 +5295,157 @@ class TestPresetSkills: "inactive agent's directory (#2948)" ) + def test_remove_reconciles_command_for_every_historical_agent( + self, project_dir, temp_dir + ): + """Removing a preset must reconcile every historical agent its + ``registered_commands`` actually targeted, not only the currently + active one. + + Preset B (lower precedence, survives) is installed while gemini is + active, then preset A (higher precedence) overrides the same + command while gemini is still active. Switching the active + integration to opencode and rescaffolding re-registers both + presets under opencode too, so preset A's ``registered_commands`` + now spans two agents: gemini (now inactive) and opencode (active). + Removing A deletes its command file from *both* directories via + ``_unregister_commands``, but active-only reconciliation used to + recreate the surviving preset B's content only for the active + agent (opencode), leaving gemini's directory with a stale/missing + file (#2948). + """ + self._write_init_options(project_dir, ai="gemini", ai_skills=False) + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + + preset_b_dir = self._create_command_preset( + temp_dir, "hist-preset-b", "speckit.specify", + "Preset B", "preset B body", + ) + preset_a_dir = self._create_command_preset( + temp_dir, "hist-preset-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) + + gemini_cmd_files = list(gemini_dir.glob("*specify*")) + assert gemini_cmd_files, "sanity: gemini should have the command file" + assert "preset A body" in gemini_cmd_files[0].read_text(), ( + "sanity: preset A (higher precedence) should win initially" + ) + + # Switch the active integration to opencode and rescaffold, mirroring + # `integration use opencode`. This merges opencode into both + # presets' registered_commands alongside the pre-existing gemini + # entry recorded while gemini was active. + self._write_init_options(project_dir, ai="opencode", ai_skills=False) + opencode_dir = project_dir / ".opencode" / "commands" + opencode_dir.mkdir(parents=True, exist_ok=True) + manager.register_enabled_presets_for_agent("opencode") + + metadata_a = manager.registry.get("hist-preset-a") + assert set(metadata_a.get("registered_commands", {})) == {"gemini", "opencode"}, ( + "sanity: preset A's registered_commands must span both the " + "historical (gemini) and currently active (opencode) agents" + ) + + assert manager.remove("hist-preset-a") is True + + gemini_cmd_files = list(gemini_dir.glob("*specify*")) + opencode_cmd_files = list(opencode_dir.glob("*specify*")) + assert gemini_cmd_files, "gemini's command file must still exist after removal" + assert opencode_cmd_files, "opencode's command file must still exist after removal" + assert "preset B body" in gemini_cmd_files[0].read_text(), ( + "removing the higher-precedence preset must restore the " + "surviving preset's content in the historical (inactive) " + "agent's directory too, not only the active agent's (#2948)" + ) + assert "preset B body" in opencode_cmd_files[0].read_text(), ( + "the surviving preset's content must also be restored for the " + "currently active agent" + ) + + def test_remove_reconciles_skill_for_every_historical_agent( + self, project_dir, temp_dir + ): + """Removing a preset must reconcile every historical skills + directory its ``registered_skills`` actually targeted, not only + the currently active one. + + Preset B (survives) is installed while claude is active, then + preset A (higher precedence) overrides the same command while + claude is still active. Switching to codex and rescaffolding + records codex too, so preset A's ``registered_skills`` spans both + claude (now inactive) and codex (active) directories. Removing A + restores both directories to core/extension via + ``_unregister_skills``, but ``_reconcile_skills`` used to only + resolve/apply the surviving winner for the currently active + skills directory, leaving claude's directory reverted to + core/extension content instead of preset B's override (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + + # A core template fallback is required so unregistering the + # top-priority preset's SKILL.md restores core content rather than + # deleting the skill directory outright when no preset remains to + # apply on top of it (mirrors the pre-existing skills-reconciliation + # fixtures elsewhere in this file). + 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-skill-preset-b", "speckit.specify", + "Preset B", "preset B body", + ) + preset_a_dir = self._create_command_preset( + temp_dir, "hist-skill-preset-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) + + claude_skill_file = claude_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:hist-skill-preset-a" in claude_skill_file.read_text(), ( + "sanity: preset A (higher precedence) should win initially" + ) + + # Switch the active integration to codex (a distinct skills + # directory) and rescaffold, mirroring `integration use codex`. + self._write_init_options(project_dir, ai="codex", ai_skills=True) + codex_skills_dir = project_dir / ".agents" / "skills" + manager.register_enabled_presets_for_agent("codex") + + metadata_a = manager.registry.get("hist-skill-preset-a") + assert set(metadata_a.get("registered_skills", {})) == {"claude", "codex"}, ( + "sanity: preset A's registered_skills must span both the " + "historical (claude) and currently active (codex) agents" + ) + + assert manager.remove("hist-skill-preset-a") is True + + codex_skill_file = codex_skills_dir / "speckit-specify" / "SKILL.md" + assert claude_skill_file.exists(), "claude's skill file must still exist after removal" + assert codex_skill_file.exists(), "codex's skill file must still exist after removal" + assert "preset:hist-skill-preset-b" in claude_skill_file.read_text(), ( + "removing the higher-precedence preset must restore the " + "surviving preset's override in the historical (inactive) " + "agent's directory too, not only the active agent's (#2948)" + ) + assert "preset:hist-skill-preset-b" in codex_skill_file.read_text(), ( + "the surviving preset's override must also be restored for " + "the currently active agent" + ) + def test_symlinked_skill_subdir_rejected_on_restore(self, project_dir, temp_dir): """Restore must validate each per-skill subdirectory, not just its parent.