From d0d152ef6cf55997fa9f358644f6a255991f9cf7 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 15:18:55 +0200 Subject: [PATCH] Verify replacement actually landed before retiring stale toggle artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- src/specify_cli/extensions/__init__.py | 65 +++++- src/specify_cli/presets/__init__.py | 59 ++++- tests/test_extension_skills.py | 215 ++++++++++++++++++ tests/test_presets.py | 292 +++++++++++++++++++++++++ 4 files changed, 607 insertions(+), 24 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index b8ec4f00f..2415b7f17 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2103,9 +2103,26 @@ class ExtensionManager: for name in existing_skills 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( - 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 # shared across every agent this extension @@ -2132,17 +2149,41 @@ class ExtensionManager: # registration is confirmed, so the stale # command-mode artifact can finally be removed # 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: - registrar.unregister_commands( - {agent_name: deferred_stale_commands}, self.project_root - ) - registered_commands = metadata.get("registered_commands", {}) - if isinstance(registered_commands, dict): - new_registered = copy.deepcopy(registered_commands) - new_registered.pop(agent_name, None) - if new_registered != registered_commands: - updates["registered_commands"] = new_registered + 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( + {agent_name: fully_replaced}, self.project_root + ) + registered_commands = metadata.get( + "registered_commands", {} + ) + if isinstance(registered_commands, dict) and ( + registered_commands.get(agent_name) + ): + 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) + if new_registered != registered_commands: + updates["registered_commands"] = new_registered if updates: self.registry.update(ext_id, updates) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index dc260935c..07b90708b 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -854,11 +854,28 @@ class PresetManager: # direction is already register-new-then-remove-old: # _register_commands (the replacement) ran unconditionally # above and only reaches here once it has already - # succeeded, so this cleanup happens only after the new - # command artifact is confirmed in place (#2948). - self._unregister_skills( - {agent_name: merged_skills.pop(agent_name)}, pack_dir - ) + # succeeded — but that call can still have returned + # empty or partial results (missing source template, + # safety-validation skip, corrupted manifest), so only + # 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 # per-agent provenance) must migrate to the dict format on # disk even when the rescaffolded names are unchanged from @@ -874,14 +891,32 @@ class PresetManager: if merged_skills != existing_skills or needs_migration: self.registry.update(pack_id, {"registered_skills": merged_skills}) - # The skills phase above completed without raising, so the - # replacement artifact is confirmed — now it's safe to - # remove the stale command-mode artifact deferred earlier - # (#2948). + # The skills phase above completed without raising, but a + # non-raising result can still be empty or partial (missing + # source template, safety-validation skip, corrupted + # 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: - self._unregister_commands({agent_name: stale_command_names}) - merged_commands.pop(agent_name, None) - self.registry.update(pack_id, {"registered_commands": merged_commands}) + 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) + self.registry.update(pack_id, {"registered_commands": merged_commands}) except Exception as pack_err: from .. import _print_cli_warning diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index c4cee6826..9ebef14b9 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -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 ): diff --git a/tests/test_presets.py b/tests/test_presets.py index 0410f0a47..0a4c8aeb8 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -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 ):