From c805fc6a317d6d6b48dc66dd6701c559f36bb7ff Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Fri, 19 Jun 2026 18:59:44 +0100 Subject: [PATCH 1/3] Fix cross-package file protection in stale cleanup (#1831) Add cross-package ownership check in the cleanup phase: before deleting a stale file, verify no other currently-installed package claims it in its deployed_files. This prevents incorrect deletion when two packages share a path and one is updated. Also add regression test for legacy lockfiles with empty deployed_files confirming the graceful-skip behaviour (no stale removal attempted, new deployed_files recorded for future diffs). Closes #1831 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/install/phases/cleanup.py | 15 +- .../unit/install/phases/test_cleanup_phase.py | 140 ++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/install/phases/cleanup.py b/src/apm_cli/install/phases/cleanup.py index 2a793346f..401e99320 100644 --- a/src/apm_cli/install/phases/cleanup.py +++ b/src/apm_cli/install/phases/cleanup.py @@ -128,7 +128,20 @@ 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. + # Without this guard, updating pkg-a (which dropped shared.md) + # would delete the file even though pkg-b still deploys it. + other_deployed: set = set() + for other_key, other_files in package_deployed_files.items(): + if other_key != dep_key: + other_deployed.update(other_files) + stale = stale - other_deployed + if not stale: continue diff --git a/tests/unit/install/phases/test_cleanup_phase.py b/tests/unit/install/phases/test_cleanup_phase.py index 7efef92ec..cfb39b3c8 100644 --- a/tests/unit/install/phases/test_cleanup_phase.py +++ b/tests/unit/install/phases/test_cleanup_phase.py @@ -504,3 +504,143 @@ 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 + + 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() From 0c3fcddb486c8b282f27786e009650a96d3b2b02 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 20 Jun 2026 20:08:34 +0200 Subject: [PATCH 2/3] Improve cross-package stale cleanup guard Precompute deployed-file ownership, add verbose breadcrumbs for protected stale paths, and add an integration regression for the shared-file update scenario. Addresses panel follow-ups from python-architect, cli-logging-expert, supply-chain-security-expert, performance-expert, and test-coverage-expert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/install/phases/cleanup.py | 20 ++-- .../integration/test_intra_package_cleanup.py | 94 +++++++++++++++++++ .../unit/install/phases/test_cleanup_phase.py | 3 + 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/src/apm_cli/install/phases/cleanup.py b/src/apm_cli/install/phases/cleanup.py index 401e99320..18819dd0b 100644 --- a/src/apm_cli/install/phases/cleanup.py +++ b/src/apm_cli/install/phases/cleanup.py @@ -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 @@ -134,13 +138,17 @@ def run(ctx: InstallContext) -> None: # Cross-package file protection (#1831): do not remove a file # that another currently-installed package still deploys. - # Without this guard, updating pkg-a (which dropped shared.md) - # would delete the file even though pkg-b still deploys it. - other_deployed: set = set() - for other_key, other_files in package_deployed_files.items(): - if other_key != dep_key: - other_deployed.update(other_files) + # 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 cfb39b3c8..901cbefd4 100644 --- a/tests/unit/install/phases/test_cleanup_phase.py +++ b/tests/unit/install/phases/test_cleanup_phase.py @@ -561,6 +561,9 @@ def test_shared_file_not_removed_when_other_package_deploys_it(self, tmp_path): 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.""" From 04e74e394765332414cbe7c1b10b62baba5887bc Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Sat, 20 Jun 2026 20:16:30 +0200 Subject: [PATCH 3/3] Refresh cleanup phase module context Update the cleanup phase module docstring so it no longer claims the file is an unchanged extraction after the cross-package stale-file protection work. Addresses python-architect panel follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/install/phases/cleanup.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/apm_cli/install/phases/cleanup.py b/src/apm_cli/install/phases/cleanup.py index 18819dd0b..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