fix: add support for custom github endpoints (host.tld/owner/repo) as apm.yml entrypoint for command apm pack (marketplace)#1288
fix: add support for custom github endpoints (host.tld/owner/repo) as apm.yml entrypoint for command apm pack (marketplace)#1288abi-jey wants to merge 5 commits into
Conversation
Allow apm.yml package entries to specify a non-default git host inline via the 'host.tld/owner/repo' source form, removing the need to set GITHUB_HOST when building marketplaces against GitHub Enterprise or other self-hosted git servers. - yml_schema: extend SOURCE_RE to accept 3-segment host-prefixed form, add split_host_from_source() helper, add 'host' field on PackageEntry populated by the loader. - builder: thread host through ResolvedPackage; per-host RefResolver cache (pre-warmed before worker dispatch to avoid races); emit full https://host/owner/repo URLs in the generated marketplace.json so Claude Code consumers can clone from the correct host. Non-subdir packages on a non-default host emit a 'url' source instead of the github shorthand which is hard-coded to github.com.
Feat/host prefixed source
…s apm.yml - test_yml_schema: add TestHostPrefixedSource covering local / default-host / host-prefixed source forms, including subdir + host combinations and rejection of malformed 4-segment / dotless 3-segment paths. - test_builder: add emitter cases verifying github shorthand for default host, source:url for host-prefixed without subdir, git-subdir+url for host-prefixed with subdir, plus per-host RefResolver isolation. Adds _build_with_host_mock helper that stubs _get_resolver_for_host so non-default hosts resolve through the in-memory mock. - publisher: route through load_marketplace_config so 'apm publish' works with the apm.yml form, not only the legacy marketplace.yml.
abi-jey
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
This PR extends marketplace packaging (apm pack marketplace block) to accept host-prefixed repository sources (e.g. ghe.example.com/owner/repo) in addition to the existing owner/repo and local ./path forms, and threads the parsed host through resolution and marketplace.json emission.
Changes:
- Broadened marketplace
sourcevalidation and parsing to splithost.tld/owner/repointoPackageEntry.host+owner/repo. - Added per-host
RefResolversupport inMarketplaceBuilderand emit fullhttps://host/owner/repoURLs for non-default hosts inmarketplace.json. - Added unit tests covering schema parsing and marketplace.json emission for host-prefixed sources.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/marketplace/yml_schema.py |
Accepts and parses host-prefixed source strings; adds PackageEntry.host. |
src/apm_cli/marketplace/builder.py |
Introduces per-host resolver caching and emits URL-based marketplace sources when a host override is present. |
src/apm_cli/marketplace/publisher.py |
Switches lazy-load to the unified marketplace config loader (apm.yml or legacy marketplace.yml). |
tests/unit/marketplace/test_yml_schema.py |
Adds schema tests for host-prefixed source parsing/splitting rules. |
tests/unit/marketplace/test_builder.py |
Adds build/output tests for host-prefixed sources and a helper to mock per-host resolver creation. |
| from .migration import load_marketplace_config | ||
| from .yml_schema import load_marketplace_yml # noqa: F401 — kept for back-compat | ||
|
|
| def _load_yml(self): | ||
| """Lazy-load marketplace.yml.""" | ||
| """Lazy-load marketplace config (apm.yml or legacy marketplace.yml).""" | ||
| if self._yml is None: | ||
| yml_path = self._root / "marketplace.yml" | ||
| self._yml = load_marketplace_yml(yml_path) | ||
| self._yml = load_marketplace_config(self._root) | ||
| return self._yml |
| # segment must look like a hostname, i.e. contain a dot) | ||
| # - ``./...`` (local path within the same repo) | ||
| # Used by both yml_schema and yml_editor for source field validation. | ||
| SOURCE_RE = re.compile(r"^(?:[^/\s]+\.[^/\s]+/[^/\s]+/[^/\s]+|[^/\s]+/[^/\s]+|\./.*)$") | ||
| LOCAL_SOURCE_RE = re.compile(r"^\./") | ||
| # Matches ``host.tld/owner/repo`` (3 segments, first is FQDN-ish). | ||
| _HOST_PREFIXED_SOURCE_RE = re.compile(r"^([^/\s]+\.[^/\s]+)/([^/\s]+/[^/\s]+)$") |
| # Source field accepts: | ||
| # - ``owner/repo`` (remote, default host) | ||
| # - ``host.tld/owner/repo`` (remote on a non-default host -- the first | ||
| # segment must look like a hostname, i.e. contain a dot) | ||
| # - ``./...`` (local path within the same repo) |
| # Reuse the resolved token only when the override host matches the | ||
| # default host class; otherwise leave token unset and rely on | ||
| # ambient git credentials (SSH key / git credential helper). | ||
| token = self._github_token if host == self._host else None | ||
| resolver = RefResolver( | ||
| timeout_seconds=self._options.timeout_seconds, | ||
| offline=self._options.offline, | ||
| host=host, | ||
| token=token, |
| if pkg.host: | ||
| source_obj["url"] = f"https://{pkg.host}/{pkg.source_repo}" | ||
| else: | ||
| source_obj["url"] = pkg.source_repo | ||
| source_obj["path"] = pkg.subdir |
| def test_per_host_resolvers_isolated(self, tmp_path: Path) -> None: | ||
| """Two entries on different hosts each get their own resolver instance.""" |
| # Per-host RefResolver cache for entries that override the host via | ||
| # the ``host.tld/owner/repo`` source form in apm.yml. | ||
| self._host_resolvers: dict[str, RefResolver] = {} |
sergio-sisternes-epam
added
the
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 1 | 1 | 2 | Sound layered design; dead token ternary is the primary structural defect; missing threading.Lock is the only fragility. |
| CLI Logging Expert | 0 | 3 | 0 | Three logging/UX gaps: publisher.py drops DEPRECATION_MESSAGE silently; no verbose trace for per-host resolver creation; no user-facing signal for ambient-credential fallback. |
| DevX UX Expert | 0 | 4 | 1 | Feature ships undiscoverable (no docs, no help text, error message hides the syntax); silent auth fallback on credential failure; source:url output shape undocumented for consumers. |
| Supply Chain Security Expert | 1 | 3 | 1 | @ userinfo injection in host regex is a confirmed confused-deputy attack surface; port injection unblocked; no host allowlist for arbitrary external connections. |
| OSS Growth Hacker | 0 | 2 | 1 | Strong enterprise unlock; ships invisible without CHANGELOG entry and manifest-schema.md example. |
| Auth Expert | 0 | 2 | 2 | Dead ternary masks AuthResolver bypass for non-default hosts; GITHUB_APM_PAT and per-org env vars silently ignored; no diagnostic on ambient-credential fallback. |
| Doc Writer | 0 | 3 | 1 | Zero documentation updated; manifest-schema.md source field description and section 7.6 example do not reflect the new form; CHANGELOG has no entry. |
| Test Coverage Expert | 0 | 2 | 0 | Token isolation for non-default hosts and pre-warm race-prevention loop are both untested; all other critical surfaces have unit coverage. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Harden
_HOST_PREFIXED_SOURCE_REhost segment to valid hostname chars only -- block@,:,?,#from the host segment to prevent RFC 3986 userinfo injection -- Confirmed injection attack surface:evil.com@github.com/owner/repopasses current regex and propagates into composed marketplace JSON as a credential-confusion URL. - [Auth Expert] (blocking-severity) Wire non-default host resolvers through
AuthResolver.resolve(host)soGITHUB_APM_PAT,GITHUB_APM_PAT_{ORG}, andGH_TOKENare consulted before falling back to ambient credentials -- Current implementation silently ignores all APM-managed tokens for non-default hosts; this fix also eliminates the dead ternary on line 302 flagged by three panelists. - [Test Coverage Expert] (blocking-severity) Add test asserting
token=Noneisolation for non-default host resolvers (test_get_resolver_for_host_non_default_host_gets_no_token) -- All existing host-prefixed tests patch_get_resolver_for_hostaway; the token isolation guarantee has no automated verification on the secure-by-default surface. - [Doc Writer + OSS Growth Hacker] Add CHANGELOG
[Unreleased]entry and updatemanifest-schema.mdsourcefield description to include thehost.tld/owner/repoform with a commented GHE example -- An enterprise unlock with no CHANGELOG entry and no schema reference effectively does not ship for the enterprise evaluator audience. Two file edits, high impact. - [DevX UX Expert] Update validation error message to hint at all three accepted source forms:
<owner>/<repo>,<host.tld>/<owner>/<repo>, or./<path>-- Current error message omits the new syntax entirely; enterprise users who mistype a host-prefixed source see a message that makes the feature appear unsupported.
Architecture
classDiagram
direction LR
class PackageEntry {
<<ValueObject>>
+name str
+source str
+subdir str|None
+version str|None
+ref str|None
+is_local bool
+host str|None
}
class MarketplaceConfig {
<<ValueObject>>
+packages tuple~PackageEntry~
+name str
+output str
}
class ResolvedPackage {
<<ValueObject>>
+name str
+source_repo str
+subdir str|None
+ref str
+sha str
+host str|None
}
class MarketplaceBuilder {
<<Orchestrator>>
-_host str
-_github_token str|None
-_resolver RefResolver
-_host_resolvers dict
-_auth_resolved bool
+resolve() ResolveResult
+build() BuildReport
+compose_marketplace_json() dict
-_get_resolver() RefResolver
-_get_resolver_for_host(host) RefResolver
-_ensure_auth() None
-_resolve_entry(entry) ResolvedPackage
}
class RefResolver {
<<Service>>
+host str
+token str|None
+ls_remote(repo) list
}
class split_host_from_source {
<<Pure>>
+split_host_from_source(source) tuple
}
note for MarketplaceBuilder "Cache pattern: _host_resolvers\npre-warmed on main thread before\nThreadPoolExecutor spawns workers"
note for PackageEntry "host is derived by loader\nfrom host.tld/owner/repo form"
MarketplaceBuilder *-- RefResolver : default resolver
MarketplaceBuilder *-- RefResolver : per-host cache (_host_resolvers)
MarketplaceBuilder ..> PackageEntry : reads
MarketplaceBuilder ..> ResolvedPackage : produces
MarketplaceConfig *-- PackageEntry : contains
MarketplaceBuilder ..> MarketplaceConfig : loads
class PackageEntry:::touched
class ResolvedPackage:::touched
class MarketplaceBuilder:::touched
class split_host_from_source:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm.yml / marketplace.yml"]) -->|"yaml.safe_load"| B["_parse_package_entry\nyml_schema.py"]
B -->|"source field present"| C{"LOCAL_SOURCE_RE match?"}
C -->|"yes: ./path"| D["is_local=True, host=None"]
C -->|"no: remote"| E["split_host_from_source()\n_HOST_PREFIXED_SOURCE_RE"]
E -->|"3-segment host.tld/owner/repo"| F["PackageEntry\nhost='host.tld'\nsource='owner/repo'"]
E -->|"2-segment owner/repo"| G["PackageEntry\nhost=None\nsource='owner/repo'"]
D --> H["MarketplaceConfig.packages"]
F --> H
G --> H
H --> I["MarketplaceBuilder.resolve()\nbuilder.py:~530"]
I -->|"main thread"| J["_ensure_auth()\n_resolve_github_token()"]
J -->|"AuthResolver.resolve()"| K["self._github_token set"]
K --> L["pre-warm loop\nfor entry.host in entries"]
L -->|"entry.host not None"| M["_get_resolver_for_host(host)\nbuilder.py:287"]
M -->|"host==self._host or None"| N["_get_resolver() default"]
M -->|"cache miss, host!=self._host"| O["token=None [dead ternary]\nnew RefResolver(host=host, token=None)\n[no lock, no AuthResolver]"]
O --> P["_host_resolvers[host] = resolver"]
L -->|"all resolvers cached"| Q["ThreadPoolExecutor\nworkers"]
Q -->|"parallel"| R["_resolve_entry(entry)\nbuilder.py:342"]
R -->|"is_local"| S["ResolvedPackage(source_repo='')"]
R -->|"remote"| T["_get_resolver_for_host(entry.host)"]
T -->|"cache hit"| U["resolver.ls_remote(owner_repo)"]
U --> V["ResolvedPackage(host=entry.host)"]
V --> W["compose_marketplace_json()"]
W -->|"pkg.host not None, pkg.subdir"| X["source=git-subdir\nurl=(host/redacted)
W -->|"pkg.host not None, no subdir"| Y["source=url\nurl=(host/redacted)
W -->|"pkg.host None"| Z["source=github\nrepo=owner/repo"]
X --> AA(["marketplace.json"])
Y --> AA
Z --> AA
Recommendation
The feature design is worth shipping -- multi-host support is a genuine APM differentiator for enterprise shops and the architectural layering is clean. But two issues must be resolved before this is safe to expose: the @ userinfo injection in the host regex (a concrete confused-deputy attack surface) and the AuthResolver bypass for non-default hosts (which silently drops all APM-managed tokens for the exact use case this feature targets). These are not large changes -- a tightened regex pattern and a call to AuthResolver.resolve(host) -- but they are the minimum bar for a security-property-introducing feature. Bundle the CHANGELOG entry and manifest-schema.md update into the same revision; the combined changeset then tells a complete and auditable story. Once those four items land, this PR is ship-ready and the growth signal is strong.
Full per-persona findings
Python Architect
-
[blocking] Token condition on line 302 is dead code -- token is always None for non-default hosts at
src/apm_cli/marketplace/builder.py:302
The guard at line 293 already returns early whenhost == self._host, so at line 302host == self._hostis always False. Currently safe-by-accident but signals incorrect intent and could cause a regression if the early return is relaxed.
Suggested: Replacetoken = self._github_token if host == self._host else Nonewithtoken = None # override hosts rely on ambient git credentials; GITHUB_TOKEN is scoped to self._host only -
[recommended]
_host_resolversdict has no lock -- pre-warm invariant is the only thread-safety guarantee atsrc/apm_cli/marketplace/builder.py:295
_get_resolver_for_hostwrites to_host_resolverswithout a lock. Safe now due to pre-warm but fragile for future refactors.
Suggested: Addself._host_resolvers_lock = threading.Lock()in__init__and wrap the check-then-set pattern. -
[nit]
hostfield placement inPackageEntryis slightly inconsistent withis_localgrouping atsrc/apm_cli/marketplace/yml_schema.py:246
Both are derived fields; grouping under one comment block would be cleaner. -
[nit] Design pattern note: Lazy Cache Factory is appropriate; no architecture changes needed at this scope.
CLI Logging Expert
-
[recommended]
publisher.pydrops the deprecation warning into stdlib logging instead of surfacing via_rich_warningatsrc/apm_cli/marketplace/publisher.py
load_marketplace_configaccepts awarn_callbackfor routing through_rich_warning.publisher.pypasses none, so the deprecation message is invisible in CLI context.
Suggested: Passwarn_callback=lambda msg: _rich_warning(f'[!] {msg}')toload_marketplace_config. -
[recommended] No verbose-mode trace when a per-host resolver is created for a non-default host at
src/apm_cli/marketplace/builder.py
_get_resolver_for_hostis completely silent. For--verboseusers debugging resolution failures against a non-default host, there is no signal.
Suggested: Emitlogger.debug('Creating resolver for host %s (token=%s)', host, 'set' if token else 'unset')after creating the resolver. -
[recommended] Silent auth fallback to ambient credentials for non-default hosts gives no user-facing signal at
src/apm_cli/marketplace/builder.py
WhentokenisNone, the resulting git error arrives with no prior context naming the host or the expected credential mechanism.
Suggested: Emitlogger.debugor_rich_info('[i] No token for host %s -- using ambient git credentials', host)whentoken=None.
DevX UX Expert
-
[recommended] Validation error message does not mention the new 3-segment syntax at
src/apm_cli/marketplace/yml_schema.py
Whensource: myhost/org/repo(no dot) fails, the error lists only<owner>/<repo>or./<path>-- never hints athost.tld/owner/repo.
Suggested: Update error to:must match '<owner>/<repo>', '<host.tld>/<owner>/<repo>', or './<path>' shape -
[recommended] No user-visible signal when falling back to ambient credentials for non-default host at
src/apm_cli/marketplace/builder.py
Auth failures for non-default hosts produce an opaque git error with no APM-level context. -
[recommended] New
source: urloutput shape has no documented schema contract for consumers atsrc/apm_cli/marketplace/builder.py
Consumers (Claude Code, CI) that pattern-match onsource == "github"will silently skip non-default-host packages. No changelog entry or version bump. -
[recommended] Feature is invisible in all user-facing docs and help text
No entry in apm.yml reference, quick-start, orpackages/apm-guide/.apm/skills/apm-usage/commands.mdmentionshost.tld/owner/repo. -
[nit] git-subdir URL asymmetry between default and non-default host is undocumented in code comments at
src/apm_cli/marketplace/builder.py
Supply Chain Security Expert
-
[blocking]
@userinfo injection in host segment passes regex, producing credential-confusion URLs in marketplace JSON atsrc/apm_cli/marketplace/yml_schema.py:77
_HOST_PREFIXED_SOURCE_REuses[^/\s]+which permits@. Confirmed:evil.com@github.com/owner/repomatches, setshost='evil.com@github.com', and compose emitsurl: (evil.com/redacted)@github.com/owner/repo. RFC 3986 userinfo: git and HTTP clients interpretevil.comas username credential forgithub.com-- a confused-deputy credential leak.
Suggested: Restrict host segment to valid hostname chars:[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)+-- blocks@,:,?,#from the host segment. -
[recommended] Port injection in host is unblocked:
evil.com:8080/owner/repopasses validation atsrc/apm_cli/marketplace/yml_schema.py:77
host='evil.com:8080'produces `url: (evil.com/redacted) APM opens network connections to non-standard ports on arbitrary hosts. -
[recommended] Query strings and fragments in source_repo segment are unblocked at
src/apm_cli/marketplace/yml_schema.py:74
[^/\s]+in the repo segment permits?,#,@. These propagate into git URLs.
Suggested: Restrict owner/repo to[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+ -
[recommended] No host allowlist: any FQDN-shaped string causes APM to open network connections to arbitrary external hosts at
src/apm_cli/marketplace/builder.py:287
A compromisedapm.ymlcan redirect resolution to any host.
Suggested: Add optionalallowed_hoststoMarketplaceOptions. Default to deny in enterprise mode. -
[nit] Dead code at builder.py line 302:
token = self._github_token if host == self._hostis always False at that point atsrc/apm_cli/marketplace/builder.py:302
OSS Growth Hacker
-
[recommended] No CHANGELOG entry -- enterprise unlock ships invisible
GHE/self-hosted support is exactly what enterprise evaluators search the changelog for.
Suggested: Add under[Unreleased] ### Added: "Marketplace packages can now be sourced from GitHub Enterprise and self-hosted Git servers using thehost.tld/owner/reposhorthand inapm.yml-- no extra flags required." -
[recommended] Marketplace authoring docs missing a host-prefixed source example
manifest-schema.mdpackages examples only showowner/reposhorthand. Enterprise users scan examples first.
Suggested: Add commented example:source: ghe.corp.example.com/org/agents # GitHub Enterprise or self-hosted GitLab -
[nit] The dot-in-first-segment host detection heuristic is implicit -- error path is opaque for orgs with dots in their names.
Auth Expert
-
[recommended] Dead ternary on line 302:
tokenis alwaysNonefor non-default hosts -- the host-class check is not implemented atsrc/apm_cli/marketplace/builder.py:302
Comment implies host-class comparison (GitHub-flavored vs. GitLab) but code does strict hostname equality. A GHE user with a compatible token on a different hostname gets no token.
Suggested: Useis_github_hostname(host) and is_github_hostname(self._host)for the token-sharing decision. -
[recommended] Non-default host resolvers bypass
AuthResolverentirely --GITHUB_APM_PATand per-org env vars silently ignored atsrc/apm_cli/marketplace/builder.py:303
_get_resolver_for_hostnever callsAuthResolver.resolve(host). Enterprise user with token for secondary GHE instance silently falls back to ambient credentials.
Suggested: CallAuthResolver().resolve(host).tokenfor non-default hosts; fall back toNoneonly if unresolved. Emit[!]warning when no token found. -
[nit] Comment on lines 299-301 is misleading -- describes host-class matching but implements hostname equality at
src/apm_cli/marketplace/builder.py:299 -
[nit] Pre-warm loop calls
_ensure_auth()redundantly -- already called atresolve()entry atsrc/apm_cli/marketplace/builder.py:550
Doc Writer
-
[recommended]
manifest-schema.mdsourcefield description does not mention host-prefixed form atdocs/src/content/docs/reference/manifest-schema.md
Line 609 documentssourceas accepting only(owner)/(repo)or./(path). The new form is valid but invisible.
Suggested: Update line 609 to list all three accepted forms; add commented host-prefixed entry in section 7.6 example. -
[recommended] Credential behavior for host-prefixed
sourceis undocumented atdocs/src/content/docs/reference/manifest-schema.md
Non-default hosts resolve via ambient git credentials, not the GitHub token. Critical operational detail for enterprise users.
Suggested: Add short note under section 7.5 or callout about ambient credential requirement; cross-reference authentication docs. -
[recommended] CHANGELOG
[Unreleased]has no entry for this feature atCHANGELOG.md
Feature is user-facing and belongs in the Added section.
Suggested: Add: "marketplace.packages[].sourcenow acceptshost.tld/owner/repofor non-default git hosts (fix: add support for custom github endpoints (host.tld/owner/repo) as apm.yml entrypoint for command apm pack (marketplace) #1288)" -
[nit] Section 7.6 complete example contains only github.com-style sources at
docs/src/content/docs/reference/manifest-schema.md
Test Coverage Expert
-
[recommended] No test exercises
_get_resolver_for_hostwith a real non-default host to asserttoken=Noneon the resultingRefResolverattests/unit/marketplace/test_builder.py
All host-prefixed tests patch_get_resolver_for_hostaway entirely. The token isolation guarantee has no automated verification.
Proof (missing):tests/unit/marketplace/test_builder.py::test_get_resolver_for_host_non_default_host_gets_no_token-- proves: A token resolved for the default GitHub host is never forwarded to a non-default GHE host [secure-by-default]
resolver = builder._get_resolver_for_host('ghe.example.com'); assert resolver._token is None -
[recommended] Pre-warm loop in
resolve()is untested -- race prevention guarantee is unverified attests/unit/marketplace/test_builder.py
The loop pre-populates_host_resolversbeforeThreadPoolExecutorstarts. No test verifies it runs before any future is submitted.
Proof (missing):tests/unit/marketplace/test_builder.py::test_resolve_prewarms_per_host_resolvers_before_worker_spawn-- proves: Per-host resolvers are created on the main thread before workers start [devx]
with patch.object(builder, '_get_resolver_for_host', wraps=...) as spy: builder.resolve(...); assert spy.call_count >= unique_host_count
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- fix: add support for custom github endpoints (host.tld/owner/repo) as apm.yml entrypoint for command apm pack (marketplace) #1288
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #1288
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by PR Review Panel for issue #1288 · ● 4.8M · ◷
github-actions
Bot
removed
the
panel-review
Description
Currently apm pack (on apm yaml for marketplace block) does validate only github.com or local file. This PR extends the validation and package entries to support the items hosted in github enterprise for example with including the host (optional if exists).
Fixes # (issue)
Type of change
Testing