From 1d8f9e3989aef1b87eb7618bb6b05e1e43be19ba Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 10:56:17 +0200 Subject: [PATCH] Fix unscoped extension-skill removal and legacy preset provenance on direct remove - _unregister_extension_skills(): omitting skills_dir now always triggers the full multi-directory fallback scan instead of narrowing to the currently active agent's directory. Previously, remove() (the only caller that omits skills_dir) would resolve the active agent's dir and take the scoped fast path, orphaning a previously-active second agent's extension skill mirror during full removal. - PresetManager.remove(): infer legacy flat-list registered_skills provenance (reusing _infer_legacy_skill_provenance from the prior rescaffold fix) before invoking _unregister_skills, so a direct `preset remove` with no intervening rescaffold/switch also restores every previously-active agent's directory instead of only the currently active one. Added regression tests: - test_remove_while_second_agent_still_in_skills_mode_cleans_up_first_agent_mirror - test_remove_infers_legacy_flat_list_provenance_without_prior_rescaffold Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 30 +++++----- src/specify_cli/presets/__init__.py | 24 ++++++++ tests/test_extension_skills.py | 62 +++++++++++++++++++++ tests/test_presets.py | 77 ++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 37d8e2dc3..d4e9bb69a 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1241,32 +1241,32 @@ class ExtensionManager: Called during extension removal to clean up skill files that were created by ``_register_extension_skills()``. - If *skills_dir* is not provided and ``_get_skills_dir()`` returns - ``None`` (e.g. the user removed init-options.json or toggled - ai_skills after installation), we fall back to scanning all known - agent skills directories so that orphaned skill directories are - still cleaned up. In that case each candidate directory is - verified against the SKILL.md ``metadata.source`` field before - removal to avoid accidentally deleting user-created skills with - the same name. + When *skills_dir* is omitted, this is a genuinely unscoped removal + (e.g. full extension removal): ``registered_skills`` is a single + flat list covering mirrors created under *every* agent this + extension was ever activated under, not just the currently active + one, so we always scan all known agent skills directories rather + than narrowing to whichever agent happens to be active right now. + Each candidate directory is verified against the SKILL.md + ``metadata.source`` field before removal to avoid accidentally + deleting user-created skills with the same name. Args: skill_names: List of skill names to remove. extension_id: Extension ID used to verify ownership during fallback candidate scanning. - skills_dir: Optional explicit skills directory to use instead - of resolving via ``_get_skills_dir()``. Useful when the - caller needs to target a specific agent's skills directory - regardless of the currently-active agent in init-options. + skills_dir: Optional explicit skills directory to scope + cleanup to. Useful when the caller needs to target a + specific agent's skills directory regardless of the + currently-active agent in init-options. When omitted, + every configured agent's skills directory is scanned + instead of resolving just the currently active one. """ if not skill_names: return from ..shared_infra import _validate_safe_shared_directory - if skills_dir is None: - skills_dir = self._get_skills_dir() - if skills_dir: # Reject the candidate directory itself (any path component, # including the final one) if it's a symlink escaping the diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 9728bc909..7852eb5d1 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -2247,6 +2247,30 @@ class PresetManager: metadata = self.registry.get(pack_id) # Restore original skills when preset is removed registered_skills = metadata.get("registered_skills", []) if metadata else [] + if isinstance(registered_skills, list) and registered_skills: + # Legacy flat-list registries predate per-agent provenance + # tracking. Migration to the per-agent dict form previously + # only happened during a rescaffold (register_enabled_presets_ + # for_agent); if the *first* post-upgrade operation is instead + # `preset remove` (no intervening use/upgrade), the legacy + # branch of _unregister_skills restores only the currently + # active agent's directory, leaving this preset's overrides in + # every previously active agent's directory orphaned. Infer + # real per-agent ownership from the on-disk preset marker now, + # while pack_id is still known, and hand the resulting mapping + # through the same dict-based cleanup path already used for + # non-legacy registries (#2948). + from .. import load_init_options + + init_opts = load_init_options(self.project_root) + fallback_agent = init_opts.get("ai") if isinstance(init_opts, dict) else None + if not isinstance(fallback_agent, str): + fallback_agent = "" + registered_skills = self._infer_legacy_skill_provenance( + [name for name in registered_skills if isinstance(name, str)], + pack_id, + fallback_agent=fallback_agent, + ) registered_commands = metadata.get("registered_commands", {}) if metadata else {} pack_dir = self.presets_dir / pack_id diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 17f9d8f1a..2cd89aeeb 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1448,6 +1448,68 @@ class TestExtensionSkillRegistration: "prematurely dropped during the earlier toggle (#2948)" ) + def test_remove_while_second_agent_still_in_skills_mode_cleans_up_first_agent_mirror( + self, project_dir, temp_dir + ): + """Full extension removal must clean up every previously-active + agent's mirror, not just the currently active one, even with no + intervening toggle. + + Auggie is activated first (skills mode), writing a mirror. + Copilot is then activated (also skills mode, still active at + removal time) and writes its own mirror for the same names. + ``remove()`` calls ``_unregister_extension_skills(registered_skills, + extension_id)`` with no explicit ``skills_dir`` — genuinely + unscoped, "clean up everywhere this extension owns something". + Before this fix, omitting ``skills_dir`` caused the method to + resolve the *currently active* agent's directory via + ``_get_skills_dir()`` and take the scoped fast path instead of the + all-directory fallback scan, so only Copilot's mirror was removed + and Auggie's was silently left orphaned (#2948). + """ + _create_init_options(project_dir, ai="auggie", ai_skills=True) + manager = ExtensionManager(project_dir) + manager.install_from_directory( + _create_extension_dir(temp_dir, ext_id="remove-multi-agent-ext"), "0.1.0", + register_commands=False, + ) + manager.register_enabled_extensions_for_agent("auggie") + + auggie_skills_dir = project_dir / ".augment" / "skills" + auggie_hello = auggie_skills_dir / "speckit-remove-multi-agent-ext-hello" / "SKILL.md" + auggie_world = auggie_skills_dir / "speckit-remove-multi-agent-ext-world" / "SKILL.md" + assert auggie_hello.exists() and auggie_world.exists(), ( + "sanity: auggie's skills-mode activation should mirror both " + "extension commands as SKILL.md files" + ) + + _create_init_options(project_dir, ai="copilot", ai_skills=True) + manager.register_enabled_extensions_for_agent("copilot") + + copilot_skills_dir = project_dir / ".github" / "skills" + copilot_hello = copilot_skills_dir / "speckit-remove-multi-agent-ext-hello" / "SKILL.md" + copilot_world = copilot_skills_dir / "speckit-remove-multi-agent-ext-world" / "SKILL.md" + assert copilot_hello.exists() and copilot_world.exists(), ( + "sanity: copilot's skills-mode activation should also mirror " + "both extension commands" + ) + + # Remove the extension while copilot (the second agent) is still + # the active, skills-mode agent — no toggle, no intervening + # rescaffold for auggie. + assert manager.remove("remove-multi-agent-ext") is True + + assert not copilot_hello.exists() and not copilot_world.exists(), ( + "sanity: the currently active agent's mirrors must be removed" + ) + assert not auggie_hello.exists() and not auggie_world.exists(), ( + "removal must also clean up the first agent's (auggie) " + "mirrors even though it is no longer the active agent — " + "omitting skills_dir must trigger the all-directory fallback " + "scan, not silently narrow to the currently active agent's " + "directory (#2948)" + ) + def test_extension_owned_skill_names_rejects_symlinked_candidate_directory( self, project_dir, temp_dir ): diff --git a/tests/test_presets.py b/tests/test_presets.py index 18a1b7318..fcbcf3171 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4907,6 +4907,83 @@ class TestPresetSkills: "side effect of probing for provenance (#2948)" ) + def test_remove_infers_legacy_flat_list_provenance_without_prior_rescaffold( + self, project_dir, temp_dir + ): + """``preset remove`` on a legacy flat-list registry must restore + every previously active agent's directory, not just the currently + active one, even when it is the *very first* post-upgrade + operation (no intervening ``use``/``upgrade``/rescaffold). + + Pre-#2948 registries recorded a flat ``registered_skills`` list + because presets were rendered for every detected skill-mode agent + at once, not just the active one. Migrating that legacy format to + the per-agent dict form previously only happened as a side effect + of ``register_enabled_presets_for_agent`` (i.e. a rescaffold or + ``integration use``/``switch``). If the user's first action after + upgrading is instead directly running ``preset remove``, the + legacy branch of ``_unregister_skills`` restored only the + currently active agent's directory (via ``_get_skills_dir()``), + permanently leaving this preset's override in every other, + previously active agent's directory (#2948). + """ + self._write_init_options(project_dir, ai="claude", 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", + ) + + claude_skills_dir = project_dir / ".claude" / "skills" + self._create_skill(claude_skills_dir, "speckit-specify") + codex_skills_dir = project_dir / ".agents" / "skills" + self._create_skill(codex_skills_dir, "speckit-specify") + + preset_dir = self._create_command_preset( + temp_dir, "remove-legacy-no-rescaffold-preset", "speckit.specify", + "Remove legacy no rescaffold test", "preset body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + claude_skill = claude_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:remove-legacy-no-rescaffold-preset" in claude_skill.read_text(), ( + "sanity: install should have written the override under " + "claude's skill directory" + ) + # Simulate the pre-#2948 "register for every detected agent" + # install behaviour by also placing the marker under codex's + # directory directly (mirroring the old, non-active-only + # rendering that predates this PR). + codex_skill = codex_skills_dir / "speckit-specify" / "SKILL.md" + codex_skill.write_text(claude_skill.read_text(), encoding="utf-8") + + # Simulate a pre-#2948 registry: a flat list with no per-agent + # provenance, even though both directories actually hold this + # preset's marker on disk. + manager.registry.update( + "remove-legacy-no-rescaffold-preset", + {"registered_skills": ["speckit-specify"]}, + ) + + # No intervening use/upgrade/rescaffold: remove() is the very + # first operation run after the legacy registry was written. + assert manager.remove("remove-legacy-no-rescaffold-preset") is True + + for skill_file, label in ((claude_skill, "claude"), (codex_skill, "codex")): + assert skill_file.exists(), f"{label} skill file should still exist after removal" + content = skill_file.read_text() + assert "preset:remove-legacy-no-rescaffold-preset" not in content, ( + f"{label}'s preset override must be restored on removal " + "even with no prior rescaffold to migrate the legacy " + "flat-list format first — remove() must infer real " + "per-agent ownership from on-disk provenance itself " + "(#2948)" + ) + 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.