mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
Verify replacement actually landed before retiring stale toggle artifacts
The command<->skills toggle cleanup added for #2948 deferred destructive removal of the old-mode artifact until after the replacement registration call completed without raising. That was necessary but not sufficient: none of _register_skills(), _register_commands(), register_commands_for_agent(), or _register_extension_skills() raise on a missing source template, a safety-validation skip, or a corrupted manifest entry — they simply return an empty or partial result. Treating "did not raise" as "fully replaced" meant a stale artifact could still be deleted (or its tracking dropped) even though its specific replacement never actually landed, leaving neither artifact in place for that logical command/skill. Fix all four affected toggle directions by checking the replacement call's actual return value before allowing any destructive step: - presets command->skills (register_enabled_presets_for_agent): only unregister a stale command name once its corresponding skill name (via the existing _skill_names_for_command() helper) is confirmed present in the skills call's returned names for that agent; the remainder stays tracked and on disk. - presets skills->command (register_enabled_presets_for_agent): only unregister a stale skill name once its corresponding command name is confirmed present in the commands call's returned names for that agent, using the same helper. - extensions skills->command (register_enabled_extensions_for_agent): only remove a skill mirror once the matching command (mapped via the existing HookExecutor._skill_name_from_command() helper) is confirmed present in register_commands_for_agent's returned names. - extensions command->skills (register_enabled_extensions_for_agent): only remove a deferred stale command once its matching skill name is confirmed present in _register_extension_skills()'s returned names. All four reuse the existing command<->skill name-derivation helpers rather than inventing new mapping logic. Registry tracking is updated to retain exactly the unreplaced subset rather than being popped wholesale, so partially-successful toggles leave correct, minimal tracking behind. Added 8 new regression tests (4 presets, 4 extensions) covering both the fully-empty and genuinely-partial result cases for each of the four toggle directions, using real missing-source-file scenarios (not mocked return values) to exercise the actual code paths. Confirmed red before the fix and green after for all 8. Focused (test_presets.py, test_extension_skills.py, test_extensions.py, tests/integrations): 2551 passed, 1 skipped. Full suite (tests, excluding the pre-existing environment-local 1Password-signing git-extension failures): 3917 passed, 74 skipped, 90 deselected. ruff check: 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:
@@ -2103,9 +2103,26 @@ class ExtensionManager:
|
|||||||
for name in existing_skills
|
for name in existing_skills
|
||||||
if (agent_skills_dir / name).is_dir()
|
if (agent_skills_dir / name).is_dir()
|
||||||
]
|
]
|
||||||
if owned_here:
|
# Only retire a skill mirror when the
|
||||||
|
# replacement command for the same logical
|
||||||
|
# command was actually written this call —
|
||||||
|
# `registered` (from register_commands_for_agent
|
||||||
|
# above) may be empty or a partial subset
|
||||||
|
# (missing source file, safety rejection,
|
||||||
|
# corrupted manifest), and removing a skill
|
||||||
|
# mirror whose command replacement never
|
||||||
|
# landed would leave neither artifact (#2948).
|
||||||
|
replaced_skill_names = {
|
||||||
|
HookExecutor._skill_name_from_command(cmd_name)
|
||||||
|
for cmd_name in (registered or [])
|
||||||
|
}
|
||||||
|
to_remove = [
|
||||||
|
name for name in owned_here
|
||||||
|
if name in replaced_skill_names
|
||||||
|
]
|
||||||
|
if to_remove:
|
||||||
self._unregister_extension_skills(
|
self._unregister_extension_skills(
|
||||||
owned_here, ext_id, skills_dir=agent_skills_dir
|
to_remove, ext_id, skills_dir=agent_skills_dir
|
||||||
)
|
)
|
||||||
# registered_skills is a single flat list
|
# registered_skills is a single flat list
|
||||||
# shared across every agent this extension
|
# shared across every agent this extension
|
||||||
@@ -2132,14 +2149,38 @@ class ExtensionManager:
|
|||||||
# registration is confirmed, so the stale
|
# registration is confirmed, so the stale
|
||||||
# command-mode artifact can finally be removed
|
# command-mode artifact can finally be removed
|
||||||
# without risking a transient state where neither
|
# without risking a transient state where neither
|
||||||
# artifact exists (#2948).
|
# artifact exists. Only retire a stale command
|
||||||
|
# whose corresponding skill was actually returned
|
||||||
|
# this call — `registered_skills` may be empty or
|
||||||
|
# a partial subset (missing source file, safety
|
||||||
|
# rejection, corrupted manifest), and unregistering
|
||||||
|
# a command whose skill replacement never landed
|
||||||
|
# would leave neither artifact (#2948).
|
||||||
if deferred_stale_commands:
|
if deferred_stale_commands:
|
||||||
|
replaced_skill_names = set(registered_skills or [])
|
||||||
|
fully_replaced = [
|
||||||
|
cmd_name for cmd_name in deferred_stale_commands
|
||||||
|
if HookExecutor._skill_name_from_command(cmd_name)
|
||||||
|
in replaced_skill_names
|
||||||
|
]
|
||||||
|
if fully_replaced:
|
||||||
registrar.unregister_commands(
|
registrar.unregister_commands(
|
||||||
{agent_name: deferred_stale_commands}, self.project_root
|
{agent_name: fully_replaced}, self.project_root
|
||||||
)
|
)
|
||||||
registered_commands = metadata.get("registered_commands", {})
|
registered_commands = metadata.get(
|
||||||
if isinstance(registered_commands, dict):
|
"registered_commands", {}
|
||||||
|
)
|
||||||
|
if isinstance(registered_commands, dict) and (
|
||||||
|
registered_commands.get(agent_name)
|
||||||
|
):
|
||||||
new_registered = copy.deepcopy(registered_commands)
|
new_registered = copy.deepcopy(registered_commands)
|
||||||
|
remaining_commands = [
|
||||||
|
c for c in new_registered[agent_name]
|
||||||
|
if c not in fully_replaced
|
||||||
|
]
|
||||||
|
if remaining_commands:
|
||||||
|
new_registered[agent_name] = remaining_commands
|
||||||
|
else:
|
||||||
new_registered.pop(agent_name, None)
|
new_registered.pop(agent_name, None)
|
||||||
if new_registered != registered_commands:
|
if new_registered != registered_commands:
|
||||||
updates["registered_commands"] = new_registered
|
updates["registered_commands"] = new_registered
|
||||||
|
|||||||
@@ -854,11 +854,28 @@ class PresetManager:
|
|||||||
# direction is already register-new-then-remove-old:
|
# direction is already register-new-then-remove-old:
|
||||||
# _register_commands (the replacement) ran unconditionally
|
# _register_commands (the replacement) ran unconditionally
|
||||||
# above and only reaches here once it has already
|
# above and only reaches here once it has already
|
||||||
# succeeded, so this cleanup happens only after the new
|
# succeeded — but that call can still have returned
|
||||||
# command artifact is confirmed in place (#2948).
|
# empty or partial results (missing source template,
|
||||||
self._unregister_skills(
|
# safety-validation skip, corrupted manifest), so only
|
||||||
{agent_name: merged_skills.pop(agent_name)}, pack_dir
|
# retire the subset of stale skills whose corresponding
|
||||||
)
|
# command name was actually returned for this agent;
|
||||||
|
# anything unreplaced stays tracked and on disk (#2948).
|
||||||
|
stale_skill_names = merged_skills[agent_name]
|
||||||
|
replaced_skill_names: set = set()
|
||||||
|
for cmd_name in registered_commands.get(agent_name) or []:
|
||||||
|
modern_name, legacy_name = self._skill_names_for_command(cmd_name)
|
||||||
|
replaced_skill_names.add(modern_name)
|
||||||
|
replaced_skill_names.add(legacy_name)
|
||||||
|
to_remove = [n for n in stale_skill_names if n in replaced_skill_names]
|
||||||
|
remaining_stale = [
|
||||||
|
n for n in stale_skill_names if n not in replaced_skill_names
|
||||||
|
]
|
||||||
|
if to_remove:
|
||||||
|
self._unregister_skills({agent_name: to_remove}, pack_dir)
|
||||||
|
if remaining_stale:
|
||||||
|
merged_skills[agent_name] = remaining_stale
|
||||||
|
else:
|
||||||
|
merged_skills.pop(agent_name, None)
|
||||||
# A legacy flat-list registered_skills value (predating
|
# A legacy flat-list registered_skills value (predating
|
||||||
# per-agent provenance) must migrate to the dict format on
|
# per-agent provenance) must migrate to the dict format on
|
||||||
# disk even when the rescaffolded names are unchanged from
|
# disk even when the rescaffolded names are unchanged from
|
||||||
@@ -874,12 +891,30 @@ class PresetManager:
|
|||||||
if merged_skills != existing_skills or needs_migration:
|
if merged_skills != existing_skills or needs_migration:
|
||||||
self.registry.update(pack_id, {"registered_skills": merged_skills})
|
self.registry.update(pack_id, {"registered_skills": merged_skills})
|
||||||
|
|
||||||
# The skills phase above completed without raising, so the
|
# The skills phase above completed without raising, but a
|
||||||
# replacement artifact is confirmed — now it's safe to
|
# non-raising result can still be empty or partial (missing
|
||||||
# remove the stale command-mode artifact deferred earlier
|
# source template, safety-validation skip, corrupted
|
||||||
# (#2948).
|
# manifest) — retiring every stale command purely on "did
|
||||||
|
# not raise" would delete a command whose replacement skill
|
||||||
|
# never actually landed, leaving neither artifact. Only
|
||||||
|
# retire the subset of stale commands whose corresponding
|
||||||
|
# skill name was actually returned for this agent; anything
|
||||||
|
# unreplaced stays tracked and on disk (#2948).
|
||||||
if stale_command_names:
|
if stale_command_names:
|
||||||
self._unregister_commands({agent_name: stale_command_names})
|
replaced_skill_names = set(registered_skills.get(agent_name) or [])
|
||||||
|
fully_replaced = []
|
||||||
|
remaining_stale = []
|
||||||
|
for cmd_name in stale_command_names:
|
||||||
|
modern_name, legacy_name = self._skill_names_for_command(cmd_name)
|
||||||
|
if modern_name in replaced_skill_names or legacy_name in replaced_skill_names:
|
||||||
|
fully_replaced.append(cmd_name)
|
||||||
|
else:
|
||||||
|
remaining_stale.append(cmd_name)
|
||||||
|
if fully_replaced:
|
||||||
|
self._unregister_commands({agent_name: fully_replaced})
|
||||||
|
if remaining_stale:
|
||||||
|
merged_commands[agent_name] = remaining_stale
|
||||||
|
else:
|
||||||
merged_commands.pop(agent_name, None)
|
merged_commands.pop(agent_name, None)
|
||||||
self.registry.update(pack_id, {"registered_commands": merged_commands})
|
self.registry.update(pack_id, {"registered_commands": merged_commands})
|
||||||
except Exception as pack_err:
|
except Exception as pack_err:
|
||||||
|
|||||||
@@ -1366,6 +1366,221 @@ class TestExtensionSkillRegistration:
|
|||||||
"command file when the skills replacement failed (#2948)"
|
"command file when the skills replacement failed (#2948)"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
def test_toggle_command_to_skills_empty_result_preserves_old_extension_command(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""An empty (non-raising) skills result must not delete any old
|
||||||
|
extension command artifact.
|
||||||
|
|
||||||
|
Deleting both of the extension's own installed command source
|
||||||
|
files makes ``_register_extension_skills`` genuinely return ``[]``
|
||||||
|
for copilot without raising — a real "missing source" case, not an
|
||||||
|
injected exception — which must leave both old command-mode
|
||||||
|
artifacts and their tracking untouched (#2948).
|
||||||
|
"""
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||||
|
manager = ExtensionManager(project_dir)
|
||||||
|
manager.install_from_directory(
|
||||||
|
_create_extension_dir(temp_dir, ext_id="toggle-empty-ext"), "0.1.0",
|
||||||
|
register_commands=False,
|
||||||
|
)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
agents_dir = project_dir / ".github" / "agents"
|
||||||
|
hello_cmd_file = agents_dir / "speckit.toggle-empty-ext.hello.agent.md"
|
||||||
|
world_cmd_file = agents_dir / "speckit.toggle-empty-ext.world.agent.md"
|
||||||
|
assert hello_cmd_file.exists() and world_cmd_file.exists(), (
|
||||||
|
"sanity: command mode should have written both command files"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Remove both installed command sources so _register_extension_skills
|
||||||
|
# can find nothing to render for either command.
|
||||||
|
ext_commands_dir = manager.extensions_dir / "toggle-empty-ext" / "commands"
|
||||||
|
(ext_commands_dir / "hello.md").unlink()
|
||||||
|
(ext_commands_dir / "world.md").unlink()
|
||||||
|
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=True)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
assert hello_cmd_file.exists() and world_cmd_file.exists(), (
|
||||||
|
"an empty (non-raising) skills registration result must not "
|
||||||
|
"cause the old command-mode artifacts to be deleted (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-empty-ext")
|
||||||
|
registered_commands = metadata.get("registered_commands", {}).get("copilot", [])
|
||||||
|
assert set(registered_commands) == {
|
||||||
|
"speckit.toggle-empty-ext.hello", "speckit.toggle-empty-ext.world",
|
||||||
|
}, (
|
||||||
|
"registered_commands must keep tracking copilot's still-live "
|
||||||
|
"command files when nothing was actually replaced (#2948)"
|
||||||
|
)
|
||||||
|
skills_dir = project_dir / ".github" / "skills"
|
||||||
|
assert not (skills_dir / "speckit-toggle-empty-ext-hello").exists(), (
|
||||||
|
"sanity: no skill should have been written when both sources "
|
||||||
|
"were missing"
|
||||||
|
)
|
||||||
|
assert not (skills_dir / "speckit-toggle-empty-ext-world").exists()
|
||||||
|
|
||||||
|
def test_toggle_command_to_skills_partial_result_only_removes_replaced_extension_command(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""Only the extension command whose skill replacement actually
|
||||||
|
landed is retired.
|
||||||
|
|
||||||
|
A two-command extension (hello, world) where only ``world``'s
|
||||||
|
installed source goes missing right before the toggle, so
|
||||||
|
``_register_extension_skills`` genuinely returns a partial result
|
||||||
|
(only ``hello``'s skill). ``hello``'s old command artifact must be
|
||||||
|
retired; ``world``'s must survive with its tracking intact, since
|
||||||
|
no replacement for it landed (#2948).
|
||||||
|
"""
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||||
|
manager = ExtensionManager(project_dir)
|
||||||
|
manager.install_from_directory(
|
||||||
|
_create_extension_dir(temp_dir, ext_id="toggle-partial-ext"), "0.1.0",
|
||||||
|
register_commands=False,
|
||||||
|
)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
agents_dir = project_dir / ".github" / "agents"
|
||||||
|
hello_cmd_file = agents_dir / "speckit.toggle-partial-ext.hello.agent.md"
|
||||||
|
world_cmd_file = agents_dir / "speckit.toggle-partial-ext.world.agent.md"
|
||||||
|
assert hello_cmd_file.exists() and world_cmd_file.exists(), (
|
||||||
|
"sanity: command mode should have written both command files"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Remove only world's installed source so its skill replacement is
|
||||||
|
# silently skipped (missing source), while hello's succeeds.
|
||||||
|
ext_commands_dir = manager.extensions_dir / "toggle-partial-ext" / "commands"
|
||||||
|
(ext_commands_dir / "world.md").unlink()
|
||||||
|
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=True)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
assert not hello_cmd_file.exists(), (
|
||||||
|
"hello's old command artifact must be retired since its skill "
|
||||||
|
"replacement actually landed (#2948)"
|
||||||
|
)
|
||||||
|
assert world_cmd_file.exists(), (
|
||||||
|
"world's old command artifact must survive since its skill "
|
||||||
|
"replacement never landed (missing source) (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-partial-ext")
|
||||||
|
registered_commands = metadata.get("registered_commands", {}).get("copilot", [])
|
||||||
|
assert registered_commands == ["speckit.toggle-partial-ext.world"], (
|
||||||
|
"registered_commands must stop tracking hello (retired) but "
|
||||||
|
"keep tracking world (still live) (#2948)"
|
||||||
|
)
|
||||||
|
skills_dir = project_dir / ".github" / "skills"
|
||||||
|
assert (skills_dir / "speckit-toggle-partial-ext-hello" / "SKILL.md").exists()
|
||||||
|
assert not (skills_dir / "speckit-toggle-partial-ext-world").exists()
|
||||||
|
|
||||||
|
def test_toggle_skills_to_command_empty_result_preserves_old_extension_skill(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""An empty (non-raising) command result must not delete any old
|
||||||
|
extension skill artifact.
|
||||||
|
|
||||||
|
Mirror image of the empty-result command->skills case: deleting
|
||||||
|
both of the extension's own installed command source files makes
|
||||||
|
``register_commands_for_agent`` genuinely return an empty/falsy
|
||||||
|
result for copilot without raising, which must leave both old
|
||||||
|
skill-mode artifacts and their tracking untouched (#2948).
|
||||||
|
"""
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=True)
|
||||||
|
manager = ExtensionManager(project_dir)
|
||||||
|
manager.install_from_directory(
|
||||||
|
_create_extension_dir(temp_dir, ext_id="toggle-skill-empty-ext"), "0.1.0",
|
||||||
|
register_commands=False,
|
||||||
|
)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
skills_dir = project_dir / ".github" / "skills"
|
||||||
|
hello_skill_file = skills_dir / "speckit-toggle-skill-empty-ext-hello" / "SKILL.md"
|
||||||
|
world_skill_file = skills_dir / "speckit-toggle-skill-empty-ext-world" / "SKILL.md"
|
||||||
|
assert hello_skill_file.exists() and world_skill_file.exists(), (
|
||||||
|
"sanity: skills mode should have written both SKILL.md mirrors"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Remove both installed command sources so register_commands_for_agent
|
||||||
|
# can find nothing to render for either command.
|
||||||
|
ext_commands_dir = manager.extensions_dir / "toggle-skill-empty-ext" / "commands"
|
||||||
|
(ext_commands_dir / "hello.md").unlink()
|
||||||
|
(ext_commands_dir / "world.md").unlink()
|
||||||
|
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
assert hello_skill_file.exists() and world_skill_file.exists(), (
|
||||||
|
"an empty (non-raising) command registration result must not "
|
||||||
|
"cause the old skills-mode artifacts to be deleted (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-skill-empty-ext")
|
||||||
|
registered_skills = metadata.get("registered_skills", [])
|
||||||
|
assert {
|
||||||
|
"speckit-toggle-skill-empty-ext-hello",
|
||||||
|
"speckit-toggle-skill-empty-ext-world",
|
||||||
|
} <= set(registered_skills), (
|
||||||
|
"registered_skills must keep tracking copilot's still-live "
|
||||||
|
"skill files when nothing was actually replaced (#2948)"
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_toggle_skills_to_command_partial_result_only_removes_replaced_extension_skill(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""Only the extension skill whose command replacement actually
|
||||||
|
landed is retired.
|
||||||
|
|
||||||
|
Mirror image of the partial-result command->skills case: a
|
||||||
|
two-command extension where only ``world``'s installed source goes
|
||||||
|
missing right before the toggle, so ``register_commands_for_agent``
|
||||||
|
genuinely returns a partial result (only ``hello``'s command).
|
||||||
|
``hello``'s old skill artifact must be retired; ``world``'s must
|
||||||
|
survive with its tracking intact, since no replacement for it
|
||||||
|
landed (#2948).
|
||||||
|
"""
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=True)
|
||||||
|
manager = ExtensionManager(project_dir)
|
||||||
|
manager.install_from_directory(
|
||||||
|
_create_extension_dir(temp_dir, ext_id="toggle-skill-partial-ext"), "0.1.0",
|
||||||
|
register_commands=False,
|
||||||
|
)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
skills_dir = project_dir / ".github" / "skills"
|
||||||
|
hello_skill_file = skills_dir / "speckit-toggle-skill-partial-ext-hello" / "SKILL.md"
|
||||||
|
world_skill_file = skills_dir / "speckit-toggle-skill-partial-ext-world" / "SKILL.md"
|
||||||
|
assert hello_skill_file.exists() and world_skill_file.exists(), (
|
||||||
|
"sanity: skills mode should have written both SKILL.md mirrors"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Remove only world's installed source so its command replacement
|
||||||
|
# is silently skipped (missing source), while hello's succeeds.
|
||||||
|
ext_commands_dir = manager.extensions_dir / "toggle-skill-partial-ext" / "commands"
|
||||||
|
(ext_commands_dir / "world.md").unlink()
|
||||||
|
|
||||||
|
_create_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||||
|
manager.register_enabled_extensions_for_agent("copilot")
|
||||||
|
|
||||||
|
assert not hello_skill_file.exists(), (
|
||||||
|
"hello's old skill artifact must be retired since its command "
|
||||||
|
"replacement actually landed (#2948)"
|
||||||
|
)
|
||||||
|
assert world_skill_file.exists(), (
|
||||||
|
"world's old skill artifact must survive since its command "
|
||||||
|
"replacement never landed (missing source) (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-skill-partial-ext")
|
||||||
|
registered_skills = metadata.get("registered_skills", [])
|
||||||
|
assert "speckit-toggle-skill-partial-ext-hello" not in registered_skills, (
|
||||||
|
"hello must stop being tracked as a skill once its artifact "
|
||||||
|
"has been unregistered (#2948)"
|
||||||
|
)
|
||||||
|
assert "speckit-toggle-skill-partial-ext-world" in registered_skills, (
|
||||||
|
"world must keep being tracked as a skill since its old "
|
||||||
|
"artifact is still on disk (#2948)"
|
||||||
|
)
|
||||||
|
|
||||||
def test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file(
|
def test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file(
|
||||||
self, project_dir, temp_dir
|
self, project_dir, temp_dir
|
||||||
):
|
):
|
||||||
|
|||||||
@@ -3186,6 +3186,44 @@ class TestPresetSkills:
|
|||||||
yaml.dump(manifest_data, f)
|
yaml.dump(manifest_data, f)
|
||||||
return preset_dir
|
return preset_dir
|
||||||
|
|
||||||
|
def _create_multi_command_preset(self, temp_dir, preset_id, command_names):
|
||||||
|
"""Install-directory helper for a preset with more than one command.
|
||||||
|
|
||||||
|
Used to prove partial-result handling: a command's own template
|
||||||
|
entry can genuinely be skipped by registration (missing source
|
||||||
|
file, safety-validation rejection) while sibling commands in the
|
||||||
|
same preset still succeed.
|
||||||
|
"""
|
||||||
|
preset_dir = temp_dir / preset_id
|
||||||
|
preset_dir.mkdir()
|
||||||
|
(preset_dir / "commands").mkdir()
|
||||||
|
templates = []
|
||||||
|
for command_name in command_names:
|
||||||
|
command_file = f"{command_name}.md"
|
||||||
|
(preset_dir / "commands" / command_file).write_text(
|
||||||
|
f"---\ndescription: {command_name} test command\n---\n\n"
|
||||||
|
f"{command_name} body\n"
|
||||||
|
)
|
||||||
|
templates.append({
|
||||||
|
"type": "command",
|
||||||
|
"name": command_name,
|
||||||
|
"file": f"commands/{command_file}",
|
||||||
|
})
|
||||||
|
manifest_data = {
|
||||||
|
"schema_version": "1.0",
|
||||||
|
"preset": {
|
||||||
|
"id": preset_id,
|
||||||
|
"name": preset_id,
|
||||||
|
"version": "1.0.0",
|
||||||
|
"description": "Test",
|
||||||
|
},
|
||||||
|
"requires": {"speckit_version": ">=0.1.0"},
|
||||||
|
"provides": {"templates": templates},
|
||||||
|
}
|
||||||
|
with open(preset_dir / "preset.yml", "w") as f:
|
||||||
|
yaml.dump(manifest_data, f)
|
||||||
|
return preset_dir
|
||||||
|
|
||||||
def test_skill_overridden_on_preset_install(self, project_dir, temp_dir):
|
def test_skill_overridden_on_preset_install(self, project_dir, temp_dir):
|
||||||
"""When skills mode was used, a preset command override should update the skill."""
|
"""When skills mode was used, a preset command override should update the skill."""
|
||||||
# Simulate skills mode having been used: write init-options + create skill
|
# Simulate skills mode having been used: write init-options + create skill
|
||||||
@@ -4486,6 +4524,260 @@ class TestPresetSkills:
|
|||||||
"even though the file is still on disk (#2948)"
|
"even though the file is still on disk (#2948)"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
def test_toggle_command_to_skills_empty_result_preserves_old_command(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""A non-raising but empty skills result must not delete the old command.
|
||||||
|
|
||||||
|
Before the fix, the stale command-mode artifact was retired as
|
||||||
|
soon as ``_register_skills()`` completed without raising —
|
||||||
|
regardless of whether it actually wrote anything for this agent.
|
||||||
|
Deleting the preset's own command source file (simulating a
|
||||||
|
missing/corrupted override) makes ``_register_skills`` return
|
||||||
|
``{}`` for copilot without raising at all, which must leave the
|
||||||
|
old command file and its tracking untouched (#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-empty-result-preset", "speckit.specify",
|
||||||
|
"Toggle empty-result 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"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Remove the *installed* copy of the preset's source file (not the
|
||||||
|
# original temp source) so _register_skills can find nothing to
|
||||||
|
# render — a real "missing source" case, not an exception — leaving
|
||||||
|
# registered_skills empty for copilot.
|
||||||
|
(manager.presets_dir / "toggle-empty-result-preset" / "commands" / "speckit.specify.md").unlink()
|
||||||
|
|
||||||
|
self._write_init_options(project_dir, ai="copilot", ai_skills=True)
|
||||||
|
manager.register_enabled_presets_for_agent("copilot")
|
||||||
|
|
||||||
|
assert cmd_file.exists(), (
|
||||||
|
"an empty (non-raising) skills registration result must not "
|
||||||
|
"cause the old command-mode artifact to be deleted (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-empty-result-preset")
|
||||||
|
assert metadata["registered_commands"].get("copilot"), (
|
||||||
|
"registered_commands must keep tracking copilot's still-live "
|
||||||
|
"command file when nothing was actually replaced (#2948)"
|
||||||
|
)
|
||||||
|
skill_file = project_dir / ".github" / "skills" / "speckit-specify" / "SKILL.md"
|
||||||
|
assert not skill_file.exists(), (
|
||||||
|
"sanity: no skill should have been written when the source "
|
||||||
|
"was missing"
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_toggle_command_to_skills_partial_result_only_removes_replaced_command(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""Only the command whose skill replacement actually landed is retired.
|
||||||
|
|
||||||
|
A two-command preset where one command's source file goes missing
|
||||||
|
right before the toggle: ``_register_skills`` genuinely returns a
|
||||||
|
partial result (one name present, one silently skipped) without
|
||||||
|
raising. The command whose skill was written must be retired; the
|
||||||
|
other must keep both its old command file and its registry
|
||||||
|
tracking, since no replacement for it actually landed (#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_multi_command_preset(
|
||||||
|
temp_dir, "toggle-partial-result-preset",
|
||||||
|
["speckit.specify", "speckit.plan"],
|
||||||
|
)
|
||||||
|
manager = PresetManager(project_dir)
|
||||||
|
manager.install_from_directory(preset_dir, "0.1.5")
|
||||||
|
|
||||||
|
specify_cmd_file = copilot_commands_dir / "speckit.specify.agent.md"
|
||||||
|
plan_cmd_file = copilot_commands_dir / "speckit.plan.agent.md"
|
||||||
|
assert specify_cmd_file.exists() and plan_cmd_file.exists(), (
|
||||||
|
"sanity: command mode should have written both command files"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-partial-result-preset")
|
||||||
|
assert set(metadata["registered_commands"].get("copilot", [])) == {
|
||||||
|
"speckit.specify", "speckit.plan",
|
||||||
|
}, "sanity: both commands should be tracked for copilot"
|
||||||
|
|
||||||
|
# Remove only the plan command's *installed* source so its skill
|
||||||
|
# replacement is silently skipped (missing source), while
|
||||||
|
# specify's succeeds — a genuine partial result, not an injected
|
||||||
|
# exception.
|
||||||
|
(manager.presets_dir / "toggle-partial-result-preset" / "commands" / "speckit.plan.md").unlink()
|
||||||
|
|
||||||
|
self._write_init_options(project_dir, ai="copilot", ai_skills=True)
|
||||||
|
manager.register_enabled_presets_for_agent("copilot")
|
||||||
|
|
||||||
|
assert not specify_cmd_file.exists(), (
|
||||||
|
"the specify command's old artifact must be retired since its "
|
||||||
|
"skill replacement actually landed (#2948)"
|
||||||
|
)
|
||||||
|
assert plan_cmd_file.exists(), (
|
||||||
|
"the plan command's old artifact must survive since its skill "
|
||||||
|
"replacement never landed (missing source) (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-partial-result-preset")
|
||||||
|
tracked_commands = metadata["registered_commands"].get("copilot", [])
|
||||||
|
assert "speckit.specify" not in tracked_commands, (
|
||||||
|
"specify must stop being tracked as a command once its "
|
||||||
|
"artifact has been unregistered (#2948)"
|
||||||
|
)
|
||||||
|
assert "speckit.plan" in tracked_commands, (
|
||||||
|
"plan must keep being tracked as a command since its old "
|
||||||
|
"artifact is still on disk (#2948)"
|
||||||
|
)
|
||||||
|
specify_skill_file = (
|
||||||
|
project_dir / ".github" / "skills" / "speckit-specify" / "SKILL.md"
|
||||||
|
)
|
||||||
|
assert "preset:toggle-partial-result-preset" in specify_skill_file.read_text(), (
|
||||||
|
"sanity: specify's new skill artifact should exist"
|
||||||
|
)
|
||||||
|
plan_skill_file = project_dir / ".github" / "skills" / "speckit-plan"
|
||||||
|
assert not plan_skill_file.exists(), (
|
||||||
|
"sanity: no skill should have been written for plan since its "
|
||||||
|
"source was missing"
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_toggle_skills_to_command_empty_result_preserves_old_skill(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""A non-raising but empty command result must not delete the old skill.
|
||||||
|
|
||||||
|
Mirror image of the empty-result command->skills case: deleting the
|
||||||
|
preset's own source file makes ``_register_commands`` return ``{}``
|
||||||
|
for copilot without raising, which must leave the old SKILL.md and
|
||||||
|
its tracking untouched (#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")
|
||||||
|
|
||||||
|
preset_dir = self._create_command_preset(
|
||||||
|
temp_dir, "toggle-skill-empty-result-preset", "speckit.specify",
|
||||||
|
"Toggle empty-result 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-empty-result-preset" in skill_file.read_text(), (
|
||||||
|
"sanity: skills mode should have written the SKILL.md mirror"
|
||||||
|
)
|
||||||
|
|
||||||
|
# Remove the preset's own *installed* source file so
|
||||||
|
# _register_commands can find nothing to render — a real "missing
|
||||||
|
# source" case, not an exception — leaving registered_commands
|
||||||
|
# empty for copilot.
|
||||||
|
(manager.presets_dir / "toggle-skill-empty-result-preset" / "commands" / "speckit.specify.md").unlink()
|
||||||
|
|
||||||
|
self._write_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||||
|
manager.register_enabled_presets_for_agent("copilot")
|
||||||
|
|
||||||
|
assert "preset:toggle-skill-empty-result-preset" in skill_file.read_text(), (
|
||||||
|
"an empty (non-raising) command registration result must not "
|
||||||
|
"cause the old skills-mode artifact to be deleted/reverted "
|
||||||
|
"(#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-skill-empty-result-preset")
|
||||||
|
assert "speckit-specify" in metadata["registered_skills"].get("copilot", []), (
|
||||||
|
"registered_skills must keep tracking copilot's still-live "
|
||||||
|
"skill file when nothing was actually replaced (#2948)"
|
||||||
|
)
|
||||||
|
# Note: a command file may still exist here — general command
|
||||||
|
# reconciliation independently restores the next-best (e.g. core)
|
||||||
|
# layer for the *command name*, regardless of this preset's own
|
||||||
|
# missing source. That's an orthogonal, existing behaviour; the
|
||||||
|
# invariant under test is specifically that the *skill* mirror and
|
||||||
|
# its tracking survive the empty registration result.
|
||||||
|
|
||||||
|
def test_toggle_skills_to_command_partial_result_only_removes_replaced_skill(
|
||||||
|
self, project_dir, temp_dir
|
||||||
|
):
|
||||||
|
"""Only the skill whose command replacement actually landed is retired.
|
||||||
|
|
||||||
|
Mirror image of the partial-result command->skills case: a
|
||||||
|
two-command preset where one command's source file goes missing
|
||||||
|
right before the toggle, so ``_register_commands`` genuinely
|
||||||
|
returns a partial result. The skill whose command was written
|
||||||
|
must be retired; the other must keep both its old SKILL.md and
|
||||||
|
its registry tracking, since no replacement for it landed (#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")
|
||||||
|
self._create_skill(skills_dir, "speckit-plan")
|
||||||
|
|
||||||
|
# A core template lets the retired skill restore to core content
|
||||||
|
# instead of being removed entirely (it has nothing else to fall
|
||||||
|
# back to), matching _unregister_skills's behaviour 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_multi_command_preset(
|
||||||
|
temp_dir, "toggle-skill-partial-result-preset",
|
||||||
|
["speckit.specify", "speckit.plan"],
|
||||||
|
)
|
||||||
|
manager = PresetManager(project_dir)
|
||||||
|
manager.install_from_directory(preset_dir, "0.1.5")
|
||||||
|
|
||||||
|
specify_skill_file = skills_dir / "speckit-specify" / "SKILL.md"
|
||||||
|
plan_skill_file = skills_dir / "speckit-plan" / "SKILL.md"
|
||||||
|
assert "preset:toggle-skill-partial-result-preset" in specify_skill_file.read_text()
|
||||||
|
assert "preset:toggle-skill-partial-result-preset" in plan_skill_file.read_text()
|
||||||
|
metadata = manager.registry.get("toggle-skill-partial-result-preset")
|
||||||
|
assert set(metadata["registered_skills"].get("copilot", [])) == {
|
||||||
|
"speckit-specify", "speckit-plan",
|
||||||
|
}, "sanity: both skills should be tracked for copilot"
|
||||||
|
|
||||||
|
# Remove only the plan command's *installed* source so its command
|
||||||
|
# replacement is silently skipped (missing source), while
|
||||||
|
# specify's succeeds.
|
||||||
|
(manager.presets_dir / "toggle-skill-partial-result-preset" / "commands" / "speckit.plan.md").unlink()
|
||||||
|
|
||||||
|
self._write_init_options(project_dir, ai="copilot", ai_skills=False)
|
||||||
|
manager.register_enabled_presets_for_agent("copilot")
|
||||||
|
|
||||||
|
assert "preset:toggle-skill-partial-result-preset" not in specify_skill_file.read_text(), (
|
||||||
|
"the specify skill's old artifact must be retired/reverted "
|
||||||
|
"since its command replacement actually landed (#2948)"
|
||||||
|
)
|
||||||
|
assert "preset:toggle-skill-partial-result-preset" in plan_skill_file.read_text(), (
|
||||||
|
"the plan skill's old artifact must survive since its command "
|
||||||
|
"replacement never landed (missing source) (#2948)"
|
||||||
|
)
|
||||||
|
metadata = manager.registry.get("toggle-skill-partial-result-preset")
|
||||||
|
tracked_skills = metadata["registered_skills"].get("copilot", [])
|
||||||
|
assert "speckit-specify" not in tracked_skills, (
|
||||||
|
"specify must stop being tracked as a skill once its artifact "
|
||||||
|
"has been unregistered/reverted (#2948)"
|
||||||
|
)
|
||||||
|
assert "speckit-plan" in tracked_skills, (
|
||||||
|
"plan must keep being tracked as a skill since its old "
|
||||||
|
"artifact is still on disk (#2948)"
|
||||||
|
)
|
||||||
|
assert (copilot_commands_dir / "speckit.specify.agent.md").exists(), (
|
||||||
|
"sanity: specify's new command artifact should exist"
|
||||||
|
)
|
||||||
|
|
||||||
def test_rescaffold_toggle_skills_to_command_removes_stale_skill_file(
|
def test_rescaffold_toggle_skills_to_command_removes_stale_skill_file(
|
||||||
self, project_dir, temp_dir
|
self, project_dir, temp_dir
|
||||||
):
|
):
|
||||||
|
|||||||
Reference in New Issue
Block a user