fix(marketplace): resolve `marketplace check` against each entry's host + token by leocamello · Pull Request #1763 · microsoft/apm

leocamello · 2026-06-12T23:14:06Z

Description

apm marketplace check built a single default-host RefResolver with no token and called list_remote_refs(entry.source), ignoring each entry's effective host. After #1736 a relative source under marketplace.sourceBase parses as host=None with the bare relative source (the base is composed only on the build/pack path, which check doesn't share), so check ran git ls-remote https://github.com/<relative>.git and failed with exit 128 for every sourceBase entry — and for the pre-existing #1288 host.tld/owner/repo form — even when apm pack against the same apm.yml succeeded.

check now mirrors the builder's routing: it composes a host-less source onto sourceBase (via the shared split_source_base), honours per-entry host overrides, resolves the host's token through a shared marketplace/auth_helpers.resolve_token_for_host, and short-circuits local ./ packages before any network call. Default-host entries are unchanged.

Verified end-to-end against a real self-managed GitLab: the exit-128 case now resolves against the composed nested path.

Fixes #1762

Type of change

Bug fix
New feature
Documentation
Maintenance / refactor

Testing

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

Adds three check regressions in tests/unit/commands/test_marketplace_check.py: composed → base host + token, host-prefixed override → github, local → zero ls-remote. uv run pytest tests/unit/marketplace tests/unit/commands is green (2607 passed); ruff check / ruff format --check clean.

Spec conformance (OpenAPM v0.1)

N/A -- this PR does not change OpenAPM-observable behaviour.

(src/apm_cli/marketplace/ and commands/marketplace/ are not Mode-B critical paths; no normative requirement changes.)

`apm marketplace check` built a single default-host RefResolver with no token and called list_remote_refs(entry.source), ignoring the entry's effective host. After microsoft#1736 a relative source under `marketplace.sourceBase` parses as host=None with the bare relative `source`, and the base is composed only on the build (`pack`) path -- which `check` does not share. So `check` ran `git ls-remote https://github.com/<relative>.git` and failed with exit 128 for every `sourceBase` entry (and for the pre-existing microsoft#1288 host-prefixed form), even when `apm pack` against the same apm.yml succeeded. `check` now mirrors the builder's routing: it composes a host-less source onto `sourceBase` (via the shared `split_source_base`), honours per-entry host overrides, resolves a per-host token through the shared `marketplace/auth_helpers.resolve_token_for_host`, and short-circuits local `./` packages before any network call. Default-host entries are unchanged. Adds three `check` regressions: composed -> base host + token, host-prefixed override -> github, local -> zero ls-remote. Verified end-to-end against a real self-managed GitLab (the exit-128 case now resolves). Follows up microsoft#1736 / closes the `check` gap flagged there.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR aligns apm marketplace check with apm pack by resolving each package against its effective host (including sourceBase-composed sources) and using per-host auth tokens, while also skipping network checks for local packages.

Changes:

Added shared resolve_token_for_host() helper for per-host auth token resolution.
Updated marketplace check to route each entry to a per-host RefResolver (and skip ls-remote for local entries).
Added unit tests covering sourceBase composition, host overrides, and local-package behavior.

Reviewed changes

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

File	Description
tests/unit/commands/test_marketplace_check.py	Adds tests validating per-host resolution/token usage and that local entries avoid network calls.
src/apm_cli/marketplace/auth_helpers.py	Introduces shared helper to resolve host-specific tokens consistently across commands.
src/apm_cli/commands/marketplace/check.py	Implements per-host resolver caching and entry coordinate routing to match `apm pack`; skips remote checks for local entries.
CHANGELOG.md	Documents the behavioral change in `apm marketplace check`.

 )


+def _entry_coordinates(entry, source_base):


+    if source_base:
+        base_host, base_path = split_source_base(source_base)
+        return base_host, f"{base_path}/{entry.source}"


+    def _resolver_for(host: str | None) -> RefResolver:
+        if host not in resolvers:
+            if host is None:
+                resolvers[host] = RefResolver(offline=offline)
+            else:
+                token = resolve_token_for_host(host, offline=offline)
+                resolvers[host] = RefResolver(offline=offline, host=host, token=token)


  preserving hand-authored files. Use `--no-dedup` / `--force-instructions`
  to write full `AGENTS.md` files anyway. (by @tillig; closes #1730, related:
  #1138, #1550) (#1742)
+- `apm marketplace check` now resolves each package against its effective host with that host's token, matching `apm pack`. Entries on a non-default host -- a `host.tld/owner/repo` shorthand or a relative source composed onto `marketplace.sourceBase` -- previously failed with `git ls-remote` exit 128 because `check` always probed the default host with no token; local `./` packages now skip the network. Follows up #1736.


Centralize marketplace host token resolution, add verbose routing details for marketplace check, move the changelog note to Unreleased, and cover the shared auth helper branches. Addresses apm-review-panel follow-ups for PR microsoft#1763. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…-sourcebase # Conflicts: # CHANGELOG.md

Tighten the changelog symptom, simplify builder's shared auth helper delegation, and make verbose check output copy-pasteable for non-default hosts. Addresses final apm-review-panel nit follow-ups for PR microsoft#1763. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

danielmeppiel · 2026-06-15T17:26:50Z

APM Review Panel: `ship_with_followups`

PR #1763 now aligns apm marketplace check with per-entry effective host routing while preserving the contributor's original fix.

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

The panel's high-signal follow-ups were folded into this PR: marketplace check now reuses shared marketplace auth resolution, builder delegates to the same helper, local and remote verbose output explain the resolution path, the changelog entry sits under Unreleased, and the helper has direct regression tests. Remaining discussion is a broader marketplace input-hardening idea that crosses this PR's stated scope because it should be designed once for schema, pack, and check rather than added only to the check path.

Aligned with: multi-harness/multi-host support through per-host marketplace resolution; secure-by-default token handling through AuthResolver; pragmatic DevX through clearer verbose diagnostics.

Panel summary

Persona	N	Takeaway
Python Architect	0	Shared auth helper delegation is now clean; no architecture follow-up remains in scope.
CLI Logging Expert	0	Verbose-detail messages are well-formed; default output remains unchanged.
DevX UX Expert	0	`check` now mirrors `pack` host routing and gives copy-pasteable verbose context.
Supply Chain Security Expert	1	Token handling is safe; broader path-segment hardening should be shared across marketplace parsing/build/check.
OSS Growth Hacker	0	Changelog now leads with the user-visible exit-128 symptom and multi-host trust story.
Auth Expert	0	Per-host auth flows through AuthResolver and builder/check share the helper.
Doc Writer	0	Changelog placement is correct under Unreleased / Fixed; no docs drift found.
Test Coverage Expert	0	Routing and auth-helper branches have unit regression traps; targeted tests pass.
Performance Expert	0	Perf-neutral fix; resolver/auth objects are reused per check invocation.

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

Recommendation

Ship after maintainer review. The PR is mergeable, local lint/tests are green, GitHub reports all current checks successful, and the only remaining note is intentionally deferred because it is a shared marketplace validation design concern rather than part of this host-routing bug fix.

Deferred (out-of-scope follow-ups)

(panel) Add shared validation for .. path segments in marketplace sources -- scope boundary: PR scope is making check match existing pack/sourceBase routing; path-segment hardening should be designed once in schema/builder/check so the commands do not diverge.

Folded in this run

(panel) Move changelog entry to Unreleased / Fixed and tighten the user-facing wording -- resolved in 1b8efa3 and 191c15a.
(panel) Reuse one AuthResolver per marketplace check invocation and share token-resolution logic with builder -- resolved in 1b8efa3.
(panel) Add verbose details for local skips and effective remote resolution -- resolved in 1b8efa3 and 191c15a.
(panel) Refactor MarketplaceBuilder._resolve_token_for_host to delegate to the shared helper -- resolved in 1b8efa3 and 191c15a.
(panel) Add direct unit tests for resolve_token_for_host offline, exception, supplied-resolver, and lazy-resolver branches -- resolved in 1b8efa3.

Copilot signals reviewed

Copilot left a summary review and no inline comments. No LEGIT inline items were available to fold.

Regression-trap evidence (mutation-break gate)

tests/unit/marketplace/test_auth_helpers.py::test_resolve_token_for_host_offline_returns_none_without_resolver -- changed the offline guard; test FAILED as expected; guard restored.
tests/unit/marketplace/test_auth_helpers.py::test_resolve_token_for_host_returns_none_when_resolver_raises -- narrowed the exception fallback; test FAILED as expected; guard restored.
tests/unit/marketplace/test_auth_helpers.py::test_resolve_token_for_host_returns_token_from_supplied_resolver and tests/unit/marketplace/test_auth_helpers.py::test_resolve_token_for_host_creates_resolver_when_not_supplied -- removed token return; tests FAILED as expected; guard restored.

Lint contract

uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both completed with no diagnostics. Full local lint mirror also passed: pylint R0801 and scripts/lint-auth-signals.sh clean.

CI

gh pr checks 1763 --repo microsoft/apm reports all current checks successful: license/cla SUCCESS (after 0 CI fix iteration(s)).

Mergeability status

Captured from gh pr view 1763 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#1763	`191c15a`	ship_with_followups	2	5	1	2	green	MERGEABLE	BLOCKED	pending required review

Convergence

2 outer iteration(s); 2 Copilot round(s). Final panel verdict: ship_with_followups.

Ready for maintainer review.

Full per-persona findings

Supply Chain Security Expert

[nit] _entry_coordinates does not sanitize path-traversal sequences in entry.source before composing onto sourceBase.
This is deferred because the correct fix belongs in shared marketplace validation used by both pack and check.

All other panel personas returned no in-scope findings after the folded commits.

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

leocamello · 2026-06-15T21:06:35Z

Thanks for the shepherding here, @danielmeppiel -- the folded follow-ups are clear improvements: sharing one AuthResolver per check invocation, having the builder delegate to the same resolve_token_for_host helper (the shared extraction I'd flagged on #1736), the verbose resolution diagnostics, and the direct test_auth_helpers.py coverage all land well.

Agreed on deferring the .. path-segment hardening -- it's the right call to design that once across schema/pack/check rather than bolt it onto the check path here. Happy to pick up that follow-up if it'd help; otherwise I'm glad to leave it to the shared design.

Nothing outstanding from my side -- ready whenever a maintainer is.

This was referenced Jun 12, 2026

feat(marketplace): add sourceBase for nested package sources (#1519) #1731

Closed

bug: apm marketplace check resolves sourceBase entries against github.com (exit 128) #1762

Open

leocamello marked this pull request as ready for review June 14, 2026 22:21

leocamello requested review from danielmeppiel and sergio-sisternes-epam as code owners June 14, 2026 22:21

Copilot AI review requested due to automatic review settings June 14, 2026 22:21

Copilot AI reviewed Jun 14, 2026

View reviewed changes

danielmeppiel and others added 3 commits June 15, 2026 19:13

Merge remote-tracking branch 'origin/main' into fix/marketplace-check…

8201202

…-sourcebase # Conflicts: # CHANGELOG.md

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(marketplace): resolve `marketplace check` against each entry's host + token#1763

fix(marketplace): resolve `marketplace check` against each entry's host + token#1763
leocamello wants to merge 4 commits into
microsoft:mainfrom
leocamello:fix/marketplace-check-sourcebase

leocamello commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

danielmeppiel commented Jun 15, 2026

Supply Chain Security Expert

Uh oh!

leocamello commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

leocamello commented Jun 12, 2026

Description

Type of change

Testing

Spec conformance (OpenAPM v0.1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

danielmeppiel commented Jun 15, 2026

APM Review Panel: ship_with_followups

Panel summary

Recommendation

Deferred (out-of-scope follow-ups)

Folded in this run

Copilot signals reviewed

Regression-trap evidence (mutation-break gate)

Lint contract

CI

Mergeability status

Convergence

Supply Chain Security Expert

Uh oh!

leocamello commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

APM Review Panel: `ship_with_followups`