fix: skill integration bugs, transitive dep cleanup, and skill_integrator simplification#107
Merged
danielmeppiel merged 1 commit intomainfrom Feb 26, 2026
Merged
Conversation
2b9ca27 to
bcbb71e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates APM’s dependency installation/uninstall and integration flows to support native skills and virtual subdirectory package layouts (e.g. owner/repo/skills/foo), including improved lockfile usage and updated docs/tests.
Changes:
- Adds npm-style transitive dependency pruning during
apm uninstall, including lockfile updates and dry-run visibility. - Adjusts skill integration to focus on native
SKILL.mdpackages and promotes.apm/skills/*sub-skills to top-level skills. - Updates
apm depslist/tree to account for subdirectory packages and show per-primitive counts; refreshes related docs/tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/cli.py |
Implements transitive uninstall pruning + lockfile updates; improves cached install/integration flow. |
src/apm_cli/commands/deps.py |
Enhances deps list/tree with lockfile-aware orphan detection and primitive counts. |
src/apm_cli/integration/skill_integrator.py |
Integrates native skills and promotes sub-skills; tracks sub_skills_promoted. |
src/apm_cli/integration/agent_integrator.py |
Adds dual-target integration/cleanup and gitignore patterns for .claude/agents/. |
src/apm_cli/integration/prompt_integrator.py |
Refactors cleanup + gitignore writing logic for integrated prompts. |
src/apm_cli/deps/github_downloader.py |
Captures resolved commit SHA for subdirectory downloads. |
tests/unit/test_uninstall_transitive_cleanup.py |
Adds unit tests for uninstall transitive pruning + lockfile behavior. |
tests/unit/integration/test_skill_integrator.py |
Updates skill integrator tests; adds coverage for sub-skill promotion from non-skill packages. |
tests/unit/integration/test_agent_integrator.py |
Updates gitignore test expectations for .claude/agents/ patterns. |
tests/integration/test_apm_dependencies.py |
Tweaks integration assertion order for subdirectory package installs. |
docs/cli-reference.md |
Documents uninstall behavior for transitive deps + lockfile updates; updates deps list/tree docs. |
docs/dependencies.md |
Updates skills behavior and documents uninstall transitive pruning. |
docs/integrations.md |
Updates skill integration description (native-only, integrated skills). |
docs/skills.md |
Clarifies SKILL.md-only packages are treated as native skills. |
Comments suppressed due to low confidence (3)
src/apm_cli/commands/deps.py:108
- When populating
declared_sourcesfrom the lockfile, the code assigns'github'unconditionally. This mislabels Azure DevOps dependencies (which can appear inapm.lockwithhostset) and can cause incorrectSourcevalues inapm deps list. Use the locked dependency's host (or the same hostname check used byDependencyReference.is_azure_devops()) to set'azure-devops'vs'github'instead of hardcoding'github'.
for dep in lockfile.dependencies.values():
# Lockfile keys match declared_sources format (owner/repo)
dep_key = dep.get_unique_key()
if dep_key and dep_key not in declared_sources:
declared_sources[dep_key] = 'github'
except Exception:
src/apm_cli/commands/deps.py:351
- In the no-lockfile fallback scan for
apm deps tree, directories under.apm/(e.g.,.apm/skills/<sub-skill>/) can containSKILL.mdand will be incorrectly treated as top-level installed packages. Mirror the skip logic used indeps list(ignore candidates whose relative path parts include.apm) so sub-skills/internal package metadata don’t show up as separate dependencies.
for candidate in sorted(apm_modules_path.rglob("*")):
if not candidate.is_dir() or candidate.name.startswith('.'):
continue
has_apm = (candidate / "apm.yml").exists()
has_skill = (candidate / "SKILL.md").exists()
if not has_apm and not has_skill:
continue
tests/unit/test_uninstall_transitive_cleanup.py:94
- The test name/docstring says the shared transitive dependency is not removed, but the assertions and inline comments below expect it to be removed. Rename the test and/or update the docstring to match the actual expected behavior so the intent is unambiguous.
def test_uninstall_keeps_shared_transitive_dep(self):
"""Transitive dep used by another remaining package is NOT removed."""
with tempfile.TemporaryDirectory() as tmp_dir:
bcbb71e to
467633f
Compare
SebastienDegodez
approved these changes
Feb 26, 2026
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.
Bug fixes:
package_type=Nonein cached install path — ALL skill integration was silently skipped)resolved_commitin lockfile for callback-downloaded transitive deps.claude/prompts/dual-targeting fromPromptIntegrator(Claude Code uses.claude/commands/, already handled byCommandIntegrator)deps treeshowing@latestinstead of actual resolved referenceFeatures:
apm uninstallnow recursively removes orphaned transitive dependencies and updates lockfile (npm-style cleanup)apm uninstall --dry-runshows transitive deps that would be removedSimplification:
skill_integrator.py(~300 lines removed). Only native SKILL.md files shipped by packages are promoted. Packages without SKILL.md are compiled to AGENTS.md/CLAUDE.md only.Changed files
src/apm_cli/cli.py— P0 fix (package_type detection from disk), P2 fix (resolved_commit capture via callback dict), transitive dep cleanup on uninstallsrc/apm_cli/commands/deps.py— Better version display indeps tree(resolved_commit > resolved_ref > version > "latest")src/apm_cli/integration/skill_integrator.py— Removed SKILL.md generation from primitives, only promote native SKILL.md + sub-skillssrc/apm_cli/integration/prompt_integrator.py— Removed incorrect.claude/prompts/dual-targetingsrc/apm_cli/deps/github_downloader.py— Minor fixes for subdirectory package handling