diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index 3e61ded04..486b146bc 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/src/specify_cli/bundler/services/installer.py @@ -188,16 +188,24 @@ def remove_bundle( still_needed = components_still_needed(records, exclude_bundle_id=bundle_id) result = InstallResult(bundle_id=bundle_id) - for component in target.contributed_components: - key = (component.kind, component.id) - if key in still_needed: - result.skipped.append(component) - continue - if installer.is_installed(project_root, component): - installer.remove(project_root, component) - result.uninstalled.append(component) - else: - result.skipped.append(component) + try: + for component in target.contributed_components: + key = (component.kind, component.id) + if key in still_needed: + result.skipped.append(component) + continue + if installer.is_installed(project_root, component): + installer.remove(project_root, component) + result.uninstalled.append(component) + else: + result.skipped.append(component) + except BundlerError: + raise + except Exception as exc: # noqa: BLE001 + raise BundlerError( + f"Failed to remove bundle '{bundle_id}': {exc}. " + "No changes were recorded." + ) from exc save_records(project_root, remove_record(records, bundle_id)) return result diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 1705c5945..f76fafe32 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -63,6 +63,42 @@ def test_commands_outside_project_fail_with_guidance(tmp_path: Path, monkeypatch assert "Spec Kit project" in result.output +def test_remove_reports_clean_error_when_primitive_raises_raw_exception( + project: Path, +): + """A raw exception from a primitive installer (e.g. an OSError from an + unreadable workflow registry surfacing through _WorkflowKindManager's + fail-closed construction) must not propagate uncaught through + `specify bundle remove` -- the command only catches BundlerError, so + without a conversion at the remove_bundle boundary this would exit + with an unhandled exception and empty/raw output instead of a clean, + actionable message, and no removal side effects should occur either.""" + from specify_cli.bundler.models.manifest import BundleManifest + from specify_cli.bundler.models.records import load_records + from specify_cli.bundler.services.adapters import DefaultPrimitiveInstaller + from specify_cli.bundler.services.installer import install_bundle + from specify_cli.bundler.services.resolver import resolve_install_plan + from tests.bundler_helpers import FakeInstaller + + manifest = BundleManifest.from_dict(valid_manifest_dict()) + plan = resolve_install_plan( + manifest, speckit_version="0.11.2", active_integration="copilot" + ) + install_bundle(project, plan, FakeInstaller(), manifest=manifest) + + def boom(self, project_root, component): + raise OSError("workflow registry unreadable") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(DefaultPrimitiveInstaller, "is_installed", boom) + result = runner.invoke(app, ["bundle", "remove", "demo-bundle"]) + + assert result.exit_code != 0 + assert result.output.strip() != "" + assert result.exception is None or isinstance(result.exception, SystemExit) + assert {r.bundle_id for r in load_records(project)} == {"demo-bundle"} + + def test_fail_writes_error_to_stderr_not_stdout(capsys): """_fail must write to stderr, not stdout: every bundle command routes errors through it, and under --json the error would otherwise corrupt the JSON payload diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index bf066b660..3ef8ff45b 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/tests/integration/test_bundler_install_flow.py @@ -97,6 +97,32 @@ def test_remove_unknown_bundle_errors(tmp_path: Path): remove_bundle(tmp_path, "ghost", FakeInstaller()) +def test_remove_converts_raw_installer_exception_to_bundler_error(tmp_path: Path): + """A raw exception from a primitive installer (e.g. an OSError from an + unreadable workflow registry surfacing through _WorkflowKindManager's + fail-closed construction) must not propagate uncaught out of + remove_bundle: install_bundle already converts any non-BundlerError + exception into a clean BundlerError, but remove_bundle had no such + conversion, so the CLI's `bundle remove` (which only catches + BundlerError) would let a raw exception through with no clean message + and no removal side effects should occur either.""" + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + def boom(project_root, component): + raise OSError("workflow registry unreadable") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(installer, "is_installed", boom) + with pytest.raises(BundlerError): + remove_bundle(tmp_path, "demo-bundle", installer) + + # No removal side effects: the bundle record must still be present. + assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} + + def test_remove_reports_uninstalled_not_installed(tmp_path: Path): make_project(tmp_path) manifest = BundleManifest.from_dict(valid_manifest_dict())