From 158497a4187d44ac6b764b20ba29451210a04686 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 10 Jul 2026 23:37:38 +0200 Subject: [PATCH 1/3] fix(install): roll back rejected resolution staging Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../docs/troubleshooting/install-failures.md | 2 + src/apm_cli/install/phases/resolve.py | 13 ++- src/apm_cli/install/resolution_staging.py | 80 ++++++++++++++++ .../test_resolution_transactionality.py | 96 +++++++++++++++++++ 4 files changed, 184 insertions(+), 7 deletions(-) create mode 100644 src/apm_cli/install/resolution_staging.py create mode 100644 tests/unit/install/test_resolution_transactionality.py diff --git a/docs/src/content/docs/troubleshooting/install-failures.md b/docs/src/content/docs/troubleshooting/install-failures.md index c14f9aeaa..7ea9e2b2c 100644 --- a/docs/src/content/docs/troubleshooting/install-failures.md +++ b/docs/src/content/docs/troubleshooting/install-failures.md @@ -220,6 +220,8 @@ apm install The cache short-circuits already-downloaded packages and the integrate phase overwrites partially-deployed files. +If resolution rejects a cyclic dependency graph, fix the package manifests and run `apm install` again. APM rolls back only the package snapshots staged by the rejected resolution, so no manual `apm_modules/` deletion is required. + If files in `apm_modules/` or under target harness directories look corrupt, force a fresh deploy by combining cache bypass with overwrite: ```bash diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index b2b3393bb..4416115d3 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -24,11 +24,13 @@ from typing import TYPE_CHECKING from apm_cli.install.helpers.ref_seed import seed_ref_resolver_from_lockfile +from apm_cli.install.resolution_staging import transactional_resolution from apm_cli.models.apm_package import GitReferenceType, ResolvedReference from apm_cli.utils.short_sha import format_short_sha if TYPE_CHECKING: from apm_cli.install.context import InstallContext + from apm_cli.install.resolution_staging import ResolutionStagingSession from apm_cli.models.dependency.reference import DependencyReference _logger = logging.getLogger(__name__) @@ -374,13 +376,9 @@ def _fail_on_resolution_errors(ctx: InstallContext, dependency_graph) -> None: raise RuntimeError(f"Dependency resolution failed: {joined_errors}") -def _resolve_dependencies(ctx: InstallContext) -> None: - """Run ``APMDependencyResolver``, handle errors; populate ``ctx.deps_to_install`` and ``ctx.dependency_graph``. - - Also wires the download callback (which handles transitive package fetching), - builds ``ctx.dep_base_dirs``, writes ancillary state to ``ctx``, and cleans up - the shared clone cache. - """ +@transactional_resolution +def _resolve_dependencies(ctx: InstallContext, staging_session: ResolutionStagingSession) -> None: + """Resolve dependencies and populate the resolution fields on ``ctx``.""" import threading as _threading from apm_cli.core.scope import InstallScope @@ -499,6 +497,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): ) if not _force_semver_resolve: return install_path + staging_session.prepare_path(install_path) # F1 (#1116): surface a heartbeat BEFORE the network/copy work so # users see the install advancing past silent transitive lookups. # Under F7's parallel BFS this callback may run on a worker diff --git a/src/apm_cli/install/resolution_staging.py b/src/apm_cli/install/resolution_staging.py new file mode 100644 index 000000000..2797a869d --- /dev/null +++ b/src/apm_cli/install/resolution_staging.py @@ -0,0 +1,80 @@ +"""Rollback-scoped staging for dependency resolution writes.""" + +from __future__ import annotations + +import threading +import uuid +from collections.abc import Callable +from functools import wraps +from pathlib import Path +from typing import Any + +from apm_cli.utils.path_security import ensure_path_within, safe_rmtree + + +class ResolutionStagingSession: + """Track paths mutated during resolution and restore them on failure.""" + + def __init__(self, apm_modules_dir: Path) -> None: + """Create an empty staging session rooted below ``apm_modules``.""" + self._modules_dir = apm_modules_dir + self._staging_root = apm_modules_dir / ".apm-resolution-staging" / uuid.uuid4().hex + self._backups: dict[Path, Path | None] = {} + self._lock = threading.Lock() + + def prepare_path(self, path: Path) -> None: + """Record *path* and preserve its pre-resolution contents if present.""" + resolved = ensure_path_within(path, self._modules_dir) + with self._lock: + if resolved in self._backups: + return + backup: Path | None = None + if resolved.exists(): + relative = resolved.relative_to(self._modules_dir.resolve()) + backup = self._staging_root / relative + backup.parent.mkdir(parents=True, exist_ok=True) + resolved.replace(backup) + self._backups[resolved] = backup + + def commit(self) -> None: + """Discard preserved pre-resolution contents after successful validation.""" + self._remove_staging_root() + self._backups.clear() + + def rollback(self) -> None: + """Remove session-created paths and restore every replaced path.""" + with self._lock: + for path, backup in reversed(self._backups.items()): + if path.exists(): + safe_rmtree(path, self._modules_dir) + if backup is not None and backup.exists(): + path.parent.mkdir(parents=True, exist_ok=True) + backup.replace(path) + self._remove_staging_root() + self._backups.clear() + + def _remove_staging_root(self) -> None: + if self._staging_root.exists(): + safe_rmtree(self._staging_root, self._modules_dir) + staging_parent = self._staging_root.parent + if staging_parent.exists() and not any(staging_parent.iterdir()): + staging_parent.rmdir() + + +ResolveFunction = Callable[[Any, ResolutionStagingSession], None] + + +def transactional_resolution(resolve: ResolveFunction) -> Callable[[Any], None]: + """Wrap a resolve operation in a rollback-scoped staging session.""" + + @wraps(resolve) + def wrapped(ctx: Any) -> None: + session = ResolutionStagingSession(ctx.apm_modules_dir) + try: + resolve(ctx, session) + except BaseException: + session.rollback() + raise + session.commit() + + return wrapped diff --git a/tests/unit/install/test_resolution_transactionality.py b/tests/unit/install/test_resolution_transactionality.py new file mode 100644 index 000000000..1accdea9f --- /dev/null +++ b/tests/unit/install/test_resolution_transactionality.py @@ -0,0 +1,96 @@ +"""Regression coverage for transactional dependency resolution.""" + +from __future__ import annotations + +from pathlib import Path + +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.install.resolution_staging import ResolutionStagingSession +from apm_cli.models.apm_package import clear_apm_yml_cache + + +def _write_package(path: Path, name: str, dependency: str | None = None) -> None: + """Write a local package and one Copilot instruction.""" + path.mkdir(parents=True, exist_ok=True) + dependency_block = "" + if dependency is not None: + dependency_block = f"dependencies:\n apm:\n - path: {dependency}\n" + (path / "apm.yml").write_text( + f"name: {name}\nversion: 1.0.0\n{dependency_block}", + encoding="ascii", + ) + instructions = path / ".apm" / "instructions" + instructions.mkdir(parents=True, exist_ok=True) + (instructions / f"{name}.instructions.md").write_text( + f"# {name}\n", + encoding="ascii", + ) + + +def test_resolution_rollback_restores_replaced_cache_path(tmp_path: Path) -> None: + """Rollback must restore a valid cache entry replaced by this attempt.""" + modules = tmp_path / "apm_modules" + package = modules / "_local" / "package-a" + package.mkdir(parents=True) + marker = package / "marker.txt" + marker.write_text("original", encoding="ascii") + session = ResolutionStagingSession(modules) + + session.prepare_path(package) + package.mkdir(parents=True) + marker.write_text("replacement", encoding="ascii") + session.rollback() + + assert marker.read_text(encoding="ascii") == "original" + assert not (modules / ".apm-resolution-staging").exists() + + +def test_corrected_local_cycle_resumes_without_manual_cache_deletion( + tmp_path: Path, + monkeypatch, +) -> None: + """A rejected local cycle must not leave snapshots that poison the retry.""" + workspace = tmp_path / "workspace" + project = workspace / "project" + package_a = workspace / "package-a" + package_b = workspace / "package-b" + home = workspace / "home" + project.mkdir(parents=True) + home.mkdir() + (project / ".github").mkdir() + (project / "apm.yml").write_text( + "name: consumer\nversion: 1.0.0\ndependencies:\n apm:\n - path: ../package-a\n", + encoding="ascii", + ) + _write_package(package_a, "package-a", "../package-b") + _write_package(package_b, "package-b", "../package-a") + + monkeypatch.chdir(project) + monkeypatch.setenv("HOME", str(home)) + runner = CliRunner() + + rejected = runner.invoke( + cli, + ["install", "--target", "copilot", "--verbose"], + catch_exceptions=True, + ) + + assert rejected.exit_code != 0, rejected.output + modules = project / "apm_modules" + assert not (modules / "_local" / "package-a").exists() + assert not (modules / "_local" / "package-b").exists() + assert not (project / "apm.lock.yaml").exists() + + _write_package(package_b, "package-b") + clear_apm_yml_cache() + resumed = runner.invoke( + cli, + ["install", "--target", "copilot", "--verbose"], + catch_exceptions=True, + ) + + assert resumed.exit_code == 0, resumed.output + assert (project / "apm.lock.yaml").is_file() + assert (project / ".github" / "instructions" / "package-a.instructions.md").is_file() From 599dc85338d0b50e7d68f0dd8d0e15ef96bf4cb8 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 11 Jul 2026 10:23:21 +0200 Subject: [PATCH 2/3] ci: document spec waiver for reliability fix apm-spec-waiver: Transactional resolution staging enforces existing install atomicity expectations; no new OpenAPM artifact or wire behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> From 43846beb4405d1ca6ef6f1283c906dd9f63781ac Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 11 Jul 2026 10:33:47 +0200 Subject: [PATCH 3/3] fix: normalize resolution staging base path Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/install/resolution_staging.py | 3 ++- tests/unit/install/test_resolution_transactionality.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/install/resolution_staging.py b/src/apm_cli/install/resolution_staging.py index 2797a869d..3c2aff031 100644 --- a/src/apm_cli/install/resolution_staging.py +++ b/src/apm_cli/install/resolution_staging.py @@ -30,7 +30,8 @@ def prepare_path(self, path: Path) -> None: return backup: Path | None = None if resolved.exists(): - relative = resolved.relative_to(self._modules_dir.resolve()) + resolved_base = ensure_path_within(self._modules_dir, self._modules_dir) + relative = resolved.relative_to(resolved_base) backup = self._staging_root / relative backup.parent.mkdir(parents=True, exist_ok=True) resolved.replace(backup) diff --git a/tests/unit/install/test_resolution_transactionality.py b/tests/unit/install/test_resolution_transactionality.py index 1accdea9f..323fa6dc6 100644 --- a/tests/unit/install/test_resolution_transactionality.py +++ b/tests/unit/install/test_resolution_transactionality.py @@ -78,6 +78,7 @@ def test_corrected_local_cycle_resumes_without_manual_cache_deletion( ) assert rejected.exit_code != 0, rejected.output + assert "Circular dependencies detected" in rejected.output modules = project / "apm_modules" assert not (modules / "_local" / "package-a").exists() assert not (modules / "_local" / "package-b").exists()