[fix] warn when two packages deploy a native skill with the same name#545
Conversation
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 microsoft#534
There was a problem hiding this comment.
Pull request overview
This PR addresses a long-standing native-skill overwrite blind spot in SkillIntegrator: when two different dependencies deploy a root SKILL.md with the same leaf directory name, the later install can replace the earlier skill without any warning. The change adds lockfile-based ownership detection to emit an overwrite diagnostic for cross-package collisions, along with new unit tests covering the new behavior.
Changes:
- Added
_build_native_skill_owner_map()to deriveskill_name -> dep.get_unique_key()ownership from the lockfile. - Updated
_integrate_native_skill()to record an overwrite diagnostic (or warning) when a different package previously owned the skill name. - Added unit tests for cross-package collisions vs. self-reinstalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/apm_cli/integration/skill_integrator.py |
Adds native-skill ownership mapping + diagnostic emission on cross-package overwrites. |
tests/unit/integration/test_skill_integrator.py |
Adds tests asserting overwrite diagnostics for cross-package collisions and silence for self-reinstalls. |
- _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.
|
Hi @danielmeppiel -- all 8 Copilot review comments have been addressed in the latest push. Here's a quick summary:
164/164 tests pass. Happy to make any further changes if needed. |
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
Thanks for the solid work here — the ownership maps, session tracking, and tests are well done. A few changes needed before merging:
1. diagnostics.overwrite(package=skill_name) → package=current_key
The package param is used for grouping in render_summary(). Existing pattern in _promote_sub_skills passes the package name (parent_name), not the skill name. This should be current_key so diagnostics group by the package that caused the collision.
2. Warning message fails our "So What?" test
Current:
Skill 'humanizer' from 'Serendeep/dotfiles' replaced existing skill previously provided by 'brandonwise/humanizer'
Every warning must tell the user what to do. Suggested:
Skill 'humanizer' from 'Serendeep/dotfiles' replaced 'brandonwise/humanizer' -- remove one package to avoid the conflict
See .github/skills/cli-logging-ux/SKILL.md for the full convention.
3. (Nit) The _rich_warning fallback may be dead code
All call sites in install.py pass both diagnostics and logger. If this fallback is unreachable, consider removing it rather than carrying untested code. If it is reachable, add a test for it.
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.
|
@danielmeppiel all three points addressed:
175/175 tests pass. |
What
When two distinct packages both contain a native skill (a `SKILL.md` at their root) with the same leaf directory name, `_integrate_native_skill` would silently overwrite the first deployed skill with the second. The user had no indication that content from another package was lost.
Reported in #534: `brandonwise/humanizer` and `Serendeep/dotfiles/.../humanizer` both resolve to `skills/humanizer/`, causing a silent overwrite.
Why
`_integrate_native_skill` had no collision awareness -- it unconditionally did `shutil.rmtree(target_skill_dir)` whenever the destination already existed, regardless of whether the directory belonged to a completely different package.
The existing `_promote_sub_skills` logic already handles this for sub-skills, but the native-skill path had no equivalent guard.
How
Test
7 tests total in `TestNativeSkillIntegration` (5 original + 2 new):
All 164 tests in `test_skill_integrator.py` pass.
Copilot review addressed
All 8 Copilot comments resolved:
5-7. Non-ASCII em dashes replaced with --