[bug-fix] Fix bundle-update-force-mislead: add refresh() to DefaultPrimitiveInstaller#3452
Draft
github-actions[bot] wants to merge 1 commit into
Draft
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}
|
|
||
| def install(self, component: ComponentRef) -> None: ... | ||
|
|
||
| def refresh(self, component: ComponentRef) -> None: ... |
Collaborator
|
Please address Copilot feedback |
Contributor
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) |
| 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) |
Comment on lines
+400
to
+403
| 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) |
Comment on lines
+277
to
+279
| 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.""" |
Comment on lines
+302
to
+304
| from specify_cli.bundler.services.installer import install_bundle | ||
| from specify_cli.bundler.services.adapters import DefaultPrimitiveInstaller | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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