mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02: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:
@@ -1366,6 +1366,221 @@ class TestExtensionSkillRegistration:
|
||||
"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(
|
||||
self, project_dir, temp_dir
|
||||
):
|
||||
|
||||
@@ -3186,6 +3186,44 @@ class TestPresetSkills:
|
||||
yaml.dump(manifest_data, f)
|
||||
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):
|
||||
"""When skills mode was used, a preset command override should update the 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)"
|
||||
)
|
||||
|
||||
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(
|
||||
self, project_dir, temp_dir
|
||||
):
|
||||
|
||||
Reference in New Issue
Block a user