fix: make bundle step refresh rollback transactional

src/specify_cli/bundles/_commands.py

@@ -46,6 +46,13 @@ def _fail(message: str, *, as_json: bool = False) -> None:
     raise typer.Exit(code=1)
 
 
+def _fail_with_notes(exc: BaseException) -> None:
+    """Exit with the original error first, then any recovery notes."""
+    notes = [str(note) for note in getattr(exc, "__notes__", ())]
+    message = str(exc) if not notes else f"{exc}\n" + "\n".join(notes)
+    _fail(message)
+
+
 def _render_install_warnings(result: InstallResult) -> None:
     for warning in result.warnings:
         err_console.print(f"[yellow]Warning:[/yellow] {_escape_markup(warning.message)}")

src/specify_cli/bundles/command_install.py

@@ -13,7 +13,7 @@
     _build_stack,
     _bundle_overlaps,
     _default_script_type,
-    _fail,
+    _fail_with_notes,
     _render_install_warnings,
     _resolve_init_integration,
     _run_init,
@@ -141,7 +141,7 @@ def bundle_install(
             refresh=refresh,
         )
     except BundlerError as exc:
-        _fail(str(exc))
+        _fail_with_notes(exc)
         return
 
     _render_install_warnings(result)

src/specify_cli/bundles/command_update.py

@@ -9,7 +9,7 @@
 from . import BundlerError
 from ._commands import (
     _build_stack,
-    _fail,
+    _fail_with_notes,
     _render_install_warnings,
     _speckit_version,
     bundle_app,
@@ -72,5 +72,5 @@ def bundle_update(
                 f"to v{_escape_markup(str(plan.version))}."
             )
     except BundlerError as exc:
-        _fail(str(exc))
+        _fail_with_notes(exc)
         return

src/specify_cli/bundles/primitives.py

@@ -641,55 +641,154 @@ def install(self, component: ComponentRef) -> None:
                 ) from exc
 
     def refresh(self, component: ComponentRef) -> None:
-        # Preserve an existing step until we've validated we can perform refresh.
-        # For already-installed steps, keep a backup and restore it if the
-        # remove+reinstall path fails.
-        if not (self._allow_network and self.is_installed(component)):
+        # Only offline refresh can skip the lock. Online presence is the
+        # registry read under ``_step_install_transaction``: a step added
+        # after this manager was constructed is still refreshed, and a step
+        # missing from that read delegates to ``install`` below.
+        if not self._allow_network:
             self.install(component)
             return
 
+        import copy
         import shutil
         import tempfile
 
-        step_dir = self._registry.steps_dir / component.id
-        metadata = self._registry.get(component.id)
-        backup_dir = Path(tempfile.mkdtemp(prefix="speckit-step-refresh-")) / component.id
+        from ..workflows.catalog import StepRegistry
+        from ..workflows.step import command_remove
+        from ..workflows.step import installer as step_installer
+
+        # Snapshot and removal share the lock ``step add`` / ``step remove``
+        # already use. ``self.remove()`` and ``workflow_step_remove`` acquire
+        # that same lock, and ``_exclusive_project_lock`` blocks in
+        # ``fcntl.flock(LOCK_EX)`` on a new fd, so a nested acquire in this
+        # process deadlocks. Remove through ``_remove_step_locked`` instead.
+        # The catalog reinstall stays outside the lock. A step missing from
+        # the locked snapshot has nothing to roll back: release the lock and
+        # delegate to ``install``, which acquires this same lock.
+        backup_root: Path | None = None
+        keep_backup = False
+        metadata = None
+        had_snapshot = False
+        removal_error: BundlerError | None = None
         try:
-            if step_dir.exists():
-                shutil.copytree(step_dir, backup_dir)
-            self.remove(component)
             try:
+                with step_installer._step_install_transaction(self._root):
+                    registry = StepRegistry(self._root)
+                    # ``get()`` is None for a JSON null entry and for an absent
+                    # id. Installed-ness is key membership; snapshot the value
+                    # separately so a null can be restored verbatim.
+                    if registry.is_installed(component.id):
+                        had_snapshot = True
+                        metadata = copy.deepcopy(registry.get(component.id))
+                        backup_root = Path(
+                            tempfile.mkdtemp(prefix="speckit-step-refresh-")
+                        )
+                        backup_dir = backup_root / component.id
+                        step_dir = registry.steps_dir / component.id
+                        if step_dir.exists():
+                            shutil.copytree(step_dir, backup_dir)
+                        try:
+                            _delegate_command(
+                                "remove",
+                                f"step '{component.id}'",
+                                lambda: command_remove._remove_step_locked(
+                                    self._root, component.id
+                                ),
+                            )
+                        except BundlerError as exc:
+                            # ``_remove_step_locked`` drops the registry key
+                            # before the directory delete. A JSON null cannot
+                            # be put back there, so this error must reach
+                            # rollback below with the snapshot still intact.
+                            removal_error = exc
+            except step_installer.StepInstallError as exc:
+                # Lock acquisition failed before any package or registry snapshot.
+                raise BundlerError(
+                    f"Failed to refresh step '{component.id}': {exc}"
+                ) from exc
+
+            if not had_snapshot:
                 self.install(component)
-            except BundlerError:
-                if backup_dir.exists():
-                    shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True)
-                # Re-read the registry: ``StepRegistry`` snapshots the file once
-                # in ``__init__`` (``self.data = self._load()``) and
-                # ``is_installed`` only consults that snapshot. ``self.remove()``
-                # above has already deleted the entry from disk, but
-                # ``self._registry``'s snapshot still contains it -- so the
-                # guard was always False here and the restore never ran, in
-                # exactly the failure case it was written for. The step package
-                # came back but stayed unregistered: ``workflow step list``
-                # stopped showing it and ``workflow step add`` then refused with
-                # "Step directory already exists".
-                from ..workflows.catalog import StepRegistry
-
-                current = StepRegistry(self._root)
-                if metadata is not None and not current.is_installed(component.id):
-                    # Restore the saved entry verbatim rather than via ``add()``,
-                    # which would rewrite the metadata it is meant to roll back:
-                    # this registry is freshly constructed *after*
-                    # ``self.remove()`` deleted the entry, so ``add()`` sees no
-                    # existing record and stamps ``installed_at`` with
-                    # ``datetime.now()`` (it also overwrites ``updated_at``
-                    # unconditionally). ``workflow_step_remove`` bypasses
-                    # ``add()`` for exactly this reason.
-                    current.data["steps"][component.id] = metadata
-                    current.save()
+                return
+
+            try:
+                if removal_error is not None:
+                    raise removal_error
+                self.install(component)
+            except BundlerError as original:
+                try:
+                    with step_installer._step_install_transaction(self._root):
+                        assert backup_root is not None
+                        backup_dir = backup_root / component.id
+                        # Reinstall runs outside the lock, so the steps tree or
+                        # the registry file can be swapped for a symlink before
+                        # this section. ``_load`` returns an empty registry for
+                        # a symlinked registry file or parent and does not
+                        # raise. Resolve the steps base, then reject that
+                        # symlink before deleting or copying the package.
+                        # ``save()`` replaces the whole file: the document is
+                        # that guarded read plus this step's snapshot when the
+                        # id is absent. A later operation's package and registry
+                        # keys are left alone.
+                        current = StepRegistry(self._root)
+                        loaded = current._load()
+                        if not isinstance(loaded, dict):
+                            document = {
+                                "schema_version": StepRegistry.SCHEMA_VERSION,
+                                "steps": {},
+                            }
+                        else:
+                            document = loaded
+                            if not isinstance(document.get("steps"), dict):
+                                document["steps"] = {}
+                        steps = document["steps"]
+                        if component.id not in steps:
+                            steps_base = step_installer.resolve_steps_base_dir(
+                                self._root
+                            )
+                            step_dir = step_installer._resolve_step_dir(
+                                steps_base, component.id
+                            )
+                            step_installer._reject_unsafe_destination(step_dir)
+                            current.data = document
+                            step_installer._check_duplicate(
+                                current, component.id, step_dir, force=True
+                            )
+                            if (
+                                current._has_symlinked_parent()
+                                or current.registry_path.is_symlink()
+                            ):
+                                raise step_installer.StepInstallError(
+                                    "Refusing to restore step "
+                                    f"'{component.id}' through a symlinked "
+                                    "step registry at "
+                                    f"'{current.registry_path}'."
+                                )
+                            if step_dir.exists():
+                                shutil.rmtree(step_dir)
+                            if backup_dir.exists():
+                                shutil.copytree(backup_dir, step_dir)
+                            if had_snapshot:
+                                # Insert the snapshot verbatim, including JSON
+                                # null (``None``). ``StepRegistry.add`` would
+                                # rewrite ``installed_at`` and ``updated_at``.
+                                steps[component.id] = metadata
+                                current.data = document
+                                current.save()
+                except Exception as restore_exc:  # noqa: BLE001
+                    # The install error is what the caller handles. A failed
+                    # copy-back or registry write is recorded on it, and the
+                    # temp backup stays on disk for recovery.
+                    keep_backup = True
+                    original.add_note(
+                        f"Could not restore step '{component.id}' from backup "
+                        f"'{backup_root}': {restore_exc}"
+                    )
+                # Bare ``raise`` keeps the install error's ``__cause__``.
                 raise
         finally:
-            shutil.rmtree(backup_dir.parent, ignore_errors=True)
+            if backup_root is not None and not keep_backup:
+                shutil.rmtree(backup_root, ignore_errors=True)
 
     def remove(self, component: ComponentRef) -> None:
         from .. import workflow_step_remove

tests/specify_cli/bundles/test_commands.py

@@ -109,3 +109,88 @@ def test_error_paths_escape_rich_markup(project: Path, argv: list, expected: str
     # A MarkupError would surface here as an exception rather than a clean exit.
     assert isinstance(result.exception, SystemExit)
     assert expected in strip_ansi(result.output)
+
+
+@pytest.mark.parametrize(
+    ("argv", "target"),
+    [
+        (
+            ["bundle", "install", "demo"],
+            "specify_cli.bundles.project.find_project_root",
+        ),
+        (
+            ["bundle", "update", "demo"],
+            "specify_cli.bundles.command_update.require_project_root",
+        ),
+    ],
+)
+def test_refresh_cli_shows_restoration_notes(monkeypatch, argv, target):
+    """``bundle install`` and ``bundle update`` show notes after the error."""
+    from specify_cli.bundles import BundlerError
+
+    def _boom(*_args, **_kwargs):
+        exc = BundlerError("primary failure")
+        exc.add_note("Could not restore step 'my-step' from backup '/tmp/bak'")
+        raise exc
+
+    monkeypatch.setattr(target, _boom)
+    result = runner.invoke(app, argv)
+    output = " ".join(strip_ansi(result.output).split())
+    assert result.exit_code == 1, result.output
+    assert "Error:" in output
+    assert "primary failure" in output
+    assert "Could not restore step 'my-step'" in output
+    assert "/tmp/bak" in output
+    assert output.index("primary failure") < output.index("Could not restore")
+
+
+def test_update_cli_shows_notes_from_real_install_bundle_refresh(
+    project: Path, monkeypatch
+):
+    """A refresh error's notes survive ``install_bundle`` and reach the CLI.
+
+    ``install_bundle`` runs for real here: only the primitive installer is
+    faked, and its step refresh fails the way a failed rollback does (the
+    install error first, the restoration failure as a note).
+    """
+    from types import SimpleNamespace
+
+    from specify_cli.bundles import BundlerError, adapters, command_update
+    from specify_cli.bundles.manifest import BundleManifest, ComponentRef
+    from specify_cli.bundles.records import InstalledBundleRecord, save_records
+    from tests.specify_cli.bundles.helpers import FakeInstaller, valid_manifest_dict
+
+    data = valid_manifest_dict(provides={"steps": [{"id": "my-step"}]})
+    manifest = BundleManifest.from_dict(data)
+    step = ComponentRef(kind="steps", id="my-step")
+    save_records(project, [
+        InstalledBundleRecord.create(manifest.bundle.id, manifest.bundle.version, [step]),
+    ])
+
+    class _RollbackFailingInstaller(FakeInstaller):
+        def refresh(self, project_root, component):
+            super().refresh(project_root, component)
+            exc = BundlerError(f"Failed to install step '{component.id}'.")
+            exc.add_note(
+                f"Could not restore step '{component.id}' from backup "
+                "'/tmp/speckit-step-refresh-x': restore copy failed"
+            )
+            raise exc
+
+    fake = _RollbackFailingInstaller()
+    fake.installed.add(("steps", "my-step"))
+    stack = SimpleNamespace(resolve=lambda target: SimpleNamespace(install_allowed=True))
+    monkeypatch.setattr(command_update, "_build_stack", lambda *a, **kw: stack)
+    monkeypatch.setattr(command_update, "_download_manifest", lambda *a, **kw: manifest)
+    monkeypatch.setattr(adapters, "DefaultPrimitiveInstaller", lambda **kw: fake)
+
+    result = runner.invoke(app, ["bundle", "update", manifest.bundle.id])
+
+    assert result.exit_code == 1, result.output
+    assert fake.refresh_calls == [("steps", "my-step")]
+    output = " ".join(strip_ansi(result.stderr).split())
+    assert "Error: Failed to install step 'my-step'." in output
+    assert "Could not restore step 'my-step'" in output
+    assert "/tmp/speckit-step-refresh-x" in output
+    assert "restore copy failed" in output
+    assert output.index("Failed to install step") < output.index("Could not restore")