[fix] warn when two packages deploy a native skill with the same name

src/apm_cli/integration/skill_integrator.py

@@ -330,14 +330,20 @@ def copy_skill_to_target(
 
 class SkillIntegrator(BaseIntegrator):
     """Handles integration of native SKILL.md files for Claude Code, Cursor, and VS Code.
-    
+
     Claude Skills Spec:
     - SKILL.md files provide structured context for Claude Code
     - YAML frontmatter with name, description, and metadata
     - Markdown body with instructions and agent definitions
     - references/ subdirectory for prompt files
     """
-
+
+    def __init__(self) -> None:
+        # In-memory map of skill_name -> dep.get_unique_key() updated as each native
+        # skill is deployed in the current install run.  Complements the lockfile-based
+        # map so that same-manifest collisions are detected before the lockfile is written.
+        self._native_skill_session_owners: dict[str, str] = {}
+
     def find_instruction_files(self, package_path: Path) -> List[Path]:
         """Find all instruction files in a package.
 
@@ -560,26 +566,55 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
         return promoted, deployed
 
     @staticmethod
-    def _build_skill_ownership_map(project_root: Path) -> dict[str, str]:
-        """Build a map of skill_name -> owner_package_name from the lockfile.
-
-        Used to distinguish self-overwrites (no warning) from cross-package
-        conflicts (warning) when promoting sub-skills.
+    def _build_ownership_maps(project_root: Path) -> tuple[dict[str, str], dict[str, str]]:
+        """Read the lockfile once and build two ownership maps.
+
+        Returns a tuple of:
+        - owned_by: skill_name -> last-segment owner name, for sub-skill self-overwrite detection.
+        - native_owners: skill_name -> dep.get_unique_key(), for native-skill cross-package
+          collision detection.  Only paths under a ``/skills/`` prefix are included to avoid
+          false attribution from non-skill deployed_files entries (prompts, hooks, commands, etc.).
         """
         from apm_cli.deps.lockfile import LockFile, get_lockfile_path
 
         owned_by: dict[str, str] = {}
+        native_owners: dict[str, str] = {}
         lockfile = LockFile.read(get_lockfile_path(project_root))
         if not lockfile:
-            return owned_by
+            return owned_by, native_owners
         for dep in lockfile.get_all_dependencies():
-            owner = (dep.virtual_path or dep.repo_url).rsplit("/", 1)[-1]
+            short_owner = (dep.virtual_path or dep.repo_url).rsplit("/", 1)[-1]
+            unique_key = dep.get_unique_key()
             for deployed_path in dep.deployed_files:
-                # e.g. ".github/skills/context-map" -> "context-map"
-                skill_name = deployed_path.rstrip("/").rsplit("/", 1)[-1]
-                owned_by[skill_name] = owner
+                normalized = deployed_path.rstrip("/").replace("\\", "/")
+                skill_name = normalized.rsplit("/", 1)[-1]
+                # Both maps cover all paths for sub-skill self-overwrite tracking.
+                owned_by[skill_name] = short_owner
+                # Native-owner map is scoped to skill paths only to avoid false
+                # attribution from prompts/hooks/commands that share a leaf name.
+                if "/skills/" in normalized:
+                    native_owners[skill_name] = unique_key
+        return owned_by, native_owners
+
+    @staticmethod
+    def _build_skill_ownership_map(project_root: Path) -> dict[str, str]:
+        """Build a map of skill_name -> owner_package_name from the lockfile.
+
+        Used to distinguish self-overwrites (no warning) from cross-package
+        conflicts (warning) when promoting sub-skills.
+        """
+        owned_by, _ = SkillIntegrator._build_ownership_maps(project_root)
         return owned_by
 
+    @staticmethod
+    def _build_native_skill_owner_map(project_root: Path) -> dict[str, str]:
+        """Build a map of skill_name -> dep.get_unique_key() from the lockfile.
+
+        Scoped to ``/skills/`` paths only -- see ``_build_ownership_maps`` for details.
+        """
+        _, native_owners = SkillIntegrator._build_ownership_maps(project_root)
+        return native_owners
+
     def _promote_sub_skills_standalone(
         self, package_info, project_root: Path, diagnostics=None,
         managed_files=None, force: bool = False, logger=None,
@@ -714,9 +749,14 @@ def _integrate_native_skill(
         all_target_paths: list[Path] = []
         primary_skill_md: Path | None = None
 
-        owned_by = self._build_skill_ownership_map(project_root)
+        # Read lockfile once and derive both maps in a single pass.
+        owned_by, lockfile_native_owners = self._build_ownership_maps(project_root)
         sub_skills_dir = package_path / ".apm" / "skills"
 
+        # Full unique key of the package currently being installed.
+        dep_ref = package_info.dependency_ref
+        current_key: str | None = dep_ref.get_unique_key() if dep_ref is not None else None
+
         for idx, target in enumerate(targets):
             if not target.supports("skills"):
                 continue
@@ -732,6 +772,41 @@ def _integrate_native_skill(
                 primary_skill_md = target_skill_dir / "SKILL.md"
 
             if target_skill_dir.exists():
+                if is_primary:
+                    # Check both the lockfile (previous runs) and the in-memory session
+                    # map (current run) so that same-manifest collisions are caught even
+                    # before the lockfile has been written for this run.
+                    prev_owner = (
+                        lockfile_native_owners.get(skill_name)
+                        or self._native_skill_session_owners.get(skill_name)
+                    )
+                    is_self_overwrite = prev_owner is not None and prev_owner == current_key
+                    if prev_owner is not None and not is_self_overwrite:
+                        try:
+                            rel_prefix = target_skill_dir.parent.relative_to(project_root).as_posix()
+                        except ValueError:
+                            rel_prefix = "skills"
+                        rel_path = f"{rel_prefix}/{skill_name}"
+                        # Issue 1: package= should identify the package causing the
+                        # collision (current_key), not the skill name, so render_summary()
+                        # groups diagnostics by the package responsible.
+                        # Issue 2: message must tell the user what to do ("So What?" test).
+                        detail = (
+                            f"Skill '{skill_name}' from '{current_key}' replaced "
+                            f"'{prev_owner}' -- remove one package to avoid this"
+                        )
+                        if diagnostics is not None:
+                            diagnostics.overwrite(
+                                path=rel_path,
+                                package=current_key or skill_name,
+                                detail=detail,
+                            )
+                        elif logger:
+                            logger.warning(detail)
+                        else:
+                            # Reached when called without diagnostics or logger (e.g. uninstall sync).
+                            from apm_cli.utils.console import _rich_warning
+                            _rich_warning(detail)
                 shutil.rmtree(target_skill_dir)
 
             target_skill_dir.parent.mkdir(parents=True, exist_ok=True)
@@ -756,6 +831,11 @@ def _integrate_native_skill(
             )
             all_target_paths.extend(sub_deployed)
 
+        # Record ownership in the session map so subsequent packages installed in
+        # the same run can detect a collision even before the lockfile is written.
+        if current_key is not None:
+            self._native_skill_session_owners[skill_name] = current_key
+
         # Count unique sub-skills from primary target only
         primary_root = project_root / ".github" / "skills"
         sub_skills_count = sum(