feat: support marketplace notation in apm uninstall

[sergio-sisternes-epam]

Description

Add marketplace notation (name@marketplace) support to apm uninstall, making it symmetrical with apm install. Previously, _validate_uninstall_packages rejected any input without a /, forcing users to look up the canonical owner/repo before uninstalling marketplace-installed plugins.

Resolution uses a two-stage strategy:

1. Lockfile-first (offline) -- scans apm.lock.yaml for entries matching discovered_via + marketplace_plugin_name provenance fields.
2. Registry fallback (silent) -- calls the marketplace registry when not found in lockfile, mirroring install behaviour.

When both stages fail, a marketplace-specific error message is surfaced advising the user to use owner/repo format directly.

Fixes #1323

Type of change

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

Testing

• Tested locally
• All existing tests pass (8396 passed)
• Added tests for new functionality (45 tests in tests/unit/test_uninstall_engine_helpers.py covering lockfile-hit, registry-fallback, not-found, mixed batch, and backward-compatible canonical paths)

[Copilot]

Pull request overview

This PR aims to make apm uninstall symmetric with apm install by documenting support for marketplace notation (name@marketplace) when uninstalling packages.

Changes:

• Update the APM CLI command reference (skill) to state that apm uninstall accepts owner/repo or name@marketplace.
• Update the apm uninstall CLI reference docs to include marketplace notation in the accepted argument formats.
• Add an example showing apm uninstall my-plugin@official.

Reviewed changes

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

File	Description
packages/apm-guide/.apm/skills/apm-usage/commands.md	Updates the CLI reference line for apm uninstall to mention marketplace notation support.
docs/src/content/docs/reference/cli/uninstall.md	Documents marketplace notation support for uninstall and adds an example invocation.

Comments suppressed due to low confidence (1)

docs/src/content/docs/reference/cli/uninstall.md:66

• This example shows uninstalling my-plugin@official, but the current uninstall command rejects marketplace refs before resolution. Either implement support end-to-end or update/remove this example to avoid documenting a failing command.

Remove by marketplace name (resolves to the canonical `owner/repo`):

```bash
apm uninstall my-plugin@official

</details>

[github-actions]

APM Review Panel: needs_rework

feat: apm uninstall now accepts name[at]marketplace notation, closing the install/uninstall symmetry gap -- but auth, security, dry-run, and integration-test gaps need resolution before the registry fallback path is production-safe.

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

This PR delivers a meaningful usability win: users who install a package via name[at]marketplace can now uninstall it the same way. The lockfile-first offline resolution is architecturally correct and aligns with APM's portable-by-manifest commitment. The panel converges strongly on the value of the change; the disagreements are all in the safety of the Stage 2 registry fallback path.

Four findings from four different lenses all point at the same code path -- _resolve_marketplace_packages Stage 2 -- and collectively constitute the core concern. Auth Expert finds that resolve_marketplace_plugin is called without auth_resolver, diverging from the install path and causing silent unauthenticated failures for gated registries. Supply Chain Security finds that a poisoned registry can return a canonical that matches a legitimately installed but unrelated package, causing it to be silently removed -- this is a destructive action and the secure-by-default evidence row is marked missing. DevX UX finds that --dry-run triggers a live network call, violating the universal contract for that flag. Test Coverage Expert confirms no integration test exercises the full CLI -> lockfile -> registry -> manifest -> lockfile write pipeline; the missing evidence row on the marketplace surface floor (integration-with-fixtures tier) means there is no automated guardrail preventing regression on any of these gaps.

The panel has no dissent on the value of the feature. The sole strategic call is: the Stage 2 registry fallback, as shipped, is not safe enough to land without the auth and security fixes. The dry-run violation and integration test gap are close behind. Everything else -- message phrasing, quoting style, CHANGELOG, docs -- is recommended cleanup that should accompany the PR but does not independently hold it.

Dissent. No panelist argued against shipping the feature. Supply Chain Security rated the registry-trust concern recommended (not blocking), but the secure-by-default evidence row is missing on a destructive action surface; a missing test on a secure-by-default surface ranks above a recommended opinion finding. The CEO sides with treating the lockfile cross-check as a required fix before merge, not a post-merge follow-up.

Aligned with: Portable by manifest -- lockfile-first resolution preserves offline-capable uninstall for packages installed via apm install; Secure by default -- the Stage 2 registry fallback currently accepts an attacker-controlled canonical for a destructive action with no lockfile cross-check, which must be fixed before the fallback path is enabled; Pragmatic as npm -- install/uninstall symmetry on notation is table-stakes parity with every major package manager.

Growth signal. "uninstall now speaks the same language as install" is a one-line release-beat callout that lands cleanly with both individual devs and enterprise evaluators who care about CLI ergonomic consistency. The offline-first resolution story is a secondary angle for air-gapped/enterprise audiences. Both angles should appear in the CHANGELOG entry and release notes when this ships.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Two recommended fixes: import of private _MARKETPLACE_RE and silent exception swallowing in Stage 2; overall shape is sound and appropriately scoped.
CLI Logging Expert	0	2	1	Three output UX issues: silent network errors in verbose mode, inconsistent warning phrasing across input paths, and mixed quoting style on identifiers.
DevX UX Expert	0	3	2	Good symmetry with install; silent registry fallback and wrong-marketplace ambiguity are the two worthwhile fixes before shipping.
Supply Chain Security Expert	0	1	2	Registry fallback in Stage 2 can route to an attacker-controlled canonical; lockfile-first is safe but registry fallback trusts remote data with no additional guard.
OSS Growth Hacker	0	1	2	Strong friction-removal win worth a CHANGELOG highlight; one error-message gap leaves users stranded without a recovery path.
Auth Expert	0	1	0	Registry fallback calls resolve_marketplace_plugin without auth_resolver, diverging from the install path which passes it explicitly.
Doc Writer	0	2	1	Docs accurately surface the new notation; two gaps: resolution-failure behavior is undocumented, and CHANGELOG.md has no entry for this feature.
Test Coverage Expert	0	1	1	45 unit tests cover resolver logic well; no integration test exercises apm uninstall my-plugin[at]official end-to-end -- marketplace surface floor is unmet.

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

Top 5 follow-ups

1. [Auth Expert] Thread auth_resolver into _resolve_marketplace_packages and pass it to resolve_marketplace_plugin, mirroring the install path. -- Without this, Stage 2 always runs unauthenticated, silently failing for any auth-gated or rate-limited registry even when the user has valid credentials configured -- the silent except swallows the failure and the user gets a misleading 'could not be resolved' error.
2. [Supply Chain Security Expert] Before accepting a Stage 2 registry-resolved canonical, verify it appears in the lockfile; refuse resolution if it does not. -- A compromised registry can return the canonical of a different installed package, causing a silent removal of an unrelated dependency. This is a destructive action on a secure-by-default surface with no current automated test guard.
3. [DevX UX Expert] Thread dry_run flag into _resolve_marketplace_packages and skip Stage 2 when dry_run=True. -- --dry-run making a live network call violates the universal contract users and CI pipelines expect; this is a correctness defect, not a style issue.
4. [Test Coverage Expert] Add an integration test in tests/integration/ that seeds a lockfile with a marketplace-installed package and runs apm uninstall my-plugin[at]official end-to-end. -- The marketplace surface floor is integration-with-fixtures tier; no existing test exercises the full CLI -> lockfile -> registry -> manifest -> lockfile write pipeline. Without this, all four Stage 2 fixes above have no regression guard.
5. [CLI Logging Expert] Add logger.debug for registry fallback failures, align not-found warning phrasing across canonical and marketplace paths, and append 'or run apm list to find the canonical name' to the resolution-failure error. -- Three panelists flagged the silent except and divergent message phrasing; consolidating into one cleanup commit keeps the messaging surface consistent for users and agents parsing output.

Architecture

classDiagram
    direction LR

    class UninstallEngine {
        <<IOBoundary>>
        _validate_uninstall_packages()
        _resolve_marketplace_packages()
        _remove_packages_from_disk()
        _cleanup_transitive_orphans()
    }

    class MarketplaceResolver {
        <<Pure>>
        +parse_marketplace_ref(s) tuple
        +resolve_marketplace_plugin(name, mkt) Resolution
        _MARKETPLACE_RE : re.Pattern
    }

    class Resolution {
        <<ValueObject>>
        +canonical str
        +plugin_name str
    }

    class LockFile {
        <<ValueObject>>
        +dependencies dict
        +read(path) LockFile
        +write(path) None
    }

    class DependencyEntry {
        <<ValueObject>>
        +discovered_via str
        +marketplace_plugin_name str
        +get_unique_key() str
        +get_identity() str
    }

    class DependencyReference {
        <<ValueObject>>
        +parse(s) DependencyReference
        +get_identity() str
    }

    class CommandLogger {
        <<Base>>
        +error(msg) None
        +warning(msg) None
        +progress(msg) None
        +debug(msg) None
    }

    class UninstallCLI {
        <<IOBoundary>>
        +uninstall_command()
    }

    UninstallCLI --> UninstallEngine : calls
    UninstallEngine ..> MarketplaceResolver : imports _MARKETPLACE_RE (private)
    UninstallEngine ..> MarketplaceResolver : lazy-imports parse_marketplace_ref
    UninstallEngine ..> MarketplaceResolver : lazy-imports resolve_marketplace_plugin
    UninstallEngine ..> LockFile : reads dependencies
    UninstallEngine ..> DependencyReference : parses
    UninstallEngine ..> CommandLogger : logs
    LockFile *-- DependencyEntry : contains
    MarketplaceResolver ..> Resolution : returns

    class UninstallEngine:::touched
    class UninstallCLI:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm uninstall NAME[at]MARKETPLACE"]) --> B
    B["[I/O] cli.py: LockFile.read(lockfile_path)\nmoved earlier for marketplace support"] --> C
    C["_validate_uninstall_packages(packages, current_deps, logger, lockfile)"] --> D
    D{"_MARKETPLACE_RE.match(p)\nfor each package"} -->|"marketplace refs"| E
    D -->|"owner/repo refs"| G
    E["_resolve_marketplace_packages(mkt_refs, lockfile, logger)"] --> F1
    F1{"Stage 1: scan lockfile.dependencies\noffline, O(n) per ref"} -->|"found: dep.get_unique_key()"| H
    F1 -->|"not found"| F2
    F2{"Stage 2: resolve_marketplace_plugin()\nno auth_resolver -- gap"} -->|"[NET] success: resolution.canonical"| H
    F2 -->|"Exception caught -- silent"| F3
    F3["Stage 3: logger.error('could not be resolved')\nmap -> None"] --> G
    H["canonical owner/repo in mkt_resolved dict"] --> G
    G["Match canonical against current_deps\nvia DependencyReference.get_identity()"] --> I
    I{"matched?"} -->|yes| J
    I -->|no| K
    J["packages_to_remove"] --> L["[FS] remove from disk + apm.yml\n[LOCK] update lockfile"]
    K["packages_not_found -> warning"]

Loading

sequenceDiagram
    actor User
    participant CLI as cli.py
    participant Engine as engine.py
    participant LF as LockFile
    participant MR as marketplace.resolver

    User->>CLI: apm uninstall plugin[at]marketplace
    CLI->>LF: LockFile.read(lockfile_path)
    LF-->>CLI: lockfile (or None)
    CLI->>Engine: _validate_uninstall_packages([plugin[at]marketplace], deps, logger, lockfile)
    Engine->>Engine: filter mkt_refs via _MARKETPLACE_RE
    Engine->>Engine: _resolve_marketplace_packages(mkt_refs, lockfile, logger)
    Engine->>LF: scan dependencies for discovered_via + marketplace_plugin_name
    alt found in lockfile
        LF-->>Engine: dep.get_unique_key() -> canonical
    else not in lockfile
        Engine->>MR: resolve_marketplace_plugin(name, marketplace) -- no auth_resolver
        alt registry success
            MR-->>Engine: resolution.canonical
        else network / not-found exception
            Engine->>Engine: except Exception: pass (silent)
            Engine->>CLI: logger.error('could not be resolved')
        end
    end
    Engine-->>CLI: (packages_to_remove, packages_not_found)
    CLI->>Engine: remove packages, update lockfile
    CLI-->>User: success or error summary

Loading

Recommendation

Hold for the auth_resolver threading fix and the registry-canonical lockfile cross-check before merge -- both target the Stage 2 fallback path, which is the only net-new network-touching code in this PR and currently has no integration test guardrail. Once those two are in, add the dry-run guard and at least one integration fixture test, then this is ready to ship. The CHANGELOG entry, docs Behavior section update, and message-phrasing alignment should travel in the same commit to keep the release-note story clean.

---

Full per-persona findings

Python Architect

• [recommended] Module-level import of private symbol _MARKETPLACE_RE from marketplace.resolver breaks encapsulation at src/apm_cli/commands/uninstall/engine.py:10
  engine.py imports _MARKETPLACE_RE directly at module load time, coupling uninstall internals to a private implementation detail of marketplace.resolver. parse_marketplace_ref(p) is not None achieves the same goal via the public API. If resolver.py ever inlines or renames the regex, engine.py silently breaks.
  Suggested: Remove the _MARKETPLACE_RE import. Replace _MARKETPLACE_RE.match(p) with parse_marketplace_ref(p) is not None, or add a public is_marketplace_ref(s) -> bool to resolver.py.

• [recommended] Bare except Exception: pass in Stage 2 silently discards all error context, making registry failures undiagnosable at src/apm_cli/commands/uninstall/engine.py:101
  Any exception from resolve_marketplace_plugin -- network timeout, auth failure, malformed registry response, unexpected RuntimeError -- is swallowed with pass. The user receives only the generic 'could not be resolved' with no indication of whether the failure was 'not found' vs 'network down'.
  Suggested: except Exception as exc: logger.debug(f'Registry fallback for {plugin_name}@{marketplace_name} failed: {exc}')

• [nit] parse_marketplace_ref returning None is unreachable dead code in _resolve_marketplace_packages at src/apm_cli/commands/uninstall/engine.py:80
  The caller pre-filters with _MARKETPLACE_RE so parsed will never be None inside the loop. The guard is defensive but adds noise; a comment or removal clarifies intent.

CLI Logging Expert

• [recommended] Silent except Exception: pass in registry fallback emits nothing in verbose mode at src/apm_cli/commands/uninstall/engine.py
  The verbose-mode design principle requires that --verbose exposes WHY a resolution failed. A transient network error and a genuine registry 404 are collapsed into the same Stage 3 error message, giving agents running --verbose zero signal on whether to retry or fix the notation.
  Suggested: Add logger.debug(f'Registry fallback failed for {package}: {e}') in the except block.

• [recommended] Warning wording diverges between canonical-format and marketplace-format for the same event at src/apm_cli/commands/uninstall/engine.py
  Canonical not-found: '{package} - not found in apm.yml'. Marketplace resolved-but-not-found: "'{display_label}' is not installed (resolved to {canonical_for_match})". Same terminal event, completely different phrasing. Users and agents need two separate regexes.
  Suggested: Align: '{display_label} - not found in apm.yml' for canonical path; '{display_label} ({canonical_for_match}) - not found in apm.yml' for marketplace path.

• [nit] Inconsistent quoting of identifiers across new and existing messages at src/apm_cli/commands/uninstall/engine.py
  New messages wrap identifiers in single-quotes; existing messages in the same file do not. Pick one style and apply it consistently.

DevX UX Expert

• [recommended] Registry fallback is completely silent -- no progress indicator while network call may take seconds at src/apm_cli/commands/uninstall/engine.py
  Every reference package manager shows a spinner or 'Resolving...' line before a network call. A user on a slow network will see a frozen terminal with no feedback for an unbounded duration. Silence is ambiguous -- the user cannot distinguish 'still working' from 'hung'.
  Suggested: Before the resolve_marketplace_plugin call emit: logger.progress(f"Resolving '{plugin_name}@{marketplace_name}' via registry...")

• [recommended] Wrong-marketplace input silently succeeds when registry resolves to the same canonical -- no diagnostic on provenance mismatch at src/apm_cli/commands/uninstall/engine.py
  If a user installed via [at]community but types [at]official, the lockfile lookup misses, the registry resolves the same canonical, and uninstall proceeds. The user never learns they used the wrong marketplace qualifier. Breaks the mental model that marketplace names are meaningful; a user managing multiple marketplaces has no visibility into which one APM actually used.
  Suggested: After Stage 1 scan, if plugin_name matches but discovered_via differs, emit: logger.warning(f"'{plugin_name}@{marketplace_name}' not found; package was installed via '{dep.discovered_via}'. Proceeding with uninstall.")

• [recommended] --dry-run triggers a silent network call to the marketplace registry at src/apm_cli/commands/uninstall/cli.py
  Marketplace resolution happens inside _validate_uninstall_packages, which is called before the dry-run guard. A user running apm uninstall --dry-run my-plugin[at]official expects a fully offline preview -- the standard contract for --dry-run in npm, pip, and cargo. Making a network call in dry-run violates that contract and surprises users in air-gapped CI environments.
  Suggested: Thread the dry_run flag into _resolve_marketplace_packages; skip Stage 2 when dry_run=True, emitting a warning instead.

• [nit] Inconsistent message style between found and not-found paths for marketplace packages at src/apm_cli/commands/uninstall/engine.py
  Success uses dash separator: 'my-plugin[at]official - found in apm.yml (as acme/my-plugin)'. Not-found uses IS verb: "'my-plugin[at]official' is not installed (resolved to acme/my-plugin)". Quotes also appear only on the not-found branch.
  Suggested: Adopt the dash style consistently: 'my-plugin[at]official - not installed (resolved to acme/my-plugin)'.

• [nit] Error message on total resolution failure uses a period mid-sentence before the imperative clause at src/apm_cli/commands/uninstall/engine.py
  "'my-plugin[at]official' could not be resolved. Use 'owner/repo' format to uninstall directly." -- the period makes this two sentences where a dash keeps the action closer to the context.
  Suggested: "'my-plugin[at]official' could not be resolved -- use 'owner/repo' format to uninstall directly."

Supply Chain Security Expert

• [recommended] Registry fallback in Stage 2 trusts remote canonical directly for apm.yml matching without cross-checking the lockfile at src/apm_cli/commands/uninstall/engine.py:97
  A compromised or spoofed marketplace registry can return any canonical string, including the canonical of a different package the user has legitimately installed. The matching loop then finds that dep in current_deps and marks it for removal. Blast radius is bounded to packages already in apm.yml, but a poisoned registry can redirect a no-op uninstall into removing an unrelated installed dependency.
  Suggested: When Stage 2 resolves a canonical via the registry, verify the returned canonical appears in the lockfile before accepting it. If it does not appear in the lockfile, refuse resolution and ask the user to supply owner/repo directly.
  Proof (missing at): tests/unit/test_uninstall_engine_helpers.py -- proves: A compromised registry returning a different package canonical cannot cause that package to be silently removed. [secure-by-default]

• [nit] Import of private symbol _MARKETPLACE_RE creates fragile cross-module coupling at src/apm_cli/commands/uninstall/engine.py:12
  parse_marketplace_ref (the public API) wraps the same regex. Using both paths means the routing gate and parsing gate could silently diverge if the regex changes in one place.
  Suggested: Replace _MARKETPLACE_RE.match(p) with parse_marketplace_ref(p) is not None.

• [nit] except Exception: pass in Stage 2 swallows all registry errors with no trace-level log at src/apm_cli/commands/uninstall/engine.py:101
  Silent broad exception suppression makes it impossible to distinguish a transient network error from an unexpected internal failure during diagnostics.
  Suggested: Add logger.debug(f'Registry fallback failed for {plugin_name}@{marketplace_name}: {e!r}') in the except block.

OSS Growth Hacker

• [recommended] Fallback error message leaves users stranded with no recovery hint at src/apm_cli/commands/uninstall/engine.py
  When both lockfile and registry resolution fail, the error says "Use 'owner/repo' format to uninstall directly." A new user who installed via marketplace notation almost certainly does NOT know the canonical owner/repo -- that is precisely why they used the notation in the first place. Without a pointer to where they can look it up, this is a support-ticket generator and a trust-eroder on first failure.
  Suggested: Append a recovery hint: "Use 'owner/repo' format to uninstall directly, or run apm list to find the canonical name."

• [nit] Docs example is good but could pair with a copy-pasteable failure-path note at docs/src/content/docs/reference/cli/uninstall.md
  The updated uninstall.md adds a working example but does not show what happens when the name is wrong. A one-liner showing the error + recovery hint would complete the quickstart story.

• [nit] CHANGELOG entry should name the symmetry story explicitly for release-note reuse
  A CHANGELOG entry phrased as 'apm uninstall now accepts marketplace notation (name[at]marketplace), matching apm install' gives release-note authors and social copy writers a ready-made one-liner. Without it, the feature ships silently.
  Suggested: Add to [Unreleased] ### Added: 'apm uninstall now accepts marketplace notation (name[at]marketplace), achieving full symmetry with apm install. Resolution is lockfile-first (offline-capable), with silent registry fallback.'

Auth Expert

• [recommended] Registry fallback calls resolve_marketplace_plugin without auth_resolver, causing unauthenticated requests for auth-gated marketplaces at src/apm_cli/commands/uninstall/engine.py:99
  uninstall/engine.py calls resolve_marketplace_plugin(plugin_name, marketplace_name) without auth_resolver, which defaults to None. The install path passes auth_resolver=auth_resolver explicitly. For auth-gated or rate-limited marketplace registries this means the Stage 2 registry fallback always runs unauthenticated -- it will silently fail (swallowed by except Exception: pass) even when the user has valid credentials configured, producing a misleading 'could not be resolved' error instead of a successful resolution.
  Suggested: Thread auth_resolver into _resolve_marketplace_packages and pass it through: resolve_marketplace_plugin(plugin_name, marketplace_name, auth_resolver=auth_resolver), mirroring the install path.

Doc Writer

• [recommended] Resolution-failure behavior is not documented in uninstall.md at docs/src/content/docs/reference/cli/uninstall.md:87
  The code logs a clear error and skips the package rather than aborting. The Behavior section is silent on marketplace resolution failures. A user who types a wrong marketplace name will see the error logged but no explanation in the docs of what happened or what to do next.
  Suggested: Extend the Behavior section: "If a marketplace ref (name[at]marketplace) cannot be resolved -- because the plugin is not in the lockfile and the registry lookup fails -- the command logs an error and skips that package. Pass the canonical owner/repo to uninstall directly, or run apm list to find it."

• [recommended] CHANGELOG.md has no entry for marketplace notation support in apm uninstall at CHANGELOG.md
  The project maintains a Keep-a-Changelog CHANGELOG.md with an [Unreleased] section. This is a user-visible behavior addition (new input format) that belongs in [Unreleased] ### Added.
  Suggested: Add: "- apm uninstall now accepts marketplace notation (name[at]marketplace) symmetrically with apm install; resolved via lockfile (offline-first) then the registry. (feat: support marketplace notation in apm uninstall #1325)"

• [nit] Example comment 'resolves to the canonical owner/repo' slightly undersells two-stage resolution at docs/src/content/docs/reference/cli/uninstall.md:62
  The code does lockfile-first then registry-fallback. The comment implies a single-step lookup.
  Suggested: Change to "Remove by marketplace name (resolved via lockfile, then registry):"

Test Coverage Expert

• [recommended] No integration test exercises apm uninstall my-plugin[at]official end-to-end; marketplace surface floor is integration-with-fixtures at tests/integration/test_uninstall_dry_run_e2e.py
  The marketplace surface requires integration-with-fixtures tier. All 45 new tests mock resolve_marketplace_plugin at the boundary (unit tier), not exercising CLI argv parsing -> lockfile load -> registry resolution -> manifest mutation -> lockfile write -> file removal. Probed tests/integration/ for '[at]official' and 'marketplace.*uninstall' -- zero hits confirmed.
  Suggested: Add a test in tests/integration/ that seeds a lockfile with a marketplace-installed package, runs apm uninstall my-plugin[at]official, and asserts: exit code 0, entry removed from apm.yml, lockfile updated.
  Proof (missing at): tests/integration/test_uninstall_marketplace_e2e.py::test_uninstall_by_marketplace_notation_removes_package -- proves: A user who installed a package as my-plugin[at]official can uninstall it using the same marketplace notation. [devx,portability-by-manifest,vendor-neutral]
  assert result.returncode == 0 and canonical not in lockfile_after

• [nit] Unit tests for _resolve_marketplace_packages confirm resolver logic; acknowledged as sub-floor coverage for the marketplace surface at tests/unit/test_uninstall_engine_helpers.py
  45 new unit tests cover lockfile-hit, registry-fallback, network-error, non-marketplace-skip, and batch-continue paths. Valuable isolation coverage; the unit row is not the gap -- the integration tier is.
  Proof (passed): tests/unit/test_uninstall_engine_helpers.py::TestResolveMarketplacePackages::test_lockfile_first_resolution -- proves: Lockfile is consulted before the registry when resolving a marketplace ref during uninstall. [devx,portability-by-manifest]
  assert result['my-plugin[at]official'] == 'acme/my-plugin'

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

Generated by PR Review Panel for issue #1325 · ● 4.4M · ◷

[sergio-sisternes-epam]

Panel Findings -- All Addressed

All 13 Recommended and 10 Nit findings from the panel review have been resolved in commit e752da6f.

Safety hardening (Stream A)

• Auth threading -- auth_resolver now flows from CLI through _validate_uninstall_packages into resolve_marketplace_plugin, mirroring the install path.
• Supply-chain guard -- Registry-resolved canonicals are refused if absent from lockfile (verbose_detail log + canonical = None).
• Dry-run guard -- Stage 2 network call is skipped entirely when --dry-run is active.
• Debug logging -- Silent except: pass replaced with logger.verbose_detail(...) capturing the exception.

Code quality & UX (Stream B)

• Public API -- Replaced private _MARKETPLACE_RE import with _is_marketplace_ref() helper using parse_marketplace_ref (public API).
• Progress indicator -- logger.progress(..., symbol="search") before registry call.
• Provenance mismatch -- Two-pass Stage 1: exact match first, then plugin-name-only with marketplace-mismatch warning.
• Message alignment -- Consistent "label (canonical) - not found in apm.yml" format across all paths; no single-quotes on identifiers.
• Recovery hint -- Error now appends `apm list` recovery suggestion.

Documentation (Stream C)

• CHANGELOG -- Entry added under [Unreleased] > Added.
• uninstall.md -- Behaviour section documenting resolution-failure path; updated example comment.

Tests (Stream D)

• 3 new unit tests (dry-run skip, supply-chain guard, provenance mismatch) -- 48 total passing.
• New tests/integration/test_uninstall_marketplace.py with 2 end-to-end scenarios.

---

Lint clean, all tests green. Ready for re-review.

[github-actions]

APM Review Panel: ship_with_followups

ship_with_followups: marketplace uninstall parity lands clean -- two silent security UX gaps (guard refusal, cross-marketplace mismatch) and a missing integration fixture need follow-up before next minor

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

PR #1325 delivers a real user-felt win: install/uninstall symmetry for marketplace notation, closing the friction loop where users had to look up owner/repo to remove a plugin they installed by name. The design is architecturally sound -- two-stage resolution with a lockfile-first offline path and a supply-chain guard on registry fallback. 45 unit tests + 2 integration tests ship alongside the feature, and all 8396 existing tests pass. No panelist raised a blocking finding. The PR is shippable now with targeted follow-up committed.

The convergent signal across cli-logging, devx, supply-chain-security, and test-coverage is the supply-chain guard's silence at normal verbosity. When the guard fires, the user reads a generic "could not be resolved" error -- identical to the package-not-found case -- with no indication that the registry DID resolve the plugin but APM refused it. This is both a security-UX gap (the user cannot diagnose the refusal) and a discoverability risk (they may assume the feature is broken). The fix is a one-line promotion from verbose_detail to logger.warning. The no-lockfile guard bypass (supply-chain-security, recommended) is a harder trade-off: requiring --force or blocking entirely when lockfile=None is a behavioral change that could break existing workflows. I weight this as needs-discussion in a follow-up issue rather than a ship blocker -- the PR does not regress existing behavior, and the test explicitly documents the accepted behavior. The cross-marketplace provenance mismatch (security, recommended) is real but lower urgency: it requires a user to have two marketplaces with identically-named plugins AND run uninstall with the wrong marketplace name. Promote to an error (or --ignore-marketplace opt-in) in a follow-up. The unresolvable-ref exclusion from packages_not_found (python-architect) breaks the API contract and is a one-line fix; it should be bundled with the guard visibility fix. The invalid-format error omitting marketplace notation (devx) is a discoverability regression for the feature itself -- also a one-line fix.

Test coverage is thorough at unit tier. The test-coverage-expert's call for an integration-with-fixtures test covering the guard-refusal path is load-bearing: the guard protects a secure-by-default promise, and outcome: missing on that surface ranks above any opinion finding. That integration fixture should be filed as a fast-follow, not a ship blocker, because the unit-level guard logic IS tested and all existing integration tests pass. Auth findings are all nits; no auth issue approaches blocking. Doc gaps (supply-chain guard, #REF, no-lockfile) are real and should be addressed in the same follow-up wave as the guard visibility fix, since they describe the same behavior.

Dissent. supply-chain-security rates the no-lockfile registry bypass as recommended (block without --force); devx and cli-logging do not address it. CEO sides with supply-chain-security on the diagnosis but not the ship consequence: the no-lockfile path was accepted behavior before this PR and the test documents it deliberately. The correct resolution is a follow-up issue proposing the --force gate with a migration note, not a revert. supply-chain-security also rates the cross-marketplace mismatch at recommended; the fix (opt-in flag or hard error) is a behavioral change requiring its own PR after community discussion.

Aligned with: Secure by default -- supply-chain guard at Stage 2 enforces lockfile-anchored resolution; guard refusal visibility and no-lockfile bypass need follow-up. Pragmatic as npm -- install/uninstall notation symmetry directly matches npm's UX contract; closes the parity gap that existed since marketplace install shipped. OSS community driven -- external contributor delivering a symmetric feature closes a real friction point reported by the community; fast-tracking with follow-up list respects contributor momentum.

Growth signal. The oss-growth-hacker correctly identifies that install/uninstall symmetry is the "we listen" story for this release cycle. The CHANGELOG entry should be reframed from implementation detail to user benefit: "apm uninstall now accepts the same marketplace notation as apm install (e.g. my-plugin(at)official) -- no more owner/repo lookup before removing a plugin." The lockfile now stores discovered_via + marketplace_plugin_name -- an unreported surface that unlocks a future apm list --marketplace filter; flag this as a forward-looking story angle in release notes to seed community anticipation.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Solid two-stage resolution design; one docstring/contract gap (unresolvable refs excluded from packages_not_found) and one redundant regex call worth fixing.
CLI Logging Expert	0	2	2	Supply-chain guard refusal and network errors are silent at default verbosity -- user sees generic 'could not be resolved' with no actionable cause.
DevX UX Expert	0	3	1	Marketplace uninstall is a solid UX win, but three error-path gaps leave users stranded: silent supply-chain guard refusal, dry-run resolution silently degrades, and the invalid-format hint is asymmetric with install.
Supply Chain Security Expert	0	2	1	Two trust issues: no-lockfile Stage 2 skips all integrity checks; provenance-mismatch second pass silently escalates cross-marketplace removal. Canonical format divergence may silently neuter the guard.
OSS Growth Hacker	0	2	2	Symmetry fix closes a real user friction point; CHANGELOG entry exists but undersells the story -- missed launch beat for the 'install/uninstall parity' narrative.
Auth Expert	0	0	2	AuthResolver wiring is architecturally sound; one nit on eager instantiation before dry-run gate, one nit on double-instantiation safety net in client.py.
Doc Writer	0	3	1	Docs are structurally sound; two behavioral gaps -- the supply-chain guard and #REF syntax ambiguity -- need prose before the feature is complete.
Test Coverage Expert	0	1	2	Unit coverage is thorough across all 11 named scenarios; supply-chain guard proven only at unit tier (mocked resolver) -- floor for marketplace surface is integration-with-fixtures.

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] Promote supply-chain guard refusal from verbose_detail to logger.warning, and bundle the unresolvable-ref packages_not_found omission fix (python-architect engine.py:215) in the same patch -- Guard refusal is silent at normal verbosity: users cannot distinguish "package not found" from "found but blocked". The packages_not_found omission breaks the API contract callers depend on for error counts. Both are one-liners; bundle for minimal diff.
2. [Test Coverage Expert] Add integration-with-fixtures test: seed lockfile with known-plugin, run apm uninstall injected(at)official where resolver returns a canonical NOT in lockfile, assert exit != 0 and stderr contains refusal message -- Supply-chain guard protects a secure-by-default promise. outcome:missing on a secure-by-default surface at integration tier means the guard could be silently removed with no automated catch. Highest-priority test gap.
3. [Supply Chain Security Expert] Open a follow-up issue proposing --force gate (or hard error) for no-lockfile marketplace resolution at Stage 2 -- When lockfile=None, whatever canonical a registry returns is used without integrity anchor. Real supply-chain vector requiring community discussion and migration note before shipping.
4. [DevX UX Expert] Fix invalid-format error message to include marketplace notation hint: "Use 'owner/repo' or 'plugin-name(at)marketplace' format." -- Symmetric with install. A user who misspells a marketplace ref sees a hint that mentions only owner/repo, invisibilizing the new feature. One-line fix.
5. [Doc Writer] Expand uninstall.md: document supply-chain guard behavior, clarify #REF not meaningful for uninstall, add no-lockfile edge case prose -- Three undocumented behaviors leave users with no diagnosis path when they hit the new error states. Ship in the same doc wave as the guard visibility fix.

Recommendation

Merge PR #1325 now. The feature is architecturally sound, tests are thorough at unit tier, no blocking findings exist, and the external contributor delivered symmetric behavior the community asked for. Five targeted follow-ups must be filed before the next minor release: guard-refusal visibility + packages_not_found fix (one patch PR), integration guard-refusal fixture, no-lockfile --force discussion issue, invalid-format message fix, and doc expansion for supply-chain guard + #REF + no-lockfile. The cross-marketplace provenance mismatch is a real attack vector but requires its own behavioral-change PR with community input; track in a separate issue. Release note should lead with the user benefit framing, not the implementation detail.

---

Full per-persona findings

Python Architect

• [recommended] Unresolvable marketplace refs silently excluded from packages_not_found, breaking the return-value contract at src/apm_cli/commands/uninstall/engine.py:215
  The docstring for _validate_uninstall_packages declares packages_not_found contains "unresolved or unmatched package strings". But when Stage 3 fires (canonical is None), line 215 does continue, bypassing packages_not_found.append(package). Callers in cli.py (Step 10: "N package(s) not found") will undercount. This is an API contract violation.
  Suggested: Replace continue with packages_not_found.append(package); continue.
  Proof (missing): tests/unit/test_uninstall_engine_helpers.py::TestValidateUninstallPackages -- proves: No test asserts that an unresolvable marketplace ref appears in packages_not_found [devx]

• [nit] _is_marketplace_ref (regex) called twice per marketplace package in _validate_uninstall_packages at src/apm_cli/commands/uninstall/engine.py:199
  Line 199 builds mkt_refs via list comprehension calling _is_marketplace_ref once. Line 211 calls it again in the main loop. Converting to a set lookup eliminates duplicate regex work.
  Suggested: mkt_refs_set = {p for p in packages if _is_marketplace_ref(p)} and if package in mkt_refs_set in the loop.

• [nit] Supply-chain guard silently no-ops when lockfile is None; a verbose_detail log would clarify intent at src/apm_cli/commands/uninstall/engine.py:144
  When lockfile is None, the guard short-circuits and the registry canonical is trusted without an offline proof. Probably correct behavior but undocumented at the code level.
  Suggested: Add verbose_detail when accepting a registry canonical with no lockfile cross-check.

CLI Logging Expert

• [recommended] Supply-chain guard refusal is verbose_detail-only; user gets no feedback at normal verbosity at src/apm_cli/commands/uninstall/engine.py
  When registry resolves a canonical not in the lockfile, the guard refuses silently. The user then hits Stage 3 "could not be resolved" with zero explanation of why. This is an active security decision that deserves a visible warning so users understand the package was found but rejected, not missing.
  Suggested: Promote to logger.warning: "Registry resolved {plugin_name}@apm install first."

• [recommended] Registry network failure is verbose_detail-only; Stage 3 error gives no hint of the root cause at src/apm_cli/commands/uninstall/engine.py
  If the registry call raises an exception, the exception is swallowed into verbose_detail and the user reads only the Stage 3 generic error. They cannot distinguish "package not found" from "network error" without --verbose.
  Suggested: Use logger.warning for the exception case: "Registry lookup for {plugin_name}@src/apm_cli/commands/uninstall/engine.py
  In dry-run, canonical stays None after the verbose_detail skip, so logger.error fires. An error during dry-run is confusing -- the operation was never attempted.
  Suggested: Guard Stage 3 error with if not dry_run or replace with a warning when in dry-run mode.

• [nit] Provenance mismatch warning lacks the fix hint at src/apm_cli/commands/uninstall/engine.py
  The warning says "Proceeding with uninstall" but does not confirm what canonical is being used.
  Suggested: Append the resolved canonical: "...Proceeding with uninstall of {canonical}."

DevX UX Expert

• [recommended] Supply-chain guard refusal is silent at normal verbosity, producing a misleading "could not be resolved" error at src/apm_cli/commands/uninstall/engine.py:144
  When the guard refuses a registry-resolved canonical (not in lockfile), it logs only at verbose_detail, then falls through to Stage 3 "could not be resolved". The user sees the same error as if the ref was completely unknown, with zero indication that resolution SUCCEEDED but was blocked.
  Suggested: Promote to logger.warning: "Registry resolved {plugin}@apm uninstall {canonical} directly if this is correct."

• [recommended] Invalid-format error message omits marketplace notation hint, asymmetric with install at src/apm_cli/commands/uninstall/engine.py:219
  "Invalid package format: {package}. Use 'owner/repo' format." -- install says "invalid format -- use 'owner/repo' or 'plugin-name@src/apm_cli/commands/uninstall/engine.py:128
  When --dry-run, registry fallback is skipped and logged only at verbose_detail. A user running apm uninstall my-plugin(at)official --dry-run may get "could not be resolved" that would NOT occur in the real run. Makes --dry-run untrustworthy for marketplace refs.
  Suggested: Emit logger.warning when dry-run skips registry fallback: "[dry-run] Skipping registry fallback for {plugin}@apm list recovery hint may confuse users who expect marketplace notation at src/apm_cli/commands/uninstall/engine.py:159
  The error says "run apm list to find the canonical name" but list does not surface discovered_via or marketplace_plugin_name, so users see owner/repo canonicals, not name(at)marketplace.
  Suggested: "run apm list to find the owner/repo canonical name, then use apm uninstall owner/repo directly"

Supply Chain Security Expert

• [recommended] No-lockfile path: Stage 2 registry canonical accepted without any integrity anchor at src/apm_cli/commands/uninstall/engine.py:144
  When lockfile is None the guard short-circuits entirely. Whatever canonical a poisoned registry returns is used directly. test_no_lockfile_goes_directly_to_registry documents and accepts this behavior without flagging the security gap.
  Suggested: When lockfile is None AND Stage 2 fires, refuse: "Cannot verify registry resolution without a lockfile -- run apm install to regenerate apm.lock.yaml, then retry." If override needed, require --force.
  Proof (failed at unit): tests/unit/test_uninstall_engine_helpers.py -- proves: test_no_lockfile_goes_directly_to_registry confirms registry canonical used unconditionally when lockfile=None

• [recommended] Stage 1 second pass: cross-marketplace provenance mismatch proceeds with warning only -- confusion attack vector at src/apm_cli/commands/uninstall/engine.py:111
apm uninstall foo(at)attacker-marketplace will find and remove a package installed via internal-marketplace with only a warning. Name-only matching across trust boundaries is unsound (dependency confusion threat model).
  Suggested: Make the second pass opt-in behind --ignore-marketplace, or replace the warning with an error naming the actual installed marketplace.
  Proof (failed at unit): tests/unit/test_uninstall_engine_helpers.py -- proves: test_provenance_mismatch_warns_and_resolves confirms cross-marketplace removal proceeds with only a warning

• [nit] Canonical format from resolve_marketplace_plugin may never match lockfile keys -- supply-chain guard may silently no-op at src/apm_cli/commands/uninstall/engine.py:144
lockfile.dependencies is keyed by get_unique_key() (returns repo_url). resolve_marketplace_plugin can return canonicals with #ref suffixes, host prefixes, or full HTTPS URLs. If any form differs from the stored key, the guard evaluates canonical not in lockfile.dependencies as True and REFUSES -- making Stage 2 permanently blocking when a lockfile exists.
  Suggested: Normalise resolution.canonical before guard comparison using the same normalisation applied at install time.

OSS Growth Hacker

• [recommended] CHANGELOG entry is too terse -- reframe around user benefit, not implementation detail at CHANGELOG.md
  The entry describes what changed mechanically but skips the "why it matters" hook. Users scanning the changelog for upgrade motivation need the pain-felt-before-the-fix framing.
  Suggested: Prepend: "apm uninstall now accepts the same marketplace notation as apm install (e.g. my-plugin@docs/
  Showing the old error and new happy path side-by-side converts docs readers into advocates.
  Suggested: Add a before/after code block showing the old owner/repo requirement vs. the new name(at)marketplace form.

• [nit] Issue [FEATURE] Support marketplace notation (name@marketplace) in apm uninstall #1323 mention in CHANGELOG should be hyperlinked for external readers at CHANGELOG.md
  External readers (HN, Reddit, release post) cannot click a bare issue number.
  Suggested: Replace '[FEATURE] Support marketplace notation (name@marketplace) in apm uninstall #1323' with '#1323'.

• [nit] README quick-reference for uninstall notation is a missed compounding opportunity at README.md
  If the README install example shows @AuthResolver() instantiated unconditionally before dry-run gate at src/apm_cli/commands/uninstall/cli.py:113
AuthResolver() is created before the dry_run branch. In dry-run mode the resolver is passed through but never used. Construction is cheap (env-var reads only, no network), so not a correctness or security issue -- purely unnecessary work on --dry-run paths.
  Suggested: Move AuthResolver() construction inside the if not dry_run: block.

• [nit] client.py silently creates a second AuthResolver() if caller passes None at src/apm_cli/marketplace/client.py:284
fetch_marketplace() re-instantiates AuthResolver() when auth_resolver is None. Safe fallback, but encourages callers to skip the argument. The uninstall flow passes the resolver correctly so no current bug.
  Suggested: Consider removing the silent fallback or adding a debug-level log to make it observable.

Doc Writer

• [recommended] Supply-chain guard behavior is undocumented in uninstall.md at docs/src/content/docs/reference/cli/uninstall.md:89
  The CHANGELOG explicitly calls out the supply-chain guard, but the error guidance in uninstall.md only says APM "logs an error and skips that package". A user hitting this guard sees the same message as a package-not-found error with no diagnosis path.
  Suggested: Expand the error guidance: "If the registry returns a canonical that is not already recorded in the lockfile, APM refuses the resolution as a supply-chain precaution. Use owner/repo notation to bypass registry lookup entirely."

• [recommended] #REF syntax not addressed for marketplace notation in uninstall at docs/src/content/docs/reference/cli/uninstall.md:26
  Install documents apm install NAME(at)MKT[#ref] with optional #ref override. Uninstall makes no mention of whether name(at)marketplace#ref is accepted or silently ignored. The silence leaves a discoverability gap.
  Suggested: Add a note: "The #ref fragment accepted by apm install NAME(at)MKT[#ref] is not meaningful for uninstall -- packages are identified by canonical name, not ref."

• [recommended] No-lockfile behavior for marketplace resolution is undocumented at docs/src/content/docs/reference/cli/uninstall.md:89
  If no lockfile exists, neither lockfile lookup nor the supply-chain guard works as described. The existing Behavior prose does not address this edge case.
  Suggested: Add: "If no lockfile is present, marketplace notation cannot be resolved; use owner/repo form or run apm install to restore the lockfile first."

• [nit] --dry-run note placement buries a meaningful behavioral change at docs/src/content/docs/reference/cli/uninstall.md:91
  The sentence "Registry fallback is also skipped in dry-run mode." is appended to the Behavior section. A reader scanning only the Options table will miss this interaction entirely.
  Suggested: Add a parenthetical to the Options table --dry-run row: "Registry fallback for marketplace notation is also skipped."

Test Coverage Expert

• [recommended] Supply-chain guard (registry canonical not in lockfile -> refuse) is unit-only; marketplace surface floor is integration-with-fixtures at tests/integration/test_uninstall_marketplace.py
  Two unit tests cover the guard logic (test_registry_fallback_when_not_in_lockfile, test_supply_chain_guard_refuses_canonical_not_in_lockfile). Both mock resolve_marketplace_plugin and exercise a real LockFile object -- the guard LOGIC runs. However, both integration tests pre-seed the lockfile so Stage 1 always hits. If the guard were accidentally removed, no integration or e2e test would catch it.
  Suggested: Add a third integration test: seed lockfile with canonical='acme/known-plugin', run apm uninstall injected(at)official where the resolver returns acme/injected (not in lockfile). Assert exit code != 0 and stderr contains refusal message.
  Proof (missing at integration-with-fixtures): tests/integration/test_uninstall_marketplace.py::test_uninstall_marketplace_supply_chain_guard_refuses -- proves: apm uninstall refuses to remove a package when the registry-returned canonical is not present in the lockfile [secure-by-default]

• [nit] test_registry_fallback_when_not_in_lockfile and test_supply_chain_guard_refuses_canonical_not_in_lockfile are near-identical at tests/unit/test_uninstall_engine_helpers.py
  Both exercise empty LockFile + mocked resolver returning a canonical not in lockfile + assert result is None + error logged once. Could be merged into a single parametrized test.
  Suggested: Merge into a single parametrized test or promote the variant that checks verbose_detail as the canonical case.
  Proof (passed at unit): tests/unit/test_uninstall_engine_helpers.py::test_registry_fallback_when_not_in_lockfile -- proves: registry-returned canonical absent from lockfile is refused at unit level [secure-by-default]

• [nit] Integration dry-run test only covers lockfile-hit path; no-lockfile dry-run error path lacks integration coverage at tests/integration/test_uninstall_marketplace.py
test_uninstall_marketplace_dry_run_no_changes seeds the lockfile so Stage 1 always resolves. The unit test test_dry_run_skips_registry shows that dry-run with lockfile=None produces an error. This path is not exercised by any integration test.
  Suggested: Add: seed project WITHOUT apm.lock.yaml, run apm uninstall my-plugin(at)official --dry-run, assert return code != 0 and stderr mentions the resolution failure.
  Proof (missing at integration-with-fixtures): tests/integration/test_uninstall_marketplace.py::test_uninstall_marketplace_dry_run_no_lockfile_errors -- proves: apm uninstall name@panel-review label after addressing feedback to re-run.

Generated by PR Review Panel for issue #1325 · ● 5.5M · ◷