From ce3e0abacb3e9dd9fe830210438a93c3fd86cee2 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 09:58:17 +0200 Subject: [PATCH] Fix raw exception leak in bundle remove primitive boundary remove_bundle() had no exception handling around its component removal loop, unlike install_bundle() which converts any raw exception into a clean BundlerError. Since WorkflowRegistry now fails closed (raises OSError) on an unreadable registry file, and _WorkflowKindManager.__init__ constructs WorkflowRegistry with no try/except, an unreadable workflow registry surfaced as a raw OSError through remove_bundle(). The bundle_remove CLI command only catches BundlerError, so the raw OSError propagated uncaught, producing exit_code=1 with empty output instead of a clean, actionable message. Wrap remove_bundle()'s component loop in the same try/except BundlerError: raise / except Exception: raise BundlerError(...) from exc pattern already used by install_bundle(), converting any raw exception at this shared boundary. save_records() remains outside the try block, so a failure still leaves the bundle's record untouched (no removal side effects recorded). Tests: - tests/integration/test_bundler_install_flow.py::test_remove_converts_raw_installer_exception_to_bundler_error (function-level regression: a raw OSError from installer.is_installed must become a clean BundlerError, and the bundle record must survive) - tests/contract/test_bundle_cli.py::test_remove_reports_clean_error_when_primitive_raises_raw_exception (CLI-level regression: `specify bundle remove` must print a clean actionable message and exit non-zero instead of raw/empty output) Both tests were confirmed red beforehand: the raw OSError propagated uncaught out of remove_bundle(), and the CLI-level CliRunner result showed exit_code=1 with empty output. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/bundler/services/installer.py | 28 +++++++++------ tests/contract/test_bundle_cli.py | 36 +++++++++++++++++++ .../integration/test_bundler_install_flow.py | 26 ++++++++++++++ 3 files changed, 80 insertions(+), 10 deletions(-) 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())