feat(marketplace): support local paths, file:// URIs, and generic git hosts

[sergio-sisternes-epam]

TL;DR

apm marketplace add now accepts local paths, file:// URIs, SSH URLs, and HTTPS URLs to any git host -- not just GitHub owner/repo shorthand. The change is URL-first under the hood: a small parser classifies each source into local | github | gitlab | git and dispatches through a fetcher table, so the existing GitHub Contents API path is untouched while local and self-hosted git become first-class citizens. Verified end-to-end against a public GitHub marketplace exposing 64 plugins and bare/working-tree/file:// repos in one mixed registry.

Problem (WHY)

• Marketplace registration was hard-wired to the GitHub Contents API. Anyone hosting an APM package on a local disk, self-hosted git, GitLab, Gitea, Bitbucket Server, or Azure DevOps could not register a marketplace at all.
• The documented workaround was to hand-edit ~/.apm/marketplaces.json and pre-seed ~/.apm/cache/marketplace/ with a long TTL so APM would never attempt a network fetch. This silently broke after every apm pack because the cache went stale.
• --host only accepted FQDNs, so even sentinel values like --host local were rejected.
• The authoring side (apm marketplace init, apm pack) already worked against local repos. Only the consumer side (add, browse, update, install) lagged, which is exactly the asymmetry that pushed users to manual cache editing.

Note

The change keeps Progressive Disclosure for trust: only sources that actually hit the network trigger the trust prompt. Local and file:// sources never prompt. "Context arrives just-in-time, not just-in-case."

Approach (WHAT)

URL-first model with a small fetcher dispatch table:

Layer	Before	After
Source model	owner/repo/host/branch fields	MarketplaceSource with url, kind, ref; legacy fields kept as backward-compat properties
CLI parser	GitHub shorthand only	_parse_marketplace_source returns (url, kind, embedded_host) covering 6 source forms
Fetcher	Single hard-coded GitHub Contents API call	_FETCHERS dispatch on kind: _fetch_local / _fetch_via_api / _fetch_git
Resolver	Single GitCache path for every plugin	kind=local fast-paths to on-disk; kind=git/gitlab uses GitCache with hooks/submodules disabled; kind=github unchanged
marketplaces.json	No discriminator	kind field added; legacy entries migrate lazily on read

Six accepted source forms across add, list, browse, update, and install:

apm marketplace add owner/repo                                   # GitHub shorthand (unchanged)
apm marketplace add github.com/owner/repo                        # GitHub explicit
apm marketplace add https://gitlab.com/group/repo
apm marketplace add https://dev.azure.com/org/proj/_git/repo
apm marketplace add git@gitea.example.com:org/repo.git
apm marketplace add /srv/marketplaces/my-mkt                # local working tree or bare repo
apm marketplace add file:///srv/marketplaces/my-mkt.git

Implementation (HOW)

File	What changed
src/apm_cli/marketplace/models.py	MarketplaceSource rewritten URL-first. kind / display_source / ref properties added. Multi-segment owners (e.g. ADO org/proj/_git/repo) preserved by _extract_owner_repo_from_url. Legacy fields remain readable for older marketplaces.json entries.
src/apm_cli/marketplace/client.py	New _FETCHERS dispatch table. _fetch_local (bare repo via git show, working tree via direct read), _fetch_git (generic remote via git archive), _fetch_via_api (existing GitHub path, unchanged). _validate_ref rejects unsafe refs before any subprocess call.
src/apm_cli/commands/marketplace/__init__.py	_parse_marketplace_source classifies the input. add() rewritten: SOURCE metavar, --ref (with --branch alias for backward compat), 6-example epilog, kind-scoped trust gate. list_cmd shows display_source and renames the column to "Ref".
src/apm_cli/marketplace/resolver.py	resolve_marketplace_plugin branches on source.kind. Local marketplaces fast-path through _resolve_local_relative_source (no GitCache). Generic-git uses GitCache; _marketplace_https_git_url now prefers source.url so ADO _git/ segments and GitLab nested groups round-trip correctly.
src/apm_cli/cache/git_cache.py	Hooks disabled (core.hooksPath=/dev/null) and submodules not auto-fetched when materializing a generic-git marketplace. On-path hardening because generic git is newly trusted.
Tests	50 new unit tests across test_client_local.py, test_client_git.py, test_parser.py, test_resolver_local_git.py, test_legacy_migration.py, test_git_cache_hardening.py. 6 new integration tests (test_marketplace_add_local.py, test_marketplace_add_generic_git.py, test_marketplace_install_local.py).
Docs	reference/cli/marketplace.md synopsis + add section rewritten with 6 forms + trust callout + ADO note. consumer/installing-from-marketplaces.md gains a "Local and self-hosted marketplaces" section. apm-usage/commands.md row updated. CHANGELOG.md Unreleased entry.
E2E	scripts/e2e/marketplace_local_e2e.sh: not wired into CI (keeps CI hermetic) but provides one-shot reproduction against a real bare repo.

Diagram

Source classification and fetcher dispatch:

flowchart LR
    SRC["apm marketplace add SOURCE"] --> P{"_parse_marketplace_source"}
    P -->|"owner/repo"| GH["kind=github"]
    P -->|"https://gitlab.com/..."| GL["kind=gitlab"]
    P -->|"ssh / generic https"| GG["kind=git"]
    P -->|"/path or file://"| LO["kind=local"]
    GH --> FA["_fetch_via_api (Contents API)"]
    GL --> FG["_fetch_git (archive / ls-remote)"]
    GG --> FG
    LO --> FL["_fetch_local (git show / direct read)"]
    FA --> MJ["marketplace.json"]
    FG --> MJ
    FL --> MJ

Loading

Trade-offs

• Generic git uses git archive / GitCache, not a shallow clone per call. Higher first-use latency than the GitHub Contents API, but no per-host adapter to maintain. The Contents API path is still used for kind=github so existing users see no regression.
• kind discriminator persisted to marketplaces.json instead of inferred on every read. Slightly more state to migrate, but it means the trust prompt and fetcher dispatch are deterministic without re-parsing the source string on each command. Legacy entries are migrated lazily, so no upgrade step is required.
• Local sources skip the trust prompt. Justified because the user already controls the path; mirroring the existing apm install behavior for local paths. Documented in reference/cli/marketplace.md so the boundary is explicit.
• E2E script kept out of CI. Keeps CI hermetic and avoids depending on a live external repo. Reproduction is one bash scripts/e2e/marketplace_local_e2e.sh away.

Benefits

1. Self-hosted git (GitLab, Gitea, Bitbucket Server, Azure DevOps) can now publish APM marketplaces with no manual cache editing.
2. Offline-first / air-gapped workflows: a local bare repo is a valid marketplace source.
3. The "manual edit ~/.apm/marketplaces.json then pre-seed cache" workaround is retired. apm pack -> apm marketplace update is now a closed loop.
4. GitHub registrations are unchanged (Contents API path still used). 64-plugin smoke against a public GitHub marketplace passed.
5. Generic-git path is hardened on the way through: hooks disabled, submodules not auto-fetched.

Validation

Important

Lint chain green on the merge commit. 15,036 unit tests + 6 new integration tests passing. Manual end-to-end covers all four source classes side-by-side.

Test counts

uv run --extra dev pytest tests/unit tests/integration/test_marketplace_add_local.py tests/integration/test_marketplace_add_generic_git.py tests/integration/test_marketplace_install_local.py -q
15042 passed, 1 skipped, 19 warnings, 36 subtests passed in 42.50s

Lint chain (CI-mirror)

uv run --extra dev ruff check src/ tests/                 # All checks passed!
uv run --extra dev ruff format --check src/ tests/        # 1066 files already formatted
uv run --extra dev python -m pylint --disable=all --enable=R0801 \
    --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ # 10.00/10
bash scripts/lint-auth-signals.sh                          # auth-signal lint clean

Manual end-to-end (mixed registry)

=== Test 1: local working dir ===
[+] Marketplace 'e2e-working' registered (1 plugins)
=== Test 2: local bare repo ===
[+] Marketplace 'e2e-bare' registered (1 plugins)
=== Test 3: file:// URI ===
[+] Marketplace 'e2e-fileuri' registered (1 plugins)
=== Test 4: GitHub regression (public marketplace, 64 plugins) ===
[+] Marketplace 'gh-demo' registered (64 plugins)

apm marketplace list:
| e2e-working    | ~/upstream                | main | marketplace.json                |
| e2e-bare       | ~/bare.git                | main | marketplace.json                |
| e2e-fileuri    | ~/bare.git                | main | marketplace.json                |
| gh-demo        | owner/repo                | main | .claude-plugin/marketplace.json |

apm marketplace browse / update on all four: OK

Scenario evidence

User-promise scenario (APM principle)	Test
Register a marketplace from a local working tree (consumer reach)	tests/integration/test_marketplace_add_local.py::test_add_local_marketplace[working]
Register a marketplace from a bare repo (consumer reach)	tests/integration/test_marketplace_add_local.py::test_add_local_marketplace[bare]
Register from file:// URI (consumer reach)	tests/integration/test_marketplace_add_generic_git.py::test_add_file_uri_marketplace
Resolve and install a plugin from a local marketplace (closed loop authoring -> install)	tests/integration/test_marketplace_install_local.py::test_install_resolves_to_local_path
GitHub owner/repo shorthand still uses Contents API (no regression)	tests/unit/marketplace/test_marketplace_client.py (existing suite, all green)
Legacy marketplaces.json entries migrate without user action (upgrade safety)	tests/unit/marketplace/test_legacy_migration.py
Generic-git marketplaces materialize without firing hooks or submodules (supply-chain hardening)	tests/unit/cache/test_git_cache_hardening.py

How to test

1. gh pr checkout 1476
2. uv run --extra dev apm marketplace add <any-github-owner/repo> --name gh-demo -- expect registered (N plugins).
3. Create a local bare repo with a marketplace.json, then uv run --extra dev apm marketplace add /path/to/bare.git --name local-bare -- expect registered (N plugins) with no trust prompt.
4. uv run --extra dev apm marketplace list -- expect both entries with the new Ref column.
5. bash scripts/e2e/marketplace_local_e2e.sh for the full end-to-end reproduction.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR makes the marketplace consumer flow host-agnostic by moving to a URL-first marketplace source model, adding fetcher dispatch for local and generic git sources, and hardening GitCache subprocess invocations to reduce supply-chain risk.

Changes:

• Extend apm marketplace add parsing to accept local paths, file:// URIs, SSH SCP-like URLs, and generic HTTPS git URLs (routing by derived source.kind).
• Add/adjust marketplace fetching and resolution so local marketplaces can resolve relative plugin sources to on-disk paths, while generic git hosts use GitCache.
• Harden GitCache git subprocesses by disabling hooks and preventing submodule recursion; update docs and add unit/integration coverage.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
tests/unit/marketplace/test_resolver_local_git.py	New unit coverage for local vs generic-git resolution behavior.
tests/unit/marketplace/test_parser.py	New unit coverage for URL-first marketplace source parsing.
tests/unit/marketplace/test_marketplace_models.py	Update model serialization expectations for URL-first sources.
tests/unit/marketplace/test_marketplace_commands.py	Update CLI command tests for accepting generic hosts and new routing.
tests/unit/marketplace/test_marketplace_commands_surface.py	Update surface tests for new _parse_marketplace_repo return shape (now URL-first).
tests/unit/marketplace/test_marketplace_commands_phase3.py	Same surface updates for the phase3 test suite.
tests/unit/marketplace/test_marketplace_client.py	Update client tests to route generic hosts through git fetcher (no API).
tests/unit/marketplace/test_legacy_migration.py	New tests for legacy marketplaces.json upgrade/round-trip behavior.
tests/unit/marketplace/test_client_local.py	New unit tests for local fetcher behavior (bare repo vs working dir, traversal, ref validation).
tests/unit/marketplace/test_client_git.py	New unit tests for generic git fetcher (GitCache) behavior and dispatch table invariants.
tests/unit/marketplace/fixtures/legacy_marketplaces.json	Fixture for legacy registry migration testing.
tests/unit/cache/test_git_cache_hardening.py	New tests asserting hooks/submodules are disabled in all git subprocess calls.
tests/integration/test_marketplace_install_local.py	New integration test validating local marketplace resolution to on-disk install paths.
tests/integration/test_marketplace_add_local.py	New integration tests for adding local marketplaces via path and file://.
tests/integration/test_marketplace_add_generic_git.py	New integration test intended to cover generic git registration via file://.
src/apm_cli/marketplace/resolver.py	Add kind-aware explicit git+path handling and local fast-path resolution.
src/apm_cli/marketplace/models.py	Introduce URL-first MarketplaceSource shape and derived properties (kind/local_path/display_source).
src/apm_cli/marketplace/client.py	Add _FETCHERS dispatch and implement _fetch_git/_fetch_local alongside API fetchers.
src/apm_cli/commands/marketplace/init.py	Update CLI parsing/flags (SOURCE, --ref, hidden --branch) and list output columns.
src/apm_cli/cache/git_cache.py	Add _safe_git_args() and apply to all git subprocess calls; disable submodule recursion.
scripts/e2e/marketplace_local_e2e.sh	Manual (non-CI) reproduction script for local marketplace workflows.
packages/apm-guide/.apm/skills/apm-usage/commands.md	Update guide skill docs for apm marketplace add SOURCE and new flags.
docs/src/content/docs/reference/cli/marketplace.md	Update CLI reference for new SOURCE forms and trust boundary behavior.
docs/src/content/docs/consumer/installing-from-marketplaces.md	Add docs for local/self-hosted marketplaces and lockfile implications.
CHANGELOG.md	Add changelog entries for new marketplace source support and GitCache hardening.

[github-actions]

APM Review Panel: ship_with_followups

Multi-host marketplace unlocks enterprise git-anywhere adoption; ship with 5 targeted fixes for the None/None UX bug, silent spinner, missing env hardening, gitlab retry noop, and raw stderr leak.

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

This PR crosses a genuine moat: APM graduates from a GitHub-only tool to one that works wherever git works. The dispatch-table architecture is clean, the auth chain is sound (GitHub PATs correctly scoped, ADO threaded, no credential forwarding to wrong hosts), and the security hardening on GitCache subprocess calls (hooks disabled, submodule.recurse=false) is explicitly test-verified. The panel reached consensus -- no blocking findings, no specialist disagreement on severity.

Five items are quickly fixable in-PR and should be: (1) remove() displaying None/None for local/git sources is a visible UX regression today -- one-line fix using display_source; (2) logger.start fires after the 5-30s git fetch, leaving users staring at a blank terminal -- move it two lines up; (3) _fetch_local_via_git_show skips git_subprocess_env(), the only subprocess call in the new code that does not apply the hardening env vars -- a defense-in-depth gap the supply-chain expert correctly flags; (4) the gitlab retry lambda discards the resolved token, making the retry a correctness noop for gitlab auth; (5) raw git stderr forwarded verbatim into MarketplaceFetchError is inconsistent with the _sanitize_url discipline applied everywhere else. All five are single-file, low-risk changes.

The generic-git integration test gap is real -- the named generic_git integration test actually exercises the local fetcher, so the HTTPS/SSH path has no floor-tier integration verification -- but it requires a network-capable fixture or local git daemon and is correctly tracked as a follow-up rather than a merge gate. The _SAFE_REF_RE dotdot gap and the SCP regex duplication are latent correctness risks with no current exploit path; both belong in the follow-up queue. The OSS growth signal is strong: self-hostable marketplace-in-a-git-repo is a zero-infra enterprise story that converts air-gapped GitLab and ADO shops without requiring any infrastructure beyond a git remote.

Dissent. No cross-persona disagreement on severity. The test-coverage-expert finding on _fetch_git integration coverage carries an evidence.outcome: missing on a portability-by-manifest surface, which per arbitration rules ranks above recommended opinion findings. However, the remediation requires a live git remote fixture; the panel agrees it is a post-merge tracked issue rather than a merge gate. The supply-chain finding on _SAFE_REF_RE carries evidence.outcome: manual -- treated as opinion-tier; the dotdot path is not exploitable today and the fix is a follow-up.

Aligned with: Portable by manifest -- dispatch table covers all four source kinds with the GitHub Contents API path unchanged; Secure by default -- GitCache hardening is test-verified passed, two fixable gaps remain; Multi-harness / multi-host -- ADO, GitLab, Gitea, Bitbucket Server all reachable; OSS community driven -- the apm pack -> marketplace closed-loop story is missing from shipped docs, high-leverage follow-up; Pragmatic as npm -- three one-line UX fixes (None/None prompt, --host help text, logger.start placement) close the gap.

Growth signal. Launch beat: 'APM now works wherever git works.' Secondary angle: self-hostable marketplace-in-a-git-repo is the zero-infra enterprise story -- no registry, no server, just a git remote. Priority follow-up: 'Build and host your own AI package marketplace' guide closes the pack->marketplace->install loop and is the single highest-leverage contributor funnel moment this PR introduces.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	3	1	Good dispatch-table shape; two bugs (SCP regex diverges across modules, gitlab retry ignores resolved token) and a kind duplication to clean up.
CLI Logging Expert	0	2	2	logger.start fires after the slow work in add(); remove prompt uses owner/repo which is None for local/git sources. Other output is clean.
DevX UX Expert	0	3	3	Solid ergonomic lift; 3 UX gaps worth fixing before merge: misleading epilog implies --name required, Path column is noise in default list, --host help text is silent about being ignored for URLs.
Supply Chain Security Expert	0	2	1	Two defense-in-depth gaps: _SAFE_REF_RE admits '..' in refs (not exploitable today); git_subprocess_env() not applied in _fetch_local_via_git_show. No blocking exploits found.
OSS Growth Hacker	0	2	2	Major enterprise adoption unlock: APM now works everywhere git works. Strong docs. Missing the pack->marketplace closed-loop story in shipped docs.
Auth Expert	0	1	1	Auth chain is sound: AuthResolver consulted for all generic-git paths, GitHub PATs not forwarded to non-GitHub hosts, ADO correctly threaded. Two defense-in-depth nits.
Doc Writer	0	2	3	Docs are accurate and well-structured; one redundant --host in GitLab example, one internal class name leaked, one CHANGELOG ordering nit.
Test Coverage Expert	0	2	4	Hardening + local path coverage is solid; _fetch_git generic-git path has no integration test at floor tier; new integration tests missing declarative APM markers per instructions.

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

Top 5 follow-ups

1. [CLI Logging Expert] Fix remove() confirmation prompt to use source.display_source instead of source.owner/source.repo (which are None for local and generic-git sources) -- Visible UX regression today: users see 'Remove marketplace foo (None/None)?' for any non-GitHub source. One-line fix, high user-facing impact.
2. [Supply Chain Security Expert] Pass env=git_subprocess_env() to the subprocess.run call inside fetch_local_via_git_show -- Only git subprocess in the new code that skips the hardening env vars; could allow interactive credential prompts or honor attacker-supplied GIT* env vars from parent process.
3. [Python Architect] Fix _fetch_gitlab retry lambda to actually use the token resolved by the outer fallback chain -- The retry is currently a correctness noop for gitlab auth; on token rotation or fallback scenarios the retry silently re-runs with no token, defeating the retry contract.
4. [Auth Expert] Run CalledProcessError.stderr through a _sanitize_url pass before embedding in MarketplaceFetchError in _fetch_git -- Custom external credential helpers may write credentials to stderr; inconsistent with the _sanitize_url discipline applied everywhere else in the codebase.
5. [Test Coverage Expert] Add a floor-tier integration test for _fetch_git (generic-git HTTPS/SSH via GitCache) using a local git daemon or loopback bare repo fixture -- The named generic_git integration test actually exercises the local fetcher; the HTTPS/SSH path for non-GitHub/non-GitLab hosts has no integration verification. This is the core user promise of the PR (vendor-neutral marketplace sources) and inherits portability-by-manifest criticality.

Architecture

classDiagram
    direction LR
    class MarketplaceSource {
      <<ValueObject>>
      +name str
      +url str
      +ref str
      +path str
      +owner str
      +repo str
      +host str
      +kind str
      +local_path str
      +display_source str
      +to_dict() dict
      +from_dict() MarketplaceSource
    }
    class MarketplaceManifest {
      <<ValueObject>>
      +name str
      +plugins tuple
      +find_plugin() MarketplacePlugin
      +search() list
    }
    class MarketplacePlugin {
      <<ValueObject>>
      +name str
      +source Any
      +matches_query() bool
    }
    class MarketplaceClient {
      <<IOBoundary>>
      +fetch_marketplace() MarketplaceManifest
      +search_marketplace() list
      +clear_marketplace_cache() int
    }
    class _FETCHERS {
      <<DispatchTable>>
      github : _fetch_github
      gitlab : _fetch_gitlab
      git : _fetch_git
      local : _fetch_local
    }
    class _fetch_via_api {
      <<Template>>
      +url_builder Callable
      +header_builder Callable
      +parse_response Callable
    }
    class AuthResolver {
      <<Strategy>>
      +classify_host() HostInfo
      +try_with_fallback()
    }
    class GitCache {
      <<IOBoundary>>
      +get_checkout() Path
    }
    class MarketplaceResolver {
      <<Strategy>>
      +resolve_marketplace_plugin()
    }
    note for _FETCHERS "Strategy dispatch table: source.kind -> fetcher fn"
    note for MarketplaceSource "Frozen dataclass; kind property lazy-imports AuthResolver on every access"
    note for _fetch_via_api "Retry block in _fetch_gitlab discards resolved token (line 343) -- noop retry"
    MarketplaceClient ..> _FETCHERS : dispatches via source.kind
    MarketplaceClient ..> MarketplaceSource : reads
    MarketplaceClient ..> MarketplaceManifest : returns
    _FETCHERS *-- _fetch_via_api
    _fetch_via_api ..> AuthResolver : auth via try_with_fallback
    MarketplaceSource ..> AuthResolver : kind calls classify_host
    MarketplaceResolver ..> MarketplaceSource : reads kind
    MarketplaceManifest *-- MarketplacePlugin
    class MarketplaceSource:::touched
    class _FETCHERS:::touched
    class MarketplaceResolver:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A([apm marketplace add SOURCE]) --> B[_parse_marketplace_source]
    B --> C{local path or file URI?}
    C -- yes --> D[kind=local]
    C -- no --> E{SCP-like git@host:path?}
    E -- yes --> F[_host_kind_to_fetcher_kind]
    E -- no --> G{HTTPS URL?}
    G -- yes --> F
    G -- no --> H{bare owner/repo token?}
    H -- yes --> I[inject github.com host]
    I --> F
    F --> J[MarketplaceSource constructed]
    D --> J
    J --> K[marketplaces.json updated]
    K --> L([add complete])

    subgraph fetch_marketplace [fetch_marketplace client.py]
      M([fetch called]) --> R{_FETCHERS dispatch on source.kind}
      R -- github --> S[GitHub Contents API]
      R -- gitlab --> T[GitLab REST v4 -- retry noop bug]
      R -- git --> U[git sparse-clone via GitCache]
      R -- local --> V{bare repo?}
      V -- yes --> W[git show ref:file]
      V -- no --> X[direct read + ensure_path_within]
      S --> Y[parse_marketplace_json]
      T --> Y
      U --> Y
      W --> Y
      X --> Y
      Y --> Z([MarketplaceManifest])
    end

Loading

Recommendation

Ship after the author addresses the five in-PR fixes: the None/None remove() prompt, the logger.start placement, the missing git_subprocess_env() in _fetch_local_via_git_show, the gitlab retry noop, and the raw stderr leak into MarketplaceFetchError. All are single-file low-risk changes. The generic-git integration test gap, the _SAFE_REF_RE dotdot issue, the SCP regex consolidation, and the docs growth items (pack->marketplace closed loop, lockfile portability caveat prominence) are tracked follow-ups -- real but not merge gates. The architectural foundation is sound, the auth chain is correct, and the security hardening on GitCache is test-verified. This PR is a strategic unlock for enterprise and air-gapped adoption and should not wait.

---

Full per-persona findings

Python Architect

• [recommended] _SCP_LIKE_RE defined twice with divergent path constraints -- latent classification split at src/apm_cli/marketplace/models.py:21
  models.py line 21 defines the path capture group as [^/].* (path must not start with slash, SCP convention), while commands/marketplace/init.py line 412 uses [^\s:]+ (no whitespace or colon, but slash is allowed). An SCP URL like git@host:/absolute/path matches the models.py pattern but NOT the commands pattern. This means source.kind and the kind returned by _parse_marketplace_source can diverge for the same URL.
  Suggested: Extract a single _SCP_LIKE_RE into a shared helper module (e.g. marketplace/_url_utils.py) and import it in both files. Unify the path group to [^\s]+ to cover both SCP conventions.

• [recommended] _fetch_gitlab retry lambda discards the resolved token -- retry is an auth-resolution noop at src/apm_cli/marketplace/client.py:343
  client.py lines 343-352: the retry block calls auth_resolver.try_with_fallback with a lambda(token, _env) that ignores both arguments and re-calls _fetch_via_api, which internally calls try_with_fallback again. The token resolved by the outer fallback chain is never used; the retry cannot supply a different token than the first attempt.
  Suggested: Remove the retry block and add unauth_first=False to the single _fetch_via_api call. If retry parity is needed, restructure so the lambda receives and uses the resolved token directly.

• [recommended] _host_kind_to_fetcher_kind in commands duplicates the authoritative MarketplaceSource.kind mapping at src/apm_cli/commands/marketplace/__init__.py:395
  Two independent classification paths must stay in sync; if AuthResolver.classify_host semantics change (e.g. a new host kind is added), both paths need updating.
  Suggested: Rename _host_kind_to_fetcher_kind to _is_trusted_for_api_add and inline it into the validation block only. All downstream fetch dispatch should exclusively use source.kind.

• [nit] MarketplaceSource.kind does a lazy AuthResolver import on every property access at src/apm_cli/marketplace/models.py:148
  The hidden dependency makes the property harder to unit-test (callers cannot inject a mock without patching sys.modules).
  Suggested: Compute kind once in post_init via object.setattr and expose via a @property.

CLI Logging Expert

• [recommended] logger.start('Registering...') fires after all slow I/O in add(), leaving user with blank output during git fetch at src/apm_cli/commands/marketplace/__init__.py:654
  Lines 610 (_auto_detect_path) and 627 (fetch_marketplace) run before logger.start at line 654. For generic-git sources this involves a sparse clone that can take 5-30s. The user sees nothing until the work is already done.
  Suggested: Move logger.start to just before the _auto_detect_path call (~line 603), mirroring browse() ordering.

• [recommended] remove() confirmation prompt references source.owner/source.repo which are None for local paths and generic-git URLs at src/apm_cli/commands/marketplace/__init__.py:894
  Line 894: click.confirm showing 'Remove marketplace foo (None/None)?' for local-path or generic-git sources. display_source already exists for exactly this purpose.
  Suggested: Replace ({source.owner}/{source.repo}) with ({source.display_source}).

• [nit] No user-visible progress during the 3-probe _auto_detect_path loop for git sources at src/apm_cli/commands/marketplace/__init__.py:610
  _auto_detect_path tries up to 3 candidate file paths via git fetch each. For slow generic-git hosts this is silent.
  Suggested: Add logger.progress("Probing marketplace.json...", symbol='info') before the _auto_detect_path call.

• [nit] traceback.format_exc() routed through logger.progress(symbol='info') emits [i] prefix on every stack-trace line at src/apm_cli/commands/marketplace/__init__.py:685
  Multi-line stack traces with [i] prefix on first line only are visually inconsistent.
  Suggested: Use logger.verbose_detail(traceback.format_exc()) or a dedicated logger.debug_traceback() helper.

DevX UX Expert

• [recommended] All 5 non-shorthand epilog examples include --name, implying it is required when it is not at src/apm_cli/commands/marketplace/__init__.py
  Every example in _ADD_EPILOG shows --name explicitly. This trains users to always pass --name, masks the auto-inference feature, and makes the flag feel mandatory -- exactly the anti-pattern good CLIs avoid by showing the minimal invocation first.
  Suggested: Add at least one example without --name: 'apm marketplace add (gitlab.com/redacted) on its own line before the --name variants.

• [recommended] apm marketplace list shows a Path column by default that exposes internal manifest-detection detail at src/apm_cli/commands/marketplace/__init__.py
  The Path column shows the result of internal auto-detection, not something the user set. Adds noise without value for new users. Reference: npm ls does not show internal resolution paths by default.
  Suggested: Move the Path column to --verbose output only. Default table: Name, Source, Ref.

• [recommended] --host help text does not say it is ignored for full URLs or local paths at src/apm_cli/commands/marketplace/__init__.py
  Users adding a GitLab URL will naturally try --host gitlab.com, then be surprised by the 'ignored' warning. The warning is good but the help text should pre-empt it.
  Suggested: Change help text to: 'Git host FQDN for OWNER/REPO shorthand only (default: github.com). Ignored when SOURCE is a full URL, SSH URL, or local path.'

• [nit] -r shorthand for --ref conflicts with pip's -r (requirements file) at src/apm_cli/commands/marketplace/__init__.py
  pip install -r is among the most muscle-memorized flags in the Python ecosystem.
  Suggested: Consider dropping -r or documenting in CONTRIBUTING why the deviation is intentional.

• [nit] (redacted) URI form is not in the epilog examples despite being a supported and distinct source form at src/apm_cli/commands/marketplace/__init__.py
  Users who copy-paste (redacted) URIs from container or CI configs that emit (redacted) URIs will not see a matching example.
  Suggested: Add 'apm marketplace add (redacted) --name agent-forge' as the last epilog example.

• [nit] Trust gate skip for local sources is not surfaced to the user anywhere in help text or progress output at src/apm_cli/commands/marketplace/__init__.py
  A user who expects the trust prompt may wonder if registration silently failed or if a security step was skipped.
  Suggested: Add a logger.verbose_detail or logger.progress line when kind == 'local' indicating no trust prompt is required.

Supply Chain Security Expert

• [recommended] SAFE_REF_RE allows '..' within ref names (e.g. 'refs/../../etc' passes validation) at src/apm_cli/marketplace/client.py:54
  The pattern r'^[A-Za-z0-9][A-Za-z0-9./-]*$' permits '.' freely, so 'a/../b' and 'refs/../../etc' both match. Git show does not use ref names as filesystem paths so there is no current exploit path, but the guard provides a false safety assurance for future code.
  Suggested: Add a secondary check: if any segment split on '/' equals '..' or '.' raise MarketplaceFetchError.

• [recommended] fetch_local_via_git_show subprocess.run does not pass env=git_subprocess_env(), skipping hardening env vars at src/apm_cli/marketplace/client.py:483
  All other git subprocess calls in this codebase pass env=git_subprocess_env() which sets GIT_TERMINAL_PROMPT=0, GIT_ASKPASS, GIT_SSH_COMMAND hardening, etc. This call inherits the parent process environment instead. Could cause interactive credential prompts or honor attacker-supplied GIT* env vars from an outer shell.
  Suggested: Import git_subprocess_env and pass env=git_subprocess_env() to the subprocess.run call inside _fetch_local_via_git_show.

• [nit] (hostname/redacted) parsed as /path -- UNC-style file URIs silently drop the hostname at src/apm_cli/marketplace/models.py:77
  On Windows a (hostname/redacted) URI is a UNC network path; parsing as /share accesses a different local path. Behavior differs from RFC 8089 and could surprise users registering SMB shares.
  Suggested: If parsed.netloc is non-empty and not 'localhost', raise MarketplaceFetchError: '(redacted) URIs with a hostname are not supported; use a local absolute path instead.'

OSS Growth Hacker

• [recommended] apm pack -> apm marketplace update closed-loop story is not in the shipped docs, only in the PR body
  The PR body calls out 'apm pack -> apm marketplace update as a closed loop' as a key unlock, but the new docs section never mentions it. This is the hook that converts a consumer into a marketplace publisher -- the highest-leverage contributor funnel moment in this PR.
  Suggested: Add a 'Publishing your own marketplace' paragraph to the new 'Local and self-hosted marketplaces' section. One sentence + a two-command snippet (apm pack, apm marketplace update name) is enough.

• [recommended] Lockfile portability warning is buried; will silently break CI for teams that share lockfiles with local-path marketplaces
  A developer who follows the quickpath (add local marketplace, install, commit lockfile, push) will hit a CI break before reading the caveat. This could produce a wave of 'apm is broken in CI' issues and sour the enterprise story.
  Suggested: Move the lockfile portability note above the install examples, or add an inline CLI warning when apm install writes a local-path source to the lockfile.

• [nit] No launch-ready one-liner hook in the docs hero for this feature
  The section title 'Local and self-hosted marketplaces' is accurate but not shareable. A subtitle like 'APM works wherever git works -- GitLab, Azure DevOps, Gitea, or a bare repo on disk' would give users a repostable claim.
  Suggested: Open the section with a single bold summary sentence before the bullet list.

• [nit] ADO_APM_PAT token name is surfaced in docs but has no quickstart anchor; enterprise adopters will stall on auth
  Azure DevOps adopters are exactly the air-gapped enterprise segment this PR targets. The docs reference ADO_APM_PAT and point to the authentication guide but the env var is not explained inline.
  Suggested: Add a one-line inline example: 'export ADO_APM_PAT=PAT then apm marketplace add (dev.azure.com/redacted) directly in the new docs section.

Auth Expert

• [recommended] Raw git stderr surfaced in MarketplaceFetchError without _sanitize_url pass at src/apm_cli/marketplace/client.py:405
  In _fetch_git, CalledProcessError.stderr is decoded and forwarded verbatim into the user-facing MarketplaceFetchError message. Credentials are env-injected (not URL-embedded) so git will not echo them back, but a custom external credential helper could write a credential to stderr. Inconsistent with the _sanitize_url discipline applied everywhere else in git_cache.py.
  Suggested: Run stderr through a _sanitize_url pass before embedding in the error message, matching the pattern used in git_cache.py _ls_remote_resolve.

• [nit] broad except Exception uses str(exc) which for CalledProcessError includes cmd args (URL) at src/apm_cli/marketplace/client.py:408
  URL is clean (no embedded credentials) so no token leaks in practice, but inconsistent with explicit _sanitize_url wrapping used elsewhere.
  Suggested: Wrap CalledProcessError explicitly before the broad handler, or apply _sanitize_url on any URL found in the exception message.

Doc Writer

• [recommended] HOST/OWNER/REPO shorthand example redundantly passes --host at docs/src/content/docs/reference/cli/marketplace.md
  The example 'apm marketplace add gitlab.com/my-org/awesome-agents --host gitlab.com' carries a redundant --host. The implementation extracts the host from the shorthand path. May train users to always supply --host with shorthand forms, which is misleading.
  Suggested: Drop --host from the GitLab shorthand example. Add a note clarifying --host is only needed with OWNER/REPO shorthand when the target host is not github.com.

• [recommended] Local-marketplace section omits explicit 'no authentication required' statement at docs/src/content/docs/consumer/installing-from-marketplaces.md
  Users evaluating air-gapped or offline setups need to know local paths (kind=local) require zero authentication. The omission could leave readers unsure whether a PAT or credential helper is still needed.
  Suggested: Add one sentence: 'Local and (redacted) sources require no authentication -- APM reads the manifest directly from disk.'

• [nit] Internal class name LocalDependencySource leaked into user-facing prose at docs/src/content/docs/consumer/installing-from-marketplaces.md
  Class names have no meaning to end users and create maintenance coupling between docs and code.
  Suggested: Rephrase to 'install by copying files directly from disk' and drop the class-name reference.

• [nit] CHANGELOG section order violates Keep a Changelog convention (Security before Fixed) at CHANGELOG.md
  Keep a Changelog mandates: Added > Changed > Deprecated > Removed > Fixed > Security. The Unreleased block places Security before Fixed.
  Suggested: Move the Security block to after Fixed.

• [nit] 'git credential-manager' should be 'git credential helper' at docs/src/content/docs/reference/cli/marketplace.md
  'git credential-manager' is Microsoft's specific Git Credential Manager tool; 'git credential helper' is the generic term covering all helpers (osxkeychain, wincred, GCM, etc.).
  Suggested: Replace 'git credential-manager' with 'git credential helper' in both the Trust boundary and Azure DevOps callouts.

Test Coverage Expert

• [recommended] _fetch_git (generic-git HTTPS/SSH) has no integration test at the floor tier; the named 'generic_git' integration test actually exercises the local fetcher at tests/integration/test_marketplace_add_generic_git.py
  test_marketplace_add_generic_git.py routes (redacted) to a local bare repo and asserts src.kind == 'local'. So _fetch_git (kind='git', real HTTPS/SSH remote + GitCache subprocess clone) is covered ONLY by mocked unit tests in test_client_git.py. Grepped tests/integration/ for 'fetch_git', 'gitea', 'kind.*git' -- no matches at integration tier.
  Proof (missing at integration-with-fixtures): tests/integration/test_marketplace_add_generic_git.py::test_marketplace_add_real_https_generic_git_fetches_manifest -- proves: apm marketplace add HTTPS-URL for non-GitHub/non-GitLab host resolves and fetches the marketplace manifest via the generic-git fetcher [vendor-neutral, portability-by-manifest]

• [recommended] New integration tests use pytest.mark.skipif instead of declarative APM marker, violating tests.instructions.md at tests/integration/test_marketplace_add_local.py
  Per tests.instructions.md, integration tests that invoke real git subprocess should carry a declarative marker for the git binary prerequisite so the CI orchestrator can selectively skip or gate them if git is absent on a runner.
  Proof (missing at static): tests/integration/test_marketplace_add_local.py::module-level pytestmark -- proves: Integration tests in tests/integration/ declare their prerequisites via the registry marker contract so CI can gate them correctly [devx]

• [nit] Trust gate 'local kind skips trust prompt' has no explicit unit assertion that confirm branch is not called at tests/unit/marketplace/test_marketplace_commands.py
  If a refactor accidentally broadens the gate to include 'local' kind, no test would catch it before users hit a spurious prompt. Grepped tests/unit/marketplace/ for 'confirm', 'trust', 'kind.*local' -- only one unrelated match found.
  Proof (missing at unit): tests/unit/marketplace/test_marketplace_commands.py::test_add_local_path_skips_trust_prompt -- proves: apm marketplace add /local/path never asks for trust confirmation [secure-by-default, devx]

• [nit] Security hardening assertions in test_git_cache_hardening.py are solid -- hooks and submodule.recurse all explicitly asserted. No finding -- coverage is correct.
  Proof (passed at unit): tests/unit/cache/test_git_cache_hardening.py::TestSafeGitArgs.test_includes_hooks_path_dev_null -- proves: Every git subprocess invoked by GitCache disables hook execution and submodule recursion [secure-by-default]
  assert 'core.hooksPath=/dev/null' in args; assert 'submodule.recurse=false' in args

• [nit] _validate_ref whitelist regex and parametrized tests cover key unsafe forms; no gap found.
  Proof (passed at unit): tests/unit/marketplace/test_client_local.py::test_validate_ref_rejects_unsafe_inputs -- proves: User-supplied ref strings are validated against a strict whitelist before being passed to any git subprocess [secure-by-default]
  pytest.raises(MarketplaceFetchError, match='Invalid git ref') for bad_ref in ['', '-rf', 'main; rm -rf /', 'main:other', '..']

• [nit] GitHub owner/repo shorthand regression trap exists at unit tier via dispatch table lock + parser classification test.
  Proof (passed at unit): tests/unit/marketplace/test_client_git.py::test_fetchers_dispatch_table_routes_kinds_to_correct_callable -- proves: owner/repo shorthand still resolves to _fetch_github (Contents API) and has not been silently rerouted to the generic-git subprocess fetcher [portability-by-manifest]
  assert _FETCHERS['github'] is _fetch_github; assert set(_FETCHERS) == {'github', 'gitlab', 'git', 'local'}

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

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• pypi.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pypi.org"

See Network Configuration for more information.

Generated by PR Review Panel for issue #1476 · ● 6M · ◷

[sergio-sisternes-epam]

Thanks for the panel review. Addressed 4 of the 5 in-PR follow-ups in commit bec20fe0:

• CLI Logging (None/None prompt) — remove() now uses source.display_source so local / generic-git entries render truthfully.
• CLI Logging (silent spinner) — logger.start hoisted in add() so the 5-30 s generic-git probe is no longer a blank terminal.
• Supply Chain (env hardening) — _fetch_local_via_git_show now passes env=git_subprocess_env() so the bare-repo subprocess inherits GIT_TERMINAL_PROMPT=0 / GIT_ASKPASS like every other git call in APM.
• Auth (raw stderr leak) — _fetch_git runs CalledProcessError.stderr (and the broad-except message) through _sanitize_url before embedding in MarketplaceFetchError. Defense-in-depth against a custom credential helper echoing a secret back.

The Python Architect's "gitlab retry noop" finding was already addressed in 652e1ba8 (the retry block was dropped entirely; _fetch_gitlab now just delegates to _fetch_via_api, which itself runs try_with_fallback).

The 5th item (floor-tier integration test for _fetch_git over HTTPS/SSH) is tracked as a post-merge follow-up per the panel's own recommendation — the existing tests/integration/test_marketplace_add_generic_git.py covers _fetch_git end-to-end via file:// against a real bare repo, but the loopback-daemon variant for HTTPS/SSH transports lives better as a follow-up so the PR stays focused.

Two non-blocking nits (_SAFE_REF_RE admits .. segments; _auto_detect_path progress indicator) left as tracked follow-ups per the panel.