fix(marketplace): resolve source.repo vs source.repository key mismatch (#1105)

[sergio-sisternes-epam]

Description

Fix key mismatch between apm pack output and apm install resolver that caused all marketplace installs to fail with:

Invalid github source: 'repo' field must be 'owner/repo', got ''

The resolver only read the "repo" key, but:

• Old marketplaces and Copilot CLI use "repository"
• The builder emits "url" for git-subdir sources
• The builder uses "source" as discriminator, not "type"

The fix adds backward-compatible fallbacks so both old and new formats are accepted.

Fixes #1105

Type of change

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

Testing

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

7 new unit tests covering:

• "repository" fallback for github sources
• "url" fallback for git-subdir sources
• "source" discriminator key fallback
• Precedence when both old and new keys are present
• End-to-end old-format resolution via resolve_plugin_source

[sergio-sisternes-epam]

Local Validation

Unit tests

Full unit test suite passes (7094 tests, 0 failures):

uv run --extra dev pytest tests/unit tests/test_console.py --ignore=tests/unit/test_audit_report.py -x --tb=short

End-to-end marketplace resolution

Validated the fix against a real multi-package marketplace repository (46 plugins across github and git-subdir source types):

1. apm pack — built marketplace.json from the source repo. Output contains both "source": "github" and "source": "git-subdir" entries with new-format keys (repo/url/sha).

2. Registered marketplace — created a test project with apm.yml pointing at the packed marketplace. apm install --dry-run correctly lists all 46 available plugins.

3. Direct resolver call — invoked resolve_marketplace_plugin('<plugin_name>', '<marketplace_name>') and confirmed it resolves to the expected owner/repo#sha reference without error.

Before fix: all marketplace installs fail with:

Invalid github source: 'repo' field must be 'owner/repo', got ''

After fix: resolution succeeds for both github and git-subdir source types.

Lint

uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/
# All checks passed, all files already formatted

[Copilot]

Pull request overview

Fixes marketplace install failures caused by mismatched marketplace.json source field names by making the marketplace resolver accept both legacy and current builder/Copilot formats.

Changes:

• Add backward-compatible resolver fallbacks for repository vs repo, url vs repo (git-subdir), and source vs type discriminator.
• Add unit tests covering key fallbacks and precedence; update golden fixture + related builder/integration assertions to match the canonical marketplace.json shape.
• Add a changelog entry under Unreleased / Fixed for the marketplace install regression.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/apm_cli/marketplace/resolver.py	Adds key-name fallbacks to accept both old and new marketplace source shapes.
tests/unit/marketplace/test_marketplace_resolver.py	Adds unit tests for fallbacks + precedence across github/git-subdir/source discriminator.
tests/unit/marketplace/test_builder.py	Updates golden-shape assertions to match the canonical emitted marketplace.json source keys.
tests/integration/marketplace/test_build_integration.py	Updates integration assertions for source key ordering and SHA field name.
tests/fixtures/marketplace/golden.json	Updates the canonical marketplace.json fixture to the current source schema (source/`repo
CHANGELOG.md	Adds an Unreleased Fixed entry describing the marketplace install fix.

Comments suppressed due to low confidence (2)

src/apm_cli/marketplace/resolver.py:80

• The error message still says "'repo' field" even when the value came from the legacy repository key (fallback added just above). This can mislead users debugging old marketplace.json entries; consider mentioning both accepted keys (e.g., repo/repository) or dynamically naming the key that was used.

    repo = source.get("repo", "") or source.get("repository", "")
    ref = source.get("ref", "")
    path = source.get("path", "").strip("/")
    if not repo or "/" not in repo:
        raise ValueError(f"Invalid github source: 'repo' field must be 'owner/repo', got '{repo}'")
    if path:

src/apm_cli/marketplace/resolver.py:122

• With the new fallback to the url key, this validation error still references only 'repo'. If a git-subdir entry is using the builder's url field, the resulting message is confusing; update the message to reflect that repo or url is accepted (or report which field was present).

    repo = source.get("repo", "") or source.get("url", "")
    ref = source.get("ref", "")
    subdir = (source.get("subdir", "") or source.get("path", "")).strip("/")
    if not repo or "/" not in repo:
        raise ValueError(f"Invalid git-subdir source: 'repo' must be 'owner/repo', got '{repo}'")

[github-actions]

APM Review Panel: ship_with_followups

fix(marketplace): three backward-compat key fallbacks restore apm install (pkg)@(marketplace) for ALL packages; one url-alias security note and an integration coverage gap warrant fast follow.

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

This PR fixes a real, total breakage: every apm install <pkg>@<marketplace> invocation was failing because the resolver expected repo / type keys that marketplace manifests now ship as repository / source. The three fallback chains are additive and non-breaking; the 7 new unit regression traps (confirmed present by test-coverage-expert at tests/unit/marketplace/test_marketplace_resolver.py) provide meaningful protection against reversion. The panel converges on shipping with follow-ups rather than blocking.

The highest-signal concern is the supply-chain finding: the url fallback in _resolve_git_subdir_source accepts a full URL where the validator only checks for a / character. A well-formed (malicious.example.com/redacted) string passes that check. The install would not silently succeed with malicious content -- resolution would fail at a later network or auth boundary -- but the intent of the validator (enforce owner/reposhape) is defeated. The finding is real and worth a fast follow, but it is not a silent-exfiltration vector, and theurl` key was already live in marketplace manifests before this PR. Removing or guarding the fallback is the right call; it should happen in a dedicated commit rather than blocking a total-install-breakage fix.

The integration coverage gap (no fixture exercises the old-format keys through the full resolve+install pipeline, and golden.json was migrated away from the old format) is the second-highest signal. The test-coverage-expert classified it recommended with outcome: missing against the portability-by-manifest surface. A missing integration trap on a portability-critical surface ranks above opinion findings. The maintainer should add an old-format fixture path before the next release cycle, not as a condition of this merge.

Dissent. supply-chain-security-expert rated the url-alias finding recommended (not blocking); devx-ux-expert saw no UX regression. These are consistent -- the finding is a real design smell and a future hardening target, not a current exploit. Blocking a fix to a total install breakage over a non-exploitable validator gap would harm more users than it protects.

Aligned with: Portable by manifest: the fallbacks directly restore manifest portability -- packages with any of the three legacy key names now resolve correctly without manifest edits. Secure by default: the url-as-owner/repo alias in git-subdir partially undermines shape enforcement; a scheme guard is the secure-by-default correction, tracked as a follow-up. Pragmatic as npm: a three-line fallback chain that unblocks 100% of marketplace installs with zero breaking change is exactly the pragmatic compatibility posture APM should hold.

Growth signal. This fix unblocks the entire marketplace install path -- every apm install (pkg)@(marketplace) command that was silently failing now works. That is a marquee reliability beat worth a dedicated release-note callout and a short FAQ entry in the marketplace guide explaining the supported key aliases. Lead the release note with the user symptom ("apm install now works for all marketplace packages"), not the internal key-name delta.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	2	Backward-compat fallbacks are correct and minimal; one semantic smell (url key holds owner/repo, not a URL) and integration fixture no longer exercises the old format it claims to fix.
CLI Logging Expert	0	2	0	Error messages still name only 'repo' after dual-key fallbacks were added, leaving users with a misleading hint when the field they actually provided is wrong.
DevX UX Expert	0	0	1	Bug fix restores the apm install (pkg)@(marketplace) happy path; no CLI surface changes, no UX regressions.
Supply Chain Security Expert	0	1	1	url-as-repo alias in git-subdir bypasses owner/repo intent and can pass a full URL through the slash-check; other two aliases are low risk.
OSS Growth Hacker	0	1	1	Critical install-path fix with strong CHANGELOG hook; golden.json key renames risk silent confusion in contributor docs.
Doc Writer	0	2	1	CHANGELOG entry omits two of three fallback pairs; docs adequately describe format compat but the entry's prose is incomplete and may mislead.
Test Coverage Expert	0	2	1	7 unit tests confirmed; missing integration-with-fixtures regression trap for the full install pipeline consuming old-format marketplace data; PR body omits Scenario Evidence table.

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] Add a scheme guard (reject strings starting with ://) or remove the url fallback in _resolve_git_subdir_source to restore owner/repo shape enforcement. -- The current url fallback defeats the / validator: a full URL passes the check. Non-exploitable today but a latent host-redirect risk that should be closed before the next release.
2. [Test Coverage Expert] Add an integration-with-fixtures test feeding a golden.json using the OLD key names (repository, source, url) through the full resolve+install pipeline. -- golden.json was migrated to the new format, so the integration layer no longer exercises the fallback paths this PR exists to protect. A missing fixture on a portability-critical surface is a regression trap waiting to fire.
3. [CLI Logging Expert] Update the two error messages in resolver.py (~lines 79 and 122) to name all accepted key aliases, e.g. 'repo' (or 'repository') must be 'owner/repo'. -- A user who supplies the now-accepted repository or url key and gets a malformed value will see an error blaming repo -- a key they never wrote. Low effort, high clarity.
4. [Doc Writer] Expand the CHANGELOG entry to cover all three fallback pairs (repo/repository, type/source, repo/url) and rewrite in active voice. -- The current entry describes only one of three fixes; developers hitting the type or url mismatch variant won't know the release covers them.
5. [Doc Writer] Document the old marketplace format (type/repository/commit keys) as a supported input shape in docs/src/content/docs/guides/marketplaces.md. -- Contributors writing marketplace manifests have no documented reference for which key aliases are accepted; this creates ongoing key-mismatch support burden.

Architecture

classDiagram
    direction LR
    class resolver {
        <<Module / IOBoundary>>
        +parse_marketplace_ref(specifier) tuple
        +resolve_plugin_source(plugin, ...) str
        +resolve_marketplace_plugin(name, mkt, ...) str
    }
    class _resolve_github_source {
        <<Pure>>
        +__call__(source dict) str
    }
    class _resolve_git_subdir_source {
        <<Pure>>
        +__call__(source dict) str
    }
    class _resolve_url_source {
        <<Pure>>
        +__call__(source dict) str
    }
    class _resolve_relative_source {
        <<Pure>>
        +__call__(source str, owner str, repo str) str
    }
    class MarketplacePlugin {
        <<ValueObject>>
        +name str
        +source dict | str | None
        +version str | None
    }
    class DependencyReference {
        <<ValueObject>>
        +parse(url) DependencyReference
        +repo_url str
        +reference str
    }
    resolver ..> _resolve_github_source : dispatches
    resolver ..> _resolve_git_subdir_source : dispatches
    resolver ..> _resolve_url_source : dispatches
    resolver ..> _resolve_relative_source : dispatches
    resolver ..> MarketplacePlugin : reads
    _resolve_url_source ..> DependencyReference : delegates
    class _resolve_github_source:::touched
    class _resolve_git_subdir_source:::touched
    class resolver:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
    note for resolver "Dispatch: source.get('type','') or source.get('source','')\nFallbacks added by this PR"
    note for _resolve_github_source "Fallback: repo -> repository"
    note for _resolve_git_subdir_source "Fallback: repo -> url (semantic smell: url != URL)"

Loading

flowchart TD
    A(["apm install plugin@marketplace"]) --> B["resolve_marketplace_plugin\nresolver.py:246"]
    B --> C["fetch_or_cache\nclient.py"]
    C -->|"[NET] HTTP fetch marketplace manifest"| D["manifest.find_plugin(name)"]
    D --> E["resolve_plugin_source(plugin)\nresolver.py:178"]
    E --> F{"isinstance(source, str)?"}
    F -->|yes| G["_resolve_relative_source"]
    F -->|no| H{"source_type = source.get('type','')\n  OR source.get('source','')\n  [PR change line 210]"}
    H -->|"github"| I["_resolve_github_source\nrepo = source.get('repo','')\n  OR source.get('repository','')\n  [PR change line 75]"]
    H -->|"git-subdir"| J["_resolve_git_subdir_source\nrepo = source.get('repo','')\n  OR source.get('url','')\n  [PR change line 118]"]
    H -->|"url"| K["_resolve_url_source\nurl = source.get('url','')\n-> DependencyReference.parse(url)"]
    H -->|"npm"| L["ValueError: npm not supported"]
    H -->|"unknown"| M["ValueError: unsupported type"]
    I --> N["canonical owner/repo#ref"]
    J --> N
    K --> N
    G --> N
    N --> O["version_spec override if supplied"]
    O --> P["ref immutability check\nversion_pins.py"]
    P --> Q["shadow detection\nshadow_detector.py"]
    Q --> R(["return canonical, plugin"])
    style I fill:#fff3b0,stroke:#d47600
    style J fill:#fff3b0,stroke:#d47600
    style H fill:#fff3b0,stroke:#d47600

Loading

Recommendation

Merge this PR. It fixes a total install breakage with backward-compatible, minimal changes and 7 unit regression traps already in place. Track the url-alias scheme-guard hardening and the old-format integration fixture as the two highest-priority follow-ups; both belong in the next patch. The error-message and CHANGELOG improvements are low-effort and can land in the same patch window.

---

Full per-persona findings

Python Architect

• [recommended] golden.json fixture migrated to NEW format, removing integration-level coverage of OLD format at tests/fixtures/marketplace/golden.json
  The PR's stated goal is backward-compat with the old 'type'+'repository'+'commit' format. Unit tests cover old-format paths well, but golden.json was simultaneously migrated from old to new format. This means test_build_integration.py now only exercises the new schema path end-to-end; a future regression in the old-format fallback branch would not be caught at the integration-with-fixtures tier.
  Suggested: Add a second plugin entry in the fixture using the old schema keys (type/repository/commit) so the integration suite covers both code paths.
  Proof (missing at integration-with-fixtures): tests/integration/marketplace/test_build_integration.py -- proves: Old-format marketplace entries (type/repository/commit) survive the full build-then-resolve pipeline without regression. [portability-by-manifest, oss]

• [recommended] 'url' key in git-subdir source holds owner/repo coordinate, not a URL -- semantic mismatch with _resolve_url_source where 'url' is a real Git URL at src/apm_cli/marketplace/resolver.py:118
  In _resolve_url_source, 'url' is passed to DependencyReference.parse() and must be a full Git URL. In _resolve_git_subdir_source, the same key name 'url' is expected to hold an 'owner/repo' shorthand. A reader or future author can easily conflate them, leading to a bug where a real Git URL is passed to _resolve_git_subdir_source and accepted (since it contains '/'), producing a malformed canonical string.
  Suggested: Rename the builder-emitted key from 'url' to 'repo' for git-subdir sources, keeping 'url' as the fallback only for old serialized data.

• [nit] Error message still names only 'repo' field even when value came from 'repository' or 'url' fallback at src/apm_cli/marketplace/resolver.py:79
  Line 79 says "'repo' field must be 'owner/repo'" but the value might have come from 'repository'. Misleads debugging.
  Suggested: Change to: "'repo'/'repository' field must be 'owner/repo', got '{repo}'"

• [nit] Design patterns: none warranted -- straight-line procedural fallback chains using 'or' short-circuit on dict.get() are appropriate for the scope. A SourceNormalizer or Adapter layer would only pay off at a third key-name variant; at two variants the inline 'or' is cleaner.

CLI Logging Expert

• [recommended] Error message says "'repo' field must be 'owner/repo'" but resolver now also accepts 'repository' at src/apm_cli/marketplace/resolver.py:79
  A user who supplied {"repository": "just-a-name"} will see a confusing error blaming a 'repo' field they never set.
  Suggested: "Invalid github source: 'repo' (or 'repository') must be 'owner/repo', got '{repo}'"

• [recommended] Same misleading error in _resolve_git_subdir_source: says "'repo' must be 'owner/repo'" but 'url' is now also accepted at src/apm_cli/marketplace/resolver.py:122
  Suggested: "Invalid git-subdir source: 'repo' (or 'url') must be 'owner/repo', got '{repo}'"

DevX UX Expert

• [nit] Error message exposes internal key name 'repo' to the user at src/apm_cli/marketplace/resolver.py:75
  The existing error names the internal dict key 'repo', which is an implementation detail users cannot control. Pre-exists this PR but the fallback additions make it more likely a user hits a novel variant.
  Suggested: "Invalid github source: could not determine repository (expected owner/repo format)" -- avoids leaking key names.

Supply Chain Security Expert

• [recommended] git-subdir 'url' fallback lets a full URL silently pass the owner/repo validator at src/apm_cli/marketplace/resolver.py:118
  source.get('url','') accepts any truthy string with a slash. A marketplace entry setting url: '(malicious.example.com/redacted) passes the '/' guard and is used verbatim in the install URL construction. Identity check (lockfile SHA) would still catch a compromised package, but the error would be silent and confusing.
  Suggested: Remove the or source.get('url', '') fallback from _resolve_git_subdir_source, or validate: if repo.startswith(('(redacted) 'https://', '(redacted) raise ValueError(...)
  Proof (missing at unit): tests/marketplace/test_resolver.py -- proves: git-subdir source rejects a full URL in the 'url' field rather than passing it through as a canonical coordinate. [secure-by-default]

• [nit] 'source' key alias for type dispatch widens the manifest attack surface without documentation at src/apm_cli/marketplace/resolver.py:210
  Accepting an undocumented alias expands the trusted surface in the manifest schema and makes auditing harder. Should be documented in the manifest schema.

OSS Growth Hacker

• [recommended] CHANGELOG entry leads with technical key-name explanation rather than the user-symptom hook at CHANGELOG.md
  Most users who hit this saw 'Invalid github source' and gave up. The hook should lead with what broke for them. Leading with the symptom maximises the 'existing user -> upgrades and shares' conversion.
  Suggested: "apm install <pkg>@<marketplace> now works -- a source.repo vs source.repository key-name mismatch in the install resolver was silently blocking every marketplace package install. The resolver now accepts both key names with a backward-compatible fallback. (fix(marketplace): resolve source.repo vs source.repository key mismatch (#1105) #1106, closes [BUG] apm install <pkg>@<marketplace> fails — source.repo vs source.repository key mismatch #1105)"

• [nit] golden.json key renames (type->source, repository->repo, commit->sha) are not mentioned in the PR body or CHANGELOG, making them invisible to downstream contributors who copy the fixture format at tests/fixtures/marketplace/golden.json

Auth Expert -- inactive

No auth-path files touched; resolver.py changes affect source-key parsing only, not credential resolution, token management, or host classification.

Doc Writer

• [recommended] CHANGELOG entry describes only one of three key-name fallbacks introduced by the fix at CHANGELOG.md
  Two other fallback pairs -- type->source (discriminator) and repo->url (git-subdir source) -- are silently omitted. A developer debugging a marketplace entry with a type or url key mismatch will not know this fix covers them.
  Suggested: "The resolver now accepts both key names for three fields: source/type (discriminator), repo/repository (GitHub source), and repo/url (git-subdir source), with a backward-compatible fallback for each."

• [recommended] Old marketplace format (type/repository/commit keys) not documented in guides as a supported input shape at docs/src/content/docs/guides/marketplaces.md
  Given the fix's purpose is to silently normalise old entries, a one-line callout would remove ambiguity for marketplace authors.
  Suggested: Under the existing Copilot CLI format note, add: "APM also accepts type as an alias for source, commit as an alias for sha, and repository as an alias for repo, so entries in the old Copilot CLI key names are resolved without modification."

• [nit] CHANGELOG voice uses passive framing; active voice preferred per doc-writer style rules at CHANGELOG.md
  Suggested: Rephrase: "The install resolver was reading source.repo but marketplace entries use source.repository; it now accepts both key names (and two further alias pairs) with a backward-compatible fallback."

Test Coverage Expert

• [recommended] No integration-with-fixtures test exercises the install pipeline consuming old-format marketplace.json at tests/integration/marketplace/test_resolver_integration.py
  The marketplace surface floor tier is integration-with-fixtures. Probed tests/integration/marketplace/ -- contains test_build_integration.py (builder output) and test_outdated_integration.py (ref_resolver), but none exercise resolve_plugin_source with a fixture marketplace.json using 'repository' or 'url' keys. If the fallback logic is accidentally reverted, no integration test would catch it before a user hits the error.
  Suggested: Add test_resolver_integration.py seeding a local marketplace.json fixture with old-format keys, calling the resolve pipeline, asserting the resolved dep string matches 'owner/repo#ref'.
  Proof (passed at unit): tests/unit/marketplace/test_marketplace_resolver.py::TestResolvePluginSource::test_old_format_repository_key -- proves: resolve_plugin_source accepts old-format 'repository' key in isolation. [portability-by-manifest, devx]
  Proof (missing at integration-with-fixtures): tests/integration/marketplace/test_resolver_integration.py::test_install_pipeline_with_old_format_marketplace_json -- proves: a marketplace.json with old-format keys flows through the full resolver+install pipeline without raising 'Invalid github source'. [portability-by-manifest, devx]

• [recommended] PR body omits the Scenario Evidence table required by the scenario-evidence rubric
  For a bug-fix PR with user-visible impact ('all marketplace installs fail'), the author should have supplied at least one scenario row mapping the scenario to an APM principle and a verifying test.
  Proof (missing at static): PR body (github.com/microsoft/apm/pull/1106)::Scenario Evidence table -- proves: Author has mapped every user-visible behavior change to an APM principle and a verifying test. [devx]

• [nit] 7 new unit tests confirmed present and correctly structured.
  Proof (passed at unit): tests/unit/marketplace/test_marketplace_resolver.py::TestResolveGithubSource::test_repository_key_fallback -- proves: old-format marketplace entries with 'repository'/'url'/'source' keys resolve correctly instead of raising the original error. [portability-by-manifest, devx]

_{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 #1106 · ● 2.7M · ◷