diff --git a/src/apm_cli/install/phases/cleanup.py b/src/apm_cli/install/phases/cleanup.py index 2a793346f..0f2cdfb54 100644 --- a/src/apm_cli/install/phases/cleanup.py +++ b/src/apm_cli/install/phases/cleanup.py @@ -19,8 +19,8 @@ Failed deletions are re-inserted into ``ctx.package_deployed_files`` so the downstream lockfile phase records the retained paths. -This module is a faithful extraction from ``commands/install.py`` -- -no behavioural changes. +Originally extracted from ``commands/install.py``; later extended with +cross-package file protection for stale cleanup (#1831). """ from __future__ import annotations @@ -118,6 +118,10 @@ def run(ctx: InstallContext) -> None: # packages that left the manifest entirely. # ------------------------------------------------------------------ if existing_lockfile and package_deployed_files: + all_deployed_files: set[str] = set() + for deployed_files in package_deployed_files.values(): + all_deployed_files.update(deployed_files) + for dep_key, new_deployed in package_deployed_files.items(): # Skip packages whose integration reported errors this run -- # a file that failed to re-deploy would look stale and get @@ -128,7 +132,24 @@ def run(ctx: InstallContext) -> None: prev_dep = existing_lockfile.get_dependency(dep_key) if not prev_dep: continue # new package this install -- nothing stale yet - stale = detect_stale_files(prev_dep.deployed_files, new_deployed) + stale = set(detect_stale_files(prev_dep.deployed_files, new_deployed)) + if not stale: + continue + + # Cross-package file protection (#1831): do not remove a file + # that another currently-installed package still deploys. + # package_deployed_files is integration outcome, not a security + # boundary; content-hash provenance remains the deletion gate. + other_deployed = all_deployed_files - set(new_deployed) + protected = stale & other_deployed + stale = stale - other_deployed + if protected and logger: + for protected_path in sorted(protected): + logger.verbose_detail( + f"Kept stale file {protected_path} for {dep_key}; " + "still deployed by another package" + ) + if not stale: continue diff --git a/tests/integration/test_intra_package_cleanup.py b/tests/integration/test_intra_package_cleanup.py index 87e7eb800..2a94e9557 100644 --- a/tests/integration/test_intra_package_cleanup.py +++ b/tests/integration/test_intra_package_cleanup.py @@ -94,6 +94,21 @@ def _write_apm_yml_local(project_dir, local_pkg_path): ) +def _write_apm_yml_local_packages(project_dir, local_pkg_paths): + """Write apm.yml with multiple local-path package dependencies.""" + config = { + "name": "intra-package-cleanup-test", + "version": "1.0.0", + "dependencies": { + "apm": [{"path": str(path)} for path in local_pkg_paths], + "mcp": [], + }, + } + (project_dir / "apm.yml").write_text( + yaml.dump(config, default_flow_style=False), encoding="utf-8" + ) + + def _find_local_dep(lockfile): """Return the locked-dep entry that represents a local-path package, or None. @@ -111,6 +126,29 @@ def _find_local_dep(lockfile): return None +def _find_local_deps(lockfile): + """Return all locked-dep entries that represent local-path packages.""" + if not lockfile: + return [] + deps = lockfile.get("dependencies") or {} + entries = deps if isinstance(deps, list) else deps.values() + return [entry for entry in entries if entry and entry.get("source") == "local"] + + +def _make_local_prompt_package(tmp_path, name, prompts): + """Create a local-path APM package with prompt primitives.""" + pkg = tmp_path / name + prompts_dir = pkg / ".apm" / "prompts" + prompts_dir.mkdir(parents=True) + (pkg / "apm.yml").write_text( + yaml.dump({"name": name, "version": "0.0.1"}, default_flow_style=False), + encoding="utf-8", + ) + for filename, body in prompts.items(): + (prompts_dir / filename).write_text(body, encoding="utf-8") + return pkg + + class TestFileRenamedWithinPackage: """Regression tests for issue #666: renaming a file inside a still-present package must delete the stale deployed artifacts on the next apm install.""" @@ -205,3 +243,59 @@ def test_partial_install_cleans_renamed_file(self, temp_project, apm_command, lo assert stale not in deployed_after, ( f"Stale path {stale} still in lockfile deployed_files after partial install" ) + + +class TestCrossPackageSharedFileCleanup: + """Regression tests for issue #1831 across real local-path packages.""" + + def test_shared_file_survives_when_other_package_still_deploys_it( + self, temp_project, apm_command, tmp_path + ): + """If pkg-a drops a shared prompt, pkg-b's deployed copy survives.""" + shared_body = "---\ndescription: shared\n---\nshared\n" + pkg_a = _make_local_prompt_package( + tmp_path, + "pkg-a", + { + "shared.prompt.md": shared_body, + "only-a.prompt.md": "---\ndescription: only a\n---\nonly a\n", + }, + ) + pkg_b = _make_local_prompt_package( + tmp_path, + "pkg-b", + {"shared.prompt.md": shared_body}, + ) + _write_apm_yml_local_packages(temp_project, [pkg_a, pkg_b]) + + result1 = _run_apm(apm_command, ["install"], temp_project) + assert result1.returncode == 0, ( + f"Initial install failed:\nSTDOUT: {result1.stdout}\nSTDERR: {result1.stderr}" + ) + + lockfile_before = _read_lockfile(temp_project) + local_deps_before = _find_local_deps(lockfile_before) + shared_claims = [ + dep + for dep in local_deps_before + if ".github/prompts/shared.prompt.md" in (dep.get("deployed_files") or []) + ] + assert len(shared_claims) == 2, ( + "Both local packages must claim the shared prompt before testing cleanup" + ) + + shared_target = temp_project / ".github" / "prompts" / "shared.prompt.md" + only_a_target = temp_project / ".github" / "prompts" / "only-a.prompt.md" + assert shared_target.exists() + assert only_a_target.exists() + + (pkg_a / ".apm" / "prompts" / "shared.prompt.md").unlink() + (pkg_a / ".apm" / "prompts" / "only-a.prompt.md").unlink() + + result2 = _run_apm(apm_command, ["install"], temp_project) + assert result2.returncode == 0, ( + f"Re-install failed:\nSTDOUT: {result2.stdout}\nSTDERR: {result2.stderr}" + ) + + assert shared_target.exists(), "Shared prompt was deleted despite pkg-b ownership" + assert not only_a_target.exists(), "Unclaimed stale prompt survived cleanup" diff --git a/tests/unit/install/phases/test_cleanup_phase.py b/tests/unit/install/phases/test_cleanup_phase.py index 7efef92ec..901cbefd4 100644 --- a/tests/unit/install/phases/test_cleanup_phase.py +++ b/tests/unit/install/phases/test_cleanup_phase.py @@ -504,3 +504,146 @@ def test_stale_cleanup_no_logger_no_crash(self, tmp_path): patch("apm_cli.install.phases.cleanup.BaseIntegrator.cleanup_empty_parents"), ): cleanup.run(ctx) # must not raise + + +# --------------------------------------------------------------------------- +# Block 13: cross-package file protection (#1831) +# Files deployed by another package must not be removed as stale. +# --------------------------------------------------------------------------- + + +class TestCrossPackageProtection: + """Stale cleanup must skip files still claimed by another package.""" + + def test_shared_file_not_removed_when_other_package_deploys_it(self, tmp_path): + """Issue #1831: if pkg-a no longer deploys shared.md but pkg-b still + does, cleanup must NOT delete shared.md.""" + prev_dep_a = _make_orphan_dep( + [".github/agents/shared.md", ".github/agents/only-a.md"], + file_hashes={".github/agents/shared.md": "abc", ".github/agents/only-a.md": "def"}, + ) + lf = _make_lockfile({"pkg-a": prev_dep_a}) + lf.get_dependency.return_value = prev_dep_a + + # pkg-a no longer deploys either file; pkg-b still deploys shared.md + ctx = _make_ctx( + existing_lockfile=lf, + intended_dep_keys={"pkg-a", "pkg-b"}, + package_deployed_files={ + "pkg-a": [], + "pkg-b": [".github/agents/shared.md"], + }, + project_root=tmp_path, + ) + + mock_result = MagicMock() + mock_result.failed = [] + mock_result.deleted = [".github/agents/only-a.md"] + mock_result.deleted_targets = [] + mock_result.skipped_user_edit = [] + + with ( + patch( + "apm_cli.install.phases.cleanup.detect_stale_files", + return_value={".github/agents/shared.md", ".github/agents/only-a.md"}, + ), + patch( + "apm_cli.install.phases.cleanup.remove_stale_deployed_files", + return_value=mock_result, + ) as mock_rm, + patch("apm_cli.install.phases.cleanup.BaseIntegrator.cleanup_empty_parents"), + ): + cleanup.run(ctx) + + # remove_stale_deployed_files should only receive "only-a.md" + # because shared.md is still claimed by pkg-b + call_args = mock_rm.call_args + stale_passed = call_args[0][0] + assert ".github/agents/only-a.md" in stale_passed + assert ".github/agents/shared.md" not in stale_passed + ctx.logger.verbose_detail.assert_called_once_with( + "Kept stale file .github/agents/shared.md for pkg-a; still deployed by another package" + ) + + def test_file_removed_when_no_other_package_claims_it(self, tmp_path): + """Normal case: stale file not claimed by any other package is removed.""" + prev_dep = _make_orphan_dep( + [".github/agents/old.md"], + file_hashes={".github/agents/old.md": "abc"}, + ) + lf = _make_lockfile({"pkg-a": prev_dep}) + lf.get_dependency.return_value = prev_dep + + ctx = _make_ctx( + existing_lockfile=lf, + intended_dep_keys={"pkg-a", "pkg-b"}, + package_deployed_files={ + "pkg-a": [], + "pkg-b": [".github/agents/other.md"], + }, + project_root=tmp_path, + ) + + mock_result = MagicMock() + mock_result.failed = [] + mock_result.deleted = [".github/agents/old.md"] + mock_result.deleted_targets = [] + mock_result.skipped_user_edit = [] + + with ( + patch( + "apm_cli.install.phases.cleanup.detect_stale_files", + return_value={".github/agents/old.md"}, + ), + patch( + "apm_cli.install.phases.cleanup.remove_stale_deployed_files", + return_value=mock_result, + ) as mock_rm, + patch("apm_cli.install.phases.cleanup.BaseIntegrator.cleanup_empty_parents"), + ): + cleanup.run(ctx) + + # The file should be passed to removal since no other package claims it + call_args = mock_rm.call_args + stale_passed = call_args[0][0] + assert ".github/agents/old.md" in stale_passed + + +# --------------------------------------------------------------------------- +# Block 14: legacy lockfile graceful skip (#1831) +# When prev_dep.deployed_files is empty (old APM version), stale cleanup +# is safely skipped and the new deployed_files is recorded for next run. +# --------------------------------------------------------------------------- + + +class TestLegacyLockfileGracefulSkip: + """Legacy lockfiles with empty deployed_files skip stale cleanup safely.""" + + def test_empty_prev_deployed_files_skips_cleanup(self, tmp_path): + """When previous lockfile has empty deployed_files, no stale removal + is attempted. The new deployed_files will be recorded for future diffs.""" + prev_dep = _make_orphan_dep([], file_hashes={}) + lf = _make_lockfile({"legacy-pkg": prev_dep}) + lf.get_dependency.return_value = prev_dep + + ctx = _make_ctx( + existing_lockfile=lf, + intended_dep_keys={"legacy-pkg"}, + package_deployed_files={ + "legacy-pkg": [".github/agents/new-file.md"], + }, + project_root=tmp_path, + ) + + with ( + patch( + "apm_cli.install.phases.cleanup.remove_stale_deployed_files", + ) as mock_rm, + patch("apm_cli.install.phases.cleanup.BaseIntegrator.cleanup_empty_parents"), + ): + cleanup.run(ctx) + + # No removal should be attempted since there are no previously + # tracked files to compare against + mock_rm.assert_not_called() + ctx.logger.stale_cleanup.assert_not_called()