diff --git a/CHANGELOG.md b/CHANGELOG.md index 381a02947..134ae96aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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) (#2052) - Codex MCP installs and stale-server cleanup now preserve literal-quoted Windows path keys such as `C:\Users\me\Documents\Playground` in `config.toml` instead of rejecting or corrupting other projects and per-path preferences. diff --git a/src/apm_cli/install/phases/lockfile.py b/src/apm_cli/install/phases/lockfile.py index 53eac6eec..0ae4843a7 100644 --- a/src/apm_cli/install/phases/lockfile.py +++ b/src/apm_cli/install/phases/lockfile.py @@ -178,6 +178,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. @@ -190,6 +218,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, [])) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index d327815b8..fc59b03d0 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/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 diff --git a/tests/unit/install/phases/test_lockfile_cross_package_reconcile.py b/tests/unit/install/phases/test_lockfile_cross_package_reconcile.py new file mode 100644 index 000000000..d4aa6271c --- /dev/null +++ b/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 diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index 1a2e645fb..0e044b6b8 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/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).""" diff --git a/tests/unit/test_self_entry_caller_guards.py b/tests/unit/test_self_entry_caller_guards.py index 392533890..e4d30e8d2 100644 --- a/tests/unit/test_self_entry_caller_guards.py +++ b/tests/unit/test_self_entry_caller_guards.py @@ -103,7 +103,7 @@ def test_ownership_map_excludes_self_entry(self, tmp_path): """The self-entry's local skills must not pollute the package-owner map. The self-entry has repo_url='' / virtual_path=None which would - give a bogus short_owner of ''. It must be filtered out. + give a bogus owner key of ''. It must be filtered out. """ lock = _lockfile_with_self_and_remote() # The self-entry has a deployed skill file that would otherwise land @@ -121,5 +121,8 @@ def test_ownership_map_excludes_self_entry(self, tmp_path): # Self-entry's owner string would have been '' if not skipped. assert "" not in owned_by.values() assert "" not in native_owners.values() - # The remote dep's skill leaf-name is 'SKILL.md' (last path segment). - assert owned_by.get("SKILL.md") == "repo" + # Owner keys are the full unique dependency identity (owner/repo), + # not the last path segment -- two different packages can share a + # repo/leaf name, and comparing only the leaf would falsely treat + # them as the same owner (see _build_ownership_maps). + assert owned_by.get("SKILL.md") == "owner/repo"