fix(skills): key ownership tracking on owner/repo, not the leaf directory name

CHANGELOG.md

@@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Sub-skill and native-skill ownership tracking now keys on the full
+  dependency identity (`owner/repo`) instead of the last path segment.
+  Two different packages that happen to share a repo/leaf name (e.g. two
+  orgs each publishing a `shared-skill` or `utils` repo) were previously
+  treated as the same owner, silently suppressing the cross-package
+  collision warning exactly when it mattered -- an unrelated package
+  could overwrite another's skill with no signal. `apm install --force`
+  now correctly reports the collision regardless of dependency source
+  (registry or git), and the lockfile no longer records both packages as
+  owning the same deployed file after a collision. (by @nadav-y)
+
 ## [0.24.0] - 2026-07-05
 
 ### Added

src/apm_cli/install/phases/lockfile.py

@@ -177,6 +177,34 @@ def _has_orphan_lockfile_entries(self) -> bool:
         intended = self.ctx.intended_dep_keys or set()
         return any(key != _SELF_KEY and key not in intended for key in existing.dependencies)
 
+    def _reconcile_cross_package_deployed_files(self) -> None:
+        """Strip a stale ownership claim when two dep_keys report the same path.
+
+        ``ctx.package_deployed_files`` is populated once per dep_key,
+        independently, by that dep's own integration call (see
+        ``install/template.py``). When two different packages' primitives
+        resolve to the same on-disk path -- a name collision, e.g. two repos
+        both shipping a skill called ``shared-topic`` -- each package's own
+        integration call correctly and independently reports "I wrote this
+        path" at the moment it ran. Under sequential integration
+        (``install_order``), a later dep_key's write physically overwrites an
+        earlier one's at the same path, so only the last dep_key to claim a
+        given path is telling the truth by the time the lockfile is written.
+
+        Without this pass, every dep_key that ever claimed a colliding path
+        keeps it in the final lockfile, even though only one of them actually
+        owns it on disk -- a lockfile integrity bug: a future
+        ``apm uninstall``/``apm audit`` on a "losing" dep would act on a file
+        it does not control.
+        """
+        package_deployed_files = self.ctx.package_deployed_files
+        last_owner: dict[str, str] = {}
+        for dep_key, files in package_deployed_files.items():
+            for f in files:
+                last_owner[f] = dep_key  # dict iteration is insertion order -> last write wins
+        for dep_key, files in package_deployed_files.items():
+            package_deployed_files[dep_key] = [f for f in files if last_owner[f] == dep_key]
+
     def _attach_deployed_files(self, lockfile: LockFile) -> None:
         """Attach per-dependency deployed-file manifests, unioning targets.
 
@@ -189,6 +217,7 @@ def _attach_deployed_files(self, lockfile: LockFile) -> None:
         """
         from apm_cli.install.manifest_reconcile import union_preserving
 
+        self._reconcile_cross_package_deployed_files()
         existing = self.ctx.existing_lockfile
         for dep_key, locked_dep in lockfile.dependencies.items():
             current = list(self.ctx.package_deployed_files.get(dep_key, []))

src/apm_cli/integration/skill_integrator.py

@@ -753,10 +753,18 @@ def _build_ownership_maps(project_root: Path) -> tuple[dict[str, str], dict[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.
+        - owned_by: skill_name -> dep.get_unique_key(), 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.).
+
+        Both maps key on the full unique dependency identity (owner/repo, or the
+        equivalent durable key for local/registry deps), NOT the last path
+        segment. Two different packages can share a repo/leaf name (e.g. two
+        orgs each publishing a "shared-skill" or "utils" repo); comparing only
+        the last segment would treat them as the same owner and silently
+        suppress the cross-package collision warning precisely when it matters
+        most -- an unrelated package overwriting another's skill undetected.
         """
         from apm_cli.deps.lockfile import LockFile, get_lockfile_path
 
@@ -766,13 +774,12 @@ def _build_ownership_maps(project_root: Path) -> tuple[dict[str, str], dict[str,
         if not lockfile:
             return owned_by, native_owners
         for dep in lockfile.get_package_dependencies():
-            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:
                 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
+                owned_by[skill_name] = unique_key
                 # 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:
@@ -837,7 +844,11 @@ def _promote_sub_skills_standalone(
 
             targets = active_targets(project_root)
 
-        parent_name = package_path.name
+        # Durable identity for self-overwrite comparison -- NOT package_path.name,
+        # which is just the repo/leaf name and collides across owners (see
+        # _build_ownership_maps).
+        _dep_ref = getattr(package_info, "dependency_ref", None)
+        parent_name = _dep_ref.get_unique_key() if _dep_ref is not None else package_path.name
         owned_by = self._build_skill_ownership_map(project_root)
         name_filter = self._skill_subset_name_filter(skill_subset)
         count = 0
@@ -1080,12 +1091,14 @@ def _ignore_non_content_and_apm(directory, contents):
             if is_primary:
                 files_copied = sum(1 for _ in target_skill_dir.rglob("*") if _.is_file())
 
-            # Promote sub-skills for this target.
+            # Promote sub-skills for this target. Identify the parent by its
+            # durable unique key (current_key), not skill_name -- the folder
+            # name collides across owners (see _build_ownership_maps).
             target_skills_root = self._target_skills_root(target, project_root)
             _, sub_deployed = self._promote_sub_skills(
                 sub_skills_dir,
                 target_skills_root,
-                skill_name,
+                current_key or skill_name,
                 warn=is_primary,
                 owned_by=owned_by if is_primary else None,
                 diagnostics=diagnostics if is_primary else None,
@@ -1159,7 +1172,13 @@ def _integrate_skill_bundle(
 
             targets = active_targets(project_root)
 
-        parent_name = package_info.install_path.name
+        # Durable identity for self-overwrite comparison -- NOT install_path.name,
+        # which is just the repo/leaf name and collides across owners (see
+        # _build_ownership_maps).
+        _dep_ref = getattr(package_info, "dependency_ref", None)
+        parent_name = (
+            _dep_ref.get_unique_key() if _dep_ref is not None else package_info.install_path.name
+        )
         owned_by, lockfile_native_owners = self._build_ownership_maps(project_root)  # noqa: RUF059
 
         total_promoted = 0

tests/unit/install/phases/test_lockfile_cross_package_reconcile.py

@@ -0,0 +1,119 @@
+"""Regression traps for cross-package deployed-file ownership reconciliation.
+
+``ctx.package_deployed_files`` is populated once per dep_key, independently,
+by that dep's own integration call (see ``install/template.py``). When two
+different packages' primitives resolve to the same on-disk path -- a name
+collision, e.g. two repos both shipping a skill called ``shared-topic`` --
+each package's own integration call correctly and independently reports "I
+wrote this path" at the moment it ran. Without reconciliation, BOTH entries
+end up claiming ``deployed_files`` for a path only one of them actually
+owns on disk -- a lockfile integrity bug: a future ``apm uninstall`` or
+``apm audit`` on the "losing" package would act on a file it does not
+control. See ``LockfileBuilder._reconcile_cross_package_deployed_files``.
+"""
+
+from __future__ import annotations
+
+from types import SimpleNamespace
+
+from apm_cli.deps.lockfile import LockedDependency, LockFile
+from apm_cli.install.phases.lockfile import LockfileBuilder
+
+
+def _target(name, root_dir=".claude"):
+    return SimpleNamespace(name=name, root_dir=root_dir, primitives={})
+
+
+def _ctx(*, package_deployed_files, existing_lockfile=None, targets=None, project_root):
+    return SimpleNamespace(
+        package_deployed_files=package_deployed_files,
+        existing_lockfile=existing_lockfile,
+        targets=targets or [_target("claude")],
+        project_root=project_root,
+    )
+
+
+class TestReconcileCrossPackageDeployedFiles:
+    def test_colliding_path_kept_only_on_last_writer(self, tmp_path) -> None:
+        """Two dep_keys both report the same path; only the last (the actual
+        on-disk owner, under sequential integration order) keeps it."""
+        package_deployed_files = {
+            "orga/shared-skill": [".claude/skills/shared-topic/SKILL.md"],
+            "orgb/shared-skill": [".claude/skills/shared-topic/SKILL.md"],
+        }
+        ctx = _ctx(package_deployed_files=package_deployed_files, project_root=tmp_path)
+
+        LockfileBuilder(ctx)._reconcile_cross_package_deployed_files()
+
+        assert package_deployed_files["orga/shared-skill"] == []
+        assert package_deployed_files["orgb/shared-skill"] == [
+            ".claude/skills/shared-topic/SKILL.md"
+        ]
+
+    def test_non_colliding_paths_are_untouched(self, tmp_path) -> None:
+        """Normal case: no two dep_keys share a path -- nothing is stripped."""
+        package_deployed_files = {
+            "orga/repo-a": [".claude/skills/topic-a/SKILL.md"],
+            "orgb/repo-b": [".claude/skills/topic-b/SKILL.md"],
+        }
+        ctx = _ctx(package_deployed_files=package_deployed_files, project_root=tmp_path)
+
+        LockfileBuilder(ctx)._reconcile_cross_package_deployed_files()
+
+        assert package_deployed_files["orga/repo-a"] == [".claude/skills/topic-a/SKILL.md"]
+        assert package_deployed_files["orgb/repo-b"] == [".claude/skills/topic-b/SKILL.md"]
+
+    def test_partial_collision_only_strips_the_shared_path(self, tmp_path) -> None:
+        """A dep_key with multiple deployed files only loses the ONE path
+        another dep_key also claims -- its other files are untouched."""
+        package_deployed_files = {
+            "orga/shared-skill": [
+                ".claude/skills/shared-topic/SKILL.md",
+                ".claude/skills/unique-to-a/SKILL.md",
+            ],
+            "orgb/shared-skill": [".claude/skills/shared-topic/SKILL.md"],
+        }
+        ctx = _ctx(package_deployed_files=package_deployed_files, project_root=tmp_path)
+
+        LockfileBuilder(ctx)._reconcile_cross_package_deployed_files()
+
+        assert package_deployed_files["orga/shared-skill"] == [
+            ".claude/skills/unique-to-a/SKILL.md"
+        ]
+        assert package_deployed_files["orgb/shared-skill"] == [
+            ".claude/skills/shared-topic/SKILL.md"
+        ]
+
+    def test_attach_deployed_files_end_to_end_only_winner_recorded(self, tmp_path) -> None:
+        """End-to-end through _attach_deployed_files: the lockfile entry for
+        the losing package must not claim deployed_files for the collided
+        path, and must not resurrect it from a prior lockfile either."""
+        key_a = "orga/shared-skill"
+        key_b = "orgb/shared-skill"
+        collided_path = ".claude/skills/shared-topic/SKILL.md"
+
+        prior = LockFile()
+        prior.add_dependency(
+            LockedDependency(
+                repo_url=key_a,
+                deployed_files=[collided_path],
+                deployed_file_hashes={collided_path: "sha256:aaa"},
+            )
+        )
+
+        new = LockFile()
+        new.add_dependency(LockedDependency(repo_url=key_a))
+        new.add_dependency(LockedDependency(repo_url=key_b))
+
+        ctx = _ctx(
+            package_deployed_files={key_a: [collided_path], key_b: [collided_path]},
+            existing_lockfile=prior,
+            targets=[_target("claude")],
+            project_root=tmp_path,
+        )
+        LockfileBuilder(ctx)._attach_deployed_files(new)
+
+        dep_a = new.get_dependency(key_a)
+        dep_b = new.get_dependency(key_b)
+        assert collided_path not in (dep_a.deployed_files or [])
+        assert collided_path in dep_b.deployed_files

tests/unit/integration/test_skill_integrator.py

@@ -3965,6 +3965,131 @@ def test_skill_only_agents_skipped_on_cowork(self, tmp_path: Path) -> None:
         assert not (cowork_root / "my-skill" / ".apm").exists()
 
 
+class TestSubSkillOwnershipIdentity:
+    """Regression tests: sub-skill self-overwrite detection must key on the
+    durable unique dependency identity (owner/repo), not the leaf install
+    directory name. Two different packages can share a repo/leaf name (e.g.
+    two orgs each publishing a "shared-skill" or "utils" repo) -- comparing
+    only the leaf would falsely treat the second as the first re-installing
+    itself, silently suppressing the cross-package collision warning exactly
+    when it matters most."""
+
+    def test_same_leaf_name_different_owner_is_flagged_as_collision(self, tmp_path: Path) -> None:
+        from apm_cli.utils.diagnostics import CATEGORY_OVERWRITE, DiagnosticCollector
+
+        cowork_root = tmp_path / "cowork-skills"
+        cowork_root.mkdir()
+        cowork_target = _make_resolved_cowork_target(cowork_root)
+        project_root = tmp_path / "project"
+        project_root.mkdir()
+
+        # Two packages from DIFFERENT owners that happen to share the same
+        # leaf/repo name ("shared-skill") -- package_path.name is identical
+        # for both, but they are unrelated packages.
+        pkg_a_dir = tmp_path / "orga" / "shared-skill"
+        sub_a = pkg_a_dir / ".apm" / "skills" / "topic"
+        sub_a.mkdir(parents=True)
+        (sub_a / "SKILL.md").write_text("# From Org A")
+
+        pkg_b_dir = tmp_path / "orgb" / "shared-skill"
+        sub_b = pkg_b_dir / ".apm" / "skills" / "topic"
+        sub_b.mkdir(parents=True)
+        (sub_b / "SKILL.md").write_text("# From Org B")
+
+        pkg_a = _make_package_info(pkg_a_dir)
+        pkg_a.dependency_ref = MagicMock()
+        pkg_a.dependency_ref.get_unique_key.return_value = "orga/shared-skill"
+
+        pkg_b = _make_package_info(pkg_b_dir)
+        pkg_b.dependency_ref = MagicMock()
+        pkg_b.dependency_ref.get_unique_key.return_value = "orgb/shared-skill"
+
+        integrator = SkillIntegrator()
+        count_a, _ = integrator._promote_sub_skills_standalone(
+            pkg_a, project_root, targets=[cowork_target]
+        )
+        assert count_a == 1
+        assert (cowork_root / "topic" / "SKILL.md").read_text() == "# From Org A"
+
+        diag = DiagnosticCollector()
+        # Simulate the lockfile recording org A's install from a prior run --
+        # the real _build_skill_ownership_map would return the durable key
+        # "orga/shared-skill" (see _build_ownership_maps), not "shared-skill".
+        with patch.object(
+            SkillIntegrator,
+            "_build_skill_ownership_map",
+            return_value={"topic": "orga/shared-skill"},
+        ):
+            count_b, _ = integrator._promote_sub_skills_standalone(
+                pkg_b,
+                project_root,
+                targets=[cowork_target],
+                diagnostics=diag,
+                force=True,
+            )
+
+        assert count_b == 1
+        # Org B's content must have actually landed (force=True) ...
+        assert (cowork_root / "topic" / "SKILL.md").read_text() == "# From Org B"
+        # ... and the collision must be reported as cross-package, not
+        # silently swallowed as org A "re-installing itself".
+        groups = diag.by_category()
+        assert CATEGORY_OVERWRITE in groups
+        assert any(e.package == "orgb/shared-skill" for e in groups[CATEGORY_OVERWRITE])
+
+    def test_same_leaf_name_different_owner_without_force_is_protected(
+        self, tmp_path: Path
+    ) -> None:
+        """Without --force, org B's colliding sub-skill must be skipped (not
+        silently treated as org A's own content), matching the existing
+        collision-skip behavior for genuinely foreign packages."""
+        cowork_root = tmp_path / "cowork-skills"
+        cowork_root.mkdir()
+        cowork_target = _make_resolved_cowork_target(cowork_root)
+        project_root = tmp_path / "project"
+        project_root.mkdir()
+
+        pkg_a_dir = tmp_path / "orga" / "shared-skill"
+        sub_a = pkg_a_dir / ".apm" / "skills" / "topic"
+        sub_a.mkdir(parents=True)
+        (sub_a / "SKILL.md").write_text("# From Org A")
+
+        pkg_b_dir = tmp_path / "orgb" / "shared-skill"
+        sub_b = pkg_b_dir / ".apm" / "skills" / "topic"
+        sub_b.mkdir(parents=True)
+        (sub_b / "SKILL.md").write_text("# From Org B")
+
+        pkg_a = _make_package_info(pkg_a_dir)
+        pkg_a.dependency_ref = MagicMock()
+        pkg_a.dependency_ref.get_unique_key.return_value = "orga/shared-skill"
+
+        pkg_b = _make_package_info(pkg_b_dir)
+        pkg_b.dependency_ref = MagicMock()
+        pkg_b.dependency_ref.get_unique_key.return_value = "orgb/shared-skill"
+
+        integrator = SkillIntegrator()
+        integrator._promote_sub_skills_standalone(pkg_a, project_root, targets=[cowork_target])
+
+        with patch.object(
+            SkillIntegrator,
+            "_build_skill_ownership_map",
+            return_value={"topic": "orga/shared-skill"},
+        ):
+            # managed_files=set() means nothing is APM-managed from this
+            # run's perspective, exercising the "not managed, protect it"
+            # skip path rather than the cross-package overwrite path.
+            integrator._promote_sub_skills_standalone(
+                pkg_b,
+                project_root,
+                targets=[cowork_target],
+                managed_files=set(),
+                force=False,
+            )
+
+        # Org A's content must survive untouched.
+        assert (cowork_root / "topic" / "SKILL.md").read_text() == "# From Org A"
+
+
 class TestAgentSkillsDedupAndSecurity:
     """Dedup and security tests for the agent-skills target (#737)."""