From 37cceb7bc1f5583523ca2e9d92425bdf79283c81 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 09:51:21 +0200 Subject: [PATCH] fix: preserve extension skill tracking for mirrors in other agent dirs The skills -> command toggle cleanup in register_enabled_extensions_for_agent() recomputed the remaining tracked registered_skills names by checking only the toggling agent's own skills directory. Since registered_skills is a single flat list shared across every agent an extension has ever been activated under (skills are only ever rendered for the active agent, so there is no per-agent registry key), a name whose mirror still existed under a *different*, previously-active agent's directory was incorrectly dropped from tracking as soon as the current agent's own copy was removed. A later full removal only iterates registered_skills, so the orphaned mirror under the other agent's directory was never found or cleaned up. Add _extension_owned_skill_names(), which re-verifies ownership across every configured agent's skills directory (deduped by shared path) the same way the existing _unregister_extension_skills() fallback scan already does, keeping a name only when a SKILL.md with a matching metadata.source == "extension:" marker is found somewhere - read-only, no directory creation, no symlink escape. Use it instead of re-checking only the toggling agent's own directory when recomputing what remains tracked after narrow stale-mirror cleanup. Add a red-first regression test: Auggie is activated in skills mode first (writing a mirror), then Copilot is activated in skills mode (writing its own mirror for the same names), then Copilot toggles to command mode. Before the fix, registered_skills lost both names entirely even though Auggie's mirrors were untouched on disk; after the fix tracking is preserved and a subsequent full removal correctly cleans up Auggie's remaining mirrors too. 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 | 107 +++++++++++++++++++++++-- tests/test_extension_skills.py | 89 ++++++++++++++++++++ 2 files changed, 191 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 669de120c..2d023fc4b 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1366,6 +1366,93 @@ class ExtensionManager: continue shutil.rmtree(skill_subdir) + def _extension_owned_skill_names( + self, skill_names: List[str], extension_id: str + ) -> List[str]: + """Return the subset of *skill_names* still marker-verified anywhere. + + ``registered_skills`` is a single flat list shared across every + agent this extension has ever been activated under (skills are + only ever rendered for the currently active agent, so there is no + per-agent registry key to consult). A name can therefore still be + globally owned by this extension even after it's removed from one + particular agent's directory, if an earlier activation under a + *different* agent left its own marker-verified mirror behind. + + This scans the same candidate directories (every configured + agent's skills folder, deduped by shared path, plus the default + skills directory) as the fallback branch of + :meth:`_unregister_extension_skills`, but read-only: no directory + is created and a name is only kept if at least one candidate + directory contains a ``SKILL.md`` whose ``metadata.source`` field + matches this exact extension (the same ownership marker + :meth:`_register_extension_skills` writes), so an unrelated + directory or user-created skill of the same name can't cause a + false positive. Symlink/containment safety mirrors the existing + fallback scan: each candidate path is resolved and the resulting + skill subdirectory is required to stay within it before any file + is read. + """ + if not skill_names: + return [] + + from .. import AGENT_CONFIG, DEFAULT_SKILLS_DIR + + candidate_dirs: set[Path] = set() + for cfg in AGENT_CONFIG.values(): + folder = cfg.get("folder", "") + if folder: + candidate_dirs.add(self.project_root / folder.rstrip("/") / "skills") + candidate_dirs.add(self.project_root / DEFAULT_SKILLS_DIR) + + marker = f"extension:{extension_id}" + owned: set = set() + for skills_candidate in candidate_dirs: + if len(owned) == len(skill_names): + break # every name already confirmed owned somewhere + if not skills_candidate.is_dir(): + 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 + try: + skill_subdir = (skills_candidate / skill_name).resolve() + skill_subdir.relative_to(resolved_candidate) # raises if outside + except (OSError, ValueError): + continue + if not skill_subdir.is_dir(): + continue + skill_md = skill_subdir / "SKILL.md" + if not skill_md.is_file(): + continue + try: + import yaml as _yaml + + raw = skill_md.read_text(encoding="utf-8") + source = "" + if raw.startswith("---"): + parts = raw.split("---", 2) + if len(parts) >= 3: + fm = _yaml.safe_load(parts[1]) or {} + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) + except (OSError, UnicodeDecodeError, Exception): + continue + if source == marker: + owned.add(skill_name) + + return [name for name in skill_names if name in owned] + def check_compatibility( self, manifest: ExtensionManifest, speckit_version: str ) -> bool: @@ -1940,11 +2027,21 @@ class ExtensionManager: self._unregister_extension_skills( owned_here, ext_id, skills_dir=agent_skills_dir ) - remaining = [ - name - for name in existing_skills - if (agent_skills_dir / name).is_dir() - ] + # registered_skills is a single flat list + # shared across every agent this extension + # was ever activated under (unlike presets' + # per-agent dict), so a name removed from + # *this* agent's directory may still have a + # marker-verified mirror under a different, + # previously-active agent's directory. + # Recompute across every safe, supported + # skills directory rather than just this + # one, or a still-existing mirror elsewhere + # would be silently dropped from tracking + # and orphaned on later removal (#2948). + remaining = self._extension_owned_skill_names( + existing_skills, ext_id + ) if remaining != existing_skills: updates["registered_skills"] = remaining diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 8a7af933f..eee68b8f3 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1359,6 +1359,95 @@ class TestExtensionSkillRegistration: "skill file, not left dangling once it's orphaned (#2948)" ) + def test_toggle_to_command_preserves_tracking_for_mirror_in_other_agent_dir( + self, project_dir, temp_dir + ): + """Skills->command toggle cleanup must not drop global tracking for a + skill name that still has a mirror under a *different* agent's + skills directory from an earlier activation. + + ``registered_skills`` is a flat, agent-agnostic list for extensions + (skills are only ever rendered for the currently active agent, by + design). Auggie is activated first (skills mode), writing a mirror + under ``.augment/skills``. Copilot is then activated (also skills + mode), writing its own mirror under ``.github/skills`` for the same + skill names — the flat list already contains those names, so + nothing new is added. Copilot is then toggled to command mode: its + own ``.github/skills`` mirror becomes stale and must be removed, + but the still-existing Auggie mirror means the extension still + globally owns these skill names. Before this fix, the recompute + after toggle-cleanup only checked *copilot's* directory, so it + dropped the names from ``registered_skills`` entirely — losing + track of Auggie's still-existing mirror, which a later `remove()` + would then never find and clean up (or restore during override + reconciliation), permanently orphaning it (#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="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-multi-agent-ext-hello" / "SKILL.md" + auggie_world = auggie_skills_dir / "speckit-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" + ) + + # Activate copilot in skills mode too (no intervening removal of + # auggie's mirrors) — the same extension's skills get mirrored a + # second time, under a different agent's directory. + _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-multi-agent-ext-hello" / "SKILL.md" + copilot_world = copilot_skills_dir / "speckit-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" + ) + + # Toggle copilot to command mode (mirroring `integration upgrade + # copilot` with no --skills) — copilot's mirror is now stale. + _create_init_options(project_dir, ai="copilot", ai_skills=False) + manager.register_enabled_extensions_for_agent("copilot") + + assert not copilot_hello.exists() and not copilot_world.exists(), ( + "copilot's own stale skills-mode mirrors must be removed once " + "it toggles to command mode" + ) + assert auggie_hello.exists() and auggie_world.exists(), ( + "auggie's mirrors from an earlier activation must be left " + "untouched by copilot's own toggle cleanup" + ) + + metadata = manager.registry.get("multi-agent-ext") + registered_skills = metadata.get("registered_skills", []) + assert set(registered_skills) == { + "speckit-multi-agent-ext-hello", + "speckit-multi-agent-ext-world", + }, ( + "registered_skills must retain both names: auggie's mirrors " + "still exist on disk, so the extension still globally owns " + "these skill names even though copilot's own copy is now gone " + "(#2948)" + ) + + # Full removal must still find and clean up Auggie's remaining + # mirrors via the preserved tracking. + assert manager.remove("multi-agent-ext") is True + assert not auggie_hello.exists() and not auggie_world.exists(), ( + "removal must clean up every remaining extension-owned mirror, " + "not just the ones under the last-active agent's directory — " + "this only works if registered_skills tracking wasn't " + "prematurely dropped during the earlier toggle (#2948)" + ) + def test_existing_agent_command_path_file_is_not_detected( self, project_dir, temp_dir ):