From 0ab9a5f3f4fe2729ff096ac7eafdbcd190a1a95a Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 11:08:11 +0200 Subject: [PATCH] Keep unregister_agent_artifacts scoped to its agent when directory is absent ExtensionManager.unregister_agent_artifacts() converted its resolved agent_skills_dir to None whenever that directory didn't exist, before calling _unregister_extension_skills(). After 1d8f9e3, omitting skills_dir means "genuinely unscoped removal": scan every configured agent's directory, reserved for ExtensionManager.remove()'s full project cleanup. Since unregister_agent_artifacts is agent-scoped (used by switch to clean up the previous integration's artifacts), this caused it to delete every other agent's live extension skill mirrors whenever the target agent's own directory happened to be absent, e.g. unregistering an agent that was never activated. Fix: always pass the explicit, agent-scoped skills_dir, even when it doesn't exist on disk, so the fast path is a safe no-op for an absent directory instead of falling back to the all-agents scan. Registry reconciliation (dropping removed names from the flat registered_skills list) now only runs when the agent's directory actually exists, so an absent directory can't be misread as "these names were removed everywhere" and wipe tracking for mirrors that still legitimately live under other agents' directories. Added regression test: - test_unregister_agent_artifacts_stays_scoped_when_agent_dir_absent 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 | 50 ++++++++++++------- tests/test_extension_skills.py | 69 ++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 18 deletions(-) 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 ):