diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 645b2f9e44..06ab439d6d 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -69,7 +69,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. Without `--refresh`, 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. Without `--refresh`, installation is idempotent — components already present are skipped. Components installed outside any bundle are skipped and never adopted, so their installed version must match the manifest pin; if it doesn't, or can't be read, install and refresh stop before changing anything and name the component, so you can remove it or install the pinned version yourself. 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. A normal install rejects a change to an already-recorded bundle's version or owned component metadata (version, source, preset priority, or strategy), including removal of an owned component. This applies even if a local manifest keeps the same bundle version. Reordering unchanged components or adding new components does not require refresh. To apply changes to a local bundle without adding it to a catalog, pass the revised source with `--refresh`: @@ -95,7 +95,7 @@ specify bundle update [] Re-resolves a bundle and **refreshes** its components through each primitive's update path, bringing already-installed components up to the bundle's newly pinned versions while preserving primitive-level overrides (such as preset priority). Provide a bundle id, or use `--all` to update everything installed. -> **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 ` for catalog bundles or `specify bundle install --refresh` for local sources to re-apply owned components at their pinned versions. +> **Pin enforcement is install-time only.** Idempotency checks are id-based, not version-aware: a component owned by a bundle 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 ` for catalog bundles or `specify bundle install --refresh` for local sources to re-apply owned components at their pinned versions. ## Remove a Bundle diff --git a/src/specify_cli/bundles/adapters.py b/src/specify_cli/bundles/adapters.py index 5178529cbf..ea3739c576 100644 --- a/src/specify_cli/bundles/adapters.py +++ b/src/specify_cli/bundles/adapters.py @@ -324,6 +324,12 @@ def is_installed(self, project_root: Path, component: ComponentRef) -> bool: manager = self._manager_for(component, project_root) return manager.is_installed(component) + def installed_version( + self, project_root: Path, component: ComponentRef + ) -> str | None: + manager = self._manager_for(component, project_root) + return manager.installed_version(component) + def install(self, project_root: Path, component: ComponentRef) -> None: manager = self._manager_for(component, project_root) manager.install(component) diff --git a/src/specify_cli/bundles/installer.py b/src/specify_cli/bundles/installer.py index 77bfba7a6e..29c7cfae61 100644 --- a/src/specify_cli/bundles/installer.py +++ b/src/specify_cli/bundles/installer.py @@ -28,6 +28,7 @@ ) from .conflict import detect_conflicts from .resolver import InstallPlan +from .versioning import same_version class PrimitiveInstaller(Protocol): @@ -86,6 +87,10 @@ def install_bundle( guaranteed to be applied when the bundler actually performs an install or a refresh; running ``specify bundle update`` re-applies every owned component at its pinned version. + + The exception is a component installed independently of any bundle: it is + never installed or refreshed here, so its installed version must already + match the pin, or the call fails before changing anything. """ records = load_records(project_root) @@ -127,10 +132,10 @@ def install_bundle( if r.bundle_id != plan.bundle_id for c in r.contributed_components } - contributed: list[ComponentRef] = [] done: list[ComponentRef] = [] try: + _check_unowned_pins(project_root, plan, installer, prior_ours | other_tracked) for component in plan.components: key = (component.kind, component.id) if installer.is_installed(project_root, component): @@ -247,6 +252,44 @@ def remove_bundle( return result +def _check_unowned_pins( + project_root: Path, + plan: InstallPlan, + installer: PrimitiveInstaller, + owned: set[tuple[str, str]], +) -> None: + """Refuse to skip an independently installed component that misses its pin. + + A component tracked by no bundle is skipped and never refreshed (FR-022), so + skipping it is only correct when it already has the pinned version. + Otherwise the bundle record would advance while the project keeps running a + different version (#4434). Runs before any primitive is touched. A component + whose installed version can't be read fails too, since it can't be shown to + match. Installers without an ``installed_version`` hook are not checked. + """ + installed_version = getattr(installer, "installed_version", None) + if not callable(installed_version): + return + mismatches = [] + for component in plan.components: + if not component.version or (component.kind, component.id) in owned: + continue + if not installer.is_installed(project_root, component): + continue + actual = installed_version(project_root, component) + pinned = f"{component.kind[:-1]} '{component.id}' to {component.version}" + if actual is None: + mismatches.append(f"{pinned}, but its installed version is unknown") + elif not same_version(actual, component.version): + mismatches.append(f"{pinned}, but {actual} is installed") + if mismatches: + raise BundlerError( + f"Bundle '{plan.bundle_id}' pins {'; '.join(mismatches)}. Bundles " + "leave components installed outside any bundle unchanged, so remove " + "the installed version or install the pinned one yourself, then re-run." + ) + + def _refresh_component( project_root: Path, installer: PrimitiveInstaller, diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index b32342e68d..c885d62443 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -46,13 +46,9 @@ def _assert_pinned_version( actual = str(advertised).strip() if not actual: return - from .versioning import parse_version + from .versioning import same_version - try: - matches = parse_version(actual) == parse_version(pinned) - except BundlerError: - matches = actual == str(pinned).strip() - if not matches: + if not same_version(actual, pinned): raise BundlerError( f"{kind} '{component_id}' is pinned to version {pinned} in the bundle " f"manifest, but the resolved version is {actual}. Update the bundle's " @@ -84,10 +80,27 @@ def _bundled_manifest_version(manifest_path: Path, root_key: str) -> str | None: return None +def _registry_version(registry, component_id: str) -> str | None: + """Version a primitive registry recorded for an installed component. + + Returns ``None`` when there is no entry, the registry is unreadable, or the + entry has no usable version, meaning the installed version is unknown. + """ + try: + entry = registry.get(component_id) + except Exception: # noqa: BLE001 - unreadable registry: version unknown + return None + version = entry.get("version") if isinstance(entry, dict) else None + return version if isinstance(version, str) and version.strip() else None + + class _KindManager(Protocol): def is_installed(self, component: ComponentRef) -> bool: pass + def installed_version(self, component: ComponentRef) -> str | None: + pass + def install(self, component: ComponentRef) -> None: pass @@ -156,6 +169,9 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def installed_version(self, component: ComponentRef) -> str | None: + return _registry_version(self._manager.registry, component.id) + def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -243,6 +259,9 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def installed_version(self, component: ComponentRef) -> str | None: + return _registry_version(self._manager.registry, component.id) + def install(self, component: ComponentRef) -> None: self._do_install(component, force=False) @@ -334,6 +353,9 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def installed_version(self, component: ComponentRef) -> str | None: + return _registry_version(self._registry, component.id) + def install(self, component: ComponentRef) -> None: from .._assets import _locate_bundled_workflow @@ -424,6 +446,9 @@ def is_installed(self, component: ComponentRef) -> bool: except Exception: # noqa: BLE001 return False + def installed_version(self, component: ComponentRef) -> str | None: + return _registry_version(self._registry, component.id) + def install(self, component: ComponentRef) -> None: if not self._allow_network: raise BundlerError( diff --git a/src/specify_cli/bundles/versioning.py b/src/specify_cli/bundles/versioning.py index da980de94a..df73b5bab6 100644 --- a/src/specify_cli/bundles/versioning.py +++ b/src/specify_cli/bundles/versioning.py @@ -79,6 +79,18 @@ def satisfies(installed: str, constraint: str) -> bool: return spec.contains(version, prereleases=True) +def same_version(actual: str, pinned: str) -> bool: + """Return True if *actual* is the exact version *pinned* names. + + Compares parsed versions (``v1.0.0`` matches ``1.0.0``) and falls back to a + plain string comparison when either side does not parse. + """ + try: + return parse_version(actual) == parse_version(pinned) + except BundlerError: + return str(actual).strip() == str(pinned).strip() + + _SEMVER_RE = re.compile( r"^(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)" r"(?:-(?:(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)" diff --git a/tests/specify_cli/bundles/helpers.py b/tests/specify_cli/bundles/helpers.py index 196ec9d60f..e536b7e554 100644 --- a/tests/specify_cli/bundles/helpers.py +++ b/tests/specify_cli/bundles/helpers.py @@ -118,6 +118,9 @@ def __init__(self, *, fail_on: str | None = None) -> None: self.install_calls: list[tuple[str, str]] = [] self.remove_calls: list[tuple[str, str]] = [] self.refresh_calls: list[tuple[str, str]] = [] + # Installed versions reported by ``installed_version``; set by tests + # that pre-install a component at a specific version. + self.versions: dict[tuple[str, str], str] = {} self._fail_on = fail_on def _key(self, component: ComponentRef) -> tuple[str, str]: @@ -126,6 +129,9 @@ def _key(self, component: ComponentRef) -> tuple[str, str]: def is_installed(self, project_root: Path, component: ComponentRef) -> bool: return self._key(component) in self.installed + def installed_version(self, project_root: Path, component: ComponentRef) -> str | None: + return self.versions.get(self._key(component)) + def install(self, project_root: Path, component: ComponentRef) -> None: from specify_cli.bundler import BundlerError diff --git a/tests/specify_cli/bundles/test_command_install.py b/tests/specify_cli/bundles/test_command_install.py index 7a5a3ec2aa..fb6c81e380 100644 --- a/tests/specify_cli/bundles/test_command_install.py +++ b/tests/specify_cli/bundles/test_command_install.py @@ -2,6 +2,7 @@ import json import os +import shutil import zipfile from pathlib import Path from unittest.mock import patch @@ -86,6 +87,44 @@ def test_local_bundle_refuses_unbundled_workflow_offline(project: Path): assert "network access is disabled" in " ".join(result.output.lower().split()) +def test_local_bundle_refuses_independently_installed_extension_at_other_version( + project: Path, +): + from specify_cli.bundles.records import records_path + + older = project / "bug-older" + shutil.copytree(REPO_ROOT / "extensions" / "bug", older) + ext_manifest = yaml.safe_load((older / "extension.yml").read_text(encoding="utf-8")) + ext_manifest["extension"]["version"] = "0.0.1" + (older / "extension.yml").write_text(yaml.safe_dump(ext_manifest), encoding="utf-8") + added = runner.invoke(app, ["extension", "add", str(older), "--dev"]) + assert added.exit_code == 0, added.output + + pinned = bundled_extension_version("bug") + bundle_dir = project / "pins-bug" + bundle_dir.mkdir() + (bundle_dir / "bundle.yml").write_text( + yaml.safe_dump( + valid_manifest_dict( + provides={"extensions": [{"id": "bug", "version": pinned}]} + ) + ), + encoding="utf-8", + ) + + result = runner.invoke(app, ["bundle", "install", str(bundle_dir), "--offline"]) + + assert result.exit_code == 1 + assert f"extension 'bug' to {pinned}, but 0.0.1 is installed" in " ".join( + result.output.split() + ) + assert not records_path(project).exists() + registry = json.loads( + (project / ".specify" / "extensions" / ".registry").read_text(encoding="utf-8") + ) + assert registry["extensions"]["bug"]["version"] == "0.0.1" + + def test_install_refuses_discovery_only_source(project: Path, monkeypatch): # Point a discovery-only catalog at a local payload containing the bundle. catalog = project / "disc.json" diff --git a/tests/specify_cli/bundles/test_installer.py b/tests/specify_cli/bundles/test_installer.py index f004b2a0c2..57bf044568 100644 --- a/tests/specify_cli/bundles/test_installer.py +++ b/tests/specify_cli/bundles/test_installer.py @@ -492,6 +492,7 @@ def test_refresh_does_not_touch_independently_installed_component(tmp_path: Path manifest = BundleManifest.from_dict(valid_manifest_dict()) installer = FakeInstaller() installer.installed.add(("extensions", "ext-a")) + installer.versions[("extensions", "ext-a")] = "1.0.0" result = install_bundle( tmp_path, _plan(manifest), installer, manifest=manifest, refresh=True @@ -516,6 +517,7 @@ def test_pre_existing_component_is_not_attributed_or_removed(tmp_path: Path): installer = FakeInstaller() # Pre-install ext-a independently — no bundle record references it yet. installer.installed.add(("extensions", "ext-a")) + installer.versions[("extensions", "ext-a")] = "1.0.0" install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) @@ -528,6 +530,109 @@ def test_pre_existing_component_is_not_attributed_or_removed(tmp_path: Path): assert ("extensions", "ext-a") in installer.installed +def test_install_rejects_independently_installed_component_at_other_version( + tmp_path: Path, +): + # An independently installed component is skipped and never refreshed, so + # recording the bundle over a different version would leave the project + # running the old one under a record that says otherwise (#4434). + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + installer.installed.add(("extensions", "ext-a")) + installer.versions[("extensions", "ext-a")] = "0.9.0" + + with pytest.raises( + BundlerError, match=r"extension 'ext-a' to 1\.0\.0, but 0\.9\.0 is installed" + ): + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + assert installer.install_calls == [] + assert not records_path(tmp_path).exists() + + +def test_refresh_rejects_independently_installed_component_at_other_version( + tmp_path: Path, +): + # bundle update never refreshes an unowned component, so a new pin it does + # not meet must fail instead of advancing the record past it. + make_project(tmp_path) + installer = FakeInstaller() + installer.installed.add(("extensions", "ext-a")) + installer.versions[("extensions", "ext-a")] = "1.0.0" + man_v1 = _bundle("demo", ["ext-a"]) + 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") + with pytest.raises(BundlerError, match=r"to 2\.0\.0, but 1\.0\.0 is installed"): + install_bundle( + tmp_path, _plan(man_v2), installer, manifest=man_v2, refresh=True + ) + + assert installer.refresh_calls == [] + assert records_path(tmp_path).read_bytes() == original_record + + +def test_install_rejects_independently_installed_component_of_unknown_version( + tmp_path: Path, +): + # A registry entry without a readable version can't be shown to meet the + # pin, and skipping it would recreate the #4434 mismatch. + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + installer.installed.add(("extensions", "ext-a")) + + with pytest.raises( + BundlerError, match=r"ext-a' to 1\.0\.0, but its installed version is unknown" + ): + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + assert installer.install_calls == [] + assert not records_path(tmp_path).exists() + + +def test_install_converts_raw_pin_check_exception_to_bundler_error( + tmp_path: Path, +): + # The pin check runs before anything is installed, but an unreadable + # registry (e.g. _WorkflowKindManager failing closed at construction) must + # still surface as the clean BundlerError the CLI catches. + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + installer.installed.add(("extensions", "ext-a")) + + def boom(project_root, component): + raise OSError("workflow registry unreadable") + + installer.installed_version = boom + with pytest.raises(BundlerError, match="workflow registry unreadable"): + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + assert installer.install_calls == [] + assert not records_path(tmp_path).exists() + + +def test_independently_installed_component_at_pinned_version_stays_unowned( + tmp_path: Path, +): + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + installer.installed.add(("extensions", "ext-a")) + installer.versions[("extensions", "ext-a")] = "v1.0.0" + + result = install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + assert ("extensions", "ext-a") in {(c.kind, c.id) for c in result.skipped} + contributed = { + (c.kind, c.id) for c in load_records(tmp_path)[0].contributed_components + } + assert ("extensions", "ext-a") not in contributed + + def _bundle(manifest_id, ext_ids, *, version="1.0.0"): data = valid_manifest_dict() data["bundle"]["id"] = manifest_id diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index a3b4d83f45..4965b560f3 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -21,6 +21,9 @@ _WorkflowKindManager, primitive_manager, ) +from specify_cli.extensions import ExtensionRegistry +from specify_cli.presets import PresetRegistry +from specify_cli.workflows.catalog import StepRegistry, WorkflowRegistry from tests.specify_cli.bundles.helpers import valid_manifest_dict @@ -82,6 +85,23 @@ def test_offline_refresh_explains_component_needs_network(tmp_path: Path, kind: assert "install it first" not in message +_REGISTRIES = { + "extensions": lambda root: ExtensionRegistry(root / ".specify" / "extensions"), + "presets": lambda root: PresetRegistry(root / ".specify" / "presets"), + "workflows": WorkflowRegistry, + "steps": StepRegistry, +} + + +@pytest.mark.parametrize("kind", sorted(_REGISTRIES)) +def test_installed_version_reads_each_primitive_registry(tmp_path: Path, kind: str): + _REGISTRIES[kind](tmp_path).add("x", {"version": "0.9.0"}) + installer = DefaultPrimitiveInstaller() + + assert installer.installed_version(tmp_path, _component(kind)) == "0.9.0" + assert installer.installed_version(tmp_path, _component(kind, "missing")) is None + + def test_offline_workflow_allows_bundled(tmp_path: Path, monkeypatch): # A workflow that ships with Spec Kit must install even with --offline. import specify_cli