diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index c66ecaa65f..4b3c83ddf6 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2996,6 +2996,38 @@ def _restore_stranded_config_file( # the restored user config with packaged defaults. Cleanup is deferred # until after registry.add() succeeds (see post-commit cleanup below). + def rollback_package_dir() -> None: + """Remove the installed package, preserving rescued user config. + + Shared by the generic and native rollbacks: config files a prior + ``remove --keep-config`` preserved (or a --force reinstall backed + up) stay in place with a ``.keep-config`` marker; everything else + the attempt copied is removed. + """ + preserved = set(stranded_configs) + if did_remove: + backup_dir = self.extensions_dir / ".backup" / manifest.id + if backup_dir.is_dir() and not backup_dir.is_symlink(): + for config_file in backup_dir.iterdir(): + if ( + config_file.is_file() + and not config_file.is_symlink() + and config_file.name.endswith(("-config.yml", "-config.local.yml")) + ): + shutil.copy2(config_file, dest_dir / config_file.name) + preserved.add(config_file.name) + if preserved: + for child in dest_dir.iterdir(): + if child.name in preserved: + continue + if child.is_dir() and not child.is_symlink(): + shutil.rmtree(child) + else: + child.unlink() + (dest_dir / ".keep-config").write_text("", encoding="utf-8") + else: + shutil.rmtree(dest_dir) + def rollback_generic_registration() -> None: from ..shared_infra import _validate_safe_shared_directory @@ -3024,35 +3056,36 @@ def rollback_generic_registration() -> None: except OSError: pass - preserved = set(stranded_configs) - if did_remove: - backup_dir = self.extensions_dir / ".backup" / manifest.id - if backup_dir.is_dir() and not backup_dir.is_symlink(): - for config_file in backup_dir.iterdir(): - if ( - config_file.is_file() - and not config_file.is_symlink() - and config_file.name.endswith(("-config.yml", "-config.local.yml")) - ): - shutil.copy2(config_file, dest_dir / config_file.name) - preserved.add(config_file.name) - if preserved: - for child in dest_dir.iterdir(): - if child.name in preserved: - continue - if child.is_dir() and not child.is_symlink(): - shutil.rmtree(child) - else: - child.unlink() - (dest_dir / ".keep-config").write_text("", encoding="utf-8") - else: - shutil.rmtree(dest_dir) + rollback_package_dir() + + def rollback_native_registration() -> None: + """Retire native command/skill outputs and the installed package. + + Mirrors ``rollback_generic_registration`` for the non-generic + route: a failed install must leave no outputs the attempt created + (#5379). ``registered_commands`` holds the command files the + registrar wrote (for skills-mode agents these are the SKILL.md + outputs) and ``registered_skills`` the skill files the skills + registrar wrote; both lists only ever contain outputs this + attempt actually wrote. + """ + from ..agents import CommandRegistrar + + safe_commands = dict(registered_commands) + safe_commands.pop("generic", None) + if safe_commands: + CommandRegistrar( + self.project_root, include_generic=False + ).unregister_commands(safe_commands, self.project_root) + self._unregister_extension_skills(registered_skills, manifest.id) + rollback_package_dir() hooks_started = False registry_started = False try: # Register commands with AI agents (active integration only, #2948) registered_commands = {} + registered_skills: list = [] if register_commands: registered_commands = self._register_commands_for_active_agent( manifest, dest_dir, link_outputs=link_commands @@ -3152,6 +3185,32 @@ def rollback_generic_registration() -> None: f"Generic registration failed ({exc}); rollback failed: " + "; ".join(rollback_errors) ) from exc + else: + # Native route has no generic artifacts, but a failure after + # command rendering must still retire everything the attempt + # created: registry entry, hooks, native command/skill + # outputs, and the installed package (#5379). + rollback_errors = [] + if registry_started: + try: + self.registry.remove(manifest.id) + except Exception as error: + rollback_errors.append(f"registry: {error}") + if hooks_started: + try: + hook_executor.unregister_hooks(manifest.id) + except Exception as error: + rollback_errors.append(f"hooks: {error}") + try: + with changing_file(dest_dir, removal=True): + rollback_native_registration() + except Exception as error: + rollback_errors.append(f"artifacts: {error}") + if rollback_errors: + raise ExtensionError( + f"Extension install failed ({exc}); rollback failed: " + + "; ".join(rollback_errors) + ) from exc raise if did_remove: diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index fdf1a7fac5..cdfb735338 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -17,13 +17,14 @@ import yaml from typer.testing import CliRunner -from specify_cli import app +from specify_cli import app, save_init_options from specify_cli.extensions import ( CompatibilityError, ExtensionCatalog, ExtensionError, ExtensionManager, ExtensionRegistry, + HookExecutor, ValidationError, ) from tests.conftest import strip_ansi @@ -1387,3 +1388,98 @@ def test_extension_install_listing_matches_registered_command_names( assert result.exit_code == 0, output assert f"• {listed} - " in output, output assert f"• {not_listed} - " not in output, output + + +def _sample_extension_dir(tmp_path): + """Build the minimal extension used by the #5379 rollback tests.""" + ext = tmp_path / "ext" + (ext / "commands").mkdir(parents=True) + (ext / "commands/run.md").write_text("---\ndescription: d\n---\nhello\n") + (ext / "extension.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "extension": { + "id": "sample", + "name": "S", + "version": "1.0.0", + "description": "d", + "author": "t", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + {"name": "speckit.sample.run", "file": "commands/run.md"} + ] + }, + "hooks": { + "after_tasks": {"command": "speckit.sample.run", "optional": True} + }, + } + ) + ) + return ext + + +def _claude_skills_project(tmp_path): + project = tmp_path / "p" + (project / ".specify").mkdir(parents=True) + (project / ".claude/skills").mkdir(parents=True) + save_init_options(project, {"ai": "claude", "ai_skills": True, "script": "sh"}) + return project + + +def test_failed_native_install_removes_skill_outputs(tmp_path, monkeypatch): + """A failed native install must leave no outputs the attempt created (#5379).""" + project = _claude_skills_project(tmp_path) + ext = _sample_extension_dir(tmp_path) + monkeypatch.setattr( + HookExecutor, + "register_hooks", + lambda *a: (_ for _ in ()).throw(OSError("x")), + ) + with pytest.raises(OSError): + ExtensionManager(project).install_from_directory(ext, "1.0.0") + assert not ExtensionManager(project).registry.is_installed("sample") + assert not (project / ".claude/skills/speckit-sample-run/SKILL.md").exists() + assert not (project / ".specify/extensions/sample").exists() + + +def test_failed_native_install_at_registry_write_removes_skill_outputs( + tmp_path, monkeypatch +): + """A failure at the final registry write must also roll back skill outputs (#5379).""" + project = _claude_skills_project(tmp_path) + ext = _sample_extension_dir(tmp_path) + monkeypatch.setattr( + ExtensionRegistry, "add", lambda *a, **k: (_ for _ in ()).throw(OSError("x")) + ) + with pytest.raises(OSError): + ExtensionManager(project).install_from_directory(ext, "1.0.0") + assert not ExtensionManager(project).registry.is_installed("sample") + assert not (project / ".claude/skills/speckit-sample-run/SKILL.md").exists() + assert not (project / ".specify/extensions/sample").exists() + + +def test_failed_native_force_install_preserves_kept_config(tmp_path, monkeypatch): + """A failed native --force reinstall must preserve kept user config (#5379).""" + project = _claude_skills_project(tmp_path) + ext = _sample_extension_dir(tmp_path) + (ext / "sample-config.yml").write_text("default: true\n", encoding="utf-8") + manager = ExtensionManager(project) + manager.install_from_directory(ext, "1.0.0") + assert manager.remove("sample", keep_config=True) + config = project / ".specify/extensions/sample/sample-config.yml" + config.write_text("user: preserved\n", encoding="utf-8") + monkeypatch.setattr( + HookExecutor, + "register_hooks", + lambda *a: (_ for _ in ()).throw(OSError("x")), + ) + with pytest.raises(OSError): + ExtensionManager(project).install_from_directory(ext, "1.0.0", force=True) + assert not ExtensionManager(project).registry.is_installed("sample") + assert config.read_text(encoding="utf-8") == "user: preserved\n" + assert (config.parent / ".keep-config").exists() + assert not (config.parent / "extension.yml").exists() + assert not (project / ".claude/skills/speckit-sample-run/SKILL.md").exists()