From 0e95900b7ce15a0a6bb153b333736997e995f507 Mon Sep 17 00:00:00 2001 From: fangkangmi Date: Mon, 21 Sep 2026 22:45:00 +0100 Subject: [PATCH 1/5] fix(prune): detect orphan skills and preserve retained bundles --- .../owners/install-deployment.json | 7 + CHANGELOG.md | 1 + docs/src/content/docs/reference/cli/prune.md | 3 + .../checks/install_deployment_analyzers.py | 7 + .../install_uninstall_and_resolution.py | 26 +++ src/apm_cli/commands/_helpers.py | 24 ++- src/apm_cli/commands/deps/_utils.py | 13 +- src/apm_cli/commands/prune.py | 5 +- .../test_architecture_owner_rule_mutations.py | 8 + .../integration/test_prune_skill_lifecycle.py | 174 ++++++++++++++++++ .../unit/scripts/test_architecture_runner.py | 1 + tests/unit/test_deps_utils.py | 31 +++- tests/unit/test_prune_command.py | 120 ++++++++++++ 13 files changed, 408 insertions(+), 12 deletions(-) create mode 100644 tests/integration/test_prune_skill_lifecycle.py diff --git a/.apm/architecture/owners/install-deployment.json b/.apm/architecture/owners/install-deployment.json index a0872223aa..646e1c0a7c 100644 --- a/.apm/architecture/owners/install-deployment.json +++ b/.apm/architecture/owners/install-deployment.json @@ -1,6 +1,13 @@ { "version": 1, "owners": [ + { + "id": "orphan-package-selection", + "decision": "Declaration-aware selection of orphaned installed packages", + "owner": "commands/_helpers.py (_find_orphaned_packages)", + "selectors": ["src/apm_cli/commands/_helpers.py"], + "guards": ["install-deployment-orphan-selection"] + }, { "id": "effective-package-target-authorization", "decision": "Effective package target authorization", diff --git a/CHANGELOG.md b/CHANGELOG.md index 82abf1578e..4ea5c387d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm prune` removes orphaned manifestless skill installs while preserving skills inside declared or transitive skill bundles. (#3015) - Autopilot maintainer canvas removes a Decide row as soon as GitHub confirms `status/accepted`, without waiting for a full issue/PR refetch. - Issue and PR triage no longer skip bot-authored items (Copilot, Dependabot, github-actions). They stay in the queue like any other contribution. (#3024) - PR-review scheduler no longer queues every open pull request. A fresh review requires the `panel-review` label (same trigger as the Agentic Workflow), `status/accepted` on the PR, or an explicit named PR list. The reviewing session also requires `status/accepted` on the PR or a linked issue; otherwise scheduler and review-worker stop with no comment. The worker may clear `panel-review`; the scheduler does not comment or change labels. Both also apply a CODEOWNERS last-comment gate: read the last CODEOWNER comment as conditions and evaluate them against later comments AND labels on the PR and linked issues. Drop or `noop` only when those conditions are unmet or unclear. Named list does not bypass that gate. diff --git a/docs/src/content/docs/reference/cli/prune.md b/docs/src/content/docs/reference/cli/prune.md index 05ac4a5ce0..481a31a37c 100644 --- a/docs/src/content/docs/reference/cli/prune.md +++ b/docs/src/content/docs/reference/cli/prune.md @@ -125,6 +125,9 @@ reconciles hooks for packages and targets still declared. Notes: +- Manifestless installs containing `SKILL.md` are detected even after `apm install` + removes their lockfile entry. Skills inside a declared or retained transitive + package remain part of that package and are preserved. - Packages that share an install root with a still-declared sibling subdirectory dependency are not falsely protected by ancestor expansion. The check uses lockfile membership (with `apm.yml` fallback) to identify genuine standalone packages. - A manifest embedded at any depth inside an installed package is owned by that package. It is not an independent dependency, orphan, or prune candidate. diff --git a/scripts/architecture_linter/checks/install_deployment_analyzers.py b/scripts/architecture_linter/checks/install_deployment_analyzers.py index d58844e0e7..83220e7f71 100644 --- a/scripts/architecture_linter/checks/install_deployment_analyzers.py +++ b/scripts/architecture_linter/checks/install_deployment_analyzers.py @@ -64,14 +64,21 @@ check_source_plan, ) from scripts.architecture_linter.checks.install_uninstall_and_resolution import ( + _GUARD_ORPHAN_SELECTION, _GUARD_RESOLUTION_REPLACEMENT, _GUARD_UNINSTALL_SELECTION, + check_orphan_selection, check_resolution_replacement, check_uninstall_selection, ) from scripts.architecture_linter.models import Rule RULES: tuple[Rule, ...] = ( + _rule( + _GUARD_ORPHAN_SELECTION, + "Prune and orphan warnings share declaration-aware package selection.", + check_orphan_selection, + ), _rule( _GUARD_PACKAGE_TARGET, "Restriction-only package target authorization has one owner (install/target_filter.py).", diff --git a/scripts/architecture_linter/checks/install_uninstall_and_resolution.py b/scripts/architecture_linter/checks/install_uninstall_and_resolution.py index 837198b278..64e2db7692 100644 --- a/scripts/architecture_linter/checks/install_uninstall_and_resolution.py +++ b/scripts/architecture_linter/checks/install_uninstall_and_resolution.py @@ -31,6 +31,32 @@ _GUARD_UNINSTALL_SELECTION = "install-deployment-uninstall-selection" +_GUARD_ORPHAN_SELECTION = "install-deployment-orphan-selection" + + +def check_orphan_selection(provider: FactsProvider) -> tuple[Violation, ...]: + """Prune and advisory warnings share declaration-aware orphan selection.""" + findings: list[Violation] = [] + for path, scope in ( + ("src/apm_cli/commands/_helpers.py", "_check_orphaned_packages"), + ("src/apm_cli/commands/prune.py", "prune"), + ): + facts, failures = _facts_for(provider, path, _GUARD_ORPHAN_SELECTION) + findings.extend(failures) + if failures: + continue + if not any( + call.qualname == "_find_orphaned_packages" and call.scope == scope + for call in facts.calls + ): + findings.append( + _summary( + _GUARD_ORPHAN_SELECTION, + path, + f"{scope} must route orphan selection through _find_orphaned_packages", + ) + ) + return tuple(findings) def _call_terminal_name(node: ast.Call) -> str | None: diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 209581bca6..ded67729b8 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -331,14 +331,30 @@ def _check_orphaned_packages(): standalone_installed = _standalone_installed_packages( installed, apm_modules_dir, lockfile=lockfile ) - expected_with_ancestors = _expand_with_ancestors(expected, standalone_installed) - # Sort for deterministic, diffable output across runs (rglob - # traversal order is filesystem-dependent). - return sorted(p for p in installed if p not in expected_with_ancestors) + return _find_orphaned_packages(installed, expected, standalone_installed) except Exception: return [] +def _find_orphaned_packages( + installed: Iterable[str], expected: set[str], standalone: Iterable[str] +) -> list[str]: + """Select orphans while preserving the contents of retained install roots. + + Manifestless skill bundles have no root package marker. Their nested + skills belong to the declared or transitive install root, even though the + filesystem scan discovers each skill separately. Only actual expected + roots protect descendants; expanded ancestors must not protect siblings. + """ + expected_with_ancestors = _expand_with_ancestors(expected, standalone) + return sorted( + path + for path in installed + if path not in expected_with_ancestors + and not any(path.startswith(f"{root}/") for root in expected) + ) + + # ------------------------------------------------------------------ # Dependency-update helpers (shared by `apm update` and `apm deps update`) # ------------------------------------------------------------------ diff --git a/src/apm_cli/commands/deps/_utils.py b/src/apm_cli/commands/deps/_utils.py index 0be42299f7..520c5381e4 100644 --- a/src/apm_cli/commands/deps/_utils.py +++ b/src/apm_cli/commands/deps/_utils.py @@ -11,8 +11,8 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: """Scan *apm_modules_dir* for installed package paths. - Walks the tree to find top-level directories containing ``apm.yml`` or - ``.apm``, supporting aliases (1-level), GitHub (2-level), ADO (3-level), + Walks the tree to find top-level directories containing ``apm.yml``, + ``.apm``, or ``SKILL.md``, supporting aliases (1-level), GitHub (2-level), ADO (3-level), and subdirectory packages. Manifests nested below another package are part of that parent package and are excluded. @@ -27,7 +27,11 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: continue if candidate.name.startswith(".") and candidate.parent != apm_modules_dir: continue - if not ((candidate / APM_YML_FILENAME).exists() or (candidate / APM_DIR).exists()): + if not ( + (candidate / APM_YML_FILENAME).exists() + or (candidate / APM_DIR).exists() + or (candidate / SKILL_MD_FILENAME).is_file() + ): continue if _is_nested_under_package(candidate, apm_modules_dir): continue @@ -42,7 +46,7 @@ def _is_nested_under_package(candidate: Path, apm_modules_path: Path) -> bool: When a package ships nested package or skill manifests, the ``rglob`` scan would otherwise treat each sub-directory as an independent package. This helper walks up from *candidate* towards *apm_modules_path* and returns - ``True`` if any intermediate parent already contains ``apm.yml``, ``.apm``, + ``True`` if any intermediate parent already contains ``apm.yml``, ``.apm``, ``SKILL.md``, or a canonical Agent Plugin manifest -- meaning the candidate is part of that package, not a standalone one. """ @@ -51,6 +55,7 @@ def _is_nested_under_package(candidate: Path, apm_modules_path: Path) -> bool: if ( (parent / APM_YML_FILENAME).exists() or (parent / APM_DIR).exists() + or (parent / SKILL_MD_FILENAME).is_file() or _is_agent_plugin_root(parent) ): return True diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index d75067564c..d6e3809d4c 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -22,6 +22,7 @@ from ._helpers import ( _build_expected_install_paths, _expand_with_ancestors, + _find_orphaned_packages, _scan_installed_packages, _standalone_installed_packages, ) @@ -144,8 +145,8 @@ def prune(ctx, dry_run): lock_keys_by_path = ( _lock_keys_by_install_path(lockfile, apm_modules_dir) if lockfile is not None else {} ) - orphaned_packages = sorted( - p for p in installed_packages if p not in expected_with_ancestors + orphaned_packages = _find_orphaned_packages( + installed_packages, expected_installed, standalone_installed ) missing_orphaned_keys = sorted( dep_key diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 3c6599344f..287872bd9e 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -75,6 +75,14 @@ class MutationCase: MUTATIONS: tuple[MutationCase, ...] = ( + MutationCase( + guard_id="install-deployment-orphan-selection", + rule_id="install-deployment-orphan-selection", + path="src/apm_cli/commands/prune.py", + old="orphaned_packages = _find_orphaned_packages(", + new="orphaned_packages = _find_orphaned_packages_disabled(", + intent="Prune bypasses the shared declaration-aware orphan selector.", + ), MutationCase( guard_id="contracts-tests-taxonomy-classification", rule_id="contracts-tests-taxonomy-classification", diff --git a/tests/integration/test_prune_skill_lifecycle.py b/tests/integration/test_prune_skill_lifecycle.py new file mode 100644 index 0000000000..d0b833e686 --- /dev/null +++ b/tests/integration/test_prune_skill_lifecycle.py @@ -0,0 +1,174 @@ +"""Real CLI install/removal/prune contracts for manifestless skill packages.""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +import pytest + +from apm_cli.deps.lockfile import LockFile +from apm_cli.utils.yaml_io import dump_yaml, load_yaml +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner +from tests.utils.artifact_snapshot import ArtifactSnapshot, assert_unchanged +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.local_git_repository import LocalGitRepositoryFactory +from tests.utils.local_package import LocalPackageFactory + +pytestmark = [pytest.mark.integration, pytest.mark.lifecycle_smoke, pytest.mark.windows_compat] + + +@pytest.mark.parametrize("alias", [None, "azure-ai-alias"]) +def test_install_remove_install_prune_skill(tmp_path: Path, alias: str | None) -> None: + """Prune removes unlocked skill bytes while retaining a sibling and user files.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=os.environ) + repositories = LocalGitRepositoryFactory( + isolated.repository_root, env=isolated.subprocess_env() + ) + repository = repositories.create("skills") + skill_parent = ".github/plugins/azure-skills/skills" + for name in ("azure-ai", "retained"): + skill = repository.worktree / skill_parent / name + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Fixture skill\n---\n# {name}\n", + encoding="utf-8", + ) + (skill / "references").mkdir() + (skill / "references" / "guide.md").write_text("Reference bytes\n", encoding="utf-8") + commit = repositories.commit(repository, message="Add fixture skills") + # Git transport is redirected to a real local repository; no downloader, + # install, lockfile, integration, or prune code is mocked. + remote = "https://gitlab.com/fixture/skills" + environment = repositories.url_rewrite_subprocess_env(repository, remote) + dependencies = [ + {"git": remote, "path": f"{skill_parent}/{name}", "ref": commit.sha} + for name in ("azure-ai", "retained") + ] + if alias is not None: + dependencies[0]["alias"] = alias + project = LocalPackageFactory(isolated.work_root).create( + "consumer", dependencies=dependencies, targets=("copilot",) + ) + source_root = Path(__file__).resolve().parents[2] / "src" + runner = ApmLifecycleRunner( + ( + sys.executable, + "-c", + f"import sys; sys.path.insert(0, {str(source_root)!r}); " + "from apm_cli.cli import main; main()", + ), + timeout_seconds=30, + ) + + def run(*args: str) -> str: + result = runner.run(args, cwd=project.root, env=environment, scenario_id="prune-skill") + assert result.returncode == 0, result.stdout + result.stderr + return result.stdout + result.stderr + + install_args = ("install", "--no-policy", "--parallel-downloads", "0") + run(*install_args) + lock_path = project.root / "apm.lock.yaml" + lock = LockFile.read(lock_path) + removed_key, removed = next( + (key, dep) + for key, dep in lock.dependencies.items() + if dep.virtual_path.endswith("azure-ai") + ) + modules = project.root / "apm_modules" + removed_root = removed.to_dependency_ref().get_install_path(modules) + retained = next( + dep for dep in lock.dependencies.values() if dep.virtual_path.endswith("retained") + ) + retained_root = retained.to_dependency_ref().get_install_path(modules) + assert (removed_root / "SKILL.md").is_file() + assert not (removed_root / "apm.yml").exists() + deployed = project.root / ".agents" / "skills" + removed_deployment = deployed / (alias or "azure-ai") + assert (removed_deployment / "SKILL.md").is_file() + retained_before = ArtifactSnapshot.capture(retained_root) + deployed_before = ArtifactSnapshot.capture(deployed / "retained") + + manifest = load_yaml(project.manifest_path) + manifest["dependencies"]["apm"] = dependencies[1:] + dump_yaml(manifest, project.manifest_path) + run(*install_args) + assert removed_key not in LockFile.read(lock_path).dependencies + assert not removed_deployment.exists() + assert removed_root.exists(), "Install leaves orphan source bytes for prune" + sentinel = modules / "user-notes.txt" + sentinel.write_text("Keep these notes\n", encoding="utf-8") + before_dry_run = ArtifactSnapshot.capture(project.root) + + preview = run("prune", "--dry-run") + assert "1 orphaned package(s)" in preview + assert_unchanged(before_dry_run, ArtifactSnapshot.capture(project.root)) + assert "Pruned 1 orphaned package(s)" in run("prune") + assert not removed_root.exists() + assert_unchanged(retained_before, ArtifactSnapshot.capture(retained_root)) + assert_unchanged(deployed_before, ArtifactSnapshot.capture(deployed / "retained")) + assert sentinel.read_text(encoding="utf-8") == "Keep these notes\n" + before_repeat = ArtifactSnapshot.capture(project.root) + assert "No orphaned packages" in run("prune") + assert_unchanged(before_repeat, ArtifactSnapshot.capture(project.root)) + + +@pytest.mark.parametrize("alias", [None, "bundle-alias"]) +def test_prune_preserves_declared_skill_bundle(tmp_path: Path, alias: str | None) -> None: + """Real manifestless bundles survive prune and produce no compile orphan warning.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=os.environ) + repositories = LocalGitRepositoryFactory( + isolated.repository_root, env=isolated.subprocess_env() + ) + repository = repositories.create("bundle") + for name in ("alpha", "beta"): + skill = repository.worktree / "skills" / name + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Bundle fixture skill\n---\n# {name}\n", + encoding="utf-8", + ) + commit = repositories.commit(repository, message="Add manifestless skill bundle") + remote = "https://gitlab.com/fixture/bundle" + environment = repositories.url_rewrite_subprocess_env(repository, remote) + dependency = {"git": remote, "ref": commit.sha} + if alias is not None: + dependency["alias"] = alias + project = LocalPackageFactory(isolated.work_root).create( + "consumer", dependencies=(dependency,), targets=("copilot",) + ) + source_root = Path(__file__).resolve().parents[2] / "src" + runner = ApmLifecycleRunner( + ( + sys.executable, + "-c", + f"import sys; sys.path.insert(0, {str(source_root)!r}); " + "from apm_cli.cli import main; main()", + ), + timeout_seconds=30, + ) + + def run(*args: str) -> str: + result = runner.run(args, cwd=project.root, env=environment, scenario_id="prune-bundle") + assert result.returncode == 0, result.stdout + result.stderr + return result.stdout + result.stderr + + run("install", "--no-policy", "--parallel-downloads", "0") + modules = project.root / "apm_modules" + bundle = modules / (alias or "fixture/bundle") + assert not (bundle / "apm.yml").exists() + assert not (bundle / "SKILL.md").exists() + assert not (bundle / ".apm").exists() + for name in ("alpha", "beta"): + assert (bundle / "skills" / name / "SKILL.md").is_file() + assert (project.root / ".agents" / "skills" / name / "SKILL.md").is_file() + before = ArtifactSnapshot.capture(project.root) + for args in (("prune", "--dry-run"), ("prune",)): + assert "No orphaned packages" in run(*args) + assert_unchanged(before, ArtifactSnapshot.capture(project.root)) + output = run("compile") + assert "orphaned package(s)" not in output + assert "Run 'apm prune'" not in output + for name in ("alpha", "beta"): + assert (bundle / "skills" / name / "SKILL.md").is_file() diff --git a/tests/unit/scripts/test_architecture_runner.py b/tests/unit/scripts/test_architecture_runner.py index 1db5f7aafb..610a894dbb 100644 --- a/tests/unit/scripts/test_architecture_runner.py +++ b/tests/unit/scripts/test_architecture_runner.py @@ -641,6 +641,7 @@ def exiting_import( install-deployment-lifecycle-serialization install-deployment-mcp-ownership-migration install-deployment-mcp-registry-resolution +install-deployment-orphan-selection install-deployment-outcome install-deployment-package-target-authorization install-deployment-plugin-bin-eligibility diff --git a/tests/unit/test_deps_utils.py b/tests/unit/test_deps_utils.py index 78fdacff05..2dfddecf21 100644 --- a/tests/unit/test_deps_utils.py +++ b/tests/unit/test_deps_utils.py @@ -112,13 +112,14 @@ def test_scan_includes_flattened_alias_without_nested_packages(tmp_path: Path, a @pytest.mark.windows_compat -def test_scan_excludes_symlink_packages(tmp_path: Path) -> None: +@pytest.mark.parametrize("marker", [APM_YML_FILENAME, SKILL_MD_FILENAME]) +def test_scan_excludes_symlink_packages(tmp_path: Path, marker: str) -> None: """Symlink prerequisites must not skip the independent alias regression.""" modules = tmp_path / "apm_modules" modules.mkdir() outside = tmp_path / "outside" outside.mkdir() - _make_apm_yml(outside) + (outside / marker).write_text("package marker\n") try: (modules / "linked").symlink_to(outside, target_is_directory=True) except (NotImplementedError, OSError): @@ -511,6 +512,32 @@ def test_defaults_for_missing_optional_fields(self, tmp_path): class TestScanInstalledPackages: """Additional edge cases for _scan_installed_packages.""" + @pytest.mark.parametrize( + "relative_path", ["skill-alias", "org/repo", "org/repo/.github/skills/foo"] + ) + def test_skill_only_package(self, tmp_path, relative_path): + package = tmp_path / relative_path + package.mkdir(parents=True) + (package / SKILL_MD_FILENAME).write_text("# Skill\n") + + assert _scan_installed_packages(tmp_path) == [relative_path] + + def test_skill_named_directory_is_not_a_package_marker(self, tmp_path): + (tmp_path / "org" / "repo" / SKILL_MD_FILENAME).mkdir(parents=True) + + assert _scan_installed_packages(tmp_path) == [] + + @pytest.mark.parametrize("parent_marker", [APM_YML_FILENAME, APM_DIR, SKILL_MD_FILENAME]) + def test_embedded_skill_is_part_of_parent(self, tmp_path, parent_marker): + parent = tmp_path / "org" / "repo" + embedded = parent / "skills" / "child" + embedded.mkdir(parents=True) + (parent / parent_marker).write_text("package marker\n") + (embedded / SKILL_MD_FILENAME).write_text("# Embedded skill\n") + (embedded / APM_YML_FILENAME).write_text("name: child\n") + + assert _scan_installed_packages(tmp_path) == ["org/repo"] + def test_three_level_ado_packages(self, tmp_path): """ADO-style org/project/repo packages are found.""" pkg = tmp_path / "org" / "project" / "repo" diff --git a/tests/unit/test_prune_command.py b/tests/unit/test_prune_command.py index 9a609e5c5c..4b35a65040 100644 --- a/tests/unit/test_prune_command.py +++ b/tests/unit/test_prune_command.py @@ -23,6 +23,7 @@ from click.testing import CliRunner from apm_cli.cli import cli +from apm_cli.commands._helpers import _check_orphaned_packages from apm_cli.core.deployment_ledger import DeploymentLedgerCodec from apm_cli.core.deployment_state import ( DeploymentLedger, @@ -257,6 +258,71 @@ def test_prune_keeps_declared_packages(self): assert declared_dir.exists(), "Declared package must remain" assert not orphan_dir.exists(), "Orphaned package must be removed" + @pytest.mark.windows_compat + @pytest.mark.parametrize("dry_run", [False, True]) + @pytest.mark.parametrize("declared", [False, True]) + def test_prune_skill_only_subdirectory_without_lockfile(self, dry_run, declared): + """Find skill-only installs after install has dropped their lock entry.""" + with self._chdir_tmp() as tmp: + dependency = "microsoft/skills/.github/plugins/azure-skills/skills/azure-ai" + manifest = _APM_YML_NO_DEPS + if declared: + manifest = manifest.replace("apm: []", f"apm:\n - {dependency}") + (tmp / "apm.yml").write_text(manifest) + skill_dir = tmp / "apm_modules" / dependency + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Azure AI\n") + (skill_dir / ".apm-pin").write_text("a" * 40) + references = skill_dir / "references" + references.mkdir() + (references / "guide.md").write_text("Reference material\n") + + result = self.runner.invoke(cli, ["prune", *(["--dry-run"] if dry_run else [])]) + + assert result.exit_code == 0, result.output + assert skill_dir.exists() == (declared or dry_run) + if declared: + assert "No orphaned packages" in result.output + else: + assert "No orphaned packages" not in result.output + assert "1 orphaned package(s)" in result.output + if declared or dry_run: + assert (references / "guide.md").read_text() == "Reference material\n" + + @pytest.mark.parametrize("transitive", [False, True]) + def test_prune_preserves_retained_sibling_skill(self, transitive): + """Removing one skill preserves direct and locked transitive siblings.""" + with self._chdir_tmp() as tmp: + retained_key = "owner/repo/.github/skills/retained" + manifest = _APM_YML_NO_DEPS + if transitive: + LockFile( + dependencies={ + retained_key: LockedDependency( + repo_url="owner/repo", + virtual_path=".github/skills/retained", + is_virtual=True, + depth=2, + ) + } + ).write(tmp / "apm.lock.yaml") + else: + manifest = manifest.replace("apm: []", f"apm:\n - {retained_key}") + (tmp / "apm.yml").write_text(manifest) + retained = tmp / "apm_modules" / retained_key + orphan = retained.with_name("orphan") + for skill in (retained, orphan): + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text("# Skill\n") + + result = self.runner.invoke(cli, ["prune"]) + + assert result.exit_code == 0, result.output + assert not orphan.exists() + assert (retained / "SKILL.md").read_text() == "# Skill\n" + if transitive: + assert retained_key in LockFile.read(tmp / "apm.lock.yaml").dependencies + def test_prune_reports_count_removed(self): """prune output should mention how many packages were removed.""" with self._chdir_tmp() as tmp: @@ -267,6 +333,60 @@ def test_prune_reports_count_removed(self): # Output should mention the removal (count or package name) assert "Pruned" in result.output or "orphan-org/orphan-repo" in result.output + @pytest.mark.parametrize("parent_marker", ["apm.yml", "SKILL.md"]) + def test_prune_preserves_embedded_skill_bytes(self, parent_marker): + """A declared package owns its embedded skills, even without lock entries.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_APM_YML_WITH_DEP) + parent = tmp / "apm_modules" / "declared-org" / "declared-repo" + embedded = parent / "skills" / "embedded" + embedded.mkdir(parents=True) + (parent / parent_marker).write_text("package marker\n") + (embedded / "SKILL.md").write_text("# Embedded skill\n") + (embedded / "apm.yml").write_text("name: embedded\nversion: 1.0.0\n") + + result = self.runner.invoke(cli, ["prune"]) + + assert result.exit_code == 0, result.output + assert "No orphaned packages" in result.output + assert (embedded / "SKILL.md").read_text() == "# Embedded skill\n" + assert (embedded / "apm.yml").read_text() == "name: embedded\nversion: 1.0.0\n" + + @pytest.mark.parametrize("retention", ["direct", "dev", "transitive"]) + @pytest.mark.parametrize("dry_run", [False, True]) + def test_prune_preserves_manifestless_bundle(self, retention, dry_run): + """Expected bundle roots protect children without hiding adjacent orphans.""" + with self._chdir_tmp() as tmp: + manifest = _APM_YML_NO_DEPS + if retention == "direct": + manifest = manifest.replace("apm: []", "apm:\n - owner/bundle") + elif retention == "dev": + manifest += "devDependencies:\n apm:\n - owner/bundle\n" + else: + LockFile( + dependencies={ + "owner/bundle": LockedDependency(repo_url="owner/bundle", depth=2) + } + ).write(tmp / "apm.lock.yaml") + (tmp / "apm.yml").write_text(manifest) + bundle = tmp / "apm_modules" / "owner" / "bundle" + for name in ("alpha", "beta"): + skill = bundle / "skills" / name + skill.mkdir(parents=True) + (skill / "SKILL.md").write_text(f"# {name}\n") + orphan = bundle.with_name("bundle-other") / "skills" / "orphan" + orphan.mkdir(parents=True) + (orphan / "SKILL.md").write_text("# Orphan\n") + + assert _check_orphaned_packages() == ["owner/bundle-other/skills/orphan"] + result = self.runner.invoke(cli, ["prune", *(["--dry-run"] if dry_run else [])]) + + assert result.exit_code == 0, result.output + assert "1 orphaned package(s)" in result.output + assert orphan.exists() == dry_run + for name in ("alpha", "beta"): + assert (bundle / "skills" / name / "SKILL.md").read_text() == f"# {name}\n" + def test_prune_removes_multiple_orphans(self): """prune removes all orphaned packages in one pass.""" with self._chdir_tmp() as tmp: From 00016f6d11b071ef41455b0873a163c61110ff84 Mon Sep 17 00:00:00 2001 From: fangkangmi Date: Tue, 22 Sep 2026 13:06:59 +0100 Subject: [PATCH 2/5] fix(prune): classify skill roots consistently for orphan detection --- docs/src/content/docs/reference/cli/prune.md | 2 +- src/apm_cli/commands/_helpers.py | 21 +++++++++-------- .../integration/test_prune_skill_lifecycle.py | 7 +++++- tests/unit/test_prune_command.py | 23 +++++++++++++++++++ 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/docs/src/content/docs/reference/cli/prune.md b/docs/src/content/docs/reference/cli/prune.md index 481a31a37c..52e4a834c6 100644 --- a/docs/src/content/docs/reference/cli/prune.md +++ b/docs/src/content/docs/reference/cli/prune.md @@ -128,7 +128,7 @@ Notes: - Manifestless installs containing `SKILL.md` are detected even after `apm install` removes their lockfile entry. Skills inside a declared or retained transitive package remain part of that package and are preserved. -- Packages that share an install root with a still-declared sibling subdirectory dependency are not falsely protected by ancestor expansion. The check uses lockfile membership (with `apm.yml` fallback) to identify genuine standalone packages. +- Packages that share an install root with a still-declared sibling subdirectory dependency are not falsely protected by ancestor expansion. The check uses lockfile membership (with `apm.yml` or `SKILL.md` fallback) to identify genuine standalone packages. - A manifest embedded at any depth inside an installed package is owned by that package. It is not an independent dependency, orphan, or prune candidate. - Deploy paths are validated before deletion; entries that escape the project root are skipped. diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index ded67729b8..a5ed8a68ef 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -19,6 +19,7 @@ APM_MODULES_GITIGNORE_PATTERN, APM_YML_FILENAME, GITIGNORE_FILENAME, + SKILL_MD_FILENAME, ) from ..core import project_name as _project_name from ..update_policy import get_update_hint_message, is_self_update_enabled @@ -191,7 +192,7 @@ def _expand_with_ancestors( unless that path is also directly declared in *paths*. Callers should pass only the subset of installed paths that look like *real standalone packages* (i.e., directories that ship their own - ``apm.yml``) -- not filesystem intermediaries (which typically have + ``apm.yml`` or ``SKILL.md``) -- not filesystem intermediaries (which typically have only a ``.apm/`` subtree from a cloned subdir dep). This preserves orphan detection for the case where a user has a genuinely orphaned ``owner/repo`` package on disk alongside a declared sibling @@ -256,19 +257,17 @@ def _standalone_installed_packages( 1. Path appears as a dependency key in *lockfile* -- the canonical record of what APM installed. The lockfile is integrity-checked and not forgeable by dropping/omitting files in ``apm_modules/``. - 2. Fallback: path has its own ``apm.yml``. Used when the lockfile + 2. Fallback: path has its own ``apm.yml`` or ``SKILL.md`` file. Used when the lockfile is absent (older installs / fresh checkouts) or does not list the key. A directory with only a ``.apm/`` marker is treated as a filesystem intermediary, not a standalone package. - Combining both signals closes the suppression-via-absence gap - (panel finding: forgeable ``apm.yml`` heuristic) while preserving - behaviour for projects that pre-date the lockfile or have not yet - re-installed. + Package markers preserve standalone orphan detection even when a + declaration points at a subdirectory of a removed package root. Failure mode: only narrowly-typed shape errors against ``lockfile.dependencies`` (``AttributeError`` / ``TypeError`` / - ``KeyError``) are absorbed and degrade to the ``apm.yml``-only + ``KeyError``) are absorbed and degrade to the package-marker fallback. Any other exception (e.g. lockfile parse / I/O failure) propagates so the outer caller can decide whether to log or fail closed -- preventing a corrupted or attacker-crafted lockfile from @@ -287,7 +286,9 @@ def _standalone_installed_packages( if p in lockfile_keys: standalone.append(p) continue - if (apm_modules_dir / p / APM_YML_FILENAME).exists(): + if (apm_modules_dir / p / APM_YML_FILENAME).exists() or ( + apm_modules_dir / p / SKILL_MD_FILENAME + ).is_file(): standalone.append(p) return standalone @@ -322,10 +323,10 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - # Combined lockfile-membership + apm.yml fallback determines + # Combined lockfile-membership + package-marker fallback determines # which installed paths are real standalone packages (and so # must NOT be masked by ancestor expansion). The lockfile is - # the canonical, tamper-evident record; apm.yml-existence is + # the canonical, tamper-evident record; apm.yml/SKILL.md presence is # the fallback for projects without a lockfile yet. # See _expand_with_ancestors for the user-safety rationale. standalone_installed = _standalone_installed_packages( diff --git a/tests/integration/test_prune_skill_lifecycle.py b/tests/integration/test_prune_skill_lifecycle.py index d0b833e686..a6774bef75 100644 --- a/tests/integration/test_prune_skill_lifecycle.py +++ b/tests/integration/test_prune_skill_lifecycle.py @@ -16,7 +16,12 @@ from tests.utils.local_git_repository import LocalGitRepositoryFactory from tests.utils.local_package import LocalPackageFactory -pytestmark = [pytest.mark.integration, pytest.mark.lifecycle_smoke, pytest.mark.windows_compat] +pytestmark = [ + pytest.mark.integration, + pytest.mark.e2e, + pytest.mark.lifecycle_smoke, + pytest.mark.windows_compat, +] @pytest.mark.parametrize("alias", [None, "azure-ai-alias"]) diff --git a/tests/unit/test_prune_command.py b/tests/unit/test_prune_command.py index 4b35a65040..1931cb6010 100644 --- a/tests/unit/test_prune_command.py +++ b/tests/unit/test_prune_command.py @@ -387,6 +387,29 @@ def test_prune_preserves_manifestless_bundle(self, retention, dry_run): for name in ("alpha", "beta"): assert (bundle / "skills" / name / "SKILL.md").read_text() == f"# {name}\n" + @pytest.mark.parametrize("dry_run", [False, True]) + def test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory(self, dry_run): + """A skill root gets the same standalone-orphan treatment as an apm.yml root.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text( + _APM_YML_NO_DEPS.replace("apm: []", "apm:\n - owner/repo/skills/child") + ) + root = tmp / "apm_modules" / "owner" / "repo" + child = root / "skills" / "child" + child.mkdir(parents=True) + (root / "SKILL.md").write_text("# Root skill\n") + (child / "SKILL.md").write_text("# Child skill\n") + + assert _check_orphaned_packages() == ["owner/repo"] + result = self.runner.invoke(cli, ["prune", *(["--dry-run"] if dry_run else [])]) + + assert result.exit_code == 0, result.output + assert "1 orphaned package(s)" in result.output + assert root.exists() == dry_run + if dry_run: + assert (root / "SKILL.md").read_text() == "# Root skill\n" + assert (child / "SKILL.md").read_text() == "# Child skill\n" + def test_prune_removes_multiple_orphans(self): """prune removes all orphaned packages in one pass.""" with self._chdir_tmp() as tmp: From 5feddb60772dcb6daf7d95afe414f1bbe1a8478c Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 25 Sep 2026 09:04:56 +0200 Subject: [PATCH 3/5] fix(prune): retain needed ancestors during managed-root cleanup Keep direct, dev and transitive contents under recognized roots; use one indexed orphan selector for warnings, source deletion and stale lock cleanup. Exercise installed CLI lifecycles, retry failures and marker-scoped Windows coverage without requiring cache pins as ownership proof. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 4 +- CHANGELOG.md | 2 +- .../docs/contributing/integration-testing.md | 10 +- docs/src/content/docs/reference/cli/prune.md | 27 +++- .../.apm/skills/apm-usage/commands.md | 3 +- src/apm_cli/commands/_helpers.py | 55 ++++---- src/apm_cli/commands/prune.py | 26 ++-- .../test_architecture_owner_rule_mutations.py | 18 ++- .../test_prune_failure_lifecycle.py | 11 +- .../integration/test_prune_skill_lifecycle.py | 119 +++++++++++++--- tests/unit/test_command_helpers.py | 42 +++--- tests/unit/test_prune_command.py | 131 ++++++++++++------ .../unit/test_windows_compat_gate_workflow.py | 10 +- 13 files changed, 310 insertions(+), 148 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46f0b0e7e3..a3fa32af4b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -149,11 +149,13 @@ jobs: where git - name: Run cross-platform contract family + env: + APM_E2E_TESTS: '1' run: >- uv run --extra dev pytest -p no:cacheprovider -v -m windows_compat tests/unit - tests/integration/test_lifecycle_workspace_lock.py + tests/integration - name: Diagnostics on failure if: failure() diff --git a/CHANGELOG.md b/CHANGELOG.md index 7027a34cfc..b96f0d4b9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `apm prune` removes orphaned manifestless skill installs while preserving skills inside declared or transitive skill bundles. (#3015) +- `apm prune` removes unneeded manifestless skill installs after their lock entries disappear, while retaining bundles and whole roots containing needed nested packages. Personal files inside removable package roots are also removed; keep personal source outside `apm_modules/` and preview with `--dry-run`. -- by @fangkangmi (#3057) - Autopilot maintainer canvas removes a Decide row as soon as GitHub confirms `status/accepted`, without waiting for a full issue/PR refetch. - Issue and PR triage no longer skip bot-authored items (Copilot, Dependabot, github-actions). They stay in the queue like any other contribution. (#3024) - PR-review scheduler no longer queues every open pull request. A fresh review requires the `panel-review` label (same trigger as the Agentic Workflow), `status/accepted` on the PR, or an explicit named PR list. The reviewing session also requires `status/accepted` on the PR or a linked issue; otherwise scheduler and review-worker stop with no comment. The worker may clear `panel-review`; the scheduler does not comment or change labels. Both also apply a CODEOWNERS last-comment gate: read the last CODEOWNER comment as conditions and evaluate them against later comments AND labels on the PR and linked issues. Drop or `noop` only when those conditions are unmet or unclear. Named list does not bypass that gate. diff --git a/docs/src/content/docs/contributing/integration-testing.md b/docs/src/content/docs/contributing/integration-testing.md index 5763001ba0..06b4aed290 100644 --- a/docs/src/content/docs/contributing/integration-testing.md +++ b/docs/src/content/docs/contributing/integration-testing.md @@ -383,9 +383,17 @@ environment end-to-end; for local iteration prefer the direct **On PR and merge queue:** 1. PR-time unit checks and the hermetic Lifecycle Smoke gate run first; merge queue adds Linux smoke, integration, and release-validation gates. -The required Windows compatibility gate selects `windows_compat` tests. Its collection guard requires a non-empty subset, not a fixed test count, so adding marked regressions does not require raising a ceiling. The workflow's test roots and timeout bound scope and runtime. +The required Windows compatibility gate selects `windows_compat` tests within +`tests/unit` and `tests/integration`, with `APM_E2E_TESTS=1` for marked real-CLI +contracts. It runs only that marker subset, not the full integration suite. +Its collection guard requires a non-empty subset, not a fixed test count. +Collection proves a test is selected; a successful Windows job provides +Windows execution evidence. The existing job timeout bounds runtime. Linux Lifecycle Smoke runs the required marker subset with `-n 2 --dist loadgroup`. Grouped tests stay on one worker, and the six-minute job limit remains unchanged. +Its `lifecycle_smoke and not lifecycle_merge_group` selection is not all +lifecycle coverage: also run affected generated state machines, deployment +ledger, and failure/retry contracts when changing those behaviors. **On pushed version tag releases:** 1. Unit tests + Smoke tests diff --git a/docs/src/content/docs/reference/cli/prune.md b/docs/src/content/docs/reference/cli/prune.md index 52e4a834c6..1d6354129a 100644 --- a/docs/src/content/docs/reference/cli/prune.md +++ b/docs/src/content/docs/reference/cli/prune.md @@ -32,17 +32,27 @@ wiring) and rewrites the lockfile. canonical deployment ownership rows An installed package is **orphaned** when it is neither declared in either -dependency list nor retained as a lockfile-resolved transitive dependency. -`apm prune` removes the orphan's directory under `apm_modules/`, deletes every -file the orphan deployed into your harness directories (using the -`deployed_files` manifest in the lockfile), removes the entry from +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. + +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 +personal source outside `apm_modules/`; preview cleanup with `apm prune --dry-run`. +Neither a surviving lock entry nor an `.apm-pin` cache marker is required. + +`apm prune` removes the orphan's directory under `apm_modules/`, cleans up its +owned harness deployments using the protections below, removes its entry from `apm.lock.yaml`, and cleans up empty parent directories. `apm prune` also parses and reconciles the lockfile's canonical deployment ownership metadata on every run, even when `apm_modules/` does not exist or no package is orphaned. It also removes stale direct-dependency records whose -package directory is already absent, allowing a retry to finish after an -earlier lockfile write failed. A stale dependency or owner reference is not +package directory is already absent or retained for a needed nested package, +allowing a retry to finish after an earlier lockfile write failed. Retained +source content does not preserve an undeclared package's deployment ownership. +A stale dependency or owner reference is not "nothing to prune." If `apm.yml` is missing, the command exits with an error. ## Options @@ -128,7 +138,10 @@ Notes: - Manifestless installs containing `SKILL.md` are detected even after `apm install` removes their lockfile entry. Skills inside a declared or retained transitive package remain part of that package and are preserved. -- Packages that share an install root with a still-declared sibling subdirectory dependency are not falsely protected by ancestor expansion. The check uses lockfile membership (with `apm.yml` or `SKILL.md` fallback) to identify genuine standalone packages. +- A recognized ancestor containing a declared direct/dev or retained transitive + dependency is kept intact and reported as retained, not removed. +- Unrecognized directories are not package-removal candidates. Files deployed + outside `apm_modules/` retain the ownership protections described above. - A manifest embedded at any depth inside an installed package is owned by that package. It is not an independent dependency, orphan, or prune candidate. - Deploy paths are validated before deletion; entries that escape the project root are skipped. diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index b26a07f308..af16b03a24 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -31,9 +31,8 @@ a collapsed graph; `--force` does not bypass this check. | `apm uninstall PKGS...` | Remove packages and reconcile their tracked files, MCP servers, and LSP servers; identifier selection is atomic. Accepts `owner/repo`, `name@marketplace`, exact declared local paths, or portable `_local/` keys for direct local declarations with matching lock metadata. A missing or ambiguous identifier exits nonzero before scripts or APM writes. If safe LSP cleanup fails after package removal, the command exits nonzero and preserves the conflicting config; repair it, then run `apm install` (or `apm install --global` for user scope). | `--dry-run`, `-g` global | | `apm uninstall PKGS...` | Remove packages; identifier selection is atomic. Accepts `owner/repo`, `name@marketplace`, exact declared local paths, or portable `_local/` keys for direct local declarations with matching lock metadata. A missing or ambiguous identifier exits nonzero before scripts or APM writes. A managed hook beneath a symlinked parent is preserved and reported; package removal finishes, but the command exits nonzero because hook cleanup is incomplete. | `--dry-run`, `-g` global | | `apm uninstall PKGS...` | Remove packages; identifier selection is atomic. Accepts `owner/repo`, `name@marketplace`, exact declared local paths, or portable `_local/` keys for direct local declarations with matching lock metadata. A missing or ambiguous identifier exits nonzero before scripts or APM writes. A managed hook changed after the initial check or beneath a symlinked parent is preserved and reported; package removal finishes, but the command exits nonzero because hook cleanup is incomplete. | `--dry-run`, `-g` global | -| `apm prune` | Remove installed packages absent from the manifest and lockfile-resolved graph; reconcile stale dependency/deployment ownership after interrupted runs without deleting files based only on ghost metadata or dropping shared URI deployments. Orphan deletion failures exit 1 after processing remaining packages and report removed/failed counts. Successful deletions are not rolled back; resolve the errors, then rerun `apm prune`. | `--dry-run` previews package removal and ownership repair without mutation | +| `apm prune` | Remove unneeded recognized roots under `apm_modules/`, including manifestless `SKILL.md` packages after their lock entries disappear. No receipt or `.apm-pin` is required; personal content inside removable roots is also removed. Preserve declared direct/dev and retained transitive packages, bundles, and entire ancestors containing needed children. Outside deployments retain ownership-based protections; stale records never authorize deleting untrusted or shared bytes. Deletion failures exit 1 with removed/failed counts; resolve the errors and rerun to converge. | `--dry-run` previews removal, retention, and ownership repair without mutation | | `apm uninstall PKGS...` | Remove packages; reconcile tracked files, hooks, MCP, and LSP. Accepts `owner/repo`, `name@marketplace`, exact declared local paths, or portable `_local/` keys with lock metadata. Selection is atomic: missing or ambiguous identifiers exit before scripts or APM writes. Direct and orphan materialized directories are deleted before target cleanup or manifest and lockfile writes. Deletion failure exits 1 without `Uninstall complete` and retains declarations, the on-disk lockfile, and deployed ownership; earlier deletions are not rolled back. Fix permission or file-lock errors, or the unsafe path after a containment refusal, then retry the same command. Restore with `apm install` (`apm install --global` for user scope). A later target cleanup refusal can leave removed directories with retained metadata; resolve the listed files and retry. MCP cleanup attempts every recorded owner. Safe LSP or managed hook cleanup failure preserves the conflicting configuration and exits nonzero after package removal; repair it, then run the scope-appropriate install command. | `--dry-run`, `-g` global | -| `apm prune` | Remove installed packages absent from the manifest and lockfile-resolved graph; reconcile stale dependency/deployment ownership after interrupted runs without deleting files based only on ghost metadata or dropping shared URI deployments | `--dry-run` previews package removal and ownership repair without mutation | | `apm deps list` | List manifest- and lockfile-resolved packages; ignore parent-owned embedded manifests. Direct locked local packages use actionable `_local/` keys without absolute paths; transitives are removed through their parent. | `-g` global, `--all` both scopes, `--insecure` | | `apm deps tree` | Show the complete lockfile-resolved tree at any depth; mark repeated ancestors as circular | -- | | `apm deps why PKG` | Explain why a package is installed (walks lockfile bottom-up to direct deps; analogue of `npm why` / `yarn why`) | `-g` global, `--json` | diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index a5ed8a68ef..815c4c823c 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -323,37 +323,42 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - # Combined lockfile-membership + package-marker fallback determines - # which installed paths are real standalone packages (and so - # must NOT be masked by ancestor expansion). The lockfile is - # the canonical, tamper-evident record; apm.yml/SKILL.md presence is - # the fallback for projects without a lockfile yet. - # See _expand_with_ancestors for the user-safety rationale. - standalone_installed = _standalone_installed_packages( - installed, apm_modules_dir, lockfile=lockfile - ) - return _find_orphaned_packages(installed, expected, standalone_installed) + return _find_orphaned_packages(installed, expected) except Exception: return [] -def _find_orphaned_packages( - installed: Iterable[str], expected: set[str], standalone: Iterable[str] -) -> list[str]: - """Select orphans while preserving the contents of retained install roots. +def _find_orphaned_packages(installed: Iterable[str], expected: set[str]) -> list[str]: + """Select roots unrelated to the retained dependency graph. - Manifestless skill bundles have no root package marker. Their nested - skills belong to the declared or transitive install root, even though the - filesystem scan discovers each skill separately. Only actual expected - roots protect descendants; expanded ancestors must not protect siblings. + Keep both bundled descendants and whole roots containing needed children. + Ancestor prefixes protect only those containing roots, not their siblings. + Unlike dependency-list classification, deletion must retain recognized + ancestors at every depth, including aliases and hidden subdirectories. """ - expected_with_ancestors = _expand_with_ancestors(expected, standalone) - return sorted( - path - for path in installed - if path not in expected_with_ancestors - and not any(path.startswith(f"{root}/") for root in expected) - ) + roots = set() + retained = set(expected) + for path in expected: + try: + validate_path_segments(path, context="orphan selection") + except PathTraversalError: + # Invalid tokens protect only an exact match, never a wider subtree. + continue + normalized = path.replace("\\", "/") + roots.add(normalized) + parts = normalized.split("/") + retained.update("/".join(parts[:depth]) for depth in range(1, len(parts) + 1)) + + orphaned = [] + for path in installed: + normalized = path.replace("\\", "/") + if normalized in retained: + continue + parts = normalized.split("/") + if any("/".join(parts[:depth]) in roots for depth in range(1, len(parts))): + continue + orphaned.append(path) + return sorted(orphaned) # ------------------------------------------------------------------ diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index d6e3809d4c..6a2d8fcc87 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -21,10 +21,8 @@ from ..utils.path_security import safe_rmtree from ._helpers import ( _build_expected_install_paths, - _expand_with_ancestors, _find_orphaned_packages, _scan_installed_packages, - _standalone_installed_packages, ) from .uninstall.lockfile_state import lockfile_has_persisted_state @@ -126,15 +124,6 @@ def prune(ctx, dry_run): installed_packages = ( _scan_installed_packages(apm_modules_dir) if apm_modules_dir.exists() else set() ) - standalone_installed = _standalone_installed_packages( - installed_packages, - apm_modules_dir, - lockfile=lockfile, - ) - expected_with_ancestors = _expand_with_ancestors( - expected_installed, - standalone_installed, - ) expected_lock_keys = {dependency.get_unique_key() for dependency in declared_deps} if lockfile is not None: expected_lock_keys.update( @@ -145,14 +134,19 @@ def prune(ctx, dry_run): lock_keys_by_path = ( _lock_keys_by_install_path(lockfile, apm_modules_dir) if lockfile is not None else {} ) - orphaned_packages = _find_orphaned_packages( - installed_packages, expected_installed, standalone_installed + orphaned_paths = set( + _find_orphaned_packages( + set(installed_packages) | set(lock_keys_by_path), expected_installed + ) ) + orphaned_packages = sorted(set(installed_packages) & orphaned_paths) + retained_packages = set(installed_packages) - orphaned_paths + for package in sorted(retained_packages - expected_installed): + logger.progress(f"Retained {package}: required package content.") missing_orphaned_keys = sorted( dep_key for relative_path, dep_keys in lock_keys_by_path.items() - if relative_path in expected_with_ancestors - or not (apm_modules_dir / relative_path).exists() + if relative_path not in orphaned_paths or not (apm_modules_dir / relative_path).exists() for dep_key in dep_keys if dep_key not in expected_lock_keys ) @@ -209,7 +203,7 @@ def prune(ctx, dry_run): if missing_orphaned_keys: logger.progress( f"Found {len(missing_orphaned_keys)} stale lockfile dependency " - "record(s) without installed package content." + "record(s) without removable package content." ) if dry_run: diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index da52f0c387..de318213e2 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -79,8 +79,8 @@ class MutationCase: guard_id="install-deployment-orphan-selection", rule_id="install-deployment-orphan-selection", path="src/apm_cli/commands/prune.py", - old="orphaned_packages = _find_orphaned_packages(", - new="orphaned_packages = _find_orphaned_packages_disabled(", + old="_find_orphaned_packages(", + new="_find_orphaned_packages_disabled(", intent="Prune bypasses the shared declaration-aware orphan selector.", ), MutationCase( @@ -1193,6 +1193,20 @@ def test_owner_rules_report_nothing_before_mutation( assert baseline_violated_rule_ids == frozenset() +def test_orphan_selection_guard_rejects_warning_bypass() -> None: + """The shared-selector rule protects warnings as well as destructive pruning.""" + path = "src/apm_cli/commands/_helpers.py" + source = _source(path) + old = "return _find_orphaned_packages(installed, expected)" + assert source.count(old) == 1 + mutated = source.replace(old, "return sorted(set(installed) - expected)", 1) + ast.parse(mutated, filename=path) + rule_id = "install-deployment-orphan-selection" + report = run_selected_rules(ROOT, (rule_id,), source_overrides={path: mutated}) + assert report.failures == () + assert any(violation.rule_id == rule_id for violation in report.violations) + + def test_ref_freshness_guard_rejects_unconditional_cache_publication() -> None: """A checkout must not promote a lock pin into a fresh named observation.""" path = "src/apm_cli/deps/github_downloader.py" diff --git a/tests/integration/test_prune_failure_lifecycle.py b/tests/integration/test_prune_failure_lifecycle.py index cc67ed50ad..606b5e24fc 100644 --- a/tests/integration/test_prune_failure_lifecycle.py +++ b/tests/integration/test_prune_failure_lifecycle.py @@ -61,8 +61,12 @@ def _runner(blocked: Path, mode: str) -> ApmLifecycleRunner: ) +@pytest.mark.windows_compat +@pytest.mark.parametrize("package_marker", ["apm.yml", "SKILL.md"]) @pytest.mark.parametrize("mode", ["blocked", "mixed", "partial"]) -def test_prune_failure_reports_partial_state_and_retry_converges(tmp_path: Path, mode: str) -> None: +def test_prune_failure_reports_partial_state_and_retry_converges( + tmp_path: Path, mode: str, package_marker: str +) -> None: """Failure status tracks actual removals; retry preserves all unowned bytes.""" isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=os.environ) environment = isolated.subprocess_env() @@ -70,6 +74,9 @@ def test_prune_failure_reports_partial_state_and_retry_converges(tmp_path: Path, modules = consumer.root / "apm_modules" packages = LocalPackageFactory(modules / "orphan-org") blocked = packages.create("blocked") + if package_marker == "SKILL.md": + blocked.manifest_path.unlink() + (blocked.root / package_marker).write_text("# Orphan skill\n", encoding="utf-8") payload = blocked.root / "payload.txt" payload.write_bytes(b"orphan payload\n") removed_count = 2 if mode == "mixed" else 0 @@ -111,7 +118,7 @@ def test_prune_failure_reports_partial_state_and_retry_converges(tmp_path: Path, assert difference.removed == expected_removed assert difference.added == difference.changed == frozenset() assert LifecycleStateSnapshot.capture(consumer.root) == state - assert blocked.manifest_path.is_file() + assert (blocked.root / package_marker).is_file() retry_runner = _runner(blocked.root, "none") retry, repeat = retry_runner.run_sequence( diff --git a/tests/integration/test_prune_skill_lifecycle.py b/tests/integration/test_prune_skill_lifecycle.py index a6774bef75..deb465b748 100644 --- a/tests/integration/test_prune_skill_lifecycle.py +++ b/tests/integration/test_prune_skill_lifecycle.py @@ -3,7 +3,6 @@ from __future__ import annotations import os -import sys from pathlib import Path import pytest @@ -25,8 +24,11 @@ @pytest.mark.parametrize("alias", [None, "azure-ai-alias"]) -def test_install_remove_install_prune_skill(tmp_path: Path, alias: str | None) -> None: +def test_install_remove_install_prune_skill( + tmp_path_factory: pytest.TempPathFactory, apm_binary_path: Path, alias: str | None +) -> None: """Prune removes unlocked skill bytes while retaining a sibling and user files.""" + tmp_path = tmp_path_factory.mktemp("ps") isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=os.environ) repositories = LocalGitRepositoryFactory( isolated.repository_root, env=isolated.subprocess_env() @@ -56,16 +58,7 @@ def test_install_remove_install_prune_skill(tmp_path: Path, alias: str | None) - project = LocalPackageFactory(isolated.work_root).create( "consumer", dependencies=dependencies, targets=("copilot",) ) - source_root = Path(__file__).resolve().parents[2] / "src" - runner = ApmLifecycleRunner( - ( - sys.executable, - "-c", - f"import sys; sys.path.insert(0, {str(source_root)!r}); " - "from apm_cli.cli import main; main()", - ), - timeout_seconds=30, - ) + runner = ApmLifecycleRunner((str(apm_binary_path),), timeout_seconds=30) def run(*args: str) -> str: result = runner.run(args, cwd=project.root, env=environment, scenario_id="prune-skill") @@ -102,8 +95,14 @@ def run(*args: str) -> str: assert removed_key not in LockFile.read(lock_path).dependencies assert not removed_deployment.exists() assert removed_root.exists(), "Install leaves orphan source bytes for prune" + if alias is None: + (removed_root / ".apm-pin").unlink(missing_ok=True) + (removed_root / "personal-note.txt").write_text("Managed-root tradeoff\n", encoding="utf-8") sentinel = modules / "user-notes.txt" sentinel.write_text("Keep these notes\n", encoding="utf-8") + unrecognized = modules / "personal-sources" + unrecognized.mkdir() + (unrecognized / "note.txt").write_text("Not a recognized package\n", encoding="utf-8") before_dry_run = ArtifactSnapshot.capture(project.root) preview = run("prune", "--dry-run") @@ -114,14 +113,18 @@ def run(*args: str) -> str: assert_unchanged(retained_before, ArtifactSnapshot.capture(retained_root)) assert_unchanged(deployed_before, ArtifactSnapshot.capture(deployed / "retained")) assert sentinel.read_text(encoding="utf-8") == "Keep these notes\n" + assert (unrecognized / "note.txt").read_text(encoding="utf-8") == "Not a recognized package\n" before_repeat = ArtifactSnapshot.capture(project.root) assert "No orphaned packages" in run("prune") assert_unchanged(before_repeat, ArtifactSnapshot.capture(project.root)) @pytest.mark.parametrize("alias", [None, "bundle-alias"]) -def test_prune_preserves_declared_skill_bundle(tmp_path: Path, alias: str | None) -> None: +def test_prune_preserves_declared_skill_bundle( + tmp_path_factory: pytest.TempPathFactory, apm_binary_path: Path, alias: str | None +) -> None: """Real manifestless bundles survive prune and produce no compile orphan warning.""" + tmp_path = tmp_path_factory.mktemp("pb") isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=os.environ) repositories = LocalGitRepositoryFactory( isolated.repository_root, env=isolated.subprocess_env() @@ -143,16 +146,7 @@ def test_prune_preserves_declared_skill_bundle(tmp_path: Path, alias: str | None project = LocalPackageFactory(isolated.work_root).create( "consumer", dependencies=(dependency,), targets=("copilot",) ) - source_root = Path(__file__).resolve().parents[2] / "src" - runner = ApmLifecycleRunner( - ( - sys.executable, - "-c", - f"import sys; sys.path.insert(0, {str(source_root)!r}); " - "from apm_cli.cli import main; main()", - ), - timeout_seconds=30, - ) + runner = ApmLifecycleRunner((str(apm_binary_path),), timeout_seconds=30) def run(*args: str) -> str: result = runner.run(args, cwd=project.root, env=environment, scenario_id="prune-bundle") @@ -177,3 +171,82 @@ def run(*args: str) -> str: assert "Run 'apm prune'" not in output for name in ("alpha", "beta"): assert (bundle / "skills" / name / "SKILL.md").is_file() + + +@pytest.mark.parametrize("retention", ["direct", "dev", "transitive"]) +def test_prune_retains_installed_ancestor_after_root_declaration_removed( + tmp_path_factory: pytest.TempPathFactory, apm_binary_path: Path, retention: str +) -> None: + """The installed root survives lock removal while a nested dependency needs it.""" + isolated = IsolatedApmEnvironment.create( + tmp_path_factory.mktemp("pc") / "i", base_env=os.environ + ) + repositories = LocalGitRepositoryFactory( + isolated.repository_root, env=isolated.subprocess_env() + ) + repository = repositories.create("family") + child_path = ".github/plugins/skills/child" + for relative, name in (("", "family"), (child_path, "child")): + skill = repository.worktree / relative + skill.mkdir(parents=True, exist_ok=True) + (skill / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Retained-root fixture\n---\n# {name}\n", + encoding="utf-8", + ) + commit = repositories.commit(repository, message="Add root and nested skill") + remote = "https://gitlab.com/fixture/family" + environment = repositories.url_rewrite_subprocess_env(repository, remote) + root_dep = {"git": remote, "ref": commit.sha} + child_dep = {**root_dep, "path": child_path} + factory = LocalPackageFactory(isolated.work_root) + consumer = factory.create("consumer", dependencies=(root_dep,), targets=("copilot",)) + manifest = load_yaml(consumer.manifest_path) + if retention == "direct": + manifest["dependencies"]["apm"].append(child_dep) + elif retention == "dev": + manifest["devDependencies"] = {"apm": [child_dep]} + else: + keeper = factory.create("keeper", dependencies=(child_dep,), targets=("copilot",)) + factory.add_relative_dependency(consumer, keeper) + manifest = load_yaml(consumer.manifest_path) + dump_yaml(manifest, consumer.manifest_path) + runner = ApmLifecycleRunner((str(apm_binary_path),), timeout_seconds=30) + + def run(*args: str) -> str: + result = runner.run( + args, cwd=consumer.root, env=environment, scenario_id=f"retained-root-{retention}" + ) + assert result.returncode == 0, result.stdout + result.stderr + return " ".join((result.stdout + result.stderr).split()) + + install_args = ("install", "--no-policy", "--parallel-downloads", "0") + run(*install_args) + lock_path = consumer.root / "apm.lock.yaml" + installed = LockFile.read(lock_path) + root_key, root_package = next( + (key, package) + for key, package in installed.dependencies.items() + if package.repo_url == "fixture/family" and not package.virtual_path + ) + root = root_package.to_dependency_ref().get_install_path(consumer.root / "apm_modules") + assert (root / "SKILL.md").is_file() + assert (root / child_path / "SKILL.md").is_file() + manifest = load_yaml(consumer.manifest_path) + manifest["dependencies"]["apm"] = manifest["dependencies"]["apm"][1:] + dump_yaml(manifest, consumer.manifest_path) + run(*install_args) + lock = LockFile.read(lock_path) + assert root_key not in lock.dependencies + assert any(package.virtual_path == child_path for package in lock.dependencies.values()) + (root / "personal-note.txt").write_text("Keep the entire containing root\n", encoding="utf-8") + before = ArtifactSnapshot.capture(consumer.root) + + for args in (("prune", "--dry-run"), ("prune",), ("prune",)): + output = run(*args) + assert "Retained fixture/family" in output + assert "No orphaned packages" in output + assert "would be removed" not in output + assert_unchanged(before, ArtifactSnapshot.capture(consumer.root)) + output = run("compile") + assert "orphaned package(s)" not in output + assert (root / child_path / "SKILL.md").is_file() diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 804ff04ee0..4126c6f4dc 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -18,6 +18,7 @@ _check_and_notify_updates, _check_orphaned_packages, _expand_with_ancestors, + _find_orphaned_packages, _get_default_script, _list_available_scripts, _load_apm_config, @@ -496,17 +497,8 @@ def test_whole_repo_with_unrelated_orphan(self, tmp_path, monkeypatch): assert "org/my-package" not in orphaned assert "org/old-package" in orphaned - def test_real_orphan_at_owner_repo_with_sibling_subdir_dep(self, tmp_path, monkeypatch): - """Regression: a real installed ``owner/repo`` package on disk MUST - still be flagged as orphaned even when a sibling subdirectory dep - ``owner/repo/.apm/skills/foo`` is declared in apm.yml. - - Previously, ancestor expansion blindly added ``owner/repo`` to the - expected set whenever a subdir dep referenced it, silently - suppressing detection of a genuinely orphaned standalone package - that shared the same ``owner/repo`` filesystem root. ``apm prune`` - is a safety command -- it must NEVER silently miss a real orphan. - """ + def test_recognized_root_with_needed_subdir_is_not_orphaned(self, tmp_path, monkeypatch): + """Warnings must not recommend deleting a root containing a needed child.""" monkeypatch.chdir(tmp_path) # Declare ONLY the subdirectory dep. The standalone owner/repo @@ -523,9 +515,7 @@ def test_real_orphan_at_owner_repo_with_sibling_subdir_dep(self, tmp_path, monke ) apm_modules = tmp_path / "apm_modules" - # Real installed standalone package at owner/repo (with apm.yml AND - # .apm marker). This is a genuine orphan -- nothing in apm.yml - # declares the whole repo as a dep. + # The undeclared containing package must remain for its needed child. pkg_dir = apm_modules / "owner" / "repo" pkg_dir.mkdir(parents=True) (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0.0", encoding="utf-8") @@ -536,11 +526,25 @@ def test_real_orphan_at_owner_repo_with_sibling_subdir_dep(self, tmp_path, monke (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") orphaned = _check_orphaned_packages() - assert "owner/repo" in orphaned, ( - "Real orphan at owner/repo must be flagged even when a " - "sibling subdirectory dep shares the same root; got: " - f"{orphaned}" - ) + assert orphaned == [] + + +@pytest.mark.windows_compat +@pytest.mark.parametrize("separator", ["/", "\\"]) +def test_orphan_selector_uses_segment_bounded_paths(separator: str) -> None: + """Normalize Windows tokens without confusing adjacent roots with descendants.""" + expected = { + "alias/skills/child".replace("/", separator), + "owner/bundle".replace("/", separator), + } + installed = ["alias", "alias-other", "owner/bundle/skills/embedded", "owner/bundle-other"] + assert _find_orphaned_packages(installed, expected) == ["alias-other", "owner/bundle-other"] + + +@pytest.mark.parametrize("invalid", ["owner/../repo", "owner/./repo"]) +def test_orphan_selector_invalid_tokens_do_not_protect_ancestors(invalid: str) -> None: + """Malformed tokens cannot widen retention beyond an exact match.""" + assert _find_orphaned_packages(["owner", "owner/repo"], {invalid}) == ["owner", "owner/repo"] # --------------------------------------------------------------------------- diff --git a/tests/unit/test_prune_command.py b/tests/unit/test_prune_command.py index 1931cb6010..7e110dbc81 100644 --- a/tests/unit/test_prune_command.py +++ b/tests/unit/test_prune_command.py @@ -36,6 +36,7 @@ from apm_cli.deps.lockfile import LockedDependency, LockFile from apm_cli.integration.cleanup import remove_stale_deployed_files from apm_cli.models.apm_package import clear_apm_yml_cache +from tests.utils.artifact_snapshot import ArtifactSnapshot, assert_unchanged # --------------------------------------------------------------------------- # Test helpers @@ -261,7 +262,8 @@ def test_prune_keeps_declared_packages(self): @pytest.mark.windows_compat @pytest.mark.parametrize("dry_run", [False, True]) @pytest.mark.parametrize("declared", [False, True]) - def test_prune_skill_only_subdirectory_without_lockfile(self, dry_run, declared): + @pytest.mark.parametrize("with_pin", [False, True]) + def test_prune_skill_only_subdirectory_without_lockfile(self, dry_run, declared, with_pin): """Find skill-only installs after install has dropped their lock entry.""" with self._chdir_tmp() as tmp: dependency = "microsoft/skills/.github/plugins/azure-skills/skills/azure-ai" @@ -272,7 +274,8 @@ def test_prune_skill_only_subdirectory_without_lockfile(self, dry_run, declared) skill_dir = tmp / "apm_modules" / dependency skill_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text("# Azure AI\n") - (skill_dir / ".apm-pin").write_text("a" * 40) + if with_pin: + (skill_dir / ".apm-pin").write_text("a" * 40) references = skill_dir / "references" references.mkdir() (references / "guide.md").write_text("Reference material\n") @@ -387,28 +390,52 @@ def test_prune_preserves_manifestless_bundle(self, retention, dry_run): for name in ("alpha", "beta"): assert (bundle / "skills" / name / "SKILL.md").read_text() == f"# {name}\n" + @pytest.mark.windows_compat + @pytest.mark.parametrize("retention", ["direct", "dev", "transitive"]) + @pytest.mark.parametrize("parent_marker", ["apm.yml", "SKILL.md"]) + @pytest.mark.parametrize("root_path", ["owner/repo", "owner/repo/.github/plugins/bundle"]) @pytest.mark.parametrize("dry_run", [False, True]) - def test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory(self, dry_run): - """A skill root gets the same standalone-orphan treatment as an apm.yml root.""" + def test_prune_retains_root_containing_needed_child( + self, dry_run: bool, root_path: str, parent_marker: str, retention: str + ) -> None: + """A needed nested package protects its containing root at every depth.""" with self._chdir_tmp() as tmp: - (tmp / "apm.yml").write_text( - _APM_YML_NO_DEPS.replace("apm: []", "apm:\n - owner/repo/skills/child") - ) - root = tmp / "apm_modules" / "owner" / "repo" + child_key = f"{root_path}/skills/child" + manifest = _APM_YML_NO_DEPS + if retention == "direct": + manifest = manifest.replace("apm: []", f"apm:\n - {child_key}") + elif retention == "dev": + manifest += f"devDependencies:\n apm:\n - {child_key}\n" + else: + LockFile( + dependencies={ + child_key: LockedDependency( + repo_url="owner/repo", + virtual_path=child_key.removeprefix("owner/repo/"), + is_virtual=True, + depth=2, + ) + } + ).write(tmp / "apm.lock.yaml") + (tmp / "apm.yml").write_text(manifest) + root = tmp / "apm_modules" / root_path child = root / "skills" / "child" child.mkdir(parents=True) - (root / "SKILL.md").write_text("# Root skill\n") + (root / parent_marker).write_text("name: bundle\nversion: 1.0.0\n") (child / "SKILL.md").write_text("# Child skill\n") + unrelated = root.with_name(f"{root.name}-other") + unrelated.mkdir() + (unrelated / "SKILL.md").write_text("# Orphan\n") + retained = ArtifactSnapshot.capture(root) - assert _check_orphaned_packages() == ["owner/repo"] + assert _check_orphaned_packages() == [f"{root_path}-other"] result = self.runner.invoke(cli, ["prune", *(["--dry-run"] if dry_run else [])]) assert result.exit_code == 0, result.output assert "1 orphaned package(s)" in result.output - assert root.exists() == dry_run - if dry_run: - assert (root / "SKILL.md").read_text() == "# Root skill\n" - assert (child / "SKILL.md").read_text() == "# Child skill\n" + assert f"Retained {root_path}" in " ".join(result.output.split()) + assert unrelated.exists() == dry_run + assert_unchanged(retained, ArtifactSnapshot.capture(root)) def test_prune_removes_multiple_orphans(self): """prune removes all orphaned packages in one pass.""" @@ -421,19 +448,42 @@ def test_prune_removes_multiple_orphans(self): assert not dir1.exists() assert not dir2.exists() - def test_prune_removes_real_orphan_with_sibling_subdir_dep(self): - """Regression: the destructive ``apm prune`` command must - delete a genuinely orphaned ``owner/repo`` package even when - a sibling subdirectory dep ``owner/repo/.apm/skills/foo`` is - declared in apm.yml. - - Previously, ``prune.py`` called ``_expand_with_ancestors`` - without the ``standalone_installed`` guard, so ``owner/repo`` - was added to the expected set as an ancestor of the subdir - dep -- silently suppressing deletion of a real orphan and - diverging from the advisory display path. ``apm prune`` is a - safety command; missing a real orphan is a correctness bug. - """ + def test_retained_ancestor_removes_only_stale_lock_entry(self) -> None: + """Retaining source bytes does not keep an undeclared deployment owner.""" + with self._chdir_tmp() as tmp: + child_key = "owner/repo/skills/child" + (tmp / "apm.yml").write_text( + _APM_YML_NO_DEPS.replace("apm: []", f"apm:\n - {child_key}") + ) + root = tmp / "apm_modules" / "owner" / "repo" + child = root / "skills" / "child" + child.mkdir(parents=True) + (root / "SKILL.md").write_text("# Root\n") + (child / "SKILL.md").write_text("# Child\n") + lock_path = tmp / "apm.lock.yaml" + LockFile( + dependencies={ + "owner/repo": LockedDependency(repo_url="owner/repo", depth=1), + child_key: LockedDependency( + repo_url="owner/repo", + virtual_path="skills/child", + is_virtual=True, + depth=1, + ), + } + ).write(lock_path) + before = ArtifactSnapshot.capture(root) + + result = self.runner.invoke(cli, ["prune"]) + + assert result.exit_code == 0, result.output + assert "Retained owner/repo" in result.output + assert "without removable package content" in " ".join(result.output.split()) + assert set(LockFile.read(lock_path).dependencies) == {child_key} + assert_unchanged(before, ArtifactSnapshot.capture(root)) + + def test_prune_retains_shared_root_with_declared_subdir_dep(self): + """Whole-root deletion must not collateral-delete a declared subdirectory.""" with self._chdir_tmp() as tmp: # Declare ONLY the subdirectory dep. The standalone # owner/repo package is not declared anywhere. @@ -456,22 +506,13 @@ def test_prune_removes_real_orphan_with_sibling_subdir_dep(self): result = self.runner.invoke(cli, ["prune"]) assert result.exit_code == 0, result.output - # Real orphan MUST be deleted -- this is the security - # invariant the panel flagged as a required fix. - assert not (pkg_dir / "apm.yml").exists(), ( - "Real orphan owner/repo (apm.yml) must be removed even " - "when a sibling subdir dep shares the same root" - ) - # Subdir dep content collateral-damages because the whole - # owner/repo tree is the orphan's filesystem footprint; - # the user is expected to re-install. This matches the - # advisory display path in deps/cli.py. - assert not skill_dir.exists() - - def test_prune_dry_run_lists_real_orphan_with_sibling_subdir_dep(self): - """Dry-run path must also surface the real orphan (display - parity with the advisory check). - """ + assert (pkg_dir / "apm.yml").read_text() == "name: repo\nversion: 1.0\n" + assert (skill_dir / "SKILL.md").read_text() == "# Skill\n" + assert "No orphaned packages" in result.output + assert "Retained owner/repo" in result.output + + def test_prune_dry_run_reports_retained_shared_root(self): + """Dry-run reports retention rather than proposing destructive cleanup.""" with self._chdir_tmp() as tmp: (tmp / "apm.yml").write_text( "name: test\n" @@ -490,7 +531,9 @@ def test_prune_dry_run_lists_real_orphan_with_sibling_subdir_dep(self): result = self.runner.invoke(cli, ["prune", "--dry-run"]) assert result.exit_code == 0, result.output - assert "owner/repo" in result.output + assert "Retained owner/repo" in result.output + assert "would be removed" not in result.output + assert _check_orphaned_packages() == [] # No deletion occurred. assert (pkg_dir / "apm.yml").exists() diff --git a/tests/unit/test_windows_compat_gate_workflow.py b/tests/unit/test_windows_compat_gate_workflow.py index 0c5f747874..2841550592 100644 --- a/tests/unit/test_windows_compat_gate_workflow.py +++ b/tests/unit/test_windows_compat_gate_workflow.py @@ -151,7 +151,7 @@ def test_windows_compat_gate_runs_on_windows_with_bounded_timeout() -> None: def test_windows_compat_gate_selects_tests_via_registered_marker() -> None: """The gate must select tests declaratively via `-m windows_compat`, - with one explicit integration contract outside the unit-test root. + across unit and integration collection roots. This is the core anti-pattern guard: a future edit that reverts to a hardcoded file list (functionally equivalent to the old @@ -172,16 +172,16 @@ def test_windows_compat_gate_selects_tests_via_registered_marker() -> None: def test_windows_compat_gate_runs_over_narrowest_maintainable_root() -> None: - """Run the unit root plus the one load-bearing subprocess integration contract.""" + """Discover marked contracts without a per-file integration allowlist.""" job = workflow_job(_ci_workflow(), GATE_JOB) step = workflow_step(job, GATE_STEP) args = _gate_pytest_args(step) positional = _positional_test_paths(args) - expected = ["tests/unit", "tests/integration/test_lifecycle_workspace_lock.py"] + expected = ["tests/unit", "tests/integration"] assert positional == expected, ( - f"{GATE_STEP!r} must scope to the unit contracts and lifecycle subprocess " - f"contract {expected!r}, got: {positional!r}" + f"{GATE_STEP!r} must collect marked contracts from {expected!r}, got: {positional!r}" ) + assert step["env"]["APM_E2E_TESTS"] == "1" def test_windows_compat_gate_does_not_duplicate_full_suite() -> None: From e61d67ecebed3e8864e9fc1a95c2d9e7dedf519f Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 25 Sep 2026 09:11:35 +0200 Subject: [PATCH 4/5] test(prune): assert aliased ledger package paths and preserved bytes The existing fixture installs alpha-kit and beta-kit aliases but asserted nonexistent org/repo paths. Resolve paths from installed lock entries, assert both existed before prune, then assert actual removal and byte-for-byte survivor retention. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../integration/test_prune_deployment_ledger_e2e.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_prune_deployment_ledger_e2e.py b/tests/integration/test_prune_deployment_ledger_e2e.py index d511bb58ca..d39ee2a07d 100644 --- a/tests/integration/test_prune_deployment_ledger_e2e.py +++ b/tests/integration/test_prune_deployment_ledger_e2e.py @@ -18,6 +18,7 @@ from apm_cli.deps.lockfile import LockFile from apm_cli.utils.yaml_io import dump_yaml, load_yaml from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner, CommandResult +from tests.utils.artifact_snapshot import ArtifactSnapshot, assert_unchanged from tests.utils.isolated_apm_environment import IsolatedApmEnvironment from tests.utils.lifecycle_state import LifecycleStateSnapshot from tests.utils.local_git_repository import LocalGitRepositoryFactory @@ -149,6 +150,13 @@ def test_prune_cascades_dependency_state_and_audit_sees_no_ghost( env=environment, ) _assert_exit(install) + installed = LockFile.read(consumer.root / "apm.lock.yaml") + modules = consumer.root / "apm_modules" + alpha_root = installed.dependencies[_ALPHA_KEY].to_dependency_ref().get_install_path(modules) + beta_root = installed.dependencies[_BETA_KEY].to_dependency_ref().get_install_path(modules) + assert alpha_root.is_dir() + assert beta_root.is_dir() + retained_source = ArtifactSnapshot.capture(alpha_root) manifest = load_yaml(consumer.manifest_path) manifest["dependencies"]["apm"] = [ @@ -213,8 +221,9 @@ def test_prune_cascades_dependency_state_and_audit_sees_no_ghost( assert _BETA_KEY not in owners assert _ALPHA_KEY in owners - assert not (consumer.root / "apm_modules" / "apm-fixture-org" / "beta-kit").exists() - assert (consumer.root / "apm_modules" / "apm-fixture-org" / "alpha-kit").is_dir() + assert not beta_root.exists() + assert alpha_root.is_dir() + assert_unchanged(retained_source, ArtifactSnapshot.capture(alpha_root)) assert not (consumer.root / ".claude/rules/beta.md").exists() assert not (consumer.root / ".claude/skills/beta").exists() assert (consumer.root / ".claude/rules/alpha.md").is_file() From 2e8c618b1f42e324f9bfcb83da6165b913ee081e Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 25 Sep 2026 09:20:52 +0200 Subject: [PATCH 5/5] test(prune): keep owner mutation inventory ordered and clarify cleanup docs Preserve the existing deterministic mutation ordering. Align consumer and lockfile guidance with recognized-root retention and the unchanged trusted-deployment cleanup boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../content/docs/consumer/manage-dependencies.md | 12 +++++++++--- docs/src/content/docs/reference/lockfile-spec.md | 9 ++++++--- .../test_architecture_owner_rule_mutations.py | 16 ++++++++-------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/docs/src/content/docs/consumer/manage-dependencies.md b/docs/src/content/docs/consumer/manage-dependencies.md index 88226d4b98..10231d8dd5 100644 --- a/docs/src/content/docs/consumer/manage-dependencies.md +++ b/docs/src/content/docs/consumer/manage-dependencies.md @@ -378,15 +378,21 @@ apm prune --dry-run # preview what gets deleted apm prune # delete orphaned packages from apm_modules/ ``` -`apm prune` removes any directory in `apm_modules/` that no longer -corresponds to a declared dependency or a transitive dependency still -required by another package. It does not touch your manifest. +`apm prune` removes unneeded recognized package roots in `apm_modules/`, +including manifestless `SKILL.md` installs whose lock entries are already gone. +It preserves declared direct/dev and retained transitive packages, bundled +skills, and entire roots containing needed nested packages. Personal files +inside a removable root are also removed; keep personal source outside +`apm_modules/`. It does not touch your manifest. Lockfile entries, deployed harness files (`.github/`, `.claude/`, etc.), and merged hook configuration owned by the pruned package are all reconciled immediately by `apm prune` itself -- remaining direct and transitive packages keep their hooks; no follow-up `apm install` is required. +See [`apm prune`](../../reference/cli/prune/) for the managed-root boundary +and the separate ownership protections for deployed files. + If you also want to refresh remaining deps to their latest versions or refs, see [Update and refresh](../update-and-refresh/). diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 2de17dc79f..a6950bed47 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -375,10 +375,13 @@ the two checks do not double-count. Orphan detection works in two directions: -- **Orphan packages** - entries in `dependencies` that the manifest no longer - declares. `apm prune` removes them and their `deployed_files`. +- **Orphan packages** - recognized roots under `apm_modules/` no longer needed + by the dependency graph, even when their lock entries are gone. `apm prune` + removes them while preserving bundles and roots containing needed children. - **Orphan files** - files under managed target directories that no lockfile - entry claims. `apm prune` removes them too. + entry claims. A ghost record alone does not authorize deleting these bytes. + Prune repairs ownership metadata; deletion still requires a pruned + dependency's trusted pre-transition claim and preserves surviving owners. `apm prune` is the only command that reconciles `deployments` rows. The valid owner universe and metadata-only repair boundary are defined in diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index de318213e2..f72a45075a 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -75,14 +75,6 @@ class MutationCase: MUTATIONS: tuple[MutationCase, ...] = ( - MutationCase( - guard_id="install-deployment-orphan-selection", - rule_id="install-deployment-orphan-selection", - path="src/apm_cli/commands/prune.py", - old="_find_orphaned_packages(", - new="_find_orphaned_packages_disabled(", - intent="Prune bypasses the shared declaration-aware orphan selector.", - ), MutationCase( guard_id="contracts-tests-taxonomy-classification", rule_id="contracts-tests-taxonomy-classification", @@ -405,6 +397,14 @@ class MutationCase: new="def resolve_mcp_registry_url_disabled(", intent="The registry client loses the canonical MCP registry precedence resolver.", ), + MutationCase( + guard_id="install-deployment-orphan-selection", + rule_id="install-deployment-orphan-selection", + path="src/apm_cli/commands/prune.py", + old="_find_orphaned_packages(", + new="_find_orphaned_packages_disabled(", + intent="Prune bypasses the shared declaration-aware orphan selector.", + ), MutationCase( guard_id="install-deployment-outcome", rule_id="install-deployment-outcome",