diff --git a/CHANGELOG.md b/CHANGELOG.md index 688e289ac..fa3edc706 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm update` no longer leaves `apm_modules/` silently ahead of + `apm.lock.yaml` when the plan-confirmation gate does not commit: declining + the prompt, aborting in a non-interactive shell (no TTY, no `--yes`), and + `--dry-run` all now leave dependency content untouched. Resolving a semver + range re-downloads the candidate version to compute the update plan before + the user has confirmed anything; that download is now staged and rolled + back unless the update is actually applied. Affects both git-source and + registry-source dependencies. (by @nadav-y) (#2050) + ## [0.24.0] - 2026-07-05 ### Added diff --git a/src/apm_cli/install/context.py b/src/apm_cli/install/context.py index 126efa91e..575628a71 100644 --- a/src/apm_cli/install/context.py +++ b/src/apm_cli/install/context.py @@ -74,6 +74,12 @@ class InstallContext: only_packages: list[str] | None = None protocol_pref: Any = None # ProtocolPreference (NONE/SSH/HTTPS) for shorthand transport allow_protocol_fallback: bool | None = None # None => read APM_ALLOW_PROTOCOL_FALLBACK env + # Set by the pipeline before the resolve phase runs when the caller + # (``apm update``) has a plan-confirmation gate. Only its presence is + # consulted (by resolve.py, to decide whether cached semver paths must + # be staged for possible rollback rather than deleted outright) -- the + # pipeline itself still owns invoking it. + plan_callback: Any = None # ------------------------------------------------------------------ # Resolve phase outputs @@ -96,6 +102,13 @@ class InstallContext: callback_downloaded: dict[str, Any] = field(default_factory=dict) # resolve callback_failures: set[str] = field(default_factory=set) # resolve transitive_failures: list[tuple[str, str]] = field(default_factory=list) # resolve + # dep_key -> backup path for cached semver installs moved aside by + # ``update_backup.purge_cached_semver_paths_for_update`` (populated only + # when ``plan_callback`` is set). The pipeline calls + # ``update_backup.restore_update_backups`` once the plan-confirmation + # gate resolves, to either discard these (applied) or restore them + # (declined / non-interactive abort / --dry-run). + update_backups: dict[str, Any] = field(default_factory=dict) # resolve # ------------------------------------------------------------------ # Targets phase outputs diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index b2b3393bb..547fde151 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -174,56 +174,6 @@ def _maybe_resolve_git_semver( ) -def _purge_cached_semver_paths_for_update( - *, - all_apm_deps, - apm_modules_dir, - logger, -) -> None: - """Pre-purge on-disk install paths for direct git-source and registry semver deps - when ``--update`` / ``--refresh`` is set. - - Bug 1 fix (#1496): the BFS resolver short-circuits at - ``install_path.exists()`` and never invokes ``download_callback``, - which is where ``_maybe_resolve_git_semver`` lives. For git-source - semver direct deps we therefore pre-purge the install path so the - resolver is forced through the callback, re-runs ``git ls-remote``, - and rewrites the lockfile with the latest matching tag. Matches - npm / cargo / bundler: ``--update`` is the explicit re-resolve - trigger and must not be swallowed by the on-disk cache. Scoped to - direct deps to avoid disturbing transitive cached content; the - resolver re-walks transitives naturally once a direct dep's - callback rewrites its ref. Local and proxy deps are excluded (their - semver semantics belong to a different resolver path). Registry semver - deps are included: their callback also gates on install_path.exists(). - """ - from contextlib import suppress - - from apm_cli.utils.file_ops import robust_rmtree as _rrm - - for _dep in all_apm_deps: - if getattr(_dep, "ref_kind", None) != "semver": - continue - if _dep.is_local: - continue - if getattr(_dep, "artifactory_prefix", None): - continue - try: - _ip = _dep.get_install_path(apm_modules_dir) - except Exception: # noqa: S112 - # Path computation failure (e.g. malformed dep) is non-fatal - # here -- the resolver will surface a real error downstream. - continue - if _ip.exists(): - with suppress(Exception): - _rrm(_ip, ignore_errors=True) - if logger: - logger.verbose_detail( - f"[*] --update: cleared cached install path for " - f"{_dep.get_unique_key()} to force semver re-resolution" - ) - - def _load_lockfile(ctx: InstallContext) -> None: """Load ``apm.lock.yaml`` and populate ``ctx.existing_lockfile`` / ``ctx.lockfile_path``.""" # ------------------------------------------------------------------ @@ -757,10 +707,23 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): # 6. Resolver creation + dependency resolution # ------------------------------------------------------------------ if update_refs: - _purge_cached_semver_paths_for_update( + # A plan-confirmation gate (``apm update``) can decline after this + # point, or the pipeline can abort before ever reaching it + # (non-interactive shell, --dry-run) -- stage the purged content + # so ``restore_update_backups`` can put it back rather than + # leaving apm_modules/ ahead of apm.lock.yaml. Callers with no + # gate (``apm install --update``) always apply, so purge-by-delete + # is unchanged for them. + from .update_backup import purge_cached_semver_paths_for_update + + _backup_root = ( + ctx.apm_dir / ".apm-update-backup" if getattr(ctx, "plan_callback", None) else None + ) + ctx.update_backups = purge_cached_semver_paths_for_update( all_apm_deps=ctx.all_apm_deps, apm_modules_dir=ctx.apm_modules_dir, logger=ctx.logger, + backup_root=_backup_root, ) resolver = APMDependencyResolver( diff --git a/src/apm_cli/install/phases/update_backup.py b/src/apm_cli/install/phases/update_backup.py new file mode 100644 index 000000000..33d88d2c6 --- /dev/null +++ b/src/apm_cli/install/phases/update_backup.py @@ -0,0 +1,219 @@ +"""Stage-and-restore mechanism for ``apm update``'s plan-confirmation gate. + +``download_callback`` (see ``resolve.py``) materialises a re-resolved +semver dep's new content to disk as part of *resolving* the dependency +graph -- this is unavoidable, since discovering a package's transitive +deps requires reading its manifest. But ``apm update`` shows the computed +plan and asks for confirmation only *after* resolve completes. Left alone, +that means a declined confirmation, a non-interactive abort (no TTY, no +``--yes``), or ``--dry-run`` all leave ``apm_modules/`` already advanced to +the new version while ``apm.lock.yaml`` stays on the old one. + +This module closes that gap: ``purge_cached_semver_paths_for_update`` +moves a semver dep's existing install path aside (instead of deleting it) +so the resolver is still forced through ``download_callback`` to +re-resolve, and ``restore_update_backups`` reconciles the outcome once the +plan-confirmation gate resolves -- discarding the backups on commit, or +restoring them (and removing any freshly-added content) otherwise. +""" + +from __future__ import annotations + +import hashlib +import re +from contextlib import suppress +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from apm_cli.utils.file_ops import robust_rmtree as _rrm + +if TYPE_CHECKING: + from apm_cli.install.context import InstallContext + from apm_cli.models.dependency.reference import DependencyReference + +# Short enough to keep backup directory names readable, long enough that an +# accidental collision between two sanitized-but-distinct dep keys is not a +# realistic concern. +_DISAMBIGUATOR_LEN = 8 + + +def _sanitize_backup_name(dep_key: str) -> str: + """Turn a dep key into a filesystem-safe, collision-resistant directory name. + + Sanitizing alone is not injective: ``"owner/repo"``, ``"owner_repo"``, and + ``"owner:repo"`` would all collapse to the same ``"owner_repo"`` name, + which risks one dep's backup silently overwriting another's (or restoring + the wrong content). A short hash of the original, un-sanitized key is + appended to keep distinct keys apart even after sanitization. + """ + safe = re.sub(r"[^A-Za-z0-9._-]", "_", dep_key) + digest = hashlib.sha256(dep_key.encode("utf-8")).hexdigest()[:_DISAMBIGUATOR_LEN] + return f"{safe}-{digest}" + + +def purge_cached_semver_paths_for_update( + *, + all_apm_deps: list[DependencyReference], + apm_modules_dir: Path, + logger: Any, + backup_root: Path | None = None, +) -> dict[str, Path]: + """Pre-purge on-disk install paths for direct git-source and registry semver deps + when ``--update`` / ``--refresh`` is set. + + Bug 1 fix (#1496): the BFS resolver short-circuits at + ``install_path.exists()`` and never invokes ``download_callback``, + which is where ``_maybe_resolve_git_semver`` lives. For git-source + semver direct deps we therefore pre-purge the install path so the + resolver is forced through the callback, re-runs ``git ls-remote``, + and rewrites the lockfile with the latest matching tag. Matches + npm / cargo / bundler: ``--update`` is the explicit re-resolve + trigger and must not be swallowed by the on-disk cache. Scoped to + direct deps to avoid disturbing transitive cached content; the + resolver re-walks transitives naturally once a direct dep's + callback rewrites its ref. Local and proxy deps are excluded (their + semver semantics belong to a different resolver path). Registry semver + deps are included: their callback also gates on install_path.exists(). + + When *backup_root* is given, the existing content is moved there + instead of being deleted outright, and the returned dict maps + ``dep_key -> backup_path`` so a caller with a plan-confirmation gate + (``apm update``) can restore it if the plan is ultimately declined -- + see ``restore_update_backups``. When *backup_root* is ``None`` (e.g. + ``apm install --update``, which has no decline path), the old + delete-outright behaviour is unchanged. + """ + backups: dict[str, Path] = {} + for _dep in all_apm_deps: + if getattr(_dep, "ref_kind", None) != "semver": + continue + if _dep.is_local: + continue + if getattr(_dep, "artifactory_prefix", None): + continue + try: + _ip = _dep.get_install_path(apm_modules_dir) + except Exception: # noqa: S112 + # Path computation failure (e.g. malformed dep) is non-fatal + # here -- the resolver will surface a real error downstream. + continue + if not _ip.exists(): + continue + _cleared = False + if backup_root is not None: + _dep_key = _dep.get_unique_key() + _backup_path = backup_root / _sanitize_backup_name(_dep_key) + with suppress(Exception): + if _backup_path.exists(): + _rrm(_backup_path, ignore_errors=True) + _backup_path.parent.mkdir(parents=True, exist_ok=True) + _ip.rename(_backup_path) + backups[_dep_key] = _backup_path + _cleared = True + else: + with suppress(Exception): + _rrm(_ip, ignore_errors=True) + _cleared = True + # Only claim the path was cleared when the rename/rmtree actually + # succeeded -- a swallowed exception above must not be reported as + # a successful purge, which would mislead a verbose user into + # thinking semver re-resolution will occur when it may not. + if logger and _cleared: + logger.verbose_detail( + f"[*] --update: cleared cached install path for " + f"{_dep.get_unique_key()} to force semver re-resolution" + ) + return backups + + +def restore_update_backups(ctx: InstallContext, *, keep_new: bool) -> None: + """Reconcile ``ctx.update_backups`` after the plan-confirmation gate resolves. + + When *keep_new* is True (the update was confirmed and applied) AND the + dep was actually re-downloaded this run, the fresh content stays in + place and its backup is discarded. Every other backed-up dep -- either + because *keep_new* is False (declined, non-interactive abort, or + ``--dry-run``), or because it was purged but never actually + re-resolved (e.g. a failure elsewhere aborted the run first) -- has + its original content moved back into place. When not committing, a + dep with no prior backup that was nonetheless downloaded this run (a + fresh add swept up by this resolve pass) has its new content removed + entirely. This is what keeps a declined/aborted/dry-run ``apm update`` + from silently leaving ``apm_modules/`` ahead of ``apm.lock.yaml``. + + Deps are looked up via ``ctx.all_apm_deps`` merged with + ``ctx.deps_to_install`` rather than ``ctx.deps_to_install`` alone: the + latter (the full transitive closure) is only populated after the BFS + resolver returns successfully, so if resolution itself raises (a + network error, a bad transitive manifest, etc.) after some direct deps + were already purged, ``deps_to_install`` -- and ``callback_downloaded`` + -- can still be empty. ``all_apm_deps`` (direct deps only, but + populated before resolve even starts) guarantees every dep this module + could have purged is still resolvable even in that early-failure case; + ``deps_to_install``, when available, extends coverage to transitive + deps swept into the same update pass. + """ + # Coerced with isinstance rather than a plain ``or {}`` fallback: a + # loosely-mocked ctx (e.g. a bare MagicMock() in an unrelated pipeline + # test) has these as auto-generated, always-truthy Mock attributes when + # unset, which would otherwise slip past ``... or {}`` and break the + # dict/list operations below. + _raw_backups = getattr(ctx, "update_backups", None) + backups: dict[str, Path] = _raw_backups if isinstance(_raw_backups, dict) else {} + if not backups and keep_new: + return + _raw_downloaded = getattr(ctx, "callback_downloaded", None) + downloaded = _raw_downloaded if isinstance(_raw_downloaded, dict) else {} + _raw_all_deps = getattr(ctx, "all_apm_deps", None) + _raw_deps_to_install = getattr(ctx, "deps_to_install", None) + dep_by_key = { + d.get_unique_key(): d for d in (_raw_all_deps if isinstance(_raw_all_deps, list) else []) + } + dep_by_key.update( + { + d.get_unique_key(): d + for d in (_raw_deps_to_install if isinstance(_raw_deps_to_install, list) else []) + } + ) + apm_modules_dir = ctx.apm_modules_dir + + for _dep_key, _backup_path in backups.items(): + if keep_new and _dep_key in downloaded: + # New content committed -- the backup is no longer needed. + with suppress(Exception): + if _backup_path.exists(): + _rrm(_backup_path, ignore_errors=True) + continue + # Not committed, or this dep was purged but never actually + # re-resolved (e.g. an earlier failure aborted the run) -- restore + # the original content. + _dep = dep_by_key.get(_dep_key) + if _dep is None: + continue + with suppress(Exception): + _ip = _dep.get_install_path(apm_modules_dir) + if _ip.exists(): + _rrm(_ip, ignore_errors=True) + if _backup_path.exists(): + _ip.parent.mkdir(parents=True, exist_ok=True) + _backup_path.rename(_ip) + + if not keep_new: + # Freshly-downloaded deps with no prior backup (new adds swept into + # this update pass) never existed before -- remove them outright. + for _dep_key in downloaded: + if _dep_key in backups: + continue + _dep = dep_by_key.get(_dep_key) + if _dep is None: + continue + with suppress(Exception): + _ip = _dep.get_install_path(apm_modules_dir) + if _ip.exists(): + _rrm(_ip, ignore_errors=True) + + if backups: + _backup_root = next(iter(backups.values())).parent + with suppress(Exception): + if _backup_root.is_dir() and not any(_backup_root.iterdir()): + _backup_root.rmdir() diff --git a/src/apm_cli/install/pipeline.py b/src/apm_cli/install/pipeline.py index dd392b064..95b76359f 100644 --- a/src/apm_cli/install/pipeline.py +++ b/src/apm_cli/install/pipeline.py @@ -547,6 +547,7 @@ def run_install_pipeline( # noqa: PLR0913, RUF100 legacy_skill_paths=legacy_skill_paths, refresh=refresh, lockfile_only=lockfile_only, + plan_callback=plan_callback, ) # ------------------------------------------------------------------ @@ -568,6 +569,19 @@ def run_install_pipeline( # noqa: PLR0913, RUF100 try: ctx.tui.start_phase("resolve", total=len(all_apm_deps) or 1) _run_phase("resolve", _resolve_phase, ctx) + except Exception: + # A dep may already have been purged-to-backup (see + # update_backup.purge_cached_semver_paths_for_update) before + # resolution itself raised -- e.g. a network error on a different + # dep, or a bad transitive manifest. Restore it now rather than + # leaving it orphaned in the backup dir with its install path + # missing. A no-op when ``ctx.update_backups`` is empty (the + # common case: no plan_callback, or nothing needed purging). + if getattr(ctx, "update_backups", None): + from .phases.update_backup import restore_update_backups + + restore_update_backups(ctx, keep_new=False) + raise finally: ctx.tui.__exit__() @@ -593,12 +607,25 @@ def run_install_pipeline( # noqa: PLR0913, RUF100 # collision. # ------------------------------------------------------------------ if plan_callback is not None: + from .phases.update_backup import restore_update_backups from .plan import build_update_plan plan = build_update_plan(_early_lockfile, ctx.deps_to_install) - proceed = plan_callback(plan) - if not proceed: - return InstallResult() + # Resolve already downloaded/cloned any purged semver deps to + # compute this plan (see update_backup.purge_cached_semver_paths_ + # for_update, called from resolve.py). plan_callback can decline + # (return False) or abort the process outright (non-interactive + # shell -> SystemExit) -- either way, ``finally`` reconciles + # ``ctx.update_backups`` so a declined/aborted/dry-run update + # never leaves apm_modules/ ahead of apm.lock.yaml. + committed = False + try: + proceed = plan_callback(plan) + if not proceed: + return InstallResult() + committed = True + finally: + restore_update_backups(ctx, keep_new=committed) ctx.tui.__enter__() try: diff --git a/tests/unit/install/phases/test_update_backup.py b/tests/unit/install/phases/test_update_backup.py new file mode 100644 index 000000000..a27a60f13 --- /dev/null +++ b/tests/unit/install/phases/test_update_backup.py @@ -0,0 +1,304 @@ +"""Unit tests for the ``apm update`` stage-and-restore backup mechanism. + +Regression coverage for the bug where a declined confirmation, a +non-interactive abort (no TTY, no ``--yes``), or ``--dry-run`` left +``apm_modules/`` already advanced to the new version while +``apm.lock.yaml`` stayed on the old one -- because ``download_callback`` +materialises the new content to disk during resolve, before the plan- +confirmation gate ever runs. See ``update_backup.py`` for the mechanism. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from types import SimpleNamespace + +from apm_cli.install.phases.update_backup import ( + _sanitize_backup_name, + purge_cached_semver_paths_for_update, + restore_update_backups, +) + + +@dataclass +class _FakeDep: + """Minimal stand-in for the parts of DependencyReference this module uses.""" + + key: str + ref_kind: str | None = "semver" + is_local: bool = False + artifactory_prefix: str | None = None + install_rel: str | None = None # relative path under apm_modules_dir; defaults to key + + def get_unique_key(self) -> str: + return self.key + + def get_install_path(self, apm_modules_dir: Path) -> Path: + return apm_modules_dir / (self.install_rel or self.key) + + +def _write(path: Path, content: str) -> None: + path.mkdir(parents=True, exist_ok=True) + (path / "marker.txt").write_text(content, encoding="utf-8") + + +def _read(path: Path) -> str | None: + marker = path / "marker.txt" + return marker.read_text(encoding="utf-8") if marker.exists() else None + + +class TestSanitizeBackupName: + """Sanitizing alone is not injective -- distinct dep keys that differ only + in a character the sanitizer collapses (``/``, ``_``, ``:``) must not map + to the same backup directory name, or one dep's backup could silently + overwrite another's.""" + + def test_distinct_keys_produce_distinct_names(self) -> None: + names = { + _sanitize_backup_name("owner/repo"), + _sanitize_backup_name("owner_repo"), + _sanitize_backup_name("owner:repo"), + } + assert len(names) == 3 + + def test_same_key_is_deterministic(self) -> None: + assert _sanitize_backup_name("owner/repo") == _sanitize_backup_name("owner/repo") + + def test_name_is_filesystem_safe(self) -> None: + import re + + name = _sanitize_backup_name("github.com/owner/repo#v1.0.0") + assert re.fullmatch(r"[A-Za-z0-9._-]+", name) + + +class TestPurgeCachedSemverPathsForUpdate: + def test_no_backup_root_deletes_outright(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + dep = _FakeDep("owner/repo") + _write(dep.get_install_path(modules), "old") + + backups = purge_cached_semver_paths_for_update( + all_apm_deps=[dep], apm_modules_dir=modules, logger=None + ) + + assert backups == {} + assert not dep.get_install_path(modules).exists() + + def test_backup_root_moves_content_aside(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo") + _write(dep.get_install_path(modules), "old") + + backups = purge_cached_semver_paths_for_update( + all_apm_deps=[dep], + apm_modules_dir=modules, + logger=None, + backup_root=backup_root, + ) + + assert not dep.get_install_path(modules).exists() + assert "owner/repo" in backups + assert _read(backups["owner/repo"]) == "old" + + def test_skips_non_semver_deps(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo", ref_kind="literal") + _write(dep.get_install_path(modules), "old") + + backups = purge_cached_semver_paths_for_update( + all_apm_deps=[dep], + apm_modules_dir=modules, + logger=None, + backup_root=backup_root, + ) + + assert backups == {} + assert dep.get_install_path(modules).exists() + + def test_skips_local_deps(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo", is_local=True) + _write(dep.get_install_path(modules), "old") + + backups = purge_cached_semver_paths_for_update( + all_apm_deps=[dep], + apm_modules_dir=modules, + logger=None, + backup_root=backup_root, + ) + + assert backups == {} + assert dep.get_install_path(modules).exists() + + def test_skips_artifactory_deps(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo", artifactory_prefix="proxy") + _write(dep.get_install_path(modules), "old") + + backups = purge_cached_semver_paths_for_update( + all_apm_deps=[dep], + apm_modules_dir=modules, + logger=None, + backup_root=backup_root, + ) + + assert backups == {} + assert dep.get_install_path(modules).exists() + + def test_missing_install_path_is_a_noop(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo") # never created on disk + + backups = purge_cached_semver_paths_for_update( + all_apm_deps=[dep], + apm_modules_dir=modules, + logger=None, + backup_root=backup_root, + ) + + assert backups == {} + + +class TestRestoreUpdateBackups: + def _ctx(self, *, modules_dir, deps, downloaded, backups): + return SimpleNamespace( + apm_modules_dir=modules_dir, + deps_to_install=deps, + callback_downloaded=downloaded, + update_backups=backups, + ) + + def test_keep_new_discards_backup_for_downloaded_dep(self, tmp_path: Path) -> None: + """Committed update: new content stays, backup is discarded.""" + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo") + _write(backup_root / "owner_repo", "old") + _write(dep.get_install_path(modules), "new") + + ctx = self._ctx( + modules_dir=modules, + deps=[dep], + downloaded={"owner/repo": None}, + backups={"owner/repo": backup_root / "owner_repo"}, + ) + + restore_update_backups(ctx, keep_new=True) + + assert _read(dep.get_install_path(modules)) == "new" + assert not (backup_root / "owner_repo").exists() + + def test_declined_restores_original_content(self, tmp_path: Path) -> None: + """Declined/aborted/dry-run: new content is reverted to the backup.""" + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo") + _write(backup_root / "owner_repo", "old") + _write(dep.get_install_path(modules), "new") + + ctx = self._ctx( + modules_dir=modules, + deps=[dep], + downloaded={"owner/repo": None}, + backups={"owner/repo": backup_root / "owner_repo"}, + ) + + restore_update_backups(ctx, keep_new=False) + + assert _read(dep.get_install_path(modules)) == "old" + assert not (backup_root / "owner_repo").exists() + + def test_declined_removes_freshly_added_dep_with_no_backup(self, tmp_path: Path) -> None: + """A dep newly downloaded this run (no prior backup) never existed before -- + declining must remove it outright, not leave it half-installed.""" + modules = tmp_path / "apm_modules" + dep = _FakeDep("owner/new-dep") + _write(dep.get_install_path(modules), "new") + + ctx = self._ctx( + modules_dir=modules, + deps=[dep], + downloaded={"owner/new-dep": None}, + backups={}, + ) + + restore_update_backups(ctx, keep_new=False) + + assert not dep.get_install_path(modules).exists() + + def test_committed_but_not_downloaded_still_restores(self, tmp_path: Path) -> None: + """A dep purged for re-resolution but whose callback never actually ran + (e.g. an earlier failure aborted the graph walk) must not end up with an + empty install path even though the overall run is committed.""" + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo") + _write(backup_root / "owner_repo", "old") + # Note: install path was purged and NOT re-created -- simulates the + # callback never reaching this dep. + + ctx = self._ctx( + modules_dir=modules, + deps=[dep], + downloaded={}, # callback never ran for this dep + backups={"owner/repo": backup_root / "owner_repo"}, + ) + + restore_update_backups(ctx, keep_new=True) + + assert _read(dep.get_install_path(modules)) == "old" + + def test_no_backups_and_keep_new_is_a_noop(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + ctx = self._ctx(modules_dir=modules, deps=[], downloaded={}, backups={}) + + # Must not raise even though nothing was ever staged. + restore_update_backups(ctx, keep_new=True) + + def test_empty_backup_root_dir_is_removed_after_restore(self, tmp_path: Path) -> None: + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep = _FakeDep("owner/repo") + _write(backup_root / "owner_repo", "old") + _write(dep.get_install_path(modules), "new") + + ctx = self._ctx( + modules_dir=modules, + deps=[dep], + downloaded={"owner/repo": None}, + backups={"owner/repo": backup_root / "owner_repo"}, + ) + + restore_update_backups(ctx, keep_new=False) + + assert not backup_root.exists() + + def test_multiple_deps_mixed_outcome(self, tmp_path: Path) -> None: + """One dep committed, one dep declined-alongside via keep_new=False path, + exercised together to check no cross-talk between entries.""" + modules = tmp_path / "apm_modules" + backup_root = tmp_path / ".apm-update-backup" + dep_a = _FakeDep("owner/a") + dep_b = _FakeDep("owner/b") + _write(backup_root / "owner_a", "old-a") + _write(backup_root / "owner_b", "old-b") + _write(dep_a.get_install_path(modules), "new-a") + _write(dep_b.get_install_path(modules), "new-b") + + ctx = self._ctx( + modules_dir=modules, + deps=[dep_a, dep_b], + downloaded={"owner/a": None, "owner/b": None}, + backups={"owner/a": backup_root / "owner_a", "owner/b": backup_root / "owner_b"}, + ) + + restore_update_backups(ctx, keep_new=False) + + assert _read(dep_a.get_install_path(modules)) == "old-a" + assert _read(dep_b.get_install_path(modules)) == "old-b" diff --git a/tests/unit/test_install_pipeline_orchestration.py b/tests/unit/test_install_pipeline_orchestration.py index 185aee21b..31da23f62 100644 --- a/tests/unit/test_install_pipeline_orchestration.py +++ b/tests/unit/test_install_pipeline_orchestration.py @@ -312,6 +312,104 @@ def test_plan_callback_true_continues(self, tmp_path): plan_callback.assert_called_once() +# --------------------------------------------------------------------------- +# run_install_pipeline -- resolve-phase failure restores staged backups +# --------------------------------------------------------------------------- + + +class TestRunInstallPipelineResolveFailureRestoresBackups: + """A dep can be purged-to-backup before resolution itself raises (e.g. a + network error on a *different* dep, or a bad transitive manifest). The + backup must not be left orphaned with the dep's install path missing -- + see update_backup.restore_update_backups.""" + + def _make_apm_package(self): + pkg = MagicMock() + pkg.get_apm_dependencies.return_value = [MagicMock()] + pkg.get_dev_apm_dependencies.return_value = [] + pkg.get_mcp_dependencies.return_value = [] + return pkg + + def test_resolve_exception_triggers_restore_when_backups_staged(self, tmp_path): + from apm_cli.install.pipeline import run_install_pipeline + + pkg = self._make_apm_package() + + mock_ctx = MagicMock() + mock_ctx.root_has_local_primitives = False + mock_ctx.tui = MagicMock() + # Simulate: purge_cached_semver_paths_for_update already moved a dep + # aside before resolve_dependencies() raised. + mock_ctx.update_backups = {"owner/repo": tmp_path / "backup" / "owner_repo"} + + mock_resolve = MagicMock() + mock_resolve.run = MagicMock(side_effect=RuntimeError("network error on other dep")) + + mock_restore = MagicMock() + + with ( + patch("apm_cli.core.scope.get_deploy_root", return_value=tmp_path), + patch("apm_cli.core.scope.get_apm_dir", return_value=tmp_path), + patch( + "apm_cli.install.phases.local_content._project_has_root_primitives", + return_value=False, + ), + patch("apm_cli.deps.lockfile.LockFile.read", return_value=None), + patch("apm_cli.install.context.InstallContext", return_value=mock_ctx), + patch("apm_cli.utils.install_tui.InstallTui", return_value=MagicMock()), + patch("apm_cli.install.phases.resolve", mock_resolve), + patch("apm_cli.install.phases.update_backup.restore_update_backups", mock_restore), + ): + mock_ctx.tui.__enter__ = MagicMock(return_value=mock_ctx.tui) + mock_ctx.tui.__exit__ = MagicMock(return_value=False) + mock_ctx.tui.start_phase = MagicMock() + + with pytest.raises(RuntimeError, match="network error on other dep"): + run_install_pipeline(pkg, plan_callback=MagicMock()) + + mock_restore.assert_called_once_with(mock_ctx, keep_new=False) + + def test_resolve_exception_skips_restore_when_no_backups_staged(self, tmp_path): + """No plan_callback (e.g. apm install --update) never stages backups -- + the restore call must not fire, and the original exception still + propagates unchanged.""" + from apm_cli.install.pipeline import run_install_pipeline + + pkg = self._make_apm_package() + + mock_ctx = MagicMock() + mock_ctx.root_has_local_primitives = False + mock_ctx.tui = MagicMock() + mock_ctx.update_backups = {} + + mock_resolve = MagicMock() + mock_resolve.run = MagicMock(side_effect=RuntimeError("boom")) + + mock_restore = MagicMock() + + with ( + patch("apm_cli.core.scope.get_deploy_root", return_value=tmp_path), + patch("apm_cli.core.scope.get_apm_dir", return_value=tmp_path), + patch( + "apm_cli.install.phases.local_content._project_has_root_primitives", + return_value=False, + ), + patch("apm_cli.deps.lockfile.LockFile.read", return_value=None), + patch("apm_cli.install.context.InstallContext", return_value=mock_ctx), + patch("apm_cli.utils.install_tui.InstallTui", return_value=MagicMock()), + patch("apm_cli.install.phases.resolve", mock_resolve), + patch("apm_cli.install.phases.update_backup.restore_update_backups", mock_restore), + ): + mock_ctx.tui.__enter__ = MagicMock(return_value=mock_ctx.tui) + mock_ctx.tui.__exit__ = MagicMock(return_value=False) + mock_ctx.tui.start_phase = MagicMock() + + with pytest.raises(RuntimeError, match="boom"): + run_install_pipeline(pkg) + + mock_restore.assert_not_called() + + # --------------------------------------------------------------------------- # run_install_pipeline -- exception wrapping # ---------------------------------------------------------------------------