From 77cba38d54ac454b75b4fc05c4e67d8de68c0810 Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Thu, 2 Apr 2026 22:11:45 +0800 Subject: [PATCH 1/3] fix: warn on cross-package native skill name collision When two distinct packages both deploy a native skill with the same leaf directory name (e.g. brandonwise/humanizer and Serendeep/dotfiles/.../humanizer), the second install silently overwrote the first via shutil.rmtree + shutil.copytree. Add _build_native_skill_owner_map() that stores the full dep.get_unique_key() per deployed skill name. In _integrate_native_skill(), compare the incoming package's unique key against the stored owner before overwriting. If a different package owns the skill, a DiagnosticCollector.overwrite() entry is recorded (or a logger/console warning emitted) so the user is aware of the conflict. Self-reinstalls are detected correctly and stay silent. Fixes #534 --- src/apm_cli/integration/skill_integrator.py | 62 ++++++++++++ .../unit/integration/test_skill_integrator.py | 94 +++++++++++++++++++ 2 files changed, 156 insertions(+) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index d09664fc3..e5f12a14c 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -571,6 +571,30 @@ def _build_skill_ownership_map(project_root: Path) -> dict[str, str]: owned_by[skill_name] = owner 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. + + Unlike ``_build_skill_ownership_map`` (which stores only the last path + segment for sub-skill self-overwrite detection), this map stores the + full canonical unique key of the owning dependency. That makes it safe + to distinguish two distinct packages whose names share the same leaf + segment (e.g. ``brandonwise/humanizer`` vs + ``Serendeep/dotfiles/.../humanizer``). + """ + from apm_cli.deps.lockfile import LockFile, get_lockfile_path + + owners: dict[str, str] = {} + lockfile = LockFile.read(get_lockfile_path(project_root)) + if not lockfile: + return owners + for dep in lockfile.get_all_dependencies(): + for deployed_path in dep.deployed_files: + # e.g. ".github/skills/context-map/" -> "context-map" + skill_name = deployed_path.rstrip("/").rsplit("/", 1)[-1] + owners[skill_name] = dep.get_unique_key() + return owners + def _promote_sub_skills_standalone( self, package_info, project_root: Path, diagnostics=None, managed_files=None, force: bool = False, logger=None, @@ -708,6 +732,17 @@ def _integrate_native_skill( owned_by = self._build_skill_ownership_map(project_root) sub_skills_dir = package_path / ".apm" / "skills" + # Build a full-key ownership map (skill_name -> dep.get_unique_key()) for + # accurate cross-package collision detection on native skills. The coarser + # map used by _promote_sub_skills only stores the last path segment, which + # is ambiguous when two distinct packages share the same leaf name (e.g. + # brandonwise/humanizer vs Serendeep/dotfiles/.../humanizer — issue #534). + native_skill_owners = self._build_native_skill_owner_map(project_root) + + # 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 @@ -723,6 +758,33 @@ def _integrate_native_skill( primary_skill_md = target_skill_dir / "SKILL.md" if target_skill_dir.exists(): + if is_primary: + prev_owner = native_skill_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}" + if diagnostics is not None: + diagnostics.overwrite( + path=rel_path, + package=skill_name, + detail=f"Skill '{skill_name}' replaced -- previously from another package", + ) + elif logger: + logger.warning( + f"Skill '{skill_name}' overwritten by a package from a different source" + ) + else: + try: + from apm_cli.utils.console import _rich_warning + _rich_warning( + f"Skill '{skill_name}' overwritten by a package from a different source" + ) + except ImportError: + pass shutil.rmtree(target_skill_dir) target_skill_dir.parent.mkdir(parents=True, exist_ok=True) diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index fbf62d603..70495bcc2 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -1504,6 +1504,100 @@ 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" + # ============================================================================= # T7: Claude Skills Compatibility Copy Tests From c8ba7681773ff377a3c6d4568cc0717374d088f9 Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Fri, 3 Apr 2026 10:31:41 +0800 Subject: [PATCH 2/3] fix: address Copilot review feedback on skill collision detection - _build_ownership_maps(): read lockfile once and derive both maps in a single pass, eliminating the double parse. Filter native-skill entries to paths containing '/skills/' so non-skill deployed_files (prompts, hooks, commands) cannot produce false ownership attribution. _build_skill_ownership_map and _build_native_skill_owner_map are now thin wrappers around this shared method. - SkillIntegrator.__init__: add _native_skill_session_owners dict updated after each native skill deployment. The collision check in _integrate_native_skill now consults both the lockfile map and the session map, so same-manifest collisions are caught on the first install even before the lockfile has been written for the current run. - Improve diagnostic/warning messages to include both the incoming package key and the previous owner so users can identify which dependencies conflict. - Replace non-ASCII em dashes in added comments with '--' to avoid Windows cp1252 UnicodeEncodeError. - Update test_self_overwrite_silent_no_diagnostic to patch _build_ownership_maps (the new single entry point) instead of the now-thin _build_skill_ownership_map wrapper. - Add test_native_skill_collision_via_real_lockfile: exercises the full collision path using an actual apm.lock.yaml on disk, without patching any private methods. - Add test_native_skill_same_run_collision_without_lockfile: verifies that same-run collisions are detected via session tracking even when no lockfile exists yet. --- src/apm_cli/integration/skill_integrator.py | 108 +++++++++------- .../unit/integration/test_skill_integrator.py | 119 +++++++++++++++++- 2 files changed, 177 insertions(+), 50 deletions(-) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index fbad65a3e..528e0dd9d 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,49 +566,54 @@ 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. - Unlike ``_build_skill_ownership_map`` (which stores only the last path - segment for sub-skill self-overwrite detection), this map stores the - full canonical unique key of the owning dependency. That makes it safe - to distinguish two distinct packages whose names share the same leaf - segment (e.g. ``brandonwise/humanizer`` vs - ``Serendeep/dotfiles/.../humanizer``). + Scoped to ``/skills/`` paths only -- see ``_build_ownership_maps`` for details. """ - from apm_cli.deps.lockfile import LockFile, get_lockfile_path - - owners: dict[str, str] = {} - lockfile = LockFile.read(get_lockfile_path(project_root)) - if not lockfile: - return owners - for dep in lockfile.get_all_dependencies(): - for deployed_path in dep.deployed_files: - # e.g. ".github/skills/context-map/" -> "context-map" - skill_name = deployed_path.rstrip("/").rsplit("/", 1)[-1] - owners[skill_name] = dep.get_unique_key() - return owners + _, native_owners = SkillIntegrator._build_ownership_maps(project_root) + return native_owners def _promote_sub_skills_standalone( self, package_info, project_root: Path, diagnostics=None, @@ -738,16 +749,10 @@ 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" - # Build a full-key ownership map (skill_name -> dep.get_unique_key()) for - # accurate cross-package collision detection on native skills. The coarser - # map used by _promote_sub_skills only stores the last path segment, which - # is ambiguous when two distinct packages share the same leaf name (e.g. - # brandonwise/humanizer vs Serendeep/dotfiles/.../humanizer — issue #534). - native_skill_owners = self._build_native_skill_owner_map(project_root) - # 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 @@ -768,7 +773,13 @@ def _integrate_native_skill( if target_skill_dir.exists(): if is_primary: - prev_owner = native_skill_owners.get(skill_name) + # 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: @@ -776,22 +787,22 @@ def _integrate_native_skill( except ValueError: rel_prefix = "skills" rel_path = f"{rel_prefix}/{skill_name}" + detail = ( + f"Skill '{skill_name}' from '{current_key}' " + f"replaced existing skill previously provided by '{prev_owner}'" + ) if diagnostics is not None: diagnostics.overwrite( path=rel_path, package=skill_name, - detail=f"Skill '{skill_name}' replaced -- previously from another package", + detail=detail, ) elif logger: - logger.warning( - f"Skill '{skill_name}' overwritten by a package from a different source" - ) + logger.warning(detail) else: try: from apm_cli.utils.console import _rich_warning - _rich_warning( - f"Skill '{skill_name}' overwritten by a package from a different source" - ) + _rich_warning(detail) except ImportError: pass shutil.rmtree(target_skill_dir) @@ -818,6 +829,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 e35bce873..eddb806c4 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -1529,7 +1529,7 @@ def test_native_skill_cross_package_collision_records_diagnostic(self): dependency_ref=dep_ref_a, ) - # Install first package — no existing skill, no warning expected. + # 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() @@ -1595,9 +1595,117 @@ def test_native_skill_self_reinstall_no_diagnostic(self): 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. + # 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]) + # ============================================================================= # T7: Claude Skills Compatibility Copy Tests @@ -2707,13 +2815,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 From 75b97c4a1d342a7828ceafbdd9a176e7edc9ab5e Mon Sep 17 00:00:00 2001 From: xuyuanhao Date: Fri, 3 Apr 2026 20:29:12 +0800 Subject: [PATCH 3/3] fix: address danielmeppiel review feedback 1. diagnostics.overwrite(package=current_key) -- previously package=skill_name was passed, causing render_summary() to group by skill name instead of the package responsible for the collision. Changed to current_key || skill_name so diagnostics group correctly by the incoming package. 2. Actionable warning message ("So What?" test) -- changed from "replaced existing skill previously provided by X" to "replaced 'X' -- remove one package to avoid this" per cli-logging-ux SKILL.md convention that every warning must tell the user what to do. 3. _rich_warning fallback is reachable (uninstall/engine.py passes neither diagnostics nor logger), so keep it and add a test: - test_native_skill_collision_falls_back_to_rich_warning: verifies the fallback path emits a warning containing the skill name and action hint. - test_native_skill_collision_diagnostic_package_is_current_key: verifies diagnostics.overwrite() receives package=current_key, not skill_name. --- src/apm_cli/integration/skill_integrator.py | 18 +++-- .../unit/integration/test_skill_integrator.py | 78 +++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index c983e27d2..9525f099e 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -787,24 +787,26 @@ def _integrate_native_skill( 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}' " - f"replaced existing skill previously provided by '{prev_owner}'" + 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=skill_name, + package=current_key or skill_name, detail=detail, ) elif logger: logger.warning(detail) else: - try: - from apm_cli.utils.console import _rich_warning - _rich_warning(detail) - except ImportError: - pass + # 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) diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index eddb806c4..4643bc7ed 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -1706,6 +1706,84 @@ def test_native_skill_same_run_collision_without_lockfile(self): 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