From c347ee3d5dbea58b2ad984b90af96d5559d536af Mon Sep 17 00:00:00 2001 From: Shaurya Saria Date: Sat, 3 Oct 2026 15:03:02 +0530 Subject: [PATCH] fix(prune): preserve case-equivalent GitHub dependencies --- docs/src/content/docs/reference/cli/prune.md | 8 + src/apm_cli/commands/_helpers.py | 31 +++- src/apm_cli/commands/prune.py | 2 +- tests/helpers/prune_casing.py | 28 ++++ tests/integration/test_prune_casing_cli.py | 31 ++++ tests/unit/test_prune_casing.py | 158 +++++++++++++++++++ 6 files changed, 253 insertions(+), 5 deletions(-) create mode 100644 tests/helpers/prune_casing.py create mode 100644 tests/integration/test_prune_casing_cli.py create mode 100644 tests/unit/test_prune_casing.py diff --git a/docs/src/content/docs/reference/cli/prune.md b/docs/src/content/docs/reference/cli/prune.md index 1d6354129a..e5936f37f9 100644 --- a/docs/src/content/docs/reference/cli/prune.md +++ b/docs/src/content/docs/reference/cli/prune.md @@ -36,6 +36,14 @@ dependency list nor needed by a retained package. This preserves transitive dependencies, bundled skills, and whole roots containing a needed nested package. An unrelated sibling root can still be pruned. +GitHub owner and repository names are compared case-insensitively: changing +`microsoft/apm` to `Microsoft/APM` does not orphan the installed package. +Prune preserves its existing directory spelling; it does not rename packages. +Case-sensitive hosts, local paths, aliases, and paths inside repositories +retain their existing casing rules. Ambiguous case-equivalent directories +cause an error before cleanup; inspect the duplicates and run `apm install` +after keeping the intended package. + Recognized roots under `apm_modules/`, including manifestless `SKILL.md` packages, are managed installation content. Pruning an eligible root removes its contents, including manually copied packages and personal files. Keep diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 815c4c823c..8d23ecc8ad 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -22,6 +22,10 @@ SKILL_MD_FILENAME, ) from ..core import project_name as _project_name +from ..models.dependency.materialization import ( + CachedMaterializationPathReader, + find_case_equivalent_materialization_path, +) from ..update_policy import get_update_hint_message, is_self_update_enabled from ..utils.atomic_io import ( atomic_write_text as _atomic_write, # noqa: F401 -- re-exported; tests import from apm_cli.commands._helpers @@ -139,16 +143,35 @@ def _lazy_confirm(): # ------------------------------------------------------------------ -def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path) -> set: +def _build_expected_install_paths( + declared_deps, lockfile, apm_modules_dir: Path, *, preserve_installed_case: bool = False +) -> set: """Build expected package paths under *apm_modules_dir*. Combines direct deps (from ``apm.yml``) with transitive deps (depth > 1 from ``apm.lock``), using ``get_install_path()`` for - consistency with how packages are actually installed. + consistency with how packages are actually installed. Prune opts into + recognizing existing case-equivalent paths without renaming them. """ + reader = CachedMaterializationPathReader() + + def expected_path(dependency): + desired = dependency.get_install_path(apm_modules_dir) + if ( + preserve_installed_case + and dependency.alias is None + and dependency.has_case_insensitive_repo_identity + ): + existing = find_case_equivalent_materialization_path( + desired, apm_modules_dir, dependency=dependency, reader=reader + ) + if existing is not None: + return existing + return desired + expected = set() for dep in declared_deps: - install_path = dep.get_install_path(apm_modules_dir) + install_path = expected_path(dep) try: relative_path = install_path.relative_to(apm_modules_dir) expected.add(relative_path.as_posix()) @@ -159,7 +182,7 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path for dep in lockfile.get_package_dependencies(): if dep.depth is not None and dep.depth > 1: dep_ref = dep.to_dependency_ref() - install_path = dep_ref.get_install_path(apm_modules_dir) + install_path = expected_path(dep_ref) try: relative_path = install_path.relative_to(apm_modules_dir) expected.add(relative_path.as_posix()) diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index 6a2d8fcc87..a0231ff5c7 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -115,7 +115,7 @@ def prune(ctx, dry_run): lockfile_path = get_lockfile_path(project_root) lockfile = LockFile.read(lockfile_path) expected_installed = _build_expected_install_paths( - declared_deps, lockfile, apm_modules_dir + declared_deps, lockfile, apm_modules_dir, preserve_installed_case=True ) except Exception as e: logger.error(f"Failed to parse {APM_YML_FILENAME}: {e}") diff --git a/tests/helpers/prune_casing.py b/tests/helpers/prune_casing.py new file mode 100644 index 0000000000..c658c1a353 --- /dev/null +++ b/tests/helpers/prune_casing.py @@ -0,0 +1,28 @@ +"""Isolated prune casing fixture shared by command and CLI tests.""" + +from apm_cli.deps.lockfile import LockedDependency, LockFile + + +def _project(root, declared, installed, *, with_lock): + (root / "apm.yml").write_text( + "name: casing-fixture\nversion: 1.0.0\ntargets: [agent-skills]\n" + f"dependencies:\n apm:\n - {declared}\n mcp: []\n", + encoding="utf-8", + ) + package = root / "apm_modules" / installed + package.mkdir(parents=True) + (package / "apm.yml").write_text("name: retained\nversion: 1.0.0\n", encoding="utf-8") + (package / "notes.txt").write_bytes(b"user content\n") + orphan = root / "apm_modules" / "other" / "orphan" + orphan.mkdir(parents=True) + (orphan / "apm.yml").write_text("name: orphan\nversion: 1.0.0\n", encoding="utf-8") + if with_lock: + LockFile( + dependencies={ + installed.lower(): LockedDependency( + repo_url=installed, resolved_commit="a" * 40, depth=1 + ), + "other/orphan": LockedDependency(repo_url="other/orphan", depth=1), + } + ).write(root / "apm.lock.yaml") + return package, orphan diff --git a/tests/integration/test_prune_casing_cli.py b/tests/integration/test_prune_casing_cli.py new file mode 100644 index 0000000000..64a32b659c --- /dev/null +++ b/tests/integration/test_prune_casing_cli.py @@ -0,0 +1,31 @@ +"""Real source-installed CLI coverage for GitHub prune casing.""" + +import os + +import pytest + +from apm_cli.deps.lockfile import LockFile +from tests.helpers.prune_casing import _project +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment + +pytestmark = pytest.mark.e2e + + +@pytest.mark.parametrize("dry_run", [True, False]) +def test_prune_casing_real_cli(tmp_path, dry_run, apm_engine_command): + isolated = IsolatedApmEnvironment.create(tmp_path / "scenario", base_env=dict(os.environ)) + project = isolated.work_root + package, orphan = _project(project, "Microsoft/APM", "microsoft/apm", with_lock=True) + result = ApmLifecycleRunner(apm_engine_command, timeout_seconds=30).run( + ("prune", *(["--dry-run"] if dry_run else [])), + scenario_id=f"prune-github-casing-{dry_run}", + cwd=project, + env=isolated.subprocess_env(), + ) + assert result.returncode == 0, result.stdout + result.stderr + if dry_run: + assert "microsoft/apm" not in result.stdout + assert (package / "notes.txt").read_bytes() == b"user content\n" + assert orphan.exists() is dry_run + assert "microsoft/apm" in LockFile.read(project / "apm.lock.yaml").dependencies diff --git a/tests/unit/test_prune_casing.py b/tests/unit/test_prune_casing.py new file mode 100644 index 0000000000..5db8eb4dad --- /dev/null +++ b/tests/unit/test_prune_casing.py @@ -0,0 +1,158 @@ +"""Prune preserves host-aware identity without changing materialized paths.""" + +from pathlib import Path +from unittest.mock import patch + +import pytest +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.commands._helpers import _build_expected_install_paths +from apm_cli.deps.lockfile import LockedDependency, LockFile +from apm_cli.models.apm_package import clear_apm_yml_cache +from apm_cli.models.dependency.reference import DependencyReference +from tests.helpers.prune_casing import _project + +pytestmark = pytest.mark.component + + +@pytest.mark.parametrize("dry_run", [True, False]) +@pytest.mark.parametrize("with_lock", [True, False]) +@pytest.mark.parametrize( + "declared,installed", [("Microsoft/APM", "microsoft/apm"), ("microsoft/apm", "Microsoft/APM")] +) +def test_prune_preserves_casing_equivalent_github_package( + tmp_path, monkeypatch, dry_run, with_lock, declared, installed +): + package, orphan = _project(tmp_path, declared, installed, with_lock=with_lock) + before = {p.name: p.read_bytes() for p in package.iterdir()} + manifest = (tmp_path / "apm.yml").read_bytes() + lock_path = tmp_path / "apm.lock.yaml" + lock_before = lock_path.read_bytes() if with_lock else None + monkeypatch.chdir(tmp_path) + try: + result = CliRunner().invoke(cli, ["prune", *(["--dry-run"] if dry_run else [])]) + finally: + clear_apm_yml_cache() + assert result.exit_code == 0, result.output + assert {p.name: p.read_bytes() for p in package.iterdir()} == before + assert (tmp_path / "apm.yml").read_bytes() == manifest + assert "other/orphan" in result.output + if dry_run: + assert installed not in result.output + else: + assert f"Removed {installed}" not in result.output + assert orphan.exists() is dry_run + if with_lock: + if dry_run: + assert lock_path.read_bytes() == lock_before + else: + lock = LockFile.read(lock_path) + assert set(lock.dependencies) == {installed.lower()} + assert lock.dependencies[installed.lower()].resolved_commit == "a" * 40 + + +def test_prune_retains_case_equivalent_transitive_dependency(tmp_path, monkeypatch): + package, orphan = _project(tmp_path, "direct/package", "MixedOrg/Transitive", with_lock=False) + LockFile( + dependencies={ + "mixedorg/transitive": LockedDependency(repo_url="mixedorg/transitive", depth=2) + } + ).write(tmp_path / "apm.lock.yaml") + monkeypatch.chdir(tmp_path) + try: + result = CliRunner().invoke(cli, ["prune"]) + finally: + clear_apm_yml_cache() + assert result.exit_code == 0, result.output + assert (package / "notes.txt").read_bytes() == b"user content\n" + assert not orphan.exists() + assert "mixedorg/transitive" in LockFile.read(tmp_path / "apm.lock.yaml").dependencies + + +@pytest.mark.parametrize( + "dependency", + [ + DependencyReference(repo_url="MixedOrg/Repo", host="gitlab.com"), + DependencyReference(repo_url="MixedOrg/Repo", host="github.com", alias="ExactAlias"), + DependencyReference(repo_url="", is_local=True, local_path="./MixedLocal"), + ], +) +def test_prune_does_not_casefold_sensitive_materialization(dependency, tmp_path): + modules = tmp_path / "apm_modules" + source_path = dependency.get_install_path(modules) + wrong_case = modules / source_path.relative_to(modules).as_posix().lower() + wrong_case.mkdir(parents=True) + (wrong_case / "notes.txt").write_bytes(b"retained") + with patch("apm_cli.commands._helpers.find_case_equivalent_materialization_path") as lookup: + expected = _build_expected_install_paths( + [dependency], None, modules, preserve_installed_case=True + ) + lookup.assert_not_called() + assert (wrong_case / "notes.txt").read_bytes() == b"retained" + assert expected == {source_path.relative_to(modules).as_posix()} + + +def test_prune_casing_lookup_is_opt_in(tmp_path): + dependency = DependencyReference.parse("Microsoft/APM") + installed = tmp_path / "microsoft" / "apm" + installed.mkdir(parents=True) + assert _build_expected_install_paths([dependency], None, tmp_path) == {"Microsoft/APM"} + assert _build_expected_install_paths( + [dependency], None, tmp_path, preserve_installed_case=True + ) == {"microsoft/apm"} + + +def test_prune_does_not_preserve_case_changed_unknown_host_package(tmp_path, monkeypatch): + package, orphan = _project( + tmp_path, "https://gitlab.com/MixedOrg/Repo", "mixedorg/repo", with_lock=False + ) + monkeypatch.chdir(tmp_path) + try: + result = CliRunner().invoke(cli, ["prune"]) + finally: + clear_apm_yml_cache() + assert result.exit_code == 0, result.output + assert not package.exists() + assert not orphan.exists() + + +@pytest.mark.parametrize("virtual_path", ["Skills/Exact", "skills/exact"]) +def test_prune_preserves_virtual_path_case(tmp_path, virtual_path): + dependency = DependencyReference.parse_from_dict( + {"git": "Microsoft/APM", "path": "Skills/Exact"} + ) + (tmp_path / "microsoft" / "apm" / virtual_path).mkdir(parents=True) + expected = ( + "microsoft/apm/Skills/Exact" + if virtual_path == "Skills/Exact" + else "Microsoft/APM/Skills/Exact" + ) + assert _build_expected_install_paths( + [dependency], None, tmp_path, preserve_installed_case=True + ) == {expected} + + +def test_prune_ambiguous_casing_fails_before_cleanup(tmp_path, monkeypatch): + package, orphan = _project(tmp_path, "Microsoft/APM", "microsoft/apm", with_lock=True) + manifest = (tmp_path / "apm.yml").read_bytes() + lock = (tmp_path / "apm.lock.yaml").read_bytes() + monkeypatch.chdir(tmp_path) + # Windows cannot create distinct case-only siblings. Supply a directory + # listing with both spellings to the real matching algorithm instead. + with patch("apm_cli.commands._helpers.CachedMaterializationPathReader") as reader: + reader.return_value.is_dir.return_value = True + reader.return_value.iterdir.return_value = [ + Path("apm_modules/microsoft"), + Path("apm_modules/Microsoft"), + ] + try: + result = CliRunner().invoke(cli, ["prune"]) + finally: + clear_apm_yml_cache() + assert result.exit_code == 1 + assert "multiple package directories" in result.output + assert orphan.exists() + assert (package / "notes.txt").read_bytes() == b"user content\n" + assert (tmp_path / "apm.yml").read_bytes() == manifest + assert (tmp_path / "apm.lock.yaml").read_bytes() == lock