fix(pack): pack emits only lockfile-attested dependency content in every format (closes #1999)#2013
Conversation
Use lockfile deployed_files for plugin dependency components so skills filters survive stale or missing apm_modules caches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes apm pack --format plugin so git skill_bundle dependencies respect the lockfile’s skill_subset consistently by preferring lockfile deployed_files over the raw apm_modules cache, preventing over-bundling (entire upstream repo) and silent under-bundling (nothing when cache is missing).
Changes:
- Plugin exporter now collects dependency components from
LockedDependency.deployed_files(with clearer erroring when neitherdeployed_filesnor cache content is available). - Adds unit tests covering deployed-files export behavior and cache-missing behavior.
- Updates CLI/reference and producer docs to describe the new packing source-of-truth.
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/bundle/plugin_exporter.py | Switches dependency export to prefer lockfile deployed_files and adds mapping/validation helpers. |
| tests/unit/test_plugin_exporter.py | Adds regression tests for deployed-files-based skill bundling behavior. |
| docs/src/content/docs/reference/cli/pack.md | Documents deployed-files-based dependency packing behavior for plugin bundles. |
| docs/src/content/docs/producer/pack-a-bundle.md | Explains how apm pack sources dependency files during bundling. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Low
| skill_name = _skill_name_from_deployed_parts(parts) | ||
| if skill_name is not None: | ||
| if dep.skill_subset and skill_name not in set(dep.skill_subset): | ||
| return None | ||
| plugin_parts = ["skills", *parts[2:]] |
|
|
||
| - `plugin.json` -- schema-conformant manifest. Convention-dir keys are stripped because Claude Code auto-discovers them. | ||
| - Plugin-native subdirs populated from your `.apm/` content and from installed dependencies: `agents/`, `skills/`, `commands/`, `instructions/`, `hooks/`, `extensions/` (canvas extensions, when the `canvas` experimental flag is enabled). | ||
| - Plugin-native subdirs populated from your `.apm/` content and from installed dependencies: `agents/`, `skills/`, `commands/`, `instructions/`, `hooks/`, `extensions/` (canvas extensions, when the `canvas` experimental flag is enabled). Installed dependency files come from lockfile `deployed_files`; git dependency `skills:` filters are honored, so stale `apm_modules` cache content cannot add extra skills. |
| For installed dependencies, `apm pack` uses `apm.lock.yaml` | ||
| `deployed_files`. If a git dependency declares `skills:`, only those | ||
| deployed skills are emitted; raw `apm_modules` cache content is not a | ||
| source of extra skills. |
| dep = LockedDependency( | ||
| repo_url="acme/skill-bundle", | ||
| depth=1, | ||
| package_type="skill_bundle", | ||
| deployed_files=deployed_files, | ||
| skill_subset=["alpha", "beta"], | ||
| ) | ||
| _write_lockfile(project, [dep]) |
| plugin_rel = _plugin_rel_for_deployed_path(rel_path, dep) | ||
| if plugin_rel is None: | ||
| continue | ||
| source = project_root / rel_path |
Fold review follow-ups for plugin bundle export: apply skill_subset to plugin-native skill paths, cap missing deployed-file diagnostics, clarify subset errors, and add regression coverage for deployed path safety and empty subset resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fold final panel follow-ups by removing a stale parameter, adding dependency context to unsafe deployed path errors, and aligning docs with the deployed_files then apm_modules fallback behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean decomposition of deployed-files collection path; layered path safety is correct. |
| CLI Logging Expert | 0 | 0 | 0 | Error-path folds ship clean; no remaining CLI output issue. |
| DevX UX Expert | 0 | 0 | 0 | Error messages are actionable and docs describe the deployed_files/apm_modules/error cascade. |
| Supply Chain Security Expert | 0 | 0 | 0 | Path traversal, symlink, output path, and subset guards are layered correctly. |
| OSS Growth Hacker | 0 | 0 | 0 | Docs lead with user outcome and action; no conversion surface regressions. |
| Doc Writer | 0 | 0 | 0 | Docs match deployed_files-first pack behavior and fallback/error semantics. |
| Test Coverage Expert | 0 | 0 | 0 | Critical surfaces have regression-trap tests with integration-style fixtures. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class LockedDependency {
<<ValueObject>>
+repo_url str
+deployed_files list~str~
+skill_subset list~str~
}
class export_plugin_bundle {
<<Orchestrator>>
}
class _collect_deployed_components {
<<IOBoundary>>
}
class _plugin_rel_for_deployed_path {
<<Pure>>
}
class _deployed_path_parts {
<<Pure>>
}
class ensure_path_within {
<<Guard>>
}
export_plugin_bundle ..> _collect_deployed_components : deployed_files path
export_plugin_bundle ..> _collect_apm_components : legacy fallback
_collect_deployed_components ..> _plugin_rel_for_deployed_path : maps entries
_collect_deployed_components ..> ensure_path_within : validates
_plugin_rel_for_deployed_path ..> _deployed_path_parts : validates + splits
flowchart TD
A[apm pack] --> B[export_plugin_bundle]
B --> C{dep.deployed_files?}
C -->|yes| D[_collect_deployed_components]
C -->|no| E{apm_modules cache?}
E -->|yes| F[legacy collectors]
E -->|no| G[error: run apm install]
D --> H[_deployed_path_parts safety]
H --> I[_plugin_rel_for_deployed_path subset mapping]
I --> J[ensure_path_within]
J --> K[components]
K --> L[_merge_file_map]
Recommendation
Ship as-is. The change is tightly scoped to plugin bundle dependency export, panel follow-ups were folded into the PR, CI is green, and no remaining recommended follow-up changes the landing decision.
Folded in this run
- (panel) Apply
skill_subsetto plugin-nativeskills/deployed paths -- resolved in 38344e8. - (panel) Add regression coverage for deployed path safety and empty subset resolution -- resolved in 38344e8.
- (panel) Cap missing deployed-file diagnostics and clarify the subset-miss error -- resolved in 38344e8.
- (panel) Clarify docs for
deployed_filespreference, older-lockfileapm_modulesfallback, and missing dependency content errors -- resolved in 38344e8 and 434c4ca. - (panel) Remove the stale helper parameter and add dependency context to unsafe deployed path errors -- resolved in 434c4ca.
Regression-trap evidence (mutation-break gate)
tests/unit/test_plugin_exporter.py::TestPluginRelForDeployedPath::test_top_level_skills_honor_subset-- deleted top-level skill-name extraction; test FAILED as expected; guard restored.tests/unit/test_plugin_exporter.py::TestExportPluginBundle::test_dependency_skill_subset_empty_resolution_errors-- disabled the empty subset guard; test FAILED as expected; guard restored.tests/unit/test_plugin_exporter.py::TestDeployedPathParts-- disabled absolute/traversal path checks; tests FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, YAML/file-length/relative_to guards, pylint R0801, and bash scripts/lint-auth-signals.sh completed with exit code 0 after merging origin/main locally.
CI
GitHub checks are green on head 434c4cab60e7ee24151565a391c7d30e98a32da0: Lint, Build & Test Shard 1/2, Coverage Combine, PR Binary Smoke, APM Self-Check, CodeQL, Merge Gate, docs build, spec conformance, NOTICE, and CLA all completed successfully (after 0 CI fix iterations).
Mergeability status
Captured from gh pr view 2013 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2013 | 434c4ca |
ship_now | 2 | 5 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting maintainer requirements |
Convergence
2 outer iteration(s); 2 Copilot round(s). Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit]
_plugin_rel_for_deployed_pathrouting table could use a module-level mapping dict atsrc/apm_cli/bundle/plugin_exporter.py:460
Optional polish only; the current conditional chain is readable, tested, and appropriate for this PR.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
PR touches only plugin_exporter, docs, and tests; no auth, token, host, or credential resolution surface.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert -- inactive
Pack-time local filesystem changes only; no install/resolve/fetch/materialize hot path or performance claim.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Add a hermetic integration test that shells through the real apm pack plugin flow for skill-bundle dependencies with deployed_files and skill_subset metadata. The test proves only deployed subset skills enter the plugin bundle, unsafe deployed paths are rejected, and older lockfiles without deployed_files still fall back to apm_modules. Also marks live network integration modules so the non-network integration gate stays hermetic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tighten the plugin export error messages, add direct unit coverage for deployed path mapping branches, document the skills subset reference, and add the changelog release note requested by the review panel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Deployed-files collection is a clean strategy branch inside plugin export. |
| CLI Logging Expert | 0 | 0 | 0 | Pack errors are actionable and now avoid Python repr. |
| DevX UX Expert | 0 | 0 | 0 | Users get exactly installed subset skills, with clear recovery. |
| Supply Chain Security Expert | 0 | 0 | 0 | Path traversal guards and unsafe-path tests cover the pack trust boundary. |
| OSS Growth Hacker | 0 | 0 | 0 | CHANGELOG and docs surface the producer trust story. |
| Doc Writer | 0 | 0 | 0 | Docs accurately explain deployed_files-first packing and fallback. |
| Test Coverage Expert | 0 | 0 | 0 | Critical pack promises are covered by hermetic e2e and unit tests. |
| Performance Expert | 0 | 0 | 0 | deployed_files bounds pack work to installed files. |
B = highest-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
All gates are green, all panel findings are resolved, zero in-scope follow-ups remain, and backward compatibility is preserved via the fallback path. This PR is ready to merge as-is.
Folded in this run
- (panel) Added mandatory hermetic e2e coverage for plugin pack skill_subset behavior, deployed-path safety, and older-lockfile apm_modules fallback -- resolved in 140acf7.
- (panel) Marked live network integration modules with
requires_network_integrationso the non-network integration gate stays hermetic -- resolved in 140acf7. - (panel) Tightened user-facing pack errors to avoid Python repr and name the recovery action precisely -- resolved in 2d1174e.
- (panel) Added direct unit coverage for deployed path mapping branches -- resolved in 2d1174e.
- (panel) Added CHANGELOG release note for the scoped skill dependency pack fix -- resolved in 2d1174e.
- (panel) Linked the producer docs
skills:mention to the package-types reference -- resolved in 2d1174e.
Regression-trap evidence (mutation-break gate)
tests/integration/test_plugin_skill_subset_pack_e2e.py::test_pack_plugin_uses_deployed_skill_subset_and_survives_missing_cache-- changedif dep.deployed_files:toif False:; test FAILED with extragammabundled; guard restored.
Copilot signals reviewed
- No inline comments from
copilot-pull-request-reviewer[bot]were present after two fetch rounds.
Lint contract
After merging origin/main, this command exited 0:
uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ && uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ && bash scripts/lint-auth-signals.sh
CI
gh pr checks 2013 --repo microsoft/apm is green on head 2d1174e83007e7de2263d9467c8c90efbd8cc322: Lint, CodeQL, Build and Test Shard 1/2, Coverage Combine, PR Binary Smoke, APM Self-Check, Analyze, NOTICE Drift Check, Spec conformance, build, gate, and license/cla all passed (deploy skipped).
E2E evidence
- Gate A:
APM_E2E_TESTS=1 uv run pytest tests/integration/test_plugin_skill_subset_pack_e2e.py -v-> 3 passed. - Old behavior proof: with
origin/main -- src/apm_cli/bundle/plugin_exporter.py,test_pack_plugin_uses_deployed_skill_subset_and_survives_missing_cachefailed becausegammawas bundled. - Gate B:
APM_E2E_TESTS=1 uv run pytest tests/integration -m "not requires_network_integration" -p no:cacheprovider -q-> 10160 passed, 154 skipped, 37 deselected, 2 xfailed.
Mergeability status
Captured from gh pr view 2013 --json mergeable,mergeStateStatus,statusCheckRollup after the last push.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2013 | 2d1174e |
ship_now | 1 | 6 | 0 | 2 | green | MERGEABLE | BLOCKED | pending maintainer review |
Convergence
1 outer iteration; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
#1999) Redesign plugin-format `apm pack` so dependency content is sourced exclusively from lockfile-attested `deployed_files`, closing a provenance hole where the unattested `apm_modules` cache could leak stale, partial, or tampered bytes into a packed bundle. Changes in src/apm_cli/bundle/plugin_exporter.py: - Dependency components are now collected only via `_collect_deployed_components` from `dep.deployed_files`, mapped to plugin-native layout by `_plugin_rel_for_deployed_path`, honoring `skill_subset` (the #1999 fix). - Removed the raw `apm_modules` cache-walk fallback for dependency components and the unconditional dep hooks/MCP cache walk. Dep hooks config and MCP config are not recorded in `deployed_files` and are no longer packed (unattested). Hook scripts that ARE in deployed_files still pack as attested files. Root/first-party hooks and MCP are unchanged. - Added `_verify_attested_hash`: each packed deployed file is verified against its recorded `deployed_file_hashes` SHA-256 before inclusion; a mismatch fails loud (integrity). - Added `_cache_would_contribute_primitives`: when a dep has no `deployed_files`, if the cache holds packable primitives (a stale or partial install) pack fails loud with an actionable `apm install` message; otherwise the dep is skipped cleanly (preserves MCP-only or hooks-config-only deps whose deployed_files are legitimately empty). Tests: - tests/integration/test_plugin_skill_subset_pack_e2e.py: new `test_pack_plugin_excludes_unattested_cache_hooks_and_mcp` provenance gate plants unattested cache hooks/MCP and asserts they are not packed; renamed the older-lockfile test to assert the new fail-loud behavior. - tests/unit/test_plugin_exporter.py: updated to the attested-only contract (fail-loud on cache-only dep, dep cache hooks/MCP not packed). - tests/unit/bundle/test_plugin_exporter_lockfile.py: fixture now records `deployed_file_hashes` via compute_file_hash so hash verification passes. Docs: pack-a-bundle.md, reference/cli/pack.md, and CHANGELOG updated to state the attested-only guarantee. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
…og polish Fold apm-review-panel iteration-1 recommendations (all within scope of the lockfile-attested plugin-pack change): - test: add regression traps for _verify_attested_hash. New test_dependency_hash_mismatch_rejects_pack tampers a deployed file so it diverges from its recorded deployed_file_hashes and asserts pack fails; test_dependency_hash_match_packs_successfully guards the accept path so the integrity check does not become a false-positive blocker. Closes the one blocking finding (the SHA-256 verification -- the attestation mechanism this change is named for -- previously had no negative-path regression trap). - docs(pack-a-bundle): fix broken cross-reference anchor (#skill_bundle -> #skill-collection-skillsnameskillmd) and split the dependency-provenance prose into its own paragraph. - docs(cli/pack): state that dependency hook-configs and MCP-configs are no longer merged into the bundle; deps contribute only attested deployed_files. - docs(changelog): lead the #2013 entry with the supply-chain provenance guarantee before the mechanics. - cli: make the skill-subset and missing-files pack errors end with the same ", then pack again." remediation suffix as their sibling messages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…og polish Fold apm-review-panel iteration-1 recommendations (all within scope of the lockfile-attested plugin-pack change): - test: add regression traps for _verify_attested_hash. New test_dependency_hash_mismatch_rejects_pack tampers a deployed file so it diverges from its recorded deployed_file_hashes and asserts pack fails; test_dependency_hash_match_packs_successfully guards the accept path so the integrity check does not become a false-positive blocker. Closes the one blocking finding (the SHA-256 verification -- the attestation mechanism this change is named for -- previously had no negative-path regression trap). - docs(pack-a-bundle): fix broken cross-reference anchor (#skill_bundle -> #skill-collection-skillsnameskillmd) and split the dependency-provenance prose into its own paragraph. - docs(cli/pack): state that dependency hook-configs and MCP-configs are no longer merged into the bundle; deps contribute only attested deployed_files. - docs(changelog): lead the #2013 entry with the supply-chain provenance guarantee before the mechanics. - cli: make the skill-subset and missing-files pack errors end with the same ", then pack again." remediation suffix as their sibling messages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
203ea35 to
ecaffc0
Compare
… dep hooks/MCP (#2013) Extend the lockfile-attested provenance guarantee to the default archive (apm) pack format and harden the plugin format: - attest: new shared helper apm_cli/bundle/attest.verify_attested_file used by BOTH plugin_exporter and packer, so per-file SHA-256 verification lives once (no pylint R0801 duplication). - packer (--format apm): verify each dependency file against the SHA-256 in deployed_file_hashes before copy; mismatch fails loud with an actionable, ASCII-only error. Directory entries are walked with per-child containment + hash verification. Hashes keyed by the on-disk deployed path so cross-target mapped files are compared like-for-like (no false mismatch). - plugin_exporter: warn-loud (ASCII [!]) when a dependency's cached hooks/MCP config is dropped because it is not attested in apm.lock.yaml; re-assert ensure_path_within on rglob-expanded children (defense-in-depth vs directory symlink traversal); reword failure errors to reference apm.lock.yaml and the skill_subset, dropping internal cache jargon. - observability: debug diagnostic when a deployed file has no recorded hash. - e2e: new hermetic test_pack_apm_provenance_e2e (tamper-rejection on file and directory entries, cross-target no-false-positive, directory-symlink escape) and a plugin warn-loud e2e. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- pack-a-bundle.md: provenance now covers --format apm + hooks/MCP drop note and attested-file-mismatch troubleshooting. - reference/cli/pack.md: --format apm hash-verification behavior. - apm-usage skill commands.md: attested-only packing + hash-mismatch failure. - CHANGELOG: fold apm-format hardening into the #2013 Fixed entry. - ruff-format the two provenance e2e test files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…k test, attest unit coverage (#2013) Advisory panel folds (all in-scope, no blocking findings): - plugin_exporter: reword stale-cache error in user vocabulary (drop 'unattested cache' jargon; say installed content cannot be verified). - test_pack_apm_provenance_e2e: symlink-escape test now accepts both invariant-preserving outcomes (graceful skip on 3.12+ rglob OR loud containment refusal) so it is robust across Python versions. - tests/unit/bundle/test_attest.py: direct branch coverage for the shared verify_attested_file keystone (match / mismatch / no-hash tolerance). - docs: note older-lockfile files pack unverified; genericize the deleted-file troubleshooting wording across both formats. - CHANGELOG: benefit-first lead sentence for release-note mining. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve CHANGELOG.md [Unreleased] ordering conflict: keep both the audit --help scope fix (microsoft#2017) and the pack provenance entry (microsoft#2013). Co-authored-by: WilliamK112 <WilliamK112@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Plugin-format
apm packpreviously walked the rawapm_modulescache for dependency primitives, so git skill-bundle dependencies with a declaredskills:subset could export every cached upstream skill, or export none when the cache was absent. Worse,apm_modulesis an unattested cache with no provenance or integrity guarantee: any bytes packed from it could be stale, partial, or tampered.This PR redesigns pack to emit only what the lockfile attests, in both pack formats (
--format pluginand the default--format apm):deployed_files(project-root-relative POSIX paths), mapped back to the target-native layout, honoringskill_subset(the [BUG]apm packignores the declaredskills:subset filter on git dependencies — bundles the entire upstream repo, or silently bundles nothing #1999 fix).deployed_file_hashesSHA-256 before inclusion; a mismatch fails loud (integrity). The compute-compare-raise logic lives once in a shared helper (src/apm_cli/bundle/attest.py) called by both the plugin exporter and the apm-format packer, so there is no duplicated verification block (R0801-clean).apm_modulescache-walk fallback for dependency components is removed (plugin format). Dependency-contributed hooks config and MCP config are not recorded indeployed_filesand are no longer packed (unattested);apm packnow warns loud ([!]) naming the dependency when it drops such config. Hook scripts that ARE indeployed_filesstill pack as attested files. Root/first-party hooks and MCP are unchanged.deployed_filesbut its cache holds packable primitives (a stale or partial install),apm packfails loud with an actionableapm installmessage rather than silently packing unattested cache. Deps whosedeployed_filesare legitimately empty (e.g. MCP-only) are skipped cleanly.ensure_path_within(project_root)so a planted directory symlink cannot escape the project root into the bundle.apm-format hardening (folded from red-team review)
The tar/zip apm-format packer (
packer.py) sourced dependency files fromdeployed_filesbut did not previously verify their hashes. This PR extends the same attested-only guarantee to--format apm: each deployed file (and each child of a deployed directory) is SHA-256-verified againstdeployed_file_hashesbefore it enters the archive. A file tampered afterapm installnow fails the pack instead of shipping silently. Files with no recorded hash (older lockfiles) pack unverified, with a debug diagnostic.gh-aw / apm-action compatibility: the archive format, extension, output directory, the
PackResultshape, and the--archive-formatClick default are byte-unchanged; theverify-shared-apm-matrix.ymljob D drift-guard assertions still hold. The change only ADDS a verification gate on the normal install-then-pack flow.Tests
Hermetic e2e proves both formats:
tests/integration/test_plugin_skill_subset_pack_e2e.pyproves the [BUG]apm packignores the declaredskills:subset filter on git dependencies — bundles the entire upstream repo, or silently bundles nothing #1999 subset behavior, the plugin provenance guarantee (plants an unattested cache file, asserts it is absent from the bundle), and the new warn-loud on dropped dep hooks/MCP config (RED without the fix, GREEN with it).tests/integration/test_pack_apm_provenance_e2e.pyproves--format apmrejects a tampered deployed file / a tampered file inside a deployed directory, accepts matching files, does not false-positive on cross-target mapped files, and does not let a directory symlink escape.How to test:
Closes #1999.