fix(pack): select plugin sources by .apm presence#2122
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes apm pack so that once a manifest declares includes: (e.g., includes: auto), packing no longer “discovers” and publishes plugin-shaped root directories like skills/; root convention discovery is preserved only for legacy manifests that omit includes:. It also adds an end-to-end regression test and updates docs/changelog to describe the clarified source-layout boundary.
Changes:
- Gate root plugin convention directory collection in the plugin exporter on
package.includes is None(legacy-only behavior). - Add an integration e2e regression test that packs a temp project containing both
.apm/skills/(publishable) and rootskills/(should be excluded whenincludesis declared). - Update CLI/docs guidance and add a changelog entry describing the behavior change.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/bundle/plugin_exporter.py |
Stops collecting root plugin convention directories when includes: is declared, keeping root discovery only for legacy manifests. |
tests/integration/test_pack_root_skills_e2e.py |
New hermetic e2e test asserting .apm/skills/ is packed while root skills/ is excluded under includes: auto. |
tests/integration/test_intra_package_cleanup.py |
Updates an ownership precondition assertion to match current last-writer ownership semantics. |
docs/src/content/docs/reference/cli/pack.md |
Documents the includes-vs-legacy root discovery boundary for apm pack plugin bundles. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Aligns the bundled usage guide with the same .apm/<type>/ guidance and legacy behavior notes. |
CHANGELOG.md |
Adds an Unreleased “Fixed” entry for the packing behavior change. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Low
| assert [dep["name"] for dep in shared_claims] == ["pkg-b"], ( | ||
| "The last package to deploy the shared prompt must own it before cleanup" | ||
| ) |
| - `apm pack` with a declared `includes:` value no longer publishes local | ||
| root-level plugin convention directories such as `skills/`. (closes #2054, | ||
| #2122) |
| import subprocess | ||
| import sys | ||
| from pathlib import Path |
| def test_pack_auto_includes_only_apm_authored_skills(tmp_path: Path) -> None: | ||
| """The real pack command must not treat a root skills directory as publishable.""" | ||
| project = tmp_path / "project" | ||
| project.mkdir() | ||
| (project / "apm.yml").write_text( | ||
| "name: root-skills-test\n" | ||
| "version: 1.0.0\n" | ||
| "description: Root skills regression\n" | ||
| "license: MIT\n" | ||
| "target: claude\n" | ||
| "includes: auto\n" | ||
| "dependencies:\n" | ||
| " apm: []\n", | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| authored_skill = project / ".apm" / "skills" / "published" | ||
| authored_skill.mkdir(parents=True) | ||
| (authored_skill / "SKILL.md").write_text("# Published\n", encoding="utf-8") | ||
|
|
||
| local_skill = project / "skills" / "work-in-progress" | ||
| local_skill.mkdir(parents=True) | ||
| (local_skill / "SKILL.md").write_text("# Work in progress\n", encoding="utf-8") | ||
|
|
||
| apm_executable = Path(sys.executable).with_name("apm") |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python architect | 0 | 0 | 2 | Surgical guard is idiomatic and tested. |
| CLI logging expert | 0 | 0 | 1 | No default CLI output surface changed. |
| DevX UX expert | 0 | 0 | 0 | Persona return failed schema validation after retries. |
| Supply chain security expert | 0 | 0 | 0 | Prose confirmed a tighter publication boundary; structured return failed validation. |
| OSS growth hacker | 0 | 0 | 0 | Persona return failed schema validation after retries. |
| Doc writer | 0 | 3 | 1 | Canonical guides need omitted-versus-declared semantics. |
| Test coverage expert | 0 | 0 | 1 | Real CLI e2e proves the regression; direct unit coverage would add defense in depth. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc writer] Update
pack-a-bundle.mdto clarify root plugin dirs are discovered only for legacy omitted-includesmanifests. - [Doc writer] Update
repo-shapes.mdwith the same declared-includesboundary. - [Doc writer] Update
manifest-schema.mdbecause omitted andautonow differ during packing. - [Test coverage expert] Add a direct unit test for declared
includesexcluding root dirs. - [Python architect] Encapsulate the declaration check on
APMPackage.
Architecture
flowchart TD
A[apm pack] --> B[Collect .apm primitives]
B --> C{includes declared?}
C -->|yes| D[Skip root convention dirs]
C -->|no| E[Collect legacy root convention dirs]
D --> F[Write plugin bundle]
E --> F
Recommendation
Fold the five small, in-scope follow-ups, then re-run the panel against the converged diff.
Full per-persona findings
Python architect
- [nit] Consider a semantic
APMPackagehelper for the declaration check. - [nit] Consider explaining the cleanup-test ownership precondition.
CLI logging expert
- [nit] Consider a verbose breadcrumb when declared
includesskips root convention dirs.
DevX UX expert
No structured findings survived schema validation.
Supply chain security expert
No structured findings survived schema validation; prose consistently described the change as security-positive.
OSS growth hacker
No structured findings survived schema validation.
Auth expert -- inactive
No auth, token, credential, or host-classification surface changed.
Doc writer
- [recommended] Correct
pack-a-bundle.mdroot-discovery wording. - [recommended] Correct
repo-shapes.mdroot-discovery wording. - [recommended] Distinguish omitted and
autoinmanifest-schema.md. - [nit] Cross-link the pack reference to the canonical
includesdefinition.
Test coverage expert
- [nit] Add direct unit coverage for the inverse branch; the real CLI e2e already passes at the required tier.
Performance expert -- inactive
No package-manager performance hot path or performance claim changed.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
d2ab380 to
aac876b
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python architect | 0 | 0 | 0 | The semantic manifest helper and exporter guard are structurally sound. |
| CLI logging expert | 0 | 0 | 0 | No CLI logging or output regression. |
| DevX UX expert | 0 | 0 | 0 | The boundary matches the documented package-author mental model. |
| Supply chain security expert | 0 | 0 | 0 | The change tightens the publication boundary. |
| OSS growth hacker | 0 | 0 | 0 | No adoption or conversion concern remains. |
| Doc writer | 0 | 0 | 0 | Canonical docs and cross-links are consistent. |
| Test coverage expert | 0 | 0 | 0 | Real CLI e2e and direct unit coverage defend the contract. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
flowchart TD
A[apm pack] --> B[Collect .apm primitives]
B --> C{includes declared?}
C -->|yes| D[Skip root convention dirs]
C -->|no| E[Collect legacy root convention dirs]
D --> F[Write plugin bundle]
E --> F
Recommendation
All panelists are clear, all in-scope follow-ups are folded, and all evidence is green. Ship.
Folded in this run
- (panel) Corrected canonical pack guide semantics -- resolved in
1685027. - (panel) Corrected repo-shapes omitted-versus-declared semantics -- resolved in
aac876b. - (panel) Corrected manifest-schema omitted-versus-auto semantics -- resolved in
1685027. - (panel) Added direct unit coverage for declared includes excluding root dirs -- resolved in
1685027. - (panel) Encapsulated the manifest declaration check on
APMPackage-- resolved in1685027. - (panel) Split dense pack reference guidance and added canonical cross-links -- resolved in
aac876b.
Copilot signals reviewed
Copilot produced no inline comments across two fetch rounds.
Regression-trap evidence (mutation-break gate)
tests/integration/test_pack_root_skills_e2e.py::test_pack_auto_includes_only_apm_authored_skills-- deletedif not package.has_declared_includes; test FAILED as expected; guard restored and test passed.
Lint contract
The full canonical lint chain passed post-rebase: ruff check, ruff format check, YAML I/O, 2100-line, portable-relpath, pylint R0801, and auth-signal guards.
CI
CI run 29111061959 and every PR check completed successfully after 0 CI fix iterations.
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2122 | aac876b |
ship_now | 2 | 6 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting required review |
Convergence
2 outer iterations; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
All active panelists returned no findings. Auth and performance were inactive because this PR does not touch their surfaces.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fold the panel follow-ups by documenting omitted-versus-declared includes semantics and adding direct unit coverage for the exporter branch. This also names the manifest intent through APMPackage instead of repeating a storage-level None check. Addresses panel doc-writer, test-coverage, and python-architect follow-ups. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the remaining unconditional root-layout claim and link the packing guidance to the canonical includes contract. Addresses the final doc-writer and growth panel findings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Separate source-layout detection from publication consent so native plugin repositories can adopt APM without silently dropping convention-folder content. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | The presence-based design is deterministic; implementation must replace the old guard. |
| CLI Logging Expert | 1 | 3 | 1 | Mixed and empty APM-native layouts need actionable warnings through the canonical logger path. |
| DevX UX Expert | 0 | 5 | 1 | Preserve native Claude init-to-pack behavior and make source authority explainable. |
| Supply Chain Security Expert | 0 | 2 | 1 | Separate publication consent from source authority and validate explicit paths. |
| OSS Growth Hacker | 0 | 2 | 2 | Lead docs and changelog with the user outcome and progressive adoption path. |
| Doc Writer | 0 | 5 | 0 | Correct includes-based claims and document hooks, dependencies, and strict explicit lists. |
| Test Coverage Expert | 2 | 3 | 0 | Add user-visible init-to-pack and .apm/-authority regression traps. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] (blocking-severity) Add an e2e test that executes
apm initand thenapm packon a native Claude rootskills/layout, provingincludes: autopreserves the skill while.apm/remains absent. - [CLI Logging Expert] (blocking-severity) Emit one actionable warning per skipped root source when
.apm/is present, plus the empty-APM-source warning. - [Supply Chain Security Expert] Replace
has_declared_includeswith.apm/-presence authority; keep explicit lists exhaustive and fail invalid paths. - [Test Coverage Expert] Add complementary
.apm/-authoritative, root hooks, dependency preservation, explicit-list, invalid-path, and warning tests. - [Doc Writer] Update the maintained docs and agent guide to describe layout authority independently from publication consent.
Recommendation
Implement the approved presence-based model before shipping. Re-run the panel after the discriminator, warnings, strict explicit-list handling, e2e proof, unit matrix, docs, changelog, mutation-break evidence, full integration suite, and canonical lint contract are complete.
Full per-persona findings
Python Architect
- [nit] The current diagrams and
has_declared_includesproperty describe the superseded implementation, not the approved authority model. - [nit] Keep the e2e binary invocation consistent with repository integration conventions.
CLI Logging Expert
- [blocking] Root convention sources are currently skipped silently.
- [recommended] Correct the changelog discriminator.
- [recommended] Warn when
.apm/is authoritative but contains no local primitives. - [recommended] Route new warnings through the existing logger-or-rich-warning path.
- [nit] Rename tests and docstrings around
.apm/presence rather than declared includes.
DevX UX Expert
- [recommended] Preserve plugin-native roots when
.apm/is absent, including afterapm initwritesincludes: auto. - [recommended] Make mixed-layout migration succeed with precise warnings.
- [recommended] Keep explicit lists exhaustive and invalid paths actionable.
- [recommended] Avoid any implicit file moves during init or pack.
- [recommended] Prove the user journey through the real CLI.
Supply Chain Security Expert
- [recommended] Replace the includes discriminator with
.apm/presence. - [recommended] Warn on every skipped mixed-layout source.
- [nit] Fail explicit include paths that do not exist.
OSS Growth Hacker
- [recommended] Frame the changelog around preserving existing Claude plugins and preventing accidental publication.
- [recommended] Ensure the pack guide metadata does not imply
.apm/is mandatory. - [nit] Include the remove-
.apm/escape hatch in the empty-source warning. - [nit] Add a concise native-plugin onboarding callout.
Auth Expert -- inactive
No authentication, token, credential, or host-classification surface is involved.
Doc Writer
- [recommended] Replace every includes-based source-layout claim.
- [recommended] Document explicit-list strictness, deliberate root paths, and invalid-path failure.
- [recommended] Cover root hook authority and dependency independence.
- [recommended] Correct first-package onboarding for
apm initplus native roots. - [recommended] Keep canonical ownership by topic and link rather than duplicate prose.
Test Coverage Expert
- [blocking] The user-visible native Claude
apm initthenapm packregression trap is missing. - [blocking]
.apm/-presence authority lacks direct regression coverage. - [recommended] Mixed-layout warning assertions are missing.
- [recommended] Invalid explicit include failure coverage is missing.
- [recommended] Root hook authority coverage is missing.
Performance Expert -- inactive
No dependency transport, cache, materialization, parallelism, or performance-claim surface is involved.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Separates publication consent from local source authority and folds the design panel source-selection, warning, hooks, dependency, and explicit-list follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Proves through the real CLI that apm init writes includes: auto without changing a Claude-native root layout, and explains that migration path during init. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aac876b to
5f07a42
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Presence-based authority is isolated cleanly, explicit validation fails closed, and shared conventions avoid drift. |
| CLI Logging Expert | 0 | 0 | 0 | Warnings and errors are actionable, ASCII-safe, and routed through CommandLogger. |
| DevX UX Expert | 0 | 0 | 0 | The init-to-pack journey, recovery wording, and documentation are clear. |
| Supply Chain Security Expert | 0 | 0 | 0 | Explicit and implicit paths fail closed against unsafe or invalid sources. |
| OSS Growth Hacker | 0 | 0 | 0 | The zero-migration Claude plugin journey supports adoption. |
| Doc Writer | 0 | 0 | 0 | Docs distinguish install consent from exhaustive plugin-pack selection. |
| Test Coverage Expert | 0 | 0 | 0 | Critical behavior has passing regression traps with mutation-break proof. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
APMPackage --> PluginExporter : includes
PluginExporter --> PluginLayout : root conventions
PluginExporter --> PathSecurity : validates explicit paths
InitCommand --> PluginLayout : detects native sources
flowchart TD
Pack[apm pack] --> Includes{explicit list?}
Includes -->|yes| Explicit[validate and pack listed paths]
Includes -->|no| Apm{.apm present?}
Apm -->|yes| Native[pack .apm and warn on root sources]
Apm -->|no| Plugin[pack plugin-native root sources]
Recommendation
All panelists report zero findings. Test evidence is comprehensive and mutation-proven. The implementation honors existing conventions, fails closed at security boundaries, and preserves explicit-list governance. Ship now with no follow-ups.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
Pack and init behavior only; no authentication surface is touched.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert -- inactive
Local pack-time source selection only; no performance-critical surface is touched.
This panel is advisory. Re-apply the panel-review label for another pass.
fix(pack): select plugin sources by .apm presence
TL;DR
apm packnow treats.apm/presence as the local source-authority switch instead of usingincludes:as a layout discriminator. Existing Claude-style root layouts remain packable afterapm initwritesincludes: auto, while mixed layouts use.apm/and emit actionable warnings. Explicit path lists remain exhaustive and now fail closed on missing, unsafe, symlinked, invalid-hook, or unpackable paths.Note
Closes #2054. The approved design is captured in
docs/superpowers/specs/2026-07-10-plugin-pack-source-layout-design.md.Problem (WHY)
includes: autoas a signal to ignore root plugin convention folders, even thoughapm initwrites that value without creating.apm/.apm init->apm packjourney stop packing an existing rootskills/directory..apm/plus root layouts needed deterministic authority and visible migration guidance rather than silent merging.The approved design defines the behavioral authority:
.apm/is exclusive when present; otherwise official plugin convention folders remain sources. Issue #2054 supplies the original accidental-publication reproduction that the authority rule must prevent.Approach (WHAT)
includes:lists exhaustive and validate every listed source.autoor omittedincludes, select.apm/when present and root conventions otherwise..mcp.jsonbehavior unchanged.apm initthat existing plugin-native roots remain packable.Implementation (HOW)
src/apm_cli/bundle/plugin_layout.py-- centralizes supported plugin-native root directories and excludes symlinked roots from discovery.src/apm_cli/bundle/plugin_exporter.py-- implements the three collection branches, strict explicit-path validation, mixed-layout warnings, empty-.apm/guidance, hook authority, and source-directory symlink hardening.src/apm_cli/commands/init.py-- detects an existing native plugin layout after creatingapm.ymland explains thatapm packstill includes it.tests/unit/test_plugin_exporter.py-- covers the layout matrix, strict lists, warnings, hooks, dependencies, malformed inputs, and direct or nested symlinks.tests/unit/test_init_command.py-- proves init writesincludes: auto, does not create.apm/, and emits the native-layout guidance.tests/integration/test_pack_root_skills_e2e.py-- executes realapm initandapm packsubprocesses for both.apm/authority and native Claude root layouts.docs/src/content/docs/{quickstart.mdx,getting-started/first-package.md,concepts/package-anatomy.md,producer/pack-a-bundle.md,producer/repo-shapes.md,reference/cli/init.md,reference/cli/pack.md,reference/manifest-schema.md}-- documents consent, source authority, warnings, install discovery, and the init-to-pack journey.packages/apm-guide/.apm/skills/apm-usage/package-authoring.md-- mirrors the package-authoring contract in the bundled usage guide.CHANGELOG.md-- records the user-visible authority and compatibility fix.docs/superpowers/{specs,plans}/2026-07-10-plugin-pack-source-layout*.md-- preserves the approved design and executable implementation plan.Diagrams
Legend: the dashed nodes are the selection and validation behavior introduced by this PR; dependency collection remains outside this local-source decision.
flowchart LR M[apm.yml] --> E[export_plugin_bundle] E --> I{"includes is a list"} I -->|yes| X[validate listed paths] I -->|no| A{".apm directory exists"} A -->|yes| N[pack .apm sources] A -->|no| P[pack plugin root sources] X --> B[plugin bundle] N --> B P --> B D[lockfile dependencies] --> B classDef new stroke-dasharray: 5 5; class I,X,A,N,P new;Trade-offs
.apm/existence as the authority switch; rejectedincludes:because consent must not silently change a repository's source layout..apm/with actionable warnings; rejected a hard error to keep migration incremental..apm/is absent; rejected forcing existing Claude plugin authors to restructure before adopting APM.Benefits
skills/afterapm initwritesincludes: auto..apm/local content and names every skipped root source.Validation
Focused regression suite:
Canonical full integration suite
Canonical lint contract
Mutation-break proof: restoring the old
includesdiscriminator, disabling.apm/authority, relaxing explicit-list validation, suppressing warnings, removing dependency merging, or allowing direct/nested symlinks caused the corresponding regression tests to fail before each mutation was restored.Scenario Evidence
apm initfollowed byapm packpreserves an existing Claude root skilltests/integration/test_pack_root_skills_e2e.py::test_init_then_pack_preserves_native_claude_skill.apm/wins in a mixed layout and root sources are excluded with warningstests/integration/test_pack_root_skills_e2e.py::test_pack_auto_includes_only_apm_authored_skills;tests/unit/test_plugin_exporter.py::test_mixed_layout_emits_actionable_warningstests/unit/test_plugin_exporter.py::test_explicit_includes_are_exhaustive;test_missing_explicit_include_fails_pack;test_explicit_include_rejects_non_packable_pathtests/unit/test_plugin_exporter.py::test_explicit_include_rejects_symlink;test_explicit_include_rejects_nested_directory_symlink;test_native_root_symlink_is_not_packedtests/unit/test_plugin_exporter.py::test_apm_authority_preserves_dependency_componentstests/unit/test_plugin_exporter.py::test_apm_dir_excludes_root_hook_configHow to test
uv run --extra dev pytest -q tests/unit/test_plugin_exporter.py tests/unit/test_init_command.py tests/integration/test_pack_root_skills_e2e.py; expect169 passed.bash scripts/test-integration.sh; expect10223 passedand no failures.skills/demo/SKILL.md, runapm init -ythenapm pack; expect the skill in the generated bundle..apm/skills/published/SKILL.mdbeside that root layout and pack again; expect only the.apm/skill plus a skipped-root warning.Closes #2054
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com