mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
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>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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())
|
||||
|
||||
Reference in New Issue
Block a user