fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content#1416
Open
abi-jey wants to merge 2 commits into
Open
fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content#1416abi-jey wants to merge 2 commits into
abi-jey wants to merge 2 commits into
Conversation
01e1293 to
a8845bf
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a destructive edge case in plugin artifact normalization during apm install when plugin.json component paths point into an already-populated .apm/ tree, preventing accidental deletion of pre-positioned agents/skills/prompts/hooks.
Changes:
- Added a containment guard in
_map_plugin_artifacts()to skip thermtree + copycycle when sources appear to already live under.apm/. - Added regression tests ensuring
.apm/content is preserved for agents, skills, commands (prompts), and hooks. - Added a non-regression test confirming root-level
agents/are still copied into.apm/agents/.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/apm_cli/deps/plugin_parser.py | Adds _all_inside_apm() and uses it to conditionally skip destructive remapping for agents/skills/commands/hooks. |
| tests/unit/test_plugin_parser.py | Adds regression coverage for pre-positioned .apm/ artifact preservation and one non-regression copy test. |
a8845bf to
6cabc1f
Compare
…oft#1415) The `rmtree(target); copytree(source, target)` cycle wiped the source when a hybrid APM + Claude-plugin manifest pointed paths INTO `.apm/`, causing `apm install` to silently report `(files unchanged)`. The rmtree was redundant -- the downloader already clears install_path before extraction. Replaced with `dirs_exist_ok=True` plus an `_is_same_path` guard for `copy2` self-copies. Tests: 6 regression cases (pre-positioned + mixed-source layouts).
6cabc1f to
0cf8057
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1415
When a package has both
apm.ymland.claude-plugin/plugin.jsonwith manifest paths pointing into.apm/,apm installsilently drops all agents and skills — reporting(files unchanged).Root Cause
detect_package_typeclassifies packages with.claude-plugin/plugin.jsonasMARKETPLACE_PLUGIN(cascade priority — checked beforeAPM_PACKAGE), even whenapm.ymlis present. This triggers_validate_marketplace_plugin→normalize_plugin_directory→_map_plugin_artifacts._map_plugin_artifactsis designed for pure plugins whereagents/andskills/sit at the package root and need to be copied INTO.apm/. It does ashutil.rmtreeon the target (.apm/agents/,.apm/skills/) before copying from the source. When the manifest points to paths already inside.apm/, the source and target overlap — the rmtree destroys the source before the copy can read it.Sequence:
apm_modules/—.apm/agents/and.apm/skills/are presentvalidate_apm_package→_map_plugin_artifactsrunsshutil.rmtree(.apm/agents/)— deletes the sourceshutil.copy2(source_file, ...)— source no longer exists, nothing is copied.apm/→(files unchanged)Fix
Added
_all_inside_apm()helper that detects when all resolved source paths already reside under.apm/. When true, the destructive rmtree+copy cycle is skipped — the artifacts are already correctly positioned. Applied to all four component types: agents, skills, commands, hooks.Tests
5 new regression tests in
TestMapPluginArtifactsPrePositioned:test_agents_inside_apm_are_preserved— manifest agents pointing into.apm/survivetest_skills_inside_apm_are_preserved— manifest skills pointing into.apm/survivetest_commands_inside_apm_are_preserved— manifest commands pointing into.apm/survivetest_hooks_inside_apm_are_preserved— manifest hooks pointing into.apm/survivetest_external_agents_still_copied— normal root-level→.apm/copy still works (non-regression)All 4 regression tests fail without the fix, pass with it. All 73 plugin parser tests pass.