Fix cross-package file protection in stale cleanup (#1831)

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
 

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"

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()