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

[nadav-y]

Summary

Fixes two bugs in skill-ownership tracking that could let one package silently overwrite another's skill files, and leave the lockfile with incorrect ownership afterward.

Bug 1 -- ownership identity collision. _build_ownership_maps computed a sub-skill's owner from package_path.name (or dep.repo_url's last path segment) -- the repo/leaf name only, discarding the owner prefix. Two packages sharing a repo name under different owners (e.g. orga/shared-skill vs orgb/shared-skill -- common for generic names like utils, core, shared) computed the same parent_name, so the self-overwrite check (prev_owner == parent_name) wrongly concluded an unrelated package was just re-installing its own prior content. Confirmed live: apm install --force let one package's skill clobber another's with zero warning, for both registry- and git-sourced deps -- a supply-chain-adjacent risk for any package with a common/generic repo name.

Bug 2 -- lockfile double-booking. Even when the collision warning does fire correctly (distinct repo names), the lockfile ends up with both colliding packages claiming deployed_files for the same path -- each package's own integration call truthfully reports "I wrote this path" at the moment it runs, and nothing reconciles the two claims afterward. A future apm uninstall/apm audit on the "losing" package would then act on a file it doesn't actually control.

Fix

1. _build_ownership_maps now keys owned_by on dep.get_unique_key() (matching what native_owners already correctly did). The three call sites that fed parent_name into _promote_sub_skills (_promote_sub_skills_standalone, _integrate_native_skill, _integrate_skill_bundle) now use package_info.dependency_ref.get_unique_key(), with a safe fallback to the old leaf-name behavior when no dependency_ref is available. (fixes Bug 1)

2. New LockfileBuilder._reconcile_cross_package_deployed_files (called from _attach_deployed_files) detects any path claimed by more than one dep_key in ctx.package_deployed_files and keeps it only on the last writer -- the actual on-disk owner under today's sequential "last wins" integration order. (fixes Bug 2)

Test plan

• Live repro against a real JFrog registry (two packages, same repo name, different owners): confirmed the bug (silent clobber, no warning, lockfile double-booked) before the fix; confirmed the warning correctly names the actual colliding package and the lockfile records only the true owner after.
• Live repro against local git-sourced packages (same repro pattern): same result -- confirmed not registry-specific.
• Regression-checked the distinct-repo-name case (no collision) and a normal single-dependency install remain unaffected.
• 1 existing test updated (test_self_entry_caller_guards.py asserted the old, buggy leaf-name value).
• 6 new regression tests: TestSubSkillOwnershipIdentity (2, in test_skill_integrator.py) covering the cross-package-collision-is-flagged and without-force-protected cases, and TestReconcileCrossPackageDeployedFiles (4, new file test_lockfile_cross_package_reconcile.py) covering the reconciliation logic and an end-to-end _attach_deployed_files check that a prior lockfile entry doesn't resurrect the stale claim.
• Full unit suite: 18168 passed, 6 pre-existing failures confirmed identical on main (unrelated to this change), 0 new failures.
• ruff check / ruff format --check: clean.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes skill ownership/collision detection and lockfile correctness by switching sub-skill ownership identity from a leaf directory name to a durable dependency key (owner/repo), and by reconciling cross-package deployed_files collisions so only the last writer owns a path in the final lockfile.

Changes:

• Update skill ownership maps and promotion call sites to key “self-overwrite” detection on dep.get_unique_key() (with safe fallback when dependency_ref is absent).
• Add LockfileBuilder._reconcile_cross_package_deployed_files() and run it before attaching deployed_files, preventing multi-claim lockfile entries after a collision.
• Add/adjust regression tests covering same-leaf-different-owner collisions and cross-package deployed_files reconciliation; add a changelog entry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/apm_cli/integration/skill_integrator.py	Uses durable dep identity for ownership/self-overwrite detection and collision reporting.
src/apm_cli/install/phases/lockfile.py	Reconciles colliding deployed paths so lockfile ownership matches on-disk “last write wins”.
tests/unit/test_self_entry_caller_guards.py	Updates expectation for ownership map values to reflect durable dep identity.
tests/unit/integration/test_skill_integrator.py	Adds regression coverage for same-leaf repo name collisions across different owners.
tests/unit/install/phases/test_lockfile_cross_package_reconcile.py	Adds unit + end-to-end tests for cross-package deployed-file reconciliation.
CHANGELOG.md	Documents the fix under Unreleased.

[nadav-y]

Consolidated into #2053, which now combines this fix with #2050 and the new transitive-semver-recheck fix on a single branch. Please review/merge #2053 instead of this one.

[sergio-sisternes-epam]

APM Review Panel -- Advisory Comment

Note: This is an advisory panel review comment. It is non-blocking. The panel surfaces findings for the maintainer to weigh; merge decision remains with the maintainer.

---

Panel verdict: SHIP

All five mandatory specialist reviews (architecture, CLI/logging, DevX UX, supply-chain security, test coverage) converge on the same conclusion: this is a clean, targeted fix for two confirmed supply-chain-adjacent bugs. No specialist raised a blocker.

---

Summary

Bug 1 -- Ownership identity collision (fixed).
_build_ownership_maps previously derived a skill's owner from the last path segment of dep.virtual_path or dep.repo_url (the short_owner variable). Two packages from different orgs sharing a repo leaf-name (e.g. orga/utils vs orgb/utils) computed the same short_owner, so the self-overwrite guard (prev_owner == parent_name) incorrectly concluded the second package was the first one re-installing itself -- silently suppressing the cross-package collision warning. The fix keys owned_by on dep.get_unique_key(), consistent with what native_owners already correctly did.

Bug 2 -- Lockfile double-booking (fixed).
Each dep's integration call independently and truthfully recorded every path it wrote. After a name collision, both the winning and losing dep claimed the same path in ctx.package_deployed_files. The new LockfileBuilder._reconcile_cross_package_deployed_files pass (called before the union-with-existing step in _attach_deployed_files) strips the collided path from all but the last writer, preserving the on-disk "last wins" semantics in the lockfile record.

---

Specialist findings

Architecture / Python: Fix is minimal and correct. Three call sites (_promote_sub_skills_standalone, _integrate_native_skill, _integrate_skill_bundle) are all updated. The dependency_ref is None fallback to leaf-name preserves backward compatibility for edge cases (self-entry, unresolved deps). No design concerns.

Supply-chain security: This directly closes a supply-chain-adjacent silent-clobber vector: a package with a generic repo name (e.g. core, utils, shared) under one org could previously silently overwrite a different org's skill of the same leaf name on apm install --force, with no warning emitted. The fix correctly makes the collision visible. The lockfile reconciliation prevents the "losing" package from holding a false ownership claim that could drive a destructive apm uninstall later. Both mitigations are necessary; neither alone is sufficient.

CLI / logging UX: Warning messages at the collision site correctly reference the durable unique key (current_key) rather than the skill leaf name. The diagnostic output will now correctly name the colliding package. No regression in existing warning paths.

Test coverage: Coverage is comprehensive for the changed surfaces -- 2 integration tests (collision flagged with --force, protection without --force) and 4 lockfile reconciliation unit tests (last-writer wins, no collision untouched, partial collision strips only shared path, end-to-end through _attach_deployed_files with a prior lockfile). The end-to-end test explicitly guards against stale-resurrection from the existing lockfile union.

DevX UX: No CLI surface changes. Behaviour change is internal to install/integration phase. The user-facing warning message is already correct (previously existed for distinct-leaf-name collisions; now also fires for same-leaf-name-different-owner collisions). No UX regression.

---

Implicit assumption (non-blocking note)

_reconcile_cross_package_deployed_files relies on Python 3.7+ dict insertion-order semantics to correctly identify the last writer. This is valid for CPython 3.7+ (and the project targets 3.10+), but the assumption is implicit. A one-line comment in the method body could make this explicit for future readers. Suggested addition:

# Relies on dict insertion order (Python 3.7+): the final value for each
# key is the last dep_key to claim it, matching sequential install_order.

Not a blocker -- file as a follow-up if desired.

---

CI and lint

All 13 CI checks pass. ruff check, ruff format --check, pylint R0801, and lint-auth-signals.sh all clean locally and in CI.

---

Panel recommendation: APPROVE and merge.

[sergio-sisternes-epam]

Panel review complete. All checks clean. Approving for merge.

@nadav-y please only fix the conflicts on the CHANGELOG and I'll re-approve.