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 unscoped extension-skill removal and legacy preset provenance on direct remove

Browse Source 
- _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>
This commit is contained in:
marcelsafin
2026-07-11 10:56:17 +02:00
parent 28561e6fc1
commit 1d8f9e3989
4 changed files with 178 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

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

24
src/specify_cli/presets/__init__.py
View File

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

62
tests/test_extension_skills.py
View File

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

77
tests/test_presets.py
View File

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