From 471a05dbb878d81408f80890877e6af6c4b1c814 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 19:13:50 +0500 Subject: [PATCH 1/2] 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") From b18fc49aca95bd08d30c51f5e8e9bd6f3790958a Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 30 Aug 2026 20:30:07 +0500 Subject: [PATCH 2/2] fix(bundler): restore the step registry entry verbatim on refresh rollback Addresses review feedback: the rollback used `StepRegistry.add()`, which does not restore the saved metadata verbatim. The rollback deliberately constructs a *fresh* `StepRegistry` after `self.remove()` has deleted the entry from disk (that re-read is this PR's actual fix). `add()` therefore finds no existing record: metadata_to_store["installed_at"] = existing.get( "installed_at", datetime.now(timezone.utc).isoformat() ) metadata_to_store["updated_at"] = datetime.now(timezone.utc).isoformat() `existing` is `{}`, so `installed_at` falls through to `now`, and `updated_at` is overwritten unconditionally. A failed refresh still mutated the installation metadata instead of rolling it back: seeded : installed_at 2020-01-01..., updated_at 2020-02-02... after add() : installed_at 2026-08-27..., updated_at 2026-08-27... verbatim via add() -> False verbatim via direct restore -> True Restore the entry directly and save, matching the existing rollback in `workflow_step_remove`, whose comment names this very hazard: "Restore the original registry entry verbatim (bypass add() which would overwrite timestamps)." The regression test now seeds distinctive past timestamps and asserts the restored entry equals the seeded one, rather than only asserting presence. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/bundler/services/primitives.py | 11 ++++++++++- tests/unit/test_bundler_primitives.py | 12 +++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 0309aa6fad..b05b1c97be 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -442,7 +442,16 @@ def refresh(self, component: ComponentRef) -> None: current = StepRegistry(self._root) if metadata is not None and not current.is_installed(component.id): - current.add(component.id, metadata) + # 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() 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 272b63bb7f..db507e2681 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -372,6 +372,11 @@ def test_step_refresh_restores_registry_entry_when_reinstall_fails( "name": "My Step", "version": "1.0.0", "type_key": "my-step", + # Distinctive past timestamps: a rollback must put the + # entry back verbatim, and ``StepRegistry.add()`` would + # silently replace both of these with ``now``. + "installed_at": "2020-01-01T00:00:00+00:00", + "updated_at": "2020-02-02T00:00:00+00:00", } }, } @@ -379,6 +384,7 @@ def test_step_refresh_restores_registry_entry_when_reinstall_fails( encoding="utf-8", ) + seeded = StepRegistry(tmp_path).get("my-step") assert StepRegistry(tmp_path).is_installed("my-step") # Removal succeeds (real code path); only the re-install fails, which is @@ -393,6 +399,10 @@ def _boom(step_id, *args, **kwargs): 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"), ( + restored = StepRegistry(tmp_path) + assert restored.is_installed("my-step"), ( steps_dir / StepRegistry.REGISTRY_FILE ).read_text(encoding="utf-8") + # A rollback must be a rollback: the entry comes back byte-for-byte, not + # re-registered with fresh ``installed_at`` / ``updated_at`` stamps. + assert restored.get("my-step") == seeded