feat(deps): support marketplace dependencies in plugin.json

[stbenjam]

Description

Plugins like openshift-eng/ai-helpers/golang declare transitive deps as {"name": "gopls-lsp", "marketplace": "claude-plugins-official"} in plugin.json. Previously parse_from_dict() rejected these with "must have a 'git' or 'path' field". Now marketplace dicts are parsed into DependencyReference objects and resolved to concrete git refs during BFS dependency resolution via resolve_marketplace_plugin().

Note: marketplace must still have been previously registered w/ apm to work.

Fixes # (issue)

N/A

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

[Copilot]

Pull request overview

Adds support for marketplace-style dependency entries ({name: ..., marketplace: ...}) in plugin.json/apm.yml by extending dependency parsing and resolving those placeholders into concrete git refs during BFS dependency resolution.

Changes:

• Extend DependencyReference to represent and parse marketplace dependency dict entries.
• Resolve marketplace dependencies to concrete git references during BFS tree building (with auth resolver plumbing from install context).
• Add unit + integration tests covering parsing and resolution behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/apm_cli/models/dependency/reference.py	Adds marketplace fields, parsing support for {name, marketplace}, and guards for unresolved marketplace refs in install-path/serialization helpers.
src/apm_cli/deps/apm_resolver.py	Introduces marketplace resolution during BFS and threads auth_resolver into resolver calls.
src/apm_cli/install/phases/resolve.py	Passes ctx.auth_resolver into APMDependencyResolver for marketplace resolution.
tests/test_apm_package_models.py	Adds parsing/validation tests for marketplace dependency dict format.
tests/test_apm_resolver.py	Adds tests for resolver marketplace-resolution behavior and tree integration.
tests/integration/test_marketplace_plugin_integration.py	Adds integration coverage for plugin.json parsing with marketplace deps and mixed dependency scenarios.

Comments suppressed due to low confidence (2)

src/apm_cli/models/dependency/reference.py:1578

• to_apm_yml_entry() now raises ValueError for unresolved marketplace deps. Since this is a public-ish serialization helper used by manifest writers, consider documenting this in the docstring and ensuring any call sites that may encounter marketplace refs either resolve them first or preserve the original dict form instead of calling to_apm_yml_entry().

        if self.is_marketplace:
            raise ValueError(
                f"Cannot serialize unresolved marketplace dependency "
                f"'{self.marketplace_plugin_name}@{self.marketplace_name}'"
            )

src/apm_cli/models/dependency/reference.py:548

• This PR changes the supported dependency format by allowing marketplace dict entries in plugin.json/apm.yml. Per the repo's docs-update rules, please update the Starlight docs under docs/src/content/docs/ and the APM guide skill reference packages/apm-guide/.apm/skills/apm-usage/dependencies.md to document {name: ..., marketplace: ...} entries and how they are resolved.

        # Support marketplace dependencies: { name: X, marketplace: Y }

[sergio-sisternes-epam]

Technical Review -- PR #1422

Verdict: Request Changes -- the core idea is sound and well-motivated, but there are two blocking issues (fail-open semantics and dropped dependency_reference) that should be resolved before merge, plus several smaller items worth addressing.

---

Is the use case well-motivated?

Yes. Marketplace-style deps ({name: ..., marketplace: ...}) are already used in the wild (e.g. openshift-eng/ai-helpers/golang) and resolve_marketplace_plugin() already exists. Extending parse_from_dict() to accept this format is a natural progression. The PR closes a real gap: previously these entries caused a hard parse error during install.

Feature vs bug fix

This is a feature, not a bug fix. It adds a new dependency format, new resolver behaviour, 570 lines of code, and new docs. Labelling it as a bug fix under-communicates its scope for changelog/release purposes.

---

Blocking Issues

1. Fail-open semantics on marketplace resolution failure

When _resolve_marketplace_dep() fails, the dep is skipped (continue) and only recorded as a resolution_warning (later surfaced via graph.add_error()). However, the install pipeline may not treat these as fatal -- meaning apm install could succeed with a required dependency silently omitted from the install set and lockfile.

This is supply-chain hostile. A declared dependency that cannot be resolved should be a hard failure, not a silent skip.

Suggestion: Either raise during build_dependency_tree() on marketplace resolution failure, or ensure resolve.py explicitly aborts when dependency_graph.resolution_errors is non-empty.

2. _resolve_marketplace_dep() ignores MarketplacePluginResolution.dependency_reference

The existing resolve_marketplace_plugin() returns a MarketplacePluginResolution that includes an optional dependency_reference field -- specifically for GitLab-class hosts and in-marketplace subdirectory plugins where parsing the canonical string alone would produce the wrong repo/path. The new code always does DependencyReference.parse(canonical_str), ignoring this structured field.

# Current (lossy):
canonical_str, _ = resolution
return DependencyReference.parse(canonical_str)

# Should prefer:
if resolution.dependency_reference is not None:
    return resolution.dependency_reference
return DependencyReference.parse(resolution.canonical)

---

Non-Blocking Issues (should fix before merge)

3. Security warnings from marketplace resolver are not surfaced

Direct apm install plugin@marketplace passes warning_handler=logger.warning, but the BFS resolver does not pass any warning_handler to resolve_marketplace_plugin(). Ref-swap and shadow-detection warnings fall back to stdlib logging and may be invisible during dependency-tree resolution. Thread a warning collector or at minimum pass a handler.

4. Bare except Exception hides real bugs

_resolve_marketplace_dep() catches all exceptions including programmer errors, import failures, and unexpected auth issues. Required dependencies can disappear behind a generic warning. Catch the documented resolver exceptions (MarketplaceNotFoundError, PluginNotFoundError, MarketplaceFetchError, ValueError) and let unexpected exceptions propagate.

5. resolution_warnings naming vs semantics

The field is called resolution_warnings but values are folded into graph.add_error(). Future callers cannot distinguish advisory warnings from fatal resolution failures. Either rename to resolution_errors if fatal, or maintain separate lists.

6. Marketplace dict ref/version silently unsupported

resolve_marketplace_plugin() accepts version_spec, but the dict format {name, marketplace, ref} is not handled -- extra keys are silently ignored. A user writing ref: v2.0 on a marketplace dep would think they pinned a version when they did not. Either reject unsupported keys or support ref by passing it as version_spec.

---

Suggestions (non-blocking)

• Factor out duplicated resolution blocks: The same 10-line marketplace-resolution pattern appears 3 times in build_dependency_tree(). Extract a helper like _resolve_or_record_marketplace(dep_ref, tree, context) -> DependencyReference | None.

• Synthetic _marketplace/... repo_url: Works in practice since marketplace refs are resolved before entering the tree, but encoding a non-repo sentinel into repo_url is brittle. Consider overriding get_unique_key() for marketplace refs or introducing an explicit dependency kind field.

• auth_resolver: object | None typing: Acceptable given existing APIs, but a Protocol or TYPE_CHECKING import would improve IDE support.

Test coverage

The test suite is solid for the happy path and basic validation. Missing scenarios:

• Install pipeline aborts when a marketplace dep cannot be resolved (fail-closed)
• MarketplacePluginResolution.dependency_reference is preferred over canonical string parsing
• Resolver warning handler captures ref-swap/shadow warnings during BFS
• Marketplace dict with unsupported ref key either errors or correctly pins
• Transitive marketplace failure does not produce a partial successful install

Docs

The docs updates are accurate and well-structured. The three-variant table (git/path/marketplace) in dependencies.md is a clear improvement over the previous format.

---

Summary: The architectural direction is right -- marketplace deps belong in APM's dependency model. The implementation needs the two blocking fixes (fail-closed semantics and dependency_reference handling) plus the non-blocking items above. Once addressed, this is a clean merge.

[sergio-sisternes-epam]

@stbenjam thank you for this contribution and for the thorough test coverage — the implementation is solid and the docs updates are well done.

After reviewing the PR, we have a concern about the dependency format this introduces. The {name: ..., marketplace: ...} shape in plugin.json dependencies appears to be a convention adopted by community plugin authors (e.g. openshift-eng/ai-helpers) rather than part of the official Claude Code marketplace specification. Anthropic's marketplace field in plugin.json serves a different purpose (top-level metadata for indexing/categorisation).

We're cautious about adding first-class support for dependency formats that deviate from the upstream standard, as it creates a maintenance surface that could diverge further or conflict with future official specs.

That said, we recognise this is blocking real plugin resolution in the wild, so it's not a clear-cut call. Deferring to @danielmeppiel for the final decision on whether APM should support this community convention or wait for an upstream standard to emerge.

Thanks again for the effort here — regardless of the outcome on this PR, the marketplace resolver integration pattern you've laid out is valuable context for future work.

[github-actions]

APM Review Panel: needs_rework

Marketplace dep parsing: core feature is sound; 3 runtime blockers (fail-open auth, mutable lockfile ref, no lockfile placeholder test) must close before merge.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The panel converges on a single dominant failure mode: the bare except Exception in _resolve_marketplace_dep that converts auth failures, 403s, and network errors into a silent continue. Five panelists flagged this independently (python-architect, supply-chain-security, auth-expert, devx-ux, cli-logging). That convergence is decisive. This is not an opinion finding -- it is a correctness contract: an operator with a misconfigured PAT installs successfully, sees only a WARNING in logs, and ships a partially-installed environment. That violates APM's secure-by-default and pragmatic-as-npm promises simultaneously. This finding is blocking and must be resolved before merge. The fix is narrowly scoped: replace the bare except with typed catches, re-raise auth-class failures (MarketplaceFetchError, 401/403 variants) as hard errors, and let only genuinely advisory conditions (name mismatch, plugin-not-found on optional deps) land in resolution_warnings.

The lockfile mutable ref finding (supply-chain-security, blocking) stands alone in the panel but the security rationale is unimpeachable: owner/repo#main in a lockfile is not a determinism guarantee. Two installs from the same lockfile can produce different code if the upstream branch moves. This is the malicious-update threat and it is a first-class supply-chain risk. It must close before merge. The fix -- resolve the branch/tag to a commit SHA at install time and write the SHA into the lockfile -- is the same pattern APM already applies to git deps and is consistent with npm's package-lock.json behavior.

The test-coverage-expert blocking finding (no test that lockfile records resolved git coordinates, not the _marketplace/... placeholder) is load-bearing evidence: the placeholder serialization path is untested, meaning a regression would be invisible. This is a pre-ship requirement.

The doc-writer blocking finding (string shorthand plugin@marketplace#ref present in dependencies.md but absent from manage-dependencies.md) is a real inconsistency but carries no runtime risk. Either the feature exists and manage-dependencies.md needs it, or it does not and dependencies.md needs a Planned callout. This should be resolved in this PR to avoid public-facing doc drift.

Dissent. cli-logging-expert recommended graph.add_warning() instead of graph.add_error() for resolution failures (graceful degradation). supply-chain-security and python-architect both argue auth failures must be hard errors (fail-closed). These are not in conflict: discriminate by exception type -- auth failures (401, 403, expired token) re-raise as hard errors; name-not-found on optional deps uses add_warning. Both positions are satisfied by the typed-catch fix.

Aligned with: Portable by manifest -- partial; lockfile mutable ref breaks the determinism guarantee. Secure by default -- broken by bare-except fail-open and mutable ref; both have targeted fixes. Multi-harness/multi-host -- auth resolver correctly threaded; private registries will work once auth-failure re-raise lands. OSS community driven -- marketplace dep is the right ecosystem flywheel unlock; blocked by missing "how to register" docs. Pragmatic as npm -- npm/pip/cargo treat missing transitive deps as hard stops; current silent-skip behavior is below the incumbent baseline.

Growth signal. The marketplace dep feature is the plugin ecosystem flywheel: publish once, depended on by name, marketplaces gain listings, network effect compounds. The launch beat ('APM now supports plugin ecosystems') is earnable once the security fixes land and the 'how to register a marketplace' doc gap closes. Pair the merge with a CHANGELOG entry leading with the user story and a follow-up issue documenting marketplace registration end-to-end.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	1	3	1	Bare except swallows auth failures causing silent partial installs; BFS marketplace resolution is copy-pasted 3x; sentinel repo_url is a footgun waiting for the next call site.
CLI Logging Expert	0	2	2	Resolution failures are double-logged (logger.warning + resolution_warnings) and warnings are promoted to graph errors -- both are UX correctness problems.
DevX UX Expert	0	2	2	Silent resolution failure is the critical UX gap; schema ergonomics and error copy are solid, but a missing dep that disappears without terminal feedback will burn real users.
Supply Chain Security Expert	2	3	2	Marketplace resolution records mutable branch/tag refs in lockfile (no SHA pin), and bare Exception catch converts security errors to silent fail-open.
OSS Growth Hacker	0	2	1	Strong ecosystem unlock -- marketplace dep flywheel is real -- but docs leave 'registered marketplace' undefined, creating a dead-end for plugin authors who want to adopt this.
Auth Expert	0	2	1	Auth plumbing is structurally correct; two recommended fixes: tighten the type annotation and surface auth errors instead of silently dropping deps.
Doc Writer	1	2	1	Marketplace dep docs are mostly consistent, but a string-shorthand form (plugin@marketplace#ref) documented in dependencies.md is absent from manage-dependencies.md -- blocking gap.
Test Coverage Expert	1	2	1	Transitive marketplace deps and lockfile round-trip lack tests at required tier; install-path guard and to_apm_yml_entry have no integration coverage.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Supply Chain Security Expert] (blocking-severity) Resolve marketplace plugin to commit SHA at install time; write SHA (not branch/tag) to lockfile -- Missing SHA pin breaks the portability-by-manifest and secure-by-default promises. Two installs from the same lockfile can produce different code. Evidence: blocking finding with no test coverage for the correct behavior.
2. [Python Architect + Supply Chain + Auth Expert] (blocking-severity) Replace bare except Exception in _resolve_marketplace_dep with typed catches; re-raise auth-class failures as hard errors -- Five panelists converged on this finding. Auth failures silently drop dependencies and continue install -- fail-open behavior on a security boundary. Fix is narrowly scoped and unambiguous.
3. [Test Coverage Expert] (blocking-severity) Add integration test: apm install on marketplace dep writes resolved git coordinates (not _marketplace/... placeholder) to lockfile -- evidence.outcome=missing on portability-by-manifest + secure-by-default surface. No test would catch placeholder serialization regression.
4. [Doc Writer] Reconcile string shorthand plugin@marketplace#ref between dependencies.md and manage-dependencies.md -- either add it to manage-dependencies.md or add a Planned callout in dependencies.md -- Public-facing doc inconsistency. Resolve in this PR to avoid drift.
5. [OSS Growth Hacker] Open follow-up issue: document what a 'registered marketplace' is and how to register one -- Plugin authors cannot adopt marketplace deps without this. The ecosystem flywheel stalls at the first friction point. Highest-leverage post-ship action.

Architecture

classDiagram
    direction LR

    class DependencyReference {
        <<ValueObject>>
        +repo_url str
        +is_local bool
        +is_parent_repo_inheritance bool
        +is_marketplace bool
        +marketplace_name str | None
        +marketplace_plugin_name str | None
        +get_install_path(apm_modules_dir) Path
        +get_unique_key() str
        +to_apm_yml_entry() dict
        +parse_from_dict(entry) DependencyReference
        +parse(canonical_str) DependencyReference
    }

    class DependencyTree {
        <<Aggregate>>
        +nodes list[DependencyNode]
        +max_depth int
        +resolution_warnings list[str]
        +add_node(node) None
    }

    class DependencyNode {
        <<ValueObject>>
        +dependency_ref DependencyReference
        +depth int
        +parent DependencyReference | None
    }

    class APMDependencyResolver {
        <<Service>>
        +max_depth int
        -_auth_resolver object | None
        -_download_lock threading.Lock
        +resolve_dependencies(project_root) DependencyGraph
        +build_dependency_tree(root_apm_yml) DependencyTree
        +_resolve_marketplace_dep(dep_ref) DependencyReference | None
    }

    class AuthResolver {
        <<Strategy>>
        +resolve_for(host) AuthContext
    }

    class DependencyGraph {
        <<Aggregate>>
        +add_error(msg) None
        +has_errors bool
    }

    APMDependencyResolver *-- DependencyTree : builds
    APMDependencyResolver ..> DependencyReference : resolves
    APMDependencyResolver ..> AuthResolver : delegates auth
    APMDependencyResolver ..> DependencyGraph : populates
    DependencyTree *-- DependencyNode : contains
    DependencyNode *-- DependencyReference : wraps

    class DependencyReference:::touched
    class DependencyTree:::touched
    class APMDependencyResolver:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm install"]) --> B["resolve.py: APMDependencyResolver(auth_resolver=ctx.auth_resolver)"]
    B --> C["apm_resolver.py: resolve_dependencies(project_root)"]
    C --> D["build_dependency_tree(root_apm_yml)"]

    D --> E["parse root apm.yml -> initial dep_refs"]
    E --> F{"dep_ref.is_marketplace?\n(Site 1: root deps)"}
    F -- yes --> G["_resolve_marketplace_dep(dep_ref)"]
    G --> H["NET: resolve_marketplace_plugin(name, marketplace, auth_resolver)"]
    H -- success --> I["DependencyReference.parse(canonical_str)"]
    H -- Exception --> J["log warning, return None"]
    J --> K["tree.resolution_warnings.append(...)"]
    K --> L["continue - dep SILENTLY DROPPED"]
    I --> M["processing_queue.append(dep_ref, depth=1)"]
    F -- no --> M

    M --> N["BFS loop: pop dep_ref"]
    N --> O{"re_queue pass?\n(Site 2)"}
    O -- yes, is_marketplace --> G
    O -- no --> P["download + clone dep_ref"]
    P --> Q["parse sub-dep apm.yml"]
    Q --> R{"sub_dep.is_marketplace?\n(Site 3: transitive)"}
    R -- yes --> S["_resolve_marketplace_dep(sub_dep)"]
    S --> H
    R -- no --> T["processing_queue.append"]
    T --> N

    D --> U["return DependencyTree"]
    U --> V["resolve_dependencies: for w in resolution_warnings: graph.add_error(w)"]
    V --> W(["DependencyGraph returned"])

    style J fill:#ffcccc,stroke:#cc0000
    style L fill:#ffcccc,stroke:#cc0000

Loading

Recommendation

The core feature -- parsing {name: X, marketplace: Y} deps and resolving them through BFS -- is architecturally sound and fills a real gap. The auth plumbing is correct. The docs are mostly consistent. The 24 tests added are a genuine signal of author care.

However, three runtime defects must close before merge: (1) the bare-except fail-open converts auth and security failures into silent partial installs -- this is the inverse of secure-by-default; (2) the lockfile writes mutable branch/tag refs instead of commit SHAs, breaking the portability-by-manifest determinism guarantee; (3) no test proves that the _marketplace/... placeholder is not serialized to the lockfile, meaning a regression on the most critical lockfile path is invisible. These are not polish items -- they are correctness failures on promises APM has made publicly.

The fixes are narrowly scoped: typed exception catches with re-raise for auth failures, a SHA-resolution step in resolve_marketplace_plugin(), and one integration test for the lockfile write path. None of these require redesign. Recommend returning to the author with the three blockers clearly called out, and fast-tracking review once they land. The doc shorthand inconsistency should also be resolved in the same PR. The growth opportunity is real -- hold it for a clean merge.

---

Full per-persona findings

Python Architect

• [blocking] bare except Exception in _resolve_marketplace_dep silently skips deps on auth failure at src/apm_cli/deps/apm_resolver.py
  When resolve_marketplace_plugin raises an auth error (expired token, missing PAT, 401), the dep is silently dropped. Auth failures must not be downgraded to warnings.
  Suggested: Replace except Exception as exc with specific exception types. Re-raise auth-class failures as hard errors.

• [recommended] BFS marketplace resolution block copy-pasted 3x -- extract to a helper at src/apm_cli/deps/apm_resolver.py
  The pattern appears verbatim three times in build_dependency_tree. Any future change must be made in three places.
  Suggested: Extract to _apply_marketplace_resolution(self, dep_ref, tree, context: str) -> DependencyReference | None.

• [recommended] is_marketplace: bool flag + sentinel repo_url is a latent footgun as the model grows at src/apm_cli/models/dependency/reference.py
  Every new method on DependencyReference must independently guard against the unresolved state. to_lockfile_entry(), get_cache_key(), and future methods are unguarded.

• [recommended] resolution_warnings on DependencyTree conflates soft advisory vs hard missing dep at src/apm_cli/deps/dependency_graph.py
  A marketplace dep that fails to resolve is a hard missing dependency, not a warning.
  Suggested: Rename to resolution_errors: list[str].

• [nit] auth_resolver: object | None -- use the concrete AuthResolver | None type or a Protocol at src/apm_cli/deps/apm_resolver.py

CLI Logging Expert

• [recommended] Resolution failure is double-logged: once via _logger.warning, once via resolution_warnings -> graph.add_error at src/apm_cli/deps/apm_resolver.py
  A user sees the failure twice with different wording.
  Suggested: Change _logger.warning(...) in _resolve_marketplace_dep to _logger.debug(...).

• [recommended] resolution_warnings fed to graph.add_error() -- warning promoted to error, may block installs that should degrade gracefully at src/apm_cli/install/phases/resolve.py
  Suggested: Use graph.add_warning(warning) if the method exists, or add it. Reserve add_error for failures that must block the install.

• [nit] No actionable fix hint in the user-facing resolution failure message at src/apm_cli/deps/apm_resolver.py

• [nit] Two identical warning message blocks for BFS sites 1 and 2 vs a slightly better block for transitive deps at src/apm_cli/deps/apm_resolver.py

DevX UX Expert

• [recommended] Silent skip on marketplace resolution failure violates the 'failure mode is the product' contract at src/apm_cli/deps/apm_resolver.py
  npm, pip, and cargo all treat a missing transitive dep as a hard stop or prominently coloured warning. The user finishes install, their environment is silently missing a dep, with no next step.
  Suggested: Surface each resolution_warnings entry via the CLI logging layer after the install phase, and consider adding --marketplace-strict flag.

• [recommended] No install-time progress feedback that marketplace resolution is happening at src/apm_cli/deps/apm_resolver.py
  Marketplace lookup involves a network round-trip. A user in CI sees silence before either succeeding or silently skipping the dep.
  Suggested: Emit a progress line such as Resolving marketplace dep 'sec-check' from 'acme-plugins'... before the resolve call.

• [nit] The synthetic _marketplace/<mkt>/<name> sentinel URL could leak into user-facing error messages at src/apm_cli/models/dependency/reference.py

• [nit] Docs show marketplace as a table row in the string-forms table, but it is not a string form at docs/src/content/docs/consumer/manage-dependencies.md

Supply Chain Security Expert

• [blocking] Lockfile records mutable ref (branch/tag), not a commit SHA -- determinism guarantee broken at src/apm_cli/marketplace/resolver.py
resolve_marketplace_plugin() returns owner/repo#main (a branch that can be force-pushed). Two installs from the same lockfile can produce different code.
  Suggested: Resolve the ref to a commit SHA via the GitHub API and embed the SHA in the lockfile record.

• [blocking] bare Exception catch in _resolve_marketplace_dep converts any security error to silent fail-open at src/apm_cli/deps/apm_resolver.py
  MarketplaceFetchError (auth failure, 403), PluginNotFoundError, and ValueError are all caught at WARNING, then the dep is silently dropped. Security contract requires fail-closed.
  Suggested: Re-raise MarketplaceFetchError and PluginNotFoundError; only swallow transient network errors when a fresh cache entry exists.

• [recommended] Stale-while-revalidate serves poisoned cache indefinitely on network errors at src/apm_cli/marketplace/client.py
  When network fetch fails, _read_stale_cache() is called with no TTL cap. A one-time cache poisoning persists indefinitely.
  Suggested: Cap stale cache serving at a configurable maximum age (e.g. 24h).

• [recommended] No content-integrity verification on fetched marketplace.json at src/apm_cli/marketplace/client.py
  APM trusts whatever the GitHub Contents API returns. A compromised maintainer or MITM can redirect a plugin to an attacker-controlled repo.

• [recommended] Transitive marketplace dep resolution has unbounded blast radius with no explicit depth limit for marketplace hops at src/apm_cli/deps/apm_resolver.py
  Suggested: Add a configurable max_marketplace_hops (default 1 or 2).

• [nit] Synthetic repo_url _marketplace/... could collide with a real GitHub org named _marketplace

• [nit] Shadow detection swallows all exceptions silently, hiding detector bugs at src/apm_cli/marketplace/resolver.py

OSS Growth Hacker

• [recommended] Docs never explain what a 'registered marketplace' is or how to register one at docs/src/content/docs/consumer/manage-dependencies.md
  Both manage-dependencies.md and manifest-schema.md say 'registered marketplace name' but give no link, no command, no path forward.
  Suggested: Add one sentence + link: 'A marketplace must be registered in your apm.yml or a parent config before it can be referenced here.'

• [recommended] No story hook differentiating marketplace deps from git deps -- the table row alone won't drive adoption
  Plugin authors need a one-liner: 'Use marketplace deps so consumers install your plugin by name, not by URL.'

• [nit] The error/warning UX for unresolved marketplace dep is undocumented

Auth Expert

• [recommended] auth_resolver: object | None should be AuthResolver | None throughout the call chain at src/apm_cli/deps/apm_resolver.py
  The loose object type propagates through the call chain. Any non-AuthResolver object lacking try_with_fallback will crash at runtime rather than at the call site.

• [recommended] Bare except Exception silently converts auth failures into dropped dependencies at src/apm_cli/deps/apm_resolver.py
  A 401/403 from a private marketplace registry causes the dep to be silently omitted with no install-time error. An operator with a misconfigured PAT sees only a WARNING.
  Suggested: Catch MarketplaceFetchError and PluginNotFoundError specifically and re-raise on any HTTP 401/403 signal.

• [nit] Proxy path (_try_proxy_fetch) silently ignores the passed auth_resolver with no comment at src/apm_cli/marketplace/client.py

Doc Writer

• [blocking] String shorthand plugin@marketplace#ref documented in dependencies.md but absent from manage-dependencies.md at docs/src/content/docs/consumer/manage-dependencies.md
  The skill reference has a full 'Marketplace ref override' section. manage-dependencies.md shows only the object form. If the shorthand is implemented, manage-dependencies.md is missing it. If not yet implemented, dependencies.md needs a Planned callout.

• [recommended] Resolution failure path is not documented for marketplace deps at docs/src/content/docs/reference/manifest-schema.md
  manifest-schema.md describes the happy path but not what the user sees when resolution fails.
  Suggested: Add: 'If the plugin is not found or the registry is unreachable, apm install exits with a non-zero status and prints the unresolved entry.'

• [recommended] manage-dependencies.md has no cross-link from the marketplace entry to manifest-schema.md field definitions at docs/src/content/docs/consumer/manage-dependencies.md

• [nit] manage-dependencies.md table: 'Object form' row description no longer covers all object variants

Test Coverage Expert

• [recommended] No test for transitive marketplace dep (a dep's dep is marketplace) at tests/test_apm_resolver.py
  The BFS at 3 sites claim includes resolving marketplace deps discovered as sub-deps. No test covers this path.
  Proof (missing at integration-with-fixtures): tests/test_apm_resolver.py::test_transitive_marketplace_dep_resolved -- proves: When a resolved package itself lists a marketplace dep, apm resolves it transitively [portability-by-manifest, devx]

• [blocking] No test that lockfile records resolved git coordinates, not the _marketplace/... placeholder at tests/integration/test_marketplace_plugin_integration.py
  Probed all tests/ for lockfile+marketplace patterns -- no match. If the lockfile write path silently stores the placeholder, subsequent installs would fail or diverge.
  Proof (missing at integration-with-fixtures): tests/integration/test_marketplace_plugin_integration.py::test_marketplace_dep_lockfile_records_resolved_coordinates -- proves: apm install writes resolved git coordinates to the lockfile [portability-by-manifest, secure-by-default]

• [recommended] No integration test for installing an unresolved marketplace dep (get_install_path guard) at tests/integration/test_marketplace_plugin_integration.py
  Unit test covers the guard in isolation. No integration test exercises the install command call path through the guard.
  Proof (missing at integration-with-fixtures): tests/integration/test_marketplace_plugin_integration.py::test_install_unresolved_marketplace_dep_raises_clear_error [devx, secure-by-default]

• [nit] to_apm_yml_entry() guard has no integration test (unit-only coverage) at tests/test_apm_package_models.py
  Proof (passed at unit): tests/test_apm_package_models.py::test_marketplace_dep_to_apm_yml_entry_raises -- proves: Calling to_apm_yml_entry() on unresolved marketplace dep raises [devx]

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• feat(deps): support marketplace dependencies in plugin.json #1422 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1422 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1422 · ● 3.5M · ◷

[danielmeppiel]

Analysing this vs Claude's latest addition to support plugin dependencies - https://code.claude.com/docs/en/plugin-dependencies#declare-a-dependency-with-a-version-constraint, brb

[stbenjam]

After reviewing the PR, we have a concern about the dependency format this introduces. The {name: ..., marketplace: ...} shape in plugin.json dependencies appears to be a convention adopted by community plugin authors (e.g. openshift-eng/ai-helpers) rather than part of the official Claude Code marketplace specification.

Right, as I think Daniel noted this is part of the marketplace spec, I should have linked it. That said, this PR may not implement the full spec as defined, I can make it more complete if this is something you're interested in.

https://code.claude.com/docs/en/plugin-dependencies#declare-a-dependency-with-a-version-constraint