From 471a05dbb878d81408f80890877e6af6c4b1c814 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 19:13:50 +0500 Subject: [PATCH] fix(bundler): re-read the step registry when rolling back a failed refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_StepKindManager.refresh` documents that it keeps a backup and restores it "if the remove+reinstall path fails". The package half of that rollback works; the registry half was unreachable. `StepRegistry.__init__` snapshots the file once (`self.data = self._load()`) and `is_installed` consults only that snapshot. Measured: snapshot at construction: is_installed('my-step') = True after the entry is deleted on disk: same object = True <-- stale a fresh StepRegistry: = False By rollback time `self.remove()` has already deleted the entry from disk, but `self._registry`'s snapshot still contains it — so `not self._registry.is_installed(...)` was always False and the restore never ran, in exactly the failure case it was written for. The user was left with the step package back on disk but unregistered: `workflow step list` no longer shows it, the engine cannot resolve it, and a later `workflow step add ` refuses with "Step directory already exists". Read the registry fresh at rollback time. Co-Authored-By: Claude Opus 5 (1M context) --- .../bundler/services/primitives.py | 17 ++++- tests/unit/test_bundler_primitives.py | 62 +++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 31b1126a34..0309aa6fad 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -428,8 +428,21 @@ def refresh(self, component: ComponentRef) -> None: except BundlerError: if backup_dir.exists(): shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) - if metadata is not None and not self._registry.is_installed(component.id): - self._registry.add(component.id, metadata) + # 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): + current.add(component.id, metadata) raise finally: shutil.rmtree(backup_dir.parent, ignore_errors=True) diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index dc39106b50..272b63bb7f 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -334,3 +334,65 @@ def _plan(manifest): effective_integration=None, components=components, ) + + +def test_step_refresh_restores_registry_entry_when_reinstall_fails( + tmp_path: Path, monkeypatch +): + """A failed step refresh must leave the registry entry restored. + + ``refresh`` keeps a backup and restores it "if the remove+reinstall path + fails", but the registry half of that rollback was unreachable: + ``StepRegistry`` snapshots the file once in ``__init__`` and + ``is_installed`` reads only that snapshot, so after ``self.remove()`` + deleted the entry from disk the stale snapshot still reported it as + installed and ``not ...is_installed(...)`` was always False. + + 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". + """ + import json + + import specify_cli + from specify_cli.workflows.catalog import StepRegistry + + steps_dir = tmp_path / ".specify" / "workflows" / "steps" + (steps_dir / "my-step").mkdir(parents=True) + (steps_dir / "my-step" / "step.yml").write_text( + "step:\n type_key: my-step\n", encoding="utf-8" + ) + (steps_dir / "my-step" / "__init__.py").write_text("", encoding="utf-8") + (steps_dir / StepRegistry.REGISTRY_FILE).write_text( + json.dumps( + { + "schema_version": "1.0", + "steps": { + "my-step": { + "name": "My Step", + "version": "1.0.0", + "type_key": "my-step", + } + }, + } + ), + encoding="utf-8", + ) + + assert StepRegistry(tmp_path).is_installed("my-step") + + # Removal succeeds (real code path); only the re-install fails, which is + # what a catalog 404 / size-limit / type_key mismatch produces. + def _boom(step_id, *args, **kwargs): + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(specify_cli, "workflow_step_add", _boom) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + with pytest.raises(BundlerError): + manager.refresh(_component("steps", "my-step")) + + # Read the registry fresh from disk — the point of the fix. + assert StepRegistry(tmp_path).is_installed("my-step"), ( + steps_dir / StepRegistry.REGISTRY_FILE + ).read_text(encoding="utf-8")