diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index 91e3cf1cb4..9bacb0e5a2 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -184,6 +184,10 @@ def install(self, project_root: Path, component: ComponentRef) -> None: manager = self._manager_for(component, project_root) manager.install(component) + def refresh(self, project_root: Path, component: ComponentRef) -> None: + manager = self._manager_for(component, project_root) + manager.refresh(component) + def remove(self, project_root: Path, component: ComponentRef) -> None: manager = self._manager_for(component, project_root) manager.remove(component) diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 229fb61375..31b1126a34 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -85,11 +85,17 @@ def _bundled_manifest_version(manifest_path: Path, root_key: str) -> str | None: class _KindManager(Protocol): - def is_installed(self, component: ComponentRef) -> bool: ... + def is_installed(self, component: ComponentRef) -> bool: + pass - def install(self, component: ComponentRef) -> None: ... + def install(self, component: ComponentRef) -> None: + pass - def remove(self, component: ComponentRef) -> None: ... + def refresh(self, component: ComponentRef) -> None: + pass + + def remove(self, component: ComponentRef) -> None: + pass def primitive_manager( @@ -151,6 +157,12 @@ def is_installed(self, component: ComponentRef) -> bool: return False def install(self, component: ComponentRef) -> None: + self._do_install(component, force=False) + + def refresh(self, component: ComponentRef) -> None: + self._do_install(component, force=True) + + def _do_install(self, component: ComponentRef, *, force: bool) -> None: from ... import get_speckit_version from ..._assets import _locate_bundled_preset @@ -168,7 +180,9 @@ def install(self, component: ComponentRef) -> None: component.version, _bundled_manifest_version(bundled / "preset.yml", "preset"), ) - self._manager.install_from_directory(bundled, speckit_version, priority) + self._manager.install_from_directory( + bundled, speckit_version, priority, **({"force": True} if force else {}) + ) return if not self._allow_network: @@ -194,7 +208,9 @@ def install(self, component: ComponentRef) -> None: ) zip_path = catalog.download_pack(component.id) try: - self._manager.install_from_zip(zip_path, speckit_version, priority) + self._manager.install_from_zip( + zip_path, speckit_version, priority, **({"force": True} if force else {}) + ) finally: with contextlib.suppress(Exception): if zip_path.exists(): @@ -224,6 +240,12 @@ def is_installed(self, component: ComponentRef) -> bool: return False def install(self, component: ComponentRef) -> None: + self._do_install(component, force=False) + + def refresh(self, component: ComponentRef) -> None: + self._do_install(component, force=True) + + def _do_install(self, component: ComponentRef, *, force: bool) -> None: from ... import get_speckit_version from ..._assets import _locate_bundled_extension @@ -242,7 +264,7 @@ def install(self, component: ComponentRef) -> None: _bundled_manifest_version(bundled / "extension.yml", "extension"), ) self._manager.install_from_directory( - bundled, speckit_version, priority=priority + bundled, speckit_version, priority=priority, force=force ) return @@ -272,7 +294,7 @@ def install(self, component: ComponentRef) -> None: zip_path = catalog.download_extension(component.id) try: self._manager.install_from_zip( - zip_path, speckit_version, priority=priority + zip_path, speckit_version, priority=priority, force=force ) finally: with contextlib.suppress(Exception): @@ -318,6 +340,11 @@ def install(self, component: ComponentRef) -> None: lambda: workflow_add(component.id), ) + def refresh(self, component: ComponentRef) -> None: + # workflow_add is idempotent for already-installed workflows; delegate + # to the standard install path which handles version refresh correctly. + self.install(component) + def _assert_pinned_version(self, component: ComponentRef) -> None: if not component.version: return @@ -378,6 +405,35 @@ def install(self, component: ComponentRef) -> None: lambda: workflow_step_add(component.id), ) + def refresh(self, component: ComponentRef) -> None: + # Preserve an existing step until we've validated we can perform refresh. + # For already-installed steps, keep a backup and restore it if the + # remove+reinstall path fails. + if not (self._allow_network and self.is_installed(component)): + self.install(component) + return + + import shutil + import tempfile + + step_dir = self._registry.steps_dir / component.id + metadata = self._registry.get(component.id) + backup_dir = Path(tempfile.mkdtemp(prefix="speckit-step-refresh-")) / component.id + try: + if step_dir.exists(): + shutil.copytree(step_dir, backup_dir) + self.remove(component) + try: + self.install(component) + except BundlerError: + if backup_dir.exists(): + shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) + if metadata is not None and not self._registry.is_installed(component.id): + self._registry.add(component.id, metadata) + raise + finally: + shutil.rmtree(backup_dir.parent, ignore_errors=True) + def remove(self, component: ComponentRef) -> None: from ... import workflow_step_remove diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 863b6ef7dc..975c81c51a 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -1503,6 +1503,7 @@ def install_from_directory( source_dir: Path, speckit_version: str, priority: int = 10, + force: bool = False, ) -> PresetManifest: """Install preset from a local directory. @@ -1510,6 +1511,7 @@ def install_from_directory( source_dir: Path to preset directory speckit_version: Current spec-kit version priority: Resolution priority (lower = higher precedence, default 10) + force: If True and the preset is already installed, remove it first Returns: Installed preset manifest @@ -1528,10 +1530,12 @@ def install_from_directory( self.check_compatibility(manifest, speckit_version) if self.registry.is_installed(manifest.id): - raise PresetError( - f"Preset '{manifest.id}' is already installed. " - f"Use 'specify preset remove {manifest.id}' first." - ) + if not force: + raise PresetError( + f"Preset '{manifest.id}' is already installed. " + f"Use 'specify preset remove {manifest.id}' first." + ) + self.remove(manifest.id) dest_dir = self.presets_dir / manifest.id if dest_dir.exists(): @@ -1622,6 +1626,7 @@ def install_from_zip( zip_path: Path, speckit_version: str, priority: int = 10, + force: bool = False, ) -> PresetManifest: """Install preset from ZIP file. @@ -1629,6 +1634,7 @@ def install_from_zip( zip_path: Path to preset ZIP file speckit_version: Current spec-kit version priority: Resolution priority (lower = higher precedence, default 10) + force: If True and the preset is already installed, remove it first Returns: Installed preset manifest @@ -1671,7 +1677,7 @@ def install_from_zip( "No preset.yml found in ZIP file" ) - return self.install_from_directory(pack_dir, speckit_version, priority) + return self.install_from_directory(pack_dir, speckit_version, priority, force=force) def remove(self, pack_id: str) -> bool: """Remove an installed preset. diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index 9891e6f77c..dc39106b50 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -20,6 +20,7 @@ _WorkflowKindManager, primitive_manager, ) +from tests.bundler_helpers import valid_manifest_dict def _component(kind: str, cid: str = "x") -> ComponentRef: @@ -215,3 +216,121 @@ def test_bundled_preset_pin_match_installs(tmp_path: Path, monkeypatch): manager.install(ComponentRef(kind="presets", id="my-preset", version="1.0.0")) manager.install(ComponentRef(kind="presets", id="my-preset", version=None)) assert len(called) == 2 + + +def test_extension_refresh_calls_install_with_force(tmp_path: Path, monkeypatch): + """_ExtensionKindManager.refresh() must pass force=True to install_from_directory + so an already-installed extension is overwritten instead of raising an error.""" + import specify_cli._assets as assets + from specify_cli.extensions import ExtensionManager + + bundled = _write_manifest(tmp_path / "ext", "extension", "1.0.0") + monkeypatch.setattr(assets, "_locate_bundled_extension", lambda cid: bundled) + force_values: list = [] + monkeypatch.setattr( + ExtensionManager, "install_from_directory", + lambda self, *a, **k: force_values.append(k.get("force", False)), + ) + + manager = primitive_manager("extensions", tmp_path, allow_network=False) + manager.refresh(ComponentRef(kind="extensions", id="my-ext")) + assert force_values == [True], "refresh() must pass force=True" + + +def test_preset_refresh_calls_install_with_force(tmp_path: Path, monkeypatch): + """_PresetKindManager.refresh() must pass force=True to install_from_directory + so an already-installed preset is overwritten instead of raising an error.""" + import specify_cli._assets as assets + from specify_cli.presets import PresetManager + + bundled = _write_manifest(tmp_path / "preset", "preset", "1.0.0") + monkeypatch.setattr(assets, "_locate_bundled_preset", lambda cid: bundled) + force_values: list = [] + monkeypatch.setattr( + PresetManager, "install_from_directory", + lambda self, *a, **k: force_values.append(k.get("force", False)), + ) + + manager = primitive_manager("presets", tmp_path, allow_network=False) + manager.refresh(ComponentRef(kind="presets", id="my-preset")) + assert force_values == [True], "refresh() must pass force=True" + + +def test_default_installer_refresh_dispatches_to_kind_manager(tmp_path: Path, monkeypatch): + """DefaultPrimitiveInstaller.refresh() must call the kind manager's refresh(), + which is the hook _refresh_component() will find — fixing the --force leak.""" + import specify_cli._assets as assets + from specify_cli.extensions import ExtensionManager + + bundled = _write_manifest(tmp_path / "ext", "extension", "1.0.0") + monkeypatch.setattr(assets, "_locate_bundled_extension", lambda cid: bundled) + force_values: list = [] + monkeypatch.setattr( + ExtensionManager, "install_from_directory", + lambda self, *a, **k: force_values.append(k.get("force", False)), + ) + + installer = DefaultPrimitiveInstaller(allow_network=False) + installer.refresh(tmp_path, _component("extensions", "my-ext")) + assert force_values == [True], "DefaultPrimitiveInstaller.refresh() must use force=True" + + +def test_refresh_succeeds_and_passes_force_true(tmp_path: Path, monkeypatch): + """Regression: bundle update (refresh=True) of an already-installed extension + must succeed and pass force=True to install_from_directory.""" + from specify_cli.bundler.services.installer import install_bundle + from specify_cli.bundler.models.manifest import BundleManifest + import specify_cli._assets as assets + from specify_cli.extensions import ExtensionManager + + bundled = _write_manifest(tmp_path / "ext", "extension", "1.0.0") + monkeypatch.setattr(assets, "_locate_bundled_extension", lambda cid: bundled) + # Simulate refresh succeeding (force=True removes the duplicate-install guard) + force_seen: list = [] + def _fake_install_from_directory(self, *a, **k): + force_seen.append(k.get("force", False)) + self.registry.add("my-ext", {"version": "1.0.0"}) + + monkeypatch.setattr( + ExtensionManager, "install_from_directory", _fake_install_from_directory + ) + + raw = valid_manifest_dict( + bundle={ + "id": "test-bundle", + "name": "Test", + "version": "1.0.0", + "role": "developer", + "description": "Test bundle", + "author": "Spec Kit", + "license": "MIT", + }, + provides={ + "extensions": [{"id": "my-ext", "version": "1.0.0"}], + "presets": [], + "steps": [], + "workflows": [], + }, + ) + manifest = BundleManifest.from_dict(raw) + installer = DefaultPrimitiveInstaller(allow_network=False) + # First install + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + # Refresh (bundle update) — must not raise with --force hint + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest, refresh=True) + # force=True must have been passed during the refresh call + assert True in force_seen, "refresh path should have called install_from_directory with force=True" + + +def _plan(manifest): + from specify_cli.bundler.services.installer import InstallPlan + from specify_cli.bundler.models.manifest import ComponentRef as CR + + components = [CR(kind=c.kind, id=c.id) for c in manifest.components] + return InstallPlan( + bundle_id=manifest.bundle.id, + version=manifest.bundle.version, + role=manifest.bundle.role, + effective_integration=None, + components=components, + )