From 81bf741b924f346149087d5c4895d5a079269a58 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:59:53 -0500 Subject: [PATCH] [bug-fix] Fix bundle-update-force-mislead: add refresh() to DefaultPrimitiveInstaller (#3452) * Fix bundle-update-force-mislead: add refresh() to DefaultPrimitiveInstaller Apply the remediation from the bug assessment on issue #3424. DefaultPrimitiveInstaller lacked a refresh() method, causing _refresh_component() to fall back to install(), which calls ExtensionManager.install_from_directory() with force=False. This raised ExtensionError with a leaked --force hint that bundle update does not support, leaving users with no valid recovery path. Fix: add refresh() to each kind manager (ExtensionKindManager and PresetKindManager delegate to _do_install(force=True); WorkflowKindManager and StepKindManager delegate to install() as their callables are idempotent). DefaultPrimitiveInstaller.refresh() dispatches to the kind manager's refresh(). PresetManager.install_from_directory() and install_from_zip() gain a force parameter that removes the existing preset before reinstalling, mirroring ExtensionManager's force semantics. Refs #3424 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback on primitives.py and test_bundler_primitives.py - Replace ... with pass in _KindManager Protocol method stubs - Conditionally pass force= keyword only when force=True in _PresetKindManager - Fix _StepKindManager.refresh() to remove step before re-installing - Rename test to reflect actual assertion (refresh succeeds + force=True) - Remove duplicate install_bundle import Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * fix: add missing role/effective_integration to InstallPlan in _plan() and remove redundant import - Remove duplicate `DefaultPrimitiveInstaller` import inside test body (already imported at module scope on line 15) - Add required `role` and `effective_integration` fields to `InstallPlan` constructor in `_plan()` helper to prevent TypeError at runtime Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix: address latest PR review comments Assisted-by: GitHub Copilot (model: gpt-5.6-terra, autonomous) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> --- src/specify_cli/bundler/services/adapters.py | 4 + .../bundler/services/primitives.py | 70 +++++++++-- src/specify_cli/presets/__init__.py | 16 ++- tests/unit/test_bundler_primitives.py | 119 ++++++++++++++++++ 4 files changed, 197 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index 403232a7f..f6a1d466b 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -227,6 +227,10 @@ class DefaultPrimitiveInstaller: 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 229fb6137..31b1126a3 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 @@ class _PresetKindManager: 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 @@ class _PresetKindManager: 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 @@ class _PresetKindManager: ) 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 @@ class _ExtensionKindManager: 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 @@ class _ExtensionKindManager: _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 @@ class _ExtensionKindManager: 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 @@ class _WorkflowKindManager: 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 @@ class _StepKindManager: 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 9461e4fc6..de4116228 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -3359,6 +3359,7 @@ class PresetManager: source_dir: Path, speckit_version: str, priority: int = 10, + force: bool = False, ) -> PresetManifest: """Install preset from a local directory. @@ -3366,6 +3367,7 @@ class PresetManager: 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 @@ -3384,10 +3386,12 @@ class PresetManager: 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(): @@ -3535,6 +3539,7 @@ class PresetManager: zip_path: Path, speckit_version: str, priority: int = 10, + force: bool = False, ) -> PresetManifest: """Install preset from ZIP file. @@ -3542,6 +3547,7 @@ class PresetManager: 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 @@ -3573,7 +3579,7 @@ class PresetManager: "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 9891e6f77..dc39106b5 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -20,6 +20,7 @@ from specify_cli.bundler.services.primitives import ( _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, + )