fix: apm install no longer adopts files not produced by installed plugins (closes #1199)

CHANGELOG.md

@@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `apm install` no longer silently overwrites pre-existing governance files; `check_collision()` now treats `managed_files=None` (first install, no lockfile yet) as an empty set so hand-rolled files in `.github/instructions/` and other governance directories are correctly detected and protected from silent overwrite. (#1256)
 - `test_find_server_by_reference_uuid_not_found` no longer leaks a live HTTP call to `api.mcp.github.com` (fixing Windows CI failures from `socket.gaierror`); the `search_servers` fallback is now mocked. (#1264)
 - ADO full HTTPS URLs with sub-path virtual packages (e.g. `https://dev.azure.com/org/proj/_git/repo/sub/path`) are now parsed correctly instead of being rejected. (#1254)
 - Fixed `apm install` crash (exit code 128) when a mono-repo package depends on a sibling pinned to a non-HEAD commit; installs now resolve with a single in-place fetch, and multiple SHA-pinned references to the same repository share a single cached clone. (#1258)

src/apm_cli/integration/base_integrator.py

@@ -68,10 +68,14 @@ def check_collision(
         """Return True if *target_path* is a user-authored collision.
 
         A collision exists when **all** of these are true:
-        1. ``managed_files`` is not ``None`` (manifest mode)
-        2. ``target_path`` already exists on disk
-        3. ``rel_path`` is **not** in the managed set (-> user-authored)
-        4. ``force`` is ``False``
+        1. ``target_path`` already exists on disk
+        2. ``rel_path`` is **not** in the managed set (-> user-authored)
+        3. ``force`` is ``False``
+
+        When ``managed_files`` is ``None`` it is treated as an empty set:
+        no files are managed, so any pre-existing file at the target path
+        is considered a user-authored collision and is protected from
+        silent overwrite.
 
         When *diagnostics* is provided the skip is recorded there;
         otherwise a warning is emitted via ``_rich_warning``.
@@ -80,7 +84,7 @@ def check_collision(
            forward-slash separators (see ``normalize_managed_files``).
         """
         if managed_files is None:
-            return False
+            managed_files = set()
         if not target_path.exists():
             return False
         # managed_files is pre-normalized at the call site  -- O(1) lookup

tests/unit/integration/test_agent_integrator.py

@@ -157,8 +157,8 @@ def test_integrate_package_agents_creates_directory(self):
         assert result.files_integrated == 1
         assert (self.project_root / ".github" / "agents").exists()
 
-    def test_integrate_package_agents_always_overwrites(self):
-        """Test that integration always overwrites existing files."""
+    def test_integrate_package_agents_force_overwrites(self):
+        """Test that force=True overwrites existing files even with no manifest."""
         package_dir = self.project_root / "package"
         package_dir.mkdir()
         (package_dir / "security.agent.md").write_text("# Security Agent")
@@ -188,7 +188,9 @@ def test_integrate_package_agents_always_overwrites(self):
             installed_at="2024-01-01T00:00:00",
         )
 
-        result = self.integrator.integrate_package_agents(package_info, self.project_root)
+        result = self.integrator.integrate_package_agents(
+            package_info, self.project_root, force=True
+        )
 
         assert result.files_integrated == 1
         assert result.files_updated == 0
@@ -257,7 +259,7 @@ def test_integrate_first_time_copies_verbatim(self):
         assert "apm:" not in content
 
     def test_integrate_overwrites_existing_file(self):
-        """Test that integration always overwrites existing files."""
+        """Test that force=True overwrites existing files on update."""
         package_dir = self.project_root / "package"
         package_dir.mkdir()
         (package_dir / "security.agent.md").write_text("# Updated Agent Content")
@@ -287,7 +289,9 @@ def test_integrate_overwrites_existing_file(self):
             installed_at="2024-11-13T11:00:00",
         )
 
-        result = self.integrator.integrate_package_agents(package_info, self.project_root)
+        result = self.integrator.integrate_package_agents(
+            package_info, self.project_root, force=True
+        )
 
         assert result.files_integrated == 1
         assert result.files_updated == 0
@@ -299,7 +303,7 @@ def test_integrate_overwrites_existing_file(self):
         assert content == "# Updated Agent Content"
 
     def test_integrate_all_files_always_copied(self):
-        """Test integration copies all agent files regardless of existing state."""
+        """Test force=True copies all agent files regardless of existing state."""
         package_dir = self.project_root / "package"
         package_dir.mkdir()
 
@@ -334,9 +338,11 @@ def test_integrate_all_files_always_copied(self):
             installed_at="2024-11-13T11:00:00",
         )
 
-        result = self.integrator.integrate_package_agents(package_info, self.project_root)
+        result = self.integrator.integrate_package_agents(
+            package_info, self.project_root, force=True
+        )
 
-        assert result.files_integrated == 3  # All files always copied
+        assert result.files_integrated == 3  # All files copied when force=True
         assert result.files_updated == 0
         assert result.files_skipped == 0
 
@@ -719,8 +725,8 @@ def test_integrate_no_agents_returns_empty_result(self):
         assert result.files_integrated == 0
         assert not (self.project_root / ".claude" / "agents").exists()
 
-    def test_integrate_always_overwrites(self):
-        """Test that integration always overwrites existing files."""
+    def test_integrate_force_overwrites(self):
+        """Test that force=True overwrites existing files."""
         package_dir = self.project_root / "package"
         package_dir.mkdir()
         (package_dir / "security.agent.md").write_text("# Updated Content")
@@ -731,7 +737,9 @@ def test_integrate_always_overwrites(self):
         (agents_dir / "security.md").write_text("# Old Content")
 
         package_info = self._create_package_info(package_dir)
-        result = self.integrator.integrate_package_agents_claude(package_info, self.project_root)
+        result = self.integrator.integrate_package_agents_claude(
+            package_info, self.project_root, force=True
+        )
 
         assert result.files_integrated == 1
         content = (agents_dir / "security.md").read_text()

tests/unit/integration/test_base_integrator.py

@@ -75,11 +75,16 @@ def setup_method(self):
     def teardown_method(self):
         shutil.rmtree(self.tmp, ignore_errors=True)
 
-    def test_no_collision_managed_files_none(self):
-        """When managed_files is None, no collision possible."""
+    def test_collision_managed_files_none_file_exists(self):
+        """managed_files=None with existing file -> collision (None treated as empty set)."""
         target = self.root / "file.md"
         target.write_text("content")
-        assert BaseIntegrator.check_collision(target, "file.md", None, False) is False
+        assert BaseIntegrator.check_collision(target, "file.md", None, False) is True
+
+    def test_no_collision_managed_files_none_file_absent(self):
+        """managed_files=None with no existing file -> no collision."""
+        target = self.root / "nonexistent.md"
+        assert BaseIntegrator.check_collision(target, "nonexistent.md", None, False) is False
 
     def test_no_collision_file_does_not_exist(self):
         """File doesn't exist -> no collision."""

tests/unit/integration/test_check_collision.py

@@ -0,0 +1,212 @@
+"""Regression tests for BaseIntegrator.check_collision() -- managed_files=None semantics.
+
+Covers the latent defect where managed_files=None bypassed collision detection
+entirely, silently overwriting user-authored governance files on first install.
+
+After the fix, None is treated as an empty set: no file is managed, so any
+pre-existing file at the target path is a collision (protected from overwrite).
+
+Scenarios:
+    1. managed_files=None + file exists + force=False  -> collision (True)
+    2. managed_files=None + file absent               -> no collision (False)
+    3. managed_files=set() + file exists + force=False -> collision (True)
+    4. managed_files contains rel_path + file exists  -> no collision (False)
+    5. Integration: integrate_instructions_for_target skips hand-rolled file
+       when managed_files=None and the file already exists on disk.
+"""
+
+from datetime import datetime
+from pathlib import Path
+
+from apm_cli.integration.base_integrator import BaseIntegrator
+from apm_cli.integration.instruction_integrator import InstructionIntegrator
+from apm_cli.models.apm_package import APMPackage, GitReferenceType, PackageInfo, ResolvedReference
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def _make_package_info(package_dir: Path, name: str = "test-pkg") -> PackageInfo:
+    """Build a minimal PackageInfo for use in integration-level tests."""
+    package = APMPackage(
+        name=name,
+        version="1.0.0",
+        package_path=package_dir,
+        source=f"github.com/test/{name}",
+    )
+    resolved_ref = ResolvedReference(
+        original_ref="main",
+        ref_type=GitReferenceType.BRANCH,
+        resolved_commit="abc123",
+        ref_name="main",
+    )
+    return PackageInfo(
+        package=package,
+        install_path=package_dir,
+        resolved_reference=resolved_ref,
+        installed_at=datetime.now().isoformat(),
+    )
+
+
+# ---------------------------------------------------------------------------
+# Unit-level: check_collision seam
+# ---------------------------------------------------------------------------
+
+
+class TestCheckCollisionNoneSemantics:
+    """managed_files=None must behave identically to managed_files=set()."""
+
+    def test_none_with_existing_file_no_force_is_collision(self, tmp_path: Path) -> None:
+        """managed_files=None + file exists + force=False -> collision detected."""
+        target = tmp_path / "handrolled.instructions.md"
+        target.write_text("# hand-rolled content")
+
+        result = BaseIntegrator.check_collision(
+            target, ".github/instructions/handrolled.instructions.md", None, False
+        )
+
+        assert result is True, "None managed_files must NOT bypass collision detection"
+
+    def test_none_with_absent_file_no_collision(self, tmp_path: Path) -> None:
+        """managed_files=None + file absent -> safe to deploy (no collision)."""
+        target = tmp_path / "new.instructions.md"
+        # File intentionally not created
+
+        result = BaseIntegrator.check_collision(
+            target, ".github/instructions/new.instructions.md", None, False
+        )
+
+        assert result is False
+
+    def test_empty_set_with_existing_file_is_collision(self, tmp_path: Path) -> None:
+        """managed_files=set() + file exists -> collision.
+
+        Verifies that None-as-empty-set behavior matches an explicit empty set.
+        """
+        target = tmp_path / "handrolled.instructions.md"
+        target.write_text("# hand-rolled content")
+
+        result = BaseIntegrator.check_collision(
+            target, ".github/instructions/handrolled.instructions.md", set(), False
+        )
+
+        assert result is True
+
+    def test_none_matches_empty_set_for_existing_file(self, tmp_path: Path) -> None:
+        """None and set() produce identical results for an existing file."""
+        target = tmp_path / "file.instructions.md"
+        target.write_text("# content")
+        rel = ".github/instructions/file.instructions.md"
+
+        result_none = BaseIntegrator.check_collision(target, rel, None, False)
+        result_empty = BaseIntegrator.check_collision(target, rel, set(), False)
+
+        assert result_none == result_empty
+
+    def test_managed_file_in_set_no_collision(self, tmp_path: Path) -> None:
+        """File exists AND rel_path is in managed_files -> not a collision (can update)."""
+        target = tmp_path / "apm-managed.instructions.md"
+        target.write_text("# APM-managed content")
+        rel = ".github/instructions/apm-managed.instructions.md"
+
+        result = BaseIntegrator.check_collision(target, rel, {rel}, False)
+
+        assert result is False
+
+    def test_none_force_true_no_collision(self, tmp_path: Path) -> None:
+        """force=True always suppresses the collision, even when managed_files=None."""
+        target = tmp_path / "handrolled.instructions.md"
+        target.write_text("# hand-rolled content")
+
+        result = BaseIntegrator.check_collision(
+            target, ".github/instructions/handrolled.instructions.md", None, True
+        )
+
+        assert result is False
+
+
+# ---------------------------------------------------------------------------
+# Integration-level: instruction integrator flow
+# ---------------------------------------------------------------------------
+
+
+class TestIntegrateInstructionsNoneManagedFiles:
+    """End-to-end: hand-rolled file is NOT overwritten when managed_files=None."""
+
+    def test_handrolled_file_skipped_when_managed_files_none(self, tmp_path: Path) -> None:
+        """integrate_instructions_for_target must skip a pre-existing file.
+
+        Scenario:
+          - Package ships handrolled.instructions.md
+          - A hand-rolled copy already exists at .github/instructions/handrolled.instructions.md
+          - managed_files=None (first install, no lockfile yet)
+          - Expected: the hand-rolled file is NOT in target_paths (not overwritten)
+        """
+        from apm_cli.integration.targets import KNOWN_TARGETS
+
+        # Set up package with one instruction file
+        pkg_dir = tmp_path / "pkg"
+        inst_dir = pkg_dir / ".apm" / "instructions"
+        inst_dir.mkdir(parents=True)
+        (inst_dir / "handrolled.instructions.md").write_text(
+            "---\napplyTo: '**/*.py'\n---\n# APM-provided rules"
+        )
+
+        # Pre-existing hand-rolled file at the deploy target
+        github_instructions = tmp_path / ".github" / "instructions"
+        github_instructions.mkdir(parents=True)
+        handrolled_path = github_instructions / "handrolled.instructions.md"
+        handrolled_path.write_text("# user-authored content -- must not be overwritten")
+
+        pkg_info = _make_package_info(pkg_dir)
+        integrator = InstructionIntegrator()
+        copilot = KNOWN_TARGETS["copilot"]
+
+        result = integrator.integrate_instructions_for_target(
+            copilot,
+            pkg_info,
+            tmp_path,
+            force=False,
+            managed_files=None,
+        )
+
+        # The hand-rolled file must NOT appear in target_paths
+        assert handrolled_path not in result.target_paths, (
+            "Hand-rolled file must be skipped when managed_files=None (treated as empty set)"
+        )
+        # The original content must be preserved on disk
+        assert handrolled_path.read_text() == "# user-authored content -- must not be overwritten"
+        # The file should have been counted as skipped
+        assert result.files_skipped >= 1
+
+    def test_apm_file_deployed_when_path_is_absent(self, tmp_path: Path) -> None:
+        """When no pre-existing file exists, managed_files=None still allows deploy."""
+        from apm_cli.integration.targets import KNOWN_TARGETS
+
+        pkg_dir = tmp_path / "pkg"
+        inst_dir = pkg_dir / ".apm" / "instructions"
+        inst_dir.mkdir(parents=True)
+        (inst_dir / "python.instructions.md").write_text(
+            "---\napplyTo: '**/*.py'\n---\n# Python rules"
+        )
+
+        # Ensure .github exists but the target file does NOT
+        (tmp_path / ".github" / "instructions").mkdir(parents=True)
+
+        pkg_info = _make_package_info(pkg_dir)
+        integrator = InstructionIntegrator()
+        copilot = KNOWN_TARGETS["copilot"]
+
+        result = integrator.integrate_instructions_for_target(
+            copilot,
+            pkg_info,
+            tmp_path,
+            force=False,
+            managed_files=None,
+        )
+
+        deployed = tmp_path / ".github" / "instructions" / "python.instructions.md"
+        assert deployed in result.target_paths
+        assert result.files_integrated == 1
+        assert result.files_skipped == 0