diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index e51bd4b3f..9525f099e 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/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( diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index 3ac3a5548..4643bc7ed 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -1504,6 +1504,286 @@ def test_native_skill_files_copied_count(self): assert result.skill_created is True assert result.references_copied == 4 # All 4 files + def test_native_skill_cross_package_collision_records_diagnostic(self): + """Two distinct packages that both deploy a same-named skill should warn on the second install. + + Reproduces issue #534: brandonwise/humanizer and Serendeep/dotfiles/.../humanizer + both claim the 'humanizer' skill directory. The second install used to silently + overwrite the first. After the fix a diagnostic is recorded instead. + """ + from apm_cli.utils.diagnostics import DiagnosticCollector, CATEGORY_OVERWRITE + from unittest.mock import patch + + # --- First package: standalone humanizer skill --- + # The install path ends in "humanizer" so skill_name == "humanizer". + pkg_a_dir = self.project_root / "brandonwise" / "humanizer" + pkg_a_dir.mkdir(parents=True) + (pkg_a_dir / "SKILL.md").write_text( + "---\nname: humanizer\ndescription: Humanize LLM output\n---\n# Humanizer\n" + ) + + dep_ref_a = DependencyReference(repo_url="brandonwise/humanizer") + pkg_a = self._create_package_info( + name="humanizer", + install_path=pkg_a_dir, + dependency_ref=dep_ref_a, + ) + + # Install first package -- no existing skill, no warning expected. + self.integrator.integrate_package_skill(pkg_a, self.project_root) + assert (self.project_root / ".github" / "skills" / "humanizer" / "SKILL.md").exists() + + # --- Second package: virtual skill inside a dotfiles repo --- + # Also ends in "humanizer" so it would deploy to the same skills/humanizer dir. + pkg_b_dir = self.project_root / "Serendeep" / "dotfiles" / "claude" / ".claude" / "skills" / "humanizer" + pkg_b_dir.mkdir(parents=True) + (pkg_b_dir / "SKILL.md").write_text( + "---\nname: humanizer\ndescription: Different humanizer\n---\n# Humanizer v2\n" + ) + + dep_ref_b = DependencyReference( + repo_url="Serendeep/dotfiles", + virtual_path="claude/.claude/skills/humanizer", + is_virtual=True, + ) + pkg_b = self._create_package_info( + name="humanizer", + install_path=pkg_b_dir, + dependency_ref=dep_ref_b, + ) + + # Mock the native skill owner map to return pkg_a's unique key as prev owner. + owner_map = {"humanizer": dep_ref_a.get_unique_key()} # "brandonwise/humanizer" + diag = DiagnosticCollector() + + with patch.object(SkillIntegrator, "_build_native_skill_owner_map", return_value=owner_map): + self.integrator.integrate_package_skill(pkg_b, self.project_root, diagnostics=diag) + + # The overwrite should have been recorded as a diagnostic. + assert diag.has_diagnostics, "Expected an overwrite diagnostic but none were recorded" + groups = diag.by_category() + assert CATEGORY_OVERWRITE in groups + assert any("humanizer" in d.message for d in groups[CATEGORY_OVERWRITE]) + + # The skill directory should still be updated (overwrite proceeds after warning). + content = (self.project_root / ".github" / "skills" / "humanizer" / "SKILL.md").read_text() + assert "Humanizer v2" in content + + def test_native_skill_self_reinstall_no_diagnostic(self): + """Reinstalling the same native skill package should NOT emit a collision warning.""" + from apm_cli.utils.diagnostics import DiagnosticCollector + from unittest.mock import patch + + pkg_dir = self.project_root / "my-skill" + pkg_dir.mkdir() + (pkg_dir / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill\n") + + dep_ref = DependencyReference(repo_url="owner/my-skill") + pkg = self._create_package_info( + name="my-skill", + install_path=pkg_dir, + dependency_ref=dep_ref, + ) + + # First install + self.integrator.integrate_package_skill(pkg, self.project_root) + + # Simulate lockfile recording ownership as the same unique key. + owner_map = {"my-skill": dep_ref.get_unique_key()} # "owner/my-skill" + diag = DiagnosticCollector() + + with patch.object(SkillIntegrator, "_build_native_skill_owner_map", return_value=owner_map): + self.integrator.integrate_package_skill(pkg, self.project_root, diagnostics=diag) + + # Self-reinstall -- no overwrite diagnostic should be recorded. + assert not diag.has_diagnostics, "Self-reinstall should not produce a collision diagnostic" + + def test_native_skill_collision_via_real_lockfile(self): + """Collision detection works from actual lockfile data (no internal mocking). + + Writes an apm.lock.yaml with brandonwise/humanizer having deployed + .github/skills/humanizer/, then installs a second distinct package that + would claim the same skill name. Verifies that an overwrite diagnostic is + recorded without patching any private method. + """ + from apm_cli.utils.diagnostics import DiagnosticCollector, CATEGORY_OVERWRITE + from apm_cli.deps.lockfile import LockFile, LockedDependency, get_lockfile_path + + # Write a lockfile that records brandonwise/humanizer as the owner. + lockfile = LockFile() + lockfile.add_dependency(LockedDependency( + repo_url="brandonwise/humanizer", + resolved_commit="abc123", + deployed_files=[ + ".github/skills/humanizer/", + ".claude/skills/humanizer/", + ], + )) + lockfile_path = get_lockfile_path(self.project_root) + lockfile_path.write_text(lockfile.to_yaml()) + + # Deploy the existing skill directory so there is something to overwrite. + existing = self.project_root / ".github" / "skills" / "humanizer" + existing.mkdir(parents=True) + (existing / "SKILL.md").write_text("---\nname: humanizer\n---\n# Original\n") + + # Second package: a virtual skill from a dotfiles repo with the same leaf name. + # The install path MUST end in "humanizer" because skill_name = package_path.name. + pkg_b_dir = self.project_root / "Serendeep" / "dotfiles" / "claude" / ".claude" / "skills" / "humanizer" + pkg_b_dir.mkdir(parents=True) + (pkg_b_dir / "SKILL.md").write_text("---\nname: humanizer\n---\n# Fork\n") + + dep_ref_b = DependencyReference( + repo_url="Serendeep/dotfiles", + virtual_path="claude/.claude/skills/humanizer", + is_virtual=True, + ) + pkg_b = self._create_package_info( + name="humanizer", + install_path=pkg_b_dir, + dependency_ref=dep_ref_b, + ) + + diag = DiagnosticCollector() + self.integrator.integrate_package_skill(pkg_b, self.project_root, diagnostics=diag) + + # An overwrite diagnostic must be recorded because the previous owner + # (brandonwise/humanizer) differs from the incoming package. + assert diag.has_diagnostics, "Expected overwrite diagnostic from real lockfile" + groups = diag.by_category() + assert CATEGORY_OVERWRITE in groups + assert any("humanizer" in d.message for d in groups[CATEGORY_OVERWRITE]) + + def test_native_skill_same_run_collision_without_lockfile(self): + """Within a single install run, the second package colliding on a skill name is + detected via the in-memory session map even when no lockfile exists yet. + """ + from apm_cli.utils.diagnostics import DiagnosticCollector, CATEGORY_OVERWRITE + + # No lockfile present -- fresh repo. + + # Package A: installs 'humanizer' skill first. + pkg_a_dir = self.project_root / "brandonwise" / "humanizer" + pkg_a_dir.mkdir(parents=True) + (pkg_a_dir / "SKILL.md").write_text("---\nname: humanizer\n---\n# A\n") + + dep_ref_a = DependencyReference(repo_url="brandonwise/humanizer") + pkg_a = self._create_package_info( + name="humanizer", + install_path=pkg_a_dir, + dependency_ref=dep_ref_a, + ) + + diag_a = DiagnosticCollector() + self.integrator.integrate_package_skill(pkg_a, self.project_root, diagnostics=diag_a) + + # No diagnostic for the first install. + assert not diag_a.has_diagnostics + + # Package B: different package, same skill name, same integrator instance. + # Install path must also end in "humanizer" for skill_name to match. + pkg_b_dir = self.project_root / "Serendeep" / "dotfiles" / "claude" / ".claude" / "skills" / "humanizer" + pkg_b_dir.mkdir(parents=True) + (pkg_b_dir / "SKILL.md").write_text("---\nname: humanizer\n---\n# B\n") + + dep_ref_b = DependencyReference( + repo_url="Serendeep/dotfiles", + virtual_path="claude/.claude/skills/humanizer", + is_virtual=True, + ) + pkg_b = self._create_package_info( + name="humanizer", + install_path=pkg_b_dir, + dependency_ref=dep_ref_b, + ) + + diag_b = DiagnosticCollector() + self.integrator.integrate_package_skill(pkg_b, self.project_root, diagnostics=diag_b) + + # The second install should trigger a collision diagnostic via session tracking. + assert diag_b.has_diagnostics, "Same-run collision not detected without lockfile" + groups = diag_b.by_category() + assert CATEGORY_OVERWRITE in groups + assert any("humanizer" in d.message for d in groups[CATEGORY_OVERWRITE]) + + def test_native_skill_collision_falls_back_to_rich_warning(self): + """When called without diagnostics or logger (e.g. uninstall sync), the + _rich_warning fallback is used for cross-package collisions. + """ + from unittest.mock import patch + from apm_cli.deps.lockfile import LockFile, LockedDependency, get_lockfile_path + + # Write a lockfile recording a previous owner. + lockfile = LockFile() + lockfile.add_dependency(LockedDependency( + repo_url="brandonwise/humanizer", + resolved_commit="abc123", + deployed_files=[".github/skills/humanizer/"], + )) + get_lockfile_path(self.project_root).write_text(lockfile.to_yaml()) + + existing = self.project_root / ".github" / "skills" / "humanizer" + existing.mkdir(parents=True) + (existing / "SKILL.md").write_text("---\nname: humanizer\n---\n# Original\n") + + pkg_dir = self.project_root / "Serendeep" / "humanizer" + pkg_dir.mkdir(parents=True) + (pkg_dir / "SKILL.md").write_text("---\nname: humanizer\n---\n# Fork\n") + + dep_ref = DependencyReference(repo_url="Serendeep/humanizer") + pkg = self._create_package_info( + name="humanizer", + install_path=pkg_dir, + dependency_ref=dep_ref, + ) + + with patch("apm_cli.utils.console._rich_warning") as mock_warn: + # No diagnostics, no logger -- triggers _rich_warning fallback. + self.integrator.integrate_package_skill(pkg, self.project_root) + + mock_warn.assert_called_once() + msg = mock_warn.call_args[0][0] + assert "humanizer" in msg + assert "remove one package" in msg + + def test_native_skill_collision_diagnostic_package_is_current_key(self): + """diagnostics.overwrite() must receive package=current_key (not skill_name) + so render_summary() groups by the package that caused the collision. + """ + from apm_cli.utils.diagnostics import DiagnosticCollector, CATEGORY_OVERWRITE + from unittest.mock import patch + + existing = self.project_root / ".github" / "skills" / "humanizer" + existing.mkdir(parents=True) + (existing / "SKILL.md").write_text("---\nname: humanizer\n---\n# Original\n") + + pkg_dir = self.project_root / "Serendeep" / "humanizer" + pkg_dir.mkdir(parents=True) + (pkg_dir / "SKILL.md").write_text("---\nname: humanizer\n---\n# Fork\n") + + dep_ref = DependencyReference(repo_url="Serendeep/humanizer") + pkg = self._create_package_info( + name="humanizer", + install_path=pkg_dir, + dependency_ref=dep_ref, + ) + + diag = DiagnosticCollector() + + # Patch _build_ownership_maps (the single entry point) to inject prev ownership. + with patch.object(SkillIntegrator, "_build_ownership_maps", + return_value=({}, {"humanizer": "brandonwise/humanizer"})): + self.integrator.integrate_package_skill(pkg, self.project_root, diagnostics=diag) + + groups = diag.by_category() + assert CATEGORY_OVERWRITE in groups + entries = groups[CATEGORY_OVERWRITE] + # The package field must be the current package's unique key, not the skill name. + assert all(e.package != "humanizer" for e in entries), ( + "diagnostics.overwrite() was called with package=skill_name instead of package=current_key" + ) + assert any(e.package == "Serendeep/humanizer" for e in entries) + # ============================================================================= # T7: Claude Skills Compatibility Copy Tests @@ -2613,13 +2893,16 @@ def test_self_overwrite_silent_no_diagnostic(self): from apm_cli.utils.diagnostics import DiagnosticCollector diag = DiagnosticCollector() - with patch.object(SkillIntegrator, '_build_skill_ownership_map', return_value={"my-sub": "my-pkg"}): + # Patch _build_ownership_maps (the single lockfile-read entry point) to return + # ownership for both the sub-skill map and the native-owner map. + with patch.object(SkillIntegrator, '_build_ownership_maps', + return_value=({"my-sub": "my-pkg"}, {})): self.integrator.integrate_package_skill( pkg_info, self.project_root, diagnostics=diag, managed_files=managed_files, force=False, ) - # Self-overwrite — no diagnostics should be recorded + # Self-overwrite -- no diagnostics should be recorded assert not diag.has_diagnostics # Content should be updated