From fc7b2e2c8050fcaf44009dbafe67a8b97fd71244 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:47:38 +0200 Subject: [PATCH] fix: guard skill subdirectories and active-agent scoping in preset reconciliation Fix 4 issues from round-6 review of the active-only integration registration work (#2948): - remove(): removed_cmd_names only collected primary command names from registered_commands + manifest aliases, missing commands that were only ever registered via skills mode (ai_skills guard returns no command names for command-backed integrations in skills mode). This skipped reconciliation entirely when removing a higher-priority skills-mode preset, causing _unregister_skills() to fall back to core/extension content instead of the surviving lower-priority preset's override. Now every command template's primary name is added to removed_cmd_names unconditionally. - _reconcile_composed_commands(): the "composed is None" branch (fires when no replace-strategy layer remains for a command, e.g. after removing a wrap/append preset's base) called unregister_commands() across every configured non-skill agent, ignoring only_agent. This deleted historical artifacts from integrations that were never active for the preset. Now filtered by only_agent like the rest of the file. - Added _validate_skill_subdir() helper (reusing _ensure_safe_shared_directory/_validate_safe_shared_directory from shared_infra.py) and applied it at every site that reads or writes an individual skill subdirectory (_register_skills, _unregister_skills_in_dir, _reconcile_skills' override_skills restoration loop). _safe_skills_dir_for_agent only validated the parent skills directory; a symlinked leaf subdirectory (e.g. .claude/skills/speckit-specify) would slip past that check since is_dir()/exists() follow symlinks, letting write_text/rmtree operate through it to an arbitrary location outside the project. Added regression tests: removing a higher-priority skills-only preset restores the surviving lower-priority preset's content; composed-is-None unregistration only touches the active agent; symlinked skill subdirectory rejected on restore; symlinked skill subdirectory rejected on write. Targeted (934) and full (3934 passed, 109 skipped) test suites and ruff check pass clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/presets/__init__.py | 78 ++++++++++- tests/test_presets.py | 204 ++++++++++++++++++++++++++++ 2 files changed, 275 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 550f7fa77..cdc15a19f 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -992,9 +992,18 @@ class PresetManager: if isinstance(alias, str): cmd_names_to_unregister.append(alias) break + # Mirror the active-only restriction used elsewhere in + # this pass: without it, unregistering a stale composed + # command would touch every non-skill agent's directory, + # deleting historical artifacts from integrations that + # were never active when this preset registered (#2948). registrar.unregister_commands( - {agent: cmd_names_to_unregister for agent in registrar.AGENT_CONFIGS - if registrar.AGENT_CONFIGS[agent].get("extension") != "/SKILL.md"}, + { + 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) + }, self.project_root, ) continue @@ -1236,7 +1245,12 @@ class PresetManager: for skill_name, cmd_name, top_layer in override_skills: skill_subdir = skills_dir / skill_name - skill_subdir.mkdir(parents=True, exist_ok=True) + # 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 + # through it. + if not self._validate_skill_subdir(skill_subdir, create=True): + continue skill_file = skill_subdir / "SKILL.md" try: from ..agents import CommandRegistrar @@ -1539,7 +1553,13 @@ class PresetManager: skill_subdir = skills_dir / target_skill_name if skill_subdir.exists() and not skill_subdir.is_dir(): continue - skill_subdir.mkdir(parents=True, exist_ok=True) + # Validate (and create, if missing) the skill's own + # subdirectory under the same symlink guard as its parent — + # is_dir() above follows symlinks, so a symlinked subdir + # with a real parent would otherwise slip through and have + # SKILL.md written through it to an arbitrary location (#2948). + if not self._validate_skill_subdir(skill_subdir, create=True): + continue frontmatter_data = registrar.build_skill_frontmatter( selected_ai, target_skill_name, @@ -1617,6 +1637,35 @@ class PresetManager: return None return skills_dir + def _validate_skill_subdir(self, skill_subdir: Path, *, create: bool) -> bool: + """Validate a single skill's subdirectory is symlink-free. + + Unlike :meth:`_safe_skills_dir_for_agent` (which only validates the + *parent* skills directory), this validates the skill's own + subdirectory — e.g. ``.claude/skills/speckit-specify`` — so a + symlink planted at that level (with a safe parent) can't be used to + write or delete through to a location outside the project. Shared by + both the registration path (``create=True``, so a missing directory + is created component-by-component under the same guard) and the + restore/removal path (``create=False``, so a missing directory is + left for the caller's own existence check to skip). Returns + ``False`` rather than raising when the path escapes the project + root or crosses a symlink. + """ + from ..shared_infra import _ensure_safe_shared_directory, _validate_safe_shared_directory + + try: + if create: + _ensure_safe_shared_directory( + self.project_root, skill_subdir, + create=True, context="preset skill directory", + ) + else: + _validate_safe_shared_directory(self.project_root, skill_subdir) + except (ValueError, OSError): + return False + return True + def _unregister_skills( self, registered_skills: Union[Dict[str, List[str]], List[str]], @@ -1737,6 +1786,12 @@ class PresetManager: skill_file = skill_subdir / "SKILL.md" if not skill_subdir.is_dir(): continue + # is_dir() follows symlinks, so a symlinked skill subdirectory + # (with a safe, non-symlinked parent) would otherwise slip past + # _safe_skills_dir_for_agent's parent-only check and have + # write_text/rmtree operate through it (#2948). + if not self._validate_skill_subdir(skill_subdir, create=False): + continue if not skill_file.is_file(): # Only manage directories that contain the expected skill entrypoint. continue @@ -2020,9 +2075,15 @@ class PresetManager: pack_dir = self.presets_dir / pack_id # Collect ALL command names before filtering for reconciliation, - # so commands registered only for skill-based agents are also reconciled. - # Also include aliases from the manifest as a safety net for registries - # populated by older versions that may not track aliases. + # so commands registered only for skill-based agents are also + # reconciled. Every command-type template's primary name is added + # unconditionally (not just aliases) since ai_skills-mode presets + # never populate registered_commands for command-backed + # integrations (see _register_commands's ai_skills guard) — without + # this, removing a skills-mode preset that overrides a command no + # other preset registered "the normal way" would skip reconciliation + # entirely and _unregister_skills would restore core/extension + # content instead of a surviving lower-priority preset's override. removed_cmd_names = set() for cmd_names in registered_commands.values(): removed_cmd_names.update(cmd_names) @@ -2032,6 +2093,9 @@ class PresetManager: manifest = PresetManifest(manifest_path) for tmpl in manifest.templates: if tmpl.get("type") == "command": + name = tmpl.get("name") + if isinstance(name, str): + removed_cmd_names.add(name) for alias in tmpl.get("aliases", []): if isinstance(alias, str): removed_cmd_names.add(alias) diff --git a/tests/test_presets.py b/tests/test_presets.py index 25172a09b..48ee39d8e 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4510,6 +4510,210 @@ class TestPresetSkills: assert "preset:shared-dir-preset" not in content assert "Core specify body" in content + def test_remove_higher_priority_skills_only_preset_restores_lower_preset( + self, project_dir, temp_dir + ): + """Removing a skills-mode preset must reconcile against the surviving + stack, not fall back to core/extension content. + + Copilot in skills mode never populates ``registered_commands`` for + its overrides (``_register_commands``'s ``ai_skills`` guard skips + command-file registration entirely), so with two presets overriding + the same command, the removed preset's command name was never added + to ``removed_cmd_names`` and reconciliation was skipped outright. + ``_unregister_skills`` then restored straight to core/extension + content instead of resolving the lower-priority preset that should + now win (#2948). + """ + self._write_init_options(project_dir, ai="copilot", ai_skills=True) + + preset_a_dir = self._create_command_preset( + temp_dir, "skills-preset-a", "speckit.specify", + "Preset A", "preset A body", + ) + preset_b_dir = self._create_command_preset( + temp_dir, "skills-preset-b", "speckit.specify", + "Preset B", "preset B body", + ) + + manager = PresetManager(project_dir) + # Lower priority number = higher precedence. + manager.install_from_directory(preset_a_dir, "0.1.5", priority=5) + manager.install_from_directory(preset_b_dir, "0.1.5", priority=10) + + skills_dir = project_dir / ".github" / "skills" + skill_file = skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:skills-preset-a" in skill_file.read_text(), ( + "sanity: the higher-precedence preset should win initially" + ) + + assert manager.remove("skills-preset-a") is True + + content = skill_file.read_text() + assert "preset:skills-preset-b" in content, ( + "removing the higher-precedence skills-mode preset must " + "restore the surviving lower-precedence preset's override, " + "not fall back to core/extension content (#2948)" + ) + assert "preset:skills-preset-a" not in content + + def test_composed_none_unregister_respects_active_agent( + self, project_dir, temp_dir + ): + """Unregistering a stale composed command must only touch the + active agent's directory, not every configured non-skill agent. + + When a wrap preset's base layer is removed, ``resolve_content`` can + no longer find a replace layer to compose onto and returns + ``None``, triggering the "composed is None" branch of + ``_reconcile_composed_commands``. Before the fix, that + unregistration mapping covered every configured non-skill agent + regardless of ``only_agent``, deleting historical artifacts from + integrations that were never active for this preset (#2948). + """ + self._write_init_options(project_dir, ai="gemini", ai_skills=False) + gemini_commands_dir = project_dir / ".gemini" / "commands" + gemini_commands_dir.mkdir(parents=True) + + # A made-up command name with no bundled/core equivalent, so the + # *only* base layer is the "compose-base" preset installed below — + # once it's removed, no base remains for the wrap preset to compose + # onto. + cmd_name = "speckit.fake-compose-test" + base_dir = self._create_command_preset( + temp_dir, "compose-base", cmd_name, "Base", "base body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(base_dir, "0.1.5", priority=10) + + wrap_dir = temp_dir / "compose-wrap" + wrap_dir.mkdir() + (wrap_dir / "commands").mkdir() + (wrap_dir / "commands" / f"{cmd_name}.md").write_text( + "---\ndescription: Wrap\nstrategy: wrap\n---\n\n" + "wrap start\n{CORE_TEMPLATE}\nwrap end\n" + ) + manifest_data = { + "schema_version": "1.0", + "preset": { + "id": "compose-wrap", + "name": "compose-wrap", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": cmd_name, + "file": f"commands/{cmd_name}.md", + "strategy": "wrap", + } + ] + }, + } + with open(wrap_dir / "preset.yml", "w") as f: + yaml.dump(manifest_data, f) + manager.install_from_directory(wrap_dir, "0.1.5", priority=5) + + claude_dir = project_dir / ".gemini" / "commands" + cmd_file = claude_dir / f"{cmd_name}.toml" + assert cmd_file.exists(), ( + "sanity: the composed command should register for the active agent" + ) + + # Simulate a pre-existing artifact for an inactive agent, predating + # this preset entirely — active-only unregistration must never + # touch it. + opencode_dir = project_dir / ".opencode" / "commands" + opencode_dir.mkdir(parents=True, exist_ok=True) + opencode_stale_file = opencode_dir / f"{cmd_name}.md" + opencode_stale_file.write_text("stale opencode content\n") + + assert manager.remove("compose-base") is True + + assert not cmd_file.exists(), ( + "sanity: the active agent's now-uncomposable command file must " + "be unregistered" + ) + assert opencode_stale_file.read_text() == "stale opencode content\n", ( + "unregistering a stale composed command must not touch an " + "inactive agent's directory (#2948)" + ) + + def test_symlinked_skill_subdir_rejected_on_restore(self, project_dir, temp_dir): + """Restore must validate each per-skill subdirectory, not just its parent. + + ``_safe_skills_dir_for_agent`` only validates the parent skills + directory (e.g. ``.claude/skills``); a symlink planted one level + deeper at the individual skill's own subdirectory (e.g. + ``.claude/skills/speckit-specify``) has a perfectly safe parent and + would otherwise slip past that check, since ``is_dir()`` follows + symlinks. Restoration must refuse to write/rmtree through it (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + claude_skills_dir.mkdir(parents=True) + + outside_target = temp_dir / "outside-skill-subdir" + outside_target.mkdir() + sentinel = outside_target / "SKILL.md" + sentinel.write_text("do-not-touch") + (claude_skills_dir / "speckit-specify").symlink_to( + outside_target, target_is_directory=True + ) + + manager = PresetManager(project_dir) + manager._unregister_skills_in_dir( + ["speckit-specify"], claude_skills_dir, "claude" + ) + + assert sentinel.read_text() == "do-not-touch", ( + "restoration must not follow a symlinked skill subdirectory " + "to write/delete outside the project (#2948)" + ) + assert (claude_skills_dir / "speckit-specify").is_symlink(), ( + "the symlink itself should be left alone, not rmtree'd through" + ) + + def test_symlinked_skill_subdir_rejected_on_write(self, project_dir, temp_dir): + """Registration must validate each per-skill subdirectory before writing. + + A symlink planted at an individual skill's own subdirectory (safe + parent, unsafe leaf) would otherwise pass the existing + ``skill_subdir.exists() and not skill_subdir.is_dir()`` guard + (``is_dir()`` follows symlinks) and have ``SKILL.md`` written + through it to an arbitrary location (#2948). + """ + self._write_init_options(project_dir, ai="copilot", ai_skills=True) + copilot_commands_dir = project_dir / ".github" / "agents" + copilot_commands_dir.mkdir(parents=True) + skills_dir = project_dir / ".github" / "skills" + skills_dir.mkdir(parents=True) + + outside_target = temp_dir / "outside-skill-write-target" + outside_target.mkdir() + (skills_dir / "speckit-specify").symlink_to( + outside_target, target_is_directory=True + ) + + preset_dir = self._create_command_preset( + temp_dir, "symlink-write-preset", "speckit.specify", + "Symlink write test", "preset body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + assert not (outside_target / "SKILL.md").exists(), ( + "registration must not follow a symlinked skill subdirectory " + "to write outside the project (#2948)" + ) + assert (skills_dir / "speckit-specify").is_symlink(), ( + "the symlink itself should be left alone" + ) + def test_copilot_skills_registration_restored_after_process_restart( self, project_dir, temp_dir ):