[bug-fix] Fix bundle-update-force-mislead: add refresh() to DefaultPrimitiveInstaller#3452
Conversation
…taller 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>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Please address Copilot feedback |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #3424 where bundle update could surface an extension-level “retry with --force” hint even though bundle update does not support --force. It does so by introducing an explicit refresh() path in the bundler primitive adapter, and by making extension/preset refresh semantics overwrite existing installs via force=True.
Changes:
- Added
DefaultPrimitiveInstaller.refresh()so bundler refresh can call a dedicated refresh hook instead of falling back to install. - Extended kind managers with
refresh(); extensions/presets refresh now installs withforce=True, and presets gain aforceparameter to support overwrite semantics. - Added unit tests covering refresh dispatch and the “no leaked --force hint” regression scenario.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/bundler/services/adapters.py |
Adds DefaultPrimitiveInstaller.refresh() to dispatch refresh to the kind manager. |
src/specify_cli/bundler/services/primitives.py |
Adds refresh() to kind managers; implements force-refresh for extensions/presets and delegates refresh for workflows/steps. |
src/specify_cli/presets/__init__.py |
Adds force overwrite support to preset installs (directory + zip), removing existing preset first when forced. |
tests/unit/test_bundler_primitives.py |
Adds tests for refresh force propagation, refresh dispatch, and the bundle update regression path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Low
| _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) |
There was a problem hiding this comment.
Fixed — install_from_directory now uses **({"force": True} if force else {}) so the force= keyword is only forwarded when it's True. This preserves compatibility with existing test doubles (e.g. _FakeManager in test_preset_install_preserves_explicit_zero_priority) that only accept the 3-arg signature.
Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous)
| 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) |
There was a problem hiding this comment.
Fixed — same conditional-unpacking pattern applied to install_from_zip.
Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous)
| 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) |
There was a problem hiding this comment.
Fixed — _StepKindManager.refresh() now removes the step first when network is allowed and the step is already installed, then calls install(). This avoids the "already installed" error from workflow_step_add.
Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous)
| 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.""" |
There was a problem hiding this comment.
Renamed to test_refresh_succeeds_and_passes_force_true with a matching docstring.
Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous)
| from specify_cli.bundler.services.installer import install_bundle | ||
| from specify_cli.bundler.services.adapters import DefaultPrimitiveInstaller | ||
|
|
There was a problem hiding this comment.
Removed the duplicate from specify_cli.bundler.services.installer import install_bundle import.
Posted on behalf of @mnriem by GitHub Copilot (model: claude-sonnet-4.5, autonomous)
- 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)
Bug fix — bundle-update-force-mislead
Proposed fix for issue #3424, applying the remediation from the bug assessment.
Verdict: Valid · Severity: medium
Summary
DefaultPrimitiveInstallerlacked arefresh()method, so_refresh_component()fell back toinstall(), which rejects already-installed extensions with a leaked--forcehint thatbundle updatedoes not support. This fix addsrefresh()to all kind managers (passingforce=Truefor extensions and presets) and wires it up throughDefaultPrimitiveInstaller.Changes
src/specify_cli/bundler/services/adapters.pyrefresh()toDefaultPrimitiveInstaller; dispatches to the kind manager'srefresh()hooksrc/specify_cli/bundler/services/primitives.pyrefresh()to_KindManagerProtocol; refactored_ExtensionKindManager.install()and_PresetKindManager.install()into_do_install(force)helpers; addedrefresh()(calls_do_install(force=True)) to both; addedrefresh()to_WorkflowKindManagerand_StepKindManager(delegates toinstall())src/specify_cli/presets/__init__.pyforce: bool = Falseparameter toinstall_from_directory()andinstall_from_zip(); whenforce=Trueand the preset is already installed, it removes the existing preset before reinstalling (mirrorsExtensionManagersemantics)tests/unit/test_bundler_primitives.py_ExtensionKindManager.refresh()passingforce=True,_PresetKindManager.refresh()passingforce=True,DefaultPrimitiveInstaller.refresh()dispatching correctly, and a regression test verifying no--forcehint surfaces inBundlerErroronbundle updateTests Added or Updated
test_extension_refresh_calls_install_with_force— pins that_ExtensionKindManager.refresh()callsinstall_from_directorywithforce=Truetest_preset_refresh_calls_install_with_force— pins that_PresetKindManager.refresh()callsinstall_from_directorywithforce=Truetest_default_installer_refresh_dispatches_to_kind_manager— pins thatDefaultPrimitiveInstaller.refresh()reaches the kind manager'srefresh()and usesforce=Truetest_no_force_hint_in_bundler_error_on_refresh— regression test ensuringbundle update(refresh=True) on an already-installed bundle-owned extension no longer raises an error or surfaces the--forcehintLocal Verification
python3 -c "import ast; ast.parse(open(f).read())"on all four changed files → all OKDeviations from Assessment
_WorkflowKindManager.refresh()and_StepKindManager.refresh()are implemented as delegation toinstall()(the existing fallback). The assessment noted these callables are "likely idempotent" — no behavioral change for these kinds._KindManagerProtocol was extended withrefresh(). This is backwards-compatible: existing test fakes implementing onlyinstall/removewill continue to pass (Protocol methods are not enforced at runtime in Python).forceparameter was added toPresetManager.install_from_directory()andinstall_from_zip()rather than using remove+install at the bundler layer, to mirrorExtensionManager's pattern and keep atomicity within the preset manager.Risks & Review Notes
PresetManager.install_from_directory(force=True)callsself.remove(manifest.id)before copying. If the subsequent install fails, the preset is gone — same risk as noted in the assessment. The existing rollback logic ininstall_from_directoryhandles partial failures during the copy/register phase.refresh()toinstall()for workflows/steps. Ifworkflow_add/workflow_step_addare not idempotent for already-installed items, those kinds could still fail on refresh. This is a pre-existing condition for those kinds and is not worsened by this change.Refs #3424 · cc
@grafvonb