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..0e15a7bdb4 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -89,6 +89,8 @@ def is_installed(self, component: ComponentRef) -> bool: ... def install(self, component: ComponentRef) -> None: ... + def refresh(self, component: ComponentRef) -> None: ... + def remove(self, component: ComponentRef) -> None: ... @@ -151,6 +153,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 +176,7 @@ 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=force) return if not self._allow_network: @@ -194,7 +202,7 @@ 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=force) finally: with contextlib.suppress(Exception): if zip_path.exists(): @@ -224,6 +232,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 +256,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 +286,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 +332,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 +397,11 @@ def install(self, component: ComponentRef) -> None: lambda: workflow_step_add(component.id), ) + def refresh(self, component: ComponentRef) -> None: + # workflow_step_add is idempotent for already-installed steps; delegate + # to the standard install path which handles version refresh correctly. + self.install(component) + 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..cce9270121 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -215,3 +215,105 @@ 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_no_force_hint_in_bundler_error_on_refresh(tmp_path: Path, monkeypatch): + """Regression: bundle update (refresh=True) of an already-installed extension + must not surface the extension-level '--force' hint inside the BundlerError.""" + 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 = [] + monkeypatch.setattr( + ExtensionManager, "install_from_directory", + lambda self, *a, **k: force_seen.append(k.get("force", False)), + ) + + raw = { + "schema_version": "1.0", + "bundle_id": "test-bundle", + "name": "Test", + "version": "1.0.0", + "components": [{"kind": "extensions", "id": "my-ext"}], + } + manifest = BundleManifest(raw, source_id="test") + from specify_cli.bundler.services.installer import install_bundle + from specify_cli.bundler.services.adapters import DefaultPrimitiveInstaller + + 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.version, components=components)