fix(marketplace): inherit description/version from local-path apm.yml#1755
Conversation
`apm pack` now fills local-path marketplace package `description` and `version` from each package's own `apm.yml` when the root `marketplace.packages[]` entry omits them -- mirroring the remote-source fallback added in #1061. Curator-side values still win; the local read is path-traversal-guarded against the project root and skips a source that resolves to the marketplace's own `apm.yml`. Monorepo marketplaces using `source: ./plugins/<name>` no longer show empty description/version browse columns. closes #1725 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR closes the parity gap between remote and local marketplace package sources by teaching apm pack to inherit description and version for local-path packages from each package’s own apm.yml (with curator values still taking precedence), and documenting the behavior.
Changes:
- Add a local-filesystem metadata prefetch path in
MarketplaceBuilderand unify metadata precedence logic in the Claude output mapper. - Add unit tests covering local inheritance and edge cases, and adjust verbose-summary expectations.
- Update manifest schema docs and add a changelog entry describing the new fallback behavior.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/marketplace/builder.py |
Adds _fetch_local_metadata() and updates _prefetch_metadata() so local metadata is read from disk even under --offline. |
src/apm_cli/marketplace/output_mappers.py |
Unifies curator-vs-manifest precedence via _apply_field_with_precedence() for both local and remote sources; updates verbose summary text. |
tests/unit/marketplace/test_builder.py |
Adds focused unit tests for _fetch_local_metadata() covering happy path and error/edge cases. |
tests/unit/marketplace/test_local_path_compose.py |
Adds compose-level tests verifying inheritance and precedence for local sources (including project-root skip). |
tests/unit/marketplace/test_builder_logging.py |
Updates expected verbose-summary wording in logging tests. |
docs/src/content/docs/reference/manifest-schema.md |
Documents the description/version fallback-from-package-apm.yml behavior for apm pack. |
CHANGELOG.md |
Adds a Fixed entry for local-source description/version inheritance. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 3
| summary_parts.append(f"stripped from {strip_count} local source(s)") | ||
| if override_count > 0: | ||
| summary_parts.append( | ||
| f"{override_count} remote entry(ies) used curator-supplied overrides" | ||
| ) | ||
| summary_parts.append(f"{override_count} entry(ies) used curator-supplied overrides") |
| assert len(summary) == 1 | ||
| assert "stripped from 2 local source(s)" in summary[0].message | ||
| assert "1 remote entry(ies) used curator-supplied overrides" in summary[0].message | ||
| assert "1 entry(ies) used curator-supplied overrides" in summary[0].message |
Normalize the project-root comparison with the same path guard used for local package roots, cap local apm.yml metadata reads, and make override diagnostics describe field counts. Also tighten manifest docs around display-version precedence. Addresses Copilot inline review on PR #1755 and panel follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re-validate the package apm.yml file path before reading so symlinks cannot escape the project root, and scope the manifest docs to GitHub-class remote metadata fallback. Addresses final panel supply-chain and doc-writer follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Clean, bounded feature addition with shared precedence logic and guarded local metadata reads. |
| CLI Logging Expert | 0 | 0 | 0 | Diagnostic summary now describes field overrides truthfully and remains verbose-level. |
| DevX UX Expert | 0 | 0 | 0 | Local inheritance, curator precedence, and docs now match package-manager expectations. |
| Supply Chain Security Expert | 0 | 0 | 0 | Path traversal, project-root, oversized-file, and symlink escape cases are handled as cosmetic skips. |
| OSS Growth Hacker | 0 | 0 | 0 | Strong monorepo authoring DX improvement with changelog/docs coverage. |
| Doc Writer | 0 | 0 | 0 | Manifest docs now describe display-version and GitHub-class remote fallback accurately. |
| Test Coverage Expert | 0 | 0 | 0 | Regression traps cover local reads, project-root normalization, symlink escape, compose inheritance, precedence, and summary wording. |
B = high-signal correctness/security findings, R = recommended follow-ups, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Ready for maintainer review. No deferred follow-ups remain from this shepherd pass.
Folded in this run
- (copilot) Normalize the local metadata project-root equality check with the same path guard used for package roots -- resolved in 0f28bae.
- (copilot) Make the override summary describe curator-supplied field overrides instead of package entries -- resolved in 0f28bae.
- (copilot) Update the logging test assertion to match the corrected override-summary semantics -- resolved in 0f28bae.
- (panel) Clarify manifest docs for remote display-version fallback and semver ranges -- resolved in 0f28bae.
- (panel) Cap local apm.yml metadata reads before yaml.safe_load -- resolved in 0f28bae.
- (panel) Re-validate the package apm.yml path so symlinks cannot escape the project root -- resolved in abc25f6.
- (panel) Scope the docs claim to GitHub-class remote metadata fallback -- resolved in abc25f6.
Copilot signals reviewed
src/apm_cli/marketplace/builder.py-- LEGIT: project-root comparison used a different path shape on Windows extended-prefix paths (resolved in 0f28bae).src/apm_cli/marketplace/output_mappers.py-- LEGIT: override_count tracked fields while summary said entries (resolved in 0f28bae).tests/unit/marketplace/test_builder_logging.py-- LEGIT: logging assertion needed to track the corrected summary wording (resolved in 0f28bae).
Regression-trap evidence (mutation-break gate)
tests/unit/marketplace/test_builder.py::TestFetchLocalMetadata::test_project_root_skip_uses_normalized_root-- restored the oldself._project_root.resolve()comparison; test FAILED as expected; guard restored.tests/unit/marketplace/test_builder.py::TestFetchLocalMetadata::test_apm_yml_symlink_escape_returns_none-- replaced file-path containment with a direct file read; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, pylint R0801, and bash scripts/lint-auth-signals.sh passed locally before the final push.
CI
gh pr checks 1755 --repo microsoft/apm reports all checks successful: 14 successful, 1 skipped, 0 pending, 0 failing (after 0 CI fix iterations).
Mergeability status
Captured from gh pr view 1755 --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 |
|---|---|---|---|---|---|---|---|---|---|---|
| #1755 | abc25f6 |
ship_now | 2 | 7 | 0 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
2 outer iteration(s); 2 Copilot round(s). Final panel verdict: ship_now.
Ready for maintainer review.
Full per-persona findings
Final pass findings were folded or determined to be polish outside the CEO follow-up set. No in-scope follow-up remains open.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Sync the Stage 2 complexity/file-length refactor branch with main's 22 feature commits (Hermes #1726, Kiro IDE #1741, multi-host dep identity #1735, same-repo remote path deps #1732, git_file_transport #1740, revision pins #1738, marketplace sourceBase/source parity/inherit description #1736/#1739/#1755, pack --archive .zip #1720, mcp optional registry inputs #1734, and the v0.19.0/v0.20.0 releases). Conflict resolution preserved both sides: main's new features ported through the branch's extracted sibling modules, branch's tightened ruff thresholds (max-statements=120, max-branches=40, max-complexity=35, max-returns=8, max-args=12) and 800-line file limit retained. All 7 CI-mirror lint gates pass; full unit suite green (17099 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TL;DR
apm packnow auto-fillsdescriptionandversioninmarketplace.jsonfrom each local-path package's ownapm.yml-- the same fallback the remote source path has used since #1061. A curator-side value undermarketplace.packagesstill wins when set, and the local read is path-traversal-guarded against the project root.Closes #1725.
Problem (WHY)
The remote branch in
ClaudeMarketplaceMapper.composealready readsdescription/versionfrom each resolved package's ownapm.ymlwhen the curator entry omits them (_fetch_remote_metadata, over HTTPS). The local branch had no such fallback -- it emitted only what the curator wrote in the rootapm.yml'smarketplace.packages[], and a stale comment in_prefetch_metadataeven claimed local packages "carry their own metadata" while never reading any.For a monorepo marketplace using
source: ./plugins/<name>entries, this left producers duplicating the description between the root manifest and each package manifest (which drift), or running custom sync tooling. This PR closes the gap for local sources by mirroring the existing remote behavior exactly.Approach (WHAT)
builder._prefetch_metadata{}under--offline.--offline). Returns one dict the mapper consumes the same way regardless of source kind.builder._fetch_local_metadata<project_root>/<subdir>/apm.yml, extractsdescription/version. Path-traversal-guarded viaensure_path_within. Skips a source that resolves to the project root itself.output_mappers.compose_apply_field_with_precedencehelper: curator wins, else fall back to manifest metadata, with a verbose divergence diagnostic.docs/.../manifest-schema.mddescription/versionrows had no mention of the fallback.Edge cases handled
apm.yml->None; no key emitted.None; logged at debug; build continues.ensure_path_withinraises; caught, returnsNone.apm.ymlis never read as a package manifest.subdir-> defensive early return.--offline-> local reads still run (filesystem); remote reads skipped.Validation evidence
19 new tests: 8 unit tests for
_fetch_local_metadataand 6 compose-level inheritance/precedence tests (plus the existing suite re-greened for the unified override-summary wording).How to test
apm.ymlwith asource: ./packages/<name>entry that omitsdescription/version.packages/<name>/apm.ymladescriptionandversion.apm packand confirm the generatedmarketplace.jsonplugin entry carries both fields.Provenance
This re-implements the fix originally proposed in #1581 by @imbabamba, which could not be merged because the CLA was not signed. Thanks to @srobroek for the original report (#1725). Opened as an equivalent maintainer-authored change so the fix can land; #1581 is being closed in favor of this PR.