diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index d4e9bb69a..8288a9adb 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1871,30 +1871,44 @@ class ExtensionManager: metadata.get("registered_skills", []) ) if registered_skills: - # Only pass the resolved skills_dir when it actually exists. - # Otherwise let _unregister_extension_skills fall back to - # scanning all known agent skills directories, which is useful - # for cleaning up stale entries created by earlier installs. - skills_dir = agent_skills_dir if agent_skills_dir.is_dir() else None + # Always pass the explicit, agent-scoped skills_dir — even + # when it doesn't currently exist on disk. This method must + # stay scoped to *this* agent only; omitting skills_dir (a + # bare ``None``) tells _unregister_extension_skills "this is + # a genuinely unscoped removal", which triggers its + # all-configured-agents fallback scan — reserved for + # ExtensionManager.remove()'s full project cleanup. If this + # agent's directory doesn't exist, there is nothing under it + # to clean up; the fast path below is a safe no-op in that + # case (every candidate skill_subdir.is_dir() check fails). self._unregister_extension_skills( - registered_skills, ext_id, skills_dir=skills_dir + registered_skills, ext_id, skills_dir=agent_skills_dir ) - # Only reconcile registry state when cleanup was scoped to a - # specific existing directory. When skills_dir is None, - # _unregister_extension_skills falls back to scanning multiple - # candidate directories, so agent_skills_dir cannot be used to - # infer what was removed. When skills_dir is set, - # _unregister_extension_skills may intentionally skip deletion - # when ownership cannot be verified (e.g., corrupted/missing - # SKILL.md or mismatching metadata.source). Only drop registry - # entries for skill directories that were actually removed so - # future cleanup attempts can still find skipped ones. - if skills_dir is not None: + # Only reconcile registry state when this agent's directory + # actually exists. When it's absent, this agent never had + # any of these skills mirrored under its own directory in + # the first place, so there is nothing to conclude about + # global ``registered_skills`` tracking from that absence — + # other agents' directories may still legitimately hold + # live mirrors for these same names (the flat list is + # agent-agnostic). Recomputing "remaining" against an + # absent directory would incorrectly conclude every name + # was removed and drop them all from the registry, silently + # orphaning any still-live mirrors under other agents' + # directories from future cleanup/removal. + # + # When the directory does exist, _unregister_extension_skills + # may intentionally skip deletion when ownership cannot be + # verified (e.g., corrupted/missing SKILL.md or mismatching + # metadata.source). Only drop registry entries for skill + # directories that were actually removed so future cleanup + # attempts can still find skipped ones. + if agent_skills_dir.is_dir(): remaining_skills = [ skill_name for skill_name in registered_skills - if (skills_dir / skill_name).is_dir() + if (agent_skills_dir / skill_name).is_dir() ] if remaining_skills != registered_skills: updates["registered_skills"] = remaining_skills diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 2cd89aeeb..0cbad8007 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1510,6 +1510,75 @@ class TestExtensionSkillRegistration: "directory (#2948)" ) + def test_unregister_agent_artifacts_stays_scoped_when_agent_dir_absent( + self, project_dir, temp_dir + ): + """``unregister_agent_artifacts`` must remain scoped to the given + agent even when that agent's own skills directory does not exist — + it must never fall through to the genuinely-unscoped, all-directory + removal semantics reserved for ``remove()``. + + Auggie and Copilot are both activated in skills mode, each writing + its own mirror. ``unregister_agent_artifacts("cursor-agent")`` is + then called for a supported agent that was *never* activated, so + its skills directory does not exist on disk. Before this fix, the + caller converted the resolved-but-absent directory to ``None`` + before calling ``_unregister_extension_skills`` — and after the + prior fix (1d8f9e3), omitting ``skills_dir`` means "scan and clean + up every configured agent's directory", not "this specific agent + has nothing to clean up". That silently deleted Auggie's and + Copilot's still-live mirrors while "cleaning up" an agent that was + never even active. + """ + _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="scoped-unregister-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-scoped-unregister-ext-hello" / "SKILL.md" + auggie_world = auggie_skills_dir / "speckit-scoped-unregister-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-scoped-unregister-ext-hello" / "SKILL.md" + copilot_world = copilot_skills_dir / "speckit-scoped-unregister-ext-world" / "SKILL.md" + assert copilot_hello.exists() and copilot_world.exists(), ( + "sanity: copilot's skills-mode activation should also mirror " + "both extension commands" + ) + + cursor_skills_dir = project_dir / ".cursor" / "skills" + assert not cursor_skills_dir.exists(), ( + "sanity: cursor-agent was never activated so it has no " + "skills directory on disk" + ) + + # Unregister artifacts for an agent that was never activated (its + # skills directory is absent). This must be a no-op with respect + # to other agents' live mirrors. + manager.unregister_agent_artifacts("cursor-agent") + + assert auggie_hello.exists() and auggie_world.exists(), ( + "unregistering a never-activated agent's artifacts must not " + "delete auggie's live mirror — an absent target directory " + "must not widen cleanup to every configured agent directory" + ) + assert copilot_hello.exists() and copilot_world.exists(), ( + "unregistering a never-activated agent's artifacts must not " + "delete copilot's live mirror — an absent target directory " + "must not widen cleanup to every configured agent directory" + ) + def test_extension_owned_skill_names_rejects_symlinked_candidate_directory( self, project_dir, temp_dir ):