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: unregister stale opposite-mode preset artifact on same-agent skills toggle

Browse Source 
Fix an Important gap in register_enabled_presets_for_agent() surfaced by
quality review (#2948): toggling ai_skills for the *same already-active*
command-backed agent (e.g. `integration upgrade copilot` after flipping
ai_skills, with copilot staying active throughout) left a stale artifact
from the previous mode behind, violating the command/skill mutual-
exclusion invariant this PR otherwise enforces.

- command -> skills: _register_commands()'s ai_skills guard makes the
  commands phase a no-op, but the previously-written command file (e.g.
  .agent.md) and its registered_commands[agent] entry were never cleaned
  up, so it lingered alongside the newly written SKILL.md.
- skills -> command: _get_skills_dir() stops resolving a skills directory
  once ai_skills is off, making the skills phase a no-op, but the
  previously-written SKILL.md and its registered_skills[agent] entry were
  never cleaned up, so it lingered alongside the newly (re)written command
  file.

register_enabled_presets_for_agent() now resolves once per call whether
agent_name is a command-backed integration (extension != "/SKILL.md") and
the current ai_skills state, then narrowly unregisters the stale opposite-
mode entry for that agent via the existing _unregister_commands /
_unregister_skills helpers before persisting updated tracking — mirroring
the same per-agent, per-preset isolation already used elsewhere in this
method. Native skill-only agents (claude, codex, ...) are unaffected:
they have no command/skill toggle, so registered_commands and
registered_skills legitimately co-exist for them by design. The trailing
reconciliation pass, project-override precedence, and per-preset
partial-failure isolation are all unchanged.

Added red-first regression tests exercising the real install +
register_enabled_presets_for_agent rescaffold path in both toggle
directions:
- test_rescaffold_toggle_command_to_skills_removes_stale_command_file
- test_rescaffold_toggle_skills_to_command_removes_stale_skill_file

Both failed against the prior code (stale artifact persisted / registry
still tracked it) and pass after the fix.

Targeted (tests/test_presets.py, tests/test_extensions.py: 691 passed)
and full (3937 passed, 109 skipped) suites and ruff check on changed
files pass clean.

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 02:05:55 +02:00
parent 5b0e55946d
commit b9d9053b03
2 changed files with 164 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -751,6 +751,24 @@ class PresetManager:
if not agent_name:
return
# Resolve once: whether agent_name is a command-backed integration
# (extension != "/SKILL.md") currently running in skills mode, or
# vice versa. Native skill-only agents (extension == "/SKILL.md",
# e.g. claude/codex) have no command/skill toggle at all — both
# registered_commands and registered_skills legitimately co-exist
# for them by design, so this restriction only applies to
# command-backed integrations.
try:
from ..agents import CommandRegistrar
agent_config = CommandRegistrar().AGENT_CONFIGS.get(agent_name)
except ImportError:
agent_config = None
is_command_backed = bool(agent_config) and agent_config.get("extension") != "/SKILL.md"
ai_skills_now = is_command_backed and is_ai_skills_enabled(
load_init_options(self.project_root)
)
resolver = PresetResolver(self.project_root)
affected_cmd_names: set = set()
for pack_id, metadata in reversed(self.registry.list_by_priority()):
@@ -769,6 +787,15 @@ class PresetManager:
merged_commands = copy.deepcopy(existing_commands)
if registered_commands.get(agent_name):
merged_commands[agent_name] = registered_commands[agent_name]
elif ai_skills_now and merged_commands.get(agent_name):
# Toggled command -> skills for this same agent:
# _register_commands's ai_skills guard just made this a
# no-op, but the command file this preset wrote while
# command mode was active is still on disk and still
# tracked. Unregister it narrowly for this agent so
# command-mode and skills-mode artifacts stay mutually
# exclusive (#2948).
self._unregister_commands({agent_name: merged_commands.pop(agent_name)})
# Persist the commands phase immediately, mirroring
# install_from_directory(): _register_skills is an
# independently fallible phase, and if it raises, the files
@@ -784,6 +811,16 @@ class PresetManager:
merged_skills = copy.deepcopy(existing_skills)
if registered_skills.get(agent_name):
merged_skills[agent_name] = registered_skills[agent_name]
elif is_command_backed and not ai_skills_now and merged_skills.get(agent_name):
# Mirror image: toggled skills -> command for this same
# agent. _get_skills_dir() no longer resolves a skills
# directory once ai_skills is off, so _register_skills
# is a no-op — but the SKILL.md this preset wrote while
# skills mode was active is still tracked and still on
# disk. Restore/remove it narrowly for this agent (#2948).
self._unregister_skills(
{agent_name: merged_skills.pop(agent_name)}, pack_dir
)
if merged_skills != existing_skills:
self.registry.update(pack_id, {"registered_skills": merged_skills})

127
tests/test_presets.py
View File

@@ -4307,6 +4307,133 @@ class TestPresetSkills:
skill_file = skills_dir / "speckit-specify" / "SKILL.md"
assert "preset:copilot-skills-preset" in skill_file.read_text()
def test_rescaffold_toggle_command_to_skills_removes_stale_command_file(
self, project_dir, temp_dir
):
"""Toggling the *same* agent from command mode to skills mode must
remove the stale command-mode artifact, not just add the new one.
Copilot stays the active agent throughout (``integration upgrade
copilot`` after flipping ``ai_skills``, not a switch to a different
agent). Before the fix, ``_register_commands``'s ``ai_skills`` guard
made rescaffold a no-op for the commands phase once skills mode was
on, leaving the previously written ``.agent.md`` file and its
``registered_commands`` entry behind even though ``_register_skills``
went on to also write the ``SKILL.md`` mirror — violating the
command/skill mutual-exclusion invariant this PR otherwise enforces
(#2948).
"""
self._write_init_options(project_dir, ai="copilot", ai_skills=False)
copilot_commands_dir = project_dir / ".github" / "agents"
copilot_commands_dir.mkdir(parents=True)
preset_dir = self._create_command_preset(
temp_dir, "toggle-cmd-to-skill-preset", "speckit.specify",
"Toggle test", "preset body",
)
manager = PresetManager(project_dir)
manager.install_from_directory(preset_dir, "0.1.5")
cmd_file = copilot_commands_dir / "speckit.specify.agent.md"
assert cmd_file.exists(), (
"sanity: command mode should have written copilot's command file"
)
metadata = manager.registry.get("toggle-cmd-to-skill-preset")
assert metadata["registered_commands"].get("copilot"), (
"sanity: the command-mode write should be tracked for copilot"
)
# Flip ai_skills on for the *same* active agent and rescaffold, as
# `integration upgrade copilot` would after the mode toggle.
self._write_init_options(project_dir, ai="copilot", ai_skills=True)
manager.register_enabled_presets_for_agent("copilot")
assert not cmd_file.exists(), (
"the stale command-mode file must be removed once copilot has "
"toggled to skills mode for the same agent (#2948)"
)
metadata = manager.registry.get("toggle-cmd-to-skill-preset")
assert not metadata["registered_commands"].get("copilot"), (
"registered_commands must stop tracking copilot once its "
"artifact has been unregistered, or removal will try to clean "
"up a file that no longer exists (#2948)"
)
skill_file = project_dir / ".github" / "skills" / "speckit-specify" / "SKILL.md"
assert "preset:toggle-cmd-to-skill-preset" in skill_file.read_text(), (
"sanity: the new skills-mode artifact should still be written"
)
def test_rescaffold_toggle_skills_to_command_removes_stale_skill_file(
self, project_dir, temp_dir
):
"""Toggling the *same* agent from skills mode to command mode must
remove the stale skills-mode artifact, not just add the new one.
Mirror image of the command-to-skills toggle: once ``ai_skills`` is
turned off for copilot (still the active agent), ``_get_skills_dir``
stops resolving a skills directory for it, so ``_register_skills``
becomes a no-op — but the ``SKILL.md`` written while skills mode was
on, and its ``registered_skills`` entry, were left behind even
though ``_register_commands`` went on to (re)write the ``.agent.md``
command file, again breaking mutual exclusion (#2948).
"""
self._write_init_options(project_dir, ai="copilot", ai_skills=True)
copilot_commands_dir = project_dir / ".github" / "agents"
copilot_commands_dir.mkdir(parents=True)
skills_dir = project_dir / ".github" / "skills"
self._create_skill(skills_dir, "speckit-specify")
# A core template lets the stale skill restore to core content
# (instead of being removed entirely, since it has nothing to fall
# back to), matching how `_unregister_skills` behaves elsewhere.
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",
)
preset_dir = self._create_command_preset(
temp_dir, "toggle-skill-to-cmd-preset", "speckit.specify",
"Toggle test", "preset body",
)
manager = PresetManager(project_dir)
manager.install_from_directory(preset_dir, "0.1.5")
skill_file = skills_dir / "speckit-specify" / "SKILL.md"
assert "preset:toggle-skill-to-cmd-preset" in skill_file.read_text(), (
"sanity: skills mode should have written the SKILL.md mirror"
)
metadata = manager.registry.get("toggle-skill-to-cmd-preset")
assert metadata["registered_skills"].get("copilot"), (
"sanity: the skills-mode write should be tracked for copilot"
)
# Flip ai_skills off for the *same* active agent and rescaffold, as
# `integration upgrade copilot` would after the mode toggle.
self._write_init_options(project_dir, ai="copilot", ai_skills=False)
manager.register_enabled_presets_for_agent("copilot")
restored_content = skill_file.read_text()
assert "preset:toggle-skill-to-cmd-preset" not in restored_content, (
"the stale skills-mode artifact must be reverted once copilot "
"has toggled to command mode for the same agent (#2948)"
)
assert "Core specify body" in restored_content, (
"sanity: the skill should fall back to core content, not just "
"lose the preset's override"
)
metadata = manager.registry.get("toggle-skill-to-cmd-preset")
assert not metadata["registered_skills"].get("copilot"), (
"registered_skills must stop tracking copilot once its "
"artifact has been unregistered/restored (#2948)"
)
cmd_file = copilot_commands_dir / "speckit.specify.agent.md"
assert cmd_file.exists(), (
"sanity: the new command-mode artifact should still be written"
)
assert "preset body" in cmd_file.read_text()
def test_skill_switch_then_remove_restores_every_skill_agent_dir(
self, project_dir, temp_dir
):