github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: preserve extension skill tracking for mirrors in other agent dirs

Browse Source 
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:<id>" 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>
This commit is contained in:
marcelsafin
2026-07-11 09:51:21 +02:00
parent ee03a5055e
commit 37cceb7bc1
2 changed files with 191 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

107
src/specify_cli/extensions/__init__.py
View File

@@ -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

89
tests/test_extension_skills.py
View File

@@ -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
):