feat(marketplace): add sourceBase for package sources#1736
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a marketplace-level sourceBase that lets marketplace authors specify package repositories relative to an enterprise HTTPS git base path, while preserving existing per-entry override forms (host-prefixed, full HTTPS URL, and local ./ sources).
Changes:
- Extend marketplace schema/config to accept and validate
marketplace.sourceBase, and relaxpackages[].sourcevalidation to allow relative sources when a base is present. - Compose base-relative sources during build resolution and carry a canonical
source_urlthrough to output mappers. - Update docs/guides and add unit tests covering composition, override precedence, and security guards.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/marketplace/yml_schema.py |
Adds sourceBase parsing/validation and context-aware packages[].source validation. |
src/apm_cli/marketplace/builder.py |
Resolves base-relative sources before ref resolution and plumbs canonical source_url into ResolvedPackage. |
src/apm_cli/marketplace/output_mappers.py |
Emits canonical HTTPS URLs when available (via source_url) while retaining github shorthand where appropriate. |
src/apm_cli/marketplace/yml_editor.py |
Mirrors loader validation in the editor and derives default names from the final source segment. |
tests/unit/marketplace/test_marketplace_source_base.py |
New unit coverage for sourceBase behavior, precedence, and guardrails. |
docs/src/content/docs/reference/manifest-schema.md |
Documents marketplace.sourceBase and the new relative packages[].source behavior. |
docs/src/content/docs/producer/publish-to-a-marketplace.md |
Adds authoring guidance/examples for sourceBase. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Updates the packaged usage guide to include sourceBase authoring guidance. |
CHANGELOG.md |
Adds an Unreleased entry for the new sourceBase capability. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 1
| first_segment = source.split("/", 1)[0] | ||
| looks_like_unsupported_host_override = "/" in source and bool( | ||
| re.fullmatch(_HOST_PAT, first_segment) | ||
| ) |
Folds copilot-pull-request-reviewer follow-up on the sourceBase validator. The confused-deputy guard (board-ratified: validation inherits #1288 guards exactly) is kept intact; instead the manifest-schema sourceBase section now documents that a relative source whose FQDN-like first segment forms no valid override shape is rejected at parse time rather than silently composed onto the base. Adds a regression test locking that behavior and a clarifying code comment. Also ends the CHANGELOG entry with the PR number per repo convention. Refs #1519 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Supply Chain Security Expert | 0 | 0 | 0 | Guard inherits #1288 exactly; affirm the FQDN-first rejection -- it closes a confused-deputy footgun. Do not loosen. |
| Auth Expert | 0 | 0 | 0 | Composed host routes through _get_resolver_for_host -> AuthResolver; host->token selection correct, ADO-base reuse holds. |
| Doc Writer | 0 | 0 | 0 | Schema ref (7.5), publishing guide, packaged skill (Rule 4), and changelog all updated; no drift. |
| Python Architect | 0 | 1 | 0 | Clean helper extraction; shared _validate_source_value removes editor/loader drift. Add a version-range composition test. |
| Test Coverage Expert | 0 | 1 | 0 | Additive, override-vs-compose, and guard-rejection regressions all covered; version-range compose path is the one gap. |
| DevX UX Expert | 0 | 0 | 1 | Relative names + base are ergonomic; reject error names the override forms so authors recover fast. |
| CLI Logging Expert | 0 | 0 | 1 | Validation messages are ASCII, specific, and per-violation; consistent with existing marketplace errors. |
| OSS Growth Hacker | 0 | 0 | 0 | Unlocks nested-GitLab / enterprise-group publishing without per-entry repetition -- a real adoption lever. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Test Coverage Expert] Add a
version:semver composition test -- The build-composition test exercises the explicit-refbranch; a version-range entry undersourceBasewould lock the compose-onto-base behavior for_resolve_version_rangetoo. - [Python Architect] Add an ADO-shaped
sourceBasefixture (https://dev.azure.com/org/project/_git+ relative repo) -- Proves the cross-host generality Marketplaceaddsource parity with the Anthropic spec #676 will depend on and guards against accidental narrowing ofSOURCE_BASE_RE. - [Doc Writer] One-line cross-reference in Section 7.5 toward non-default-host reuse -- Helps future ADO/Marketplace
addsource parity with the Anthropic spec #676 authors discover thatsourceBasealready supports their base shape.
Architecture
classDiagram
class MarketplaceConfig {
+str|None source_base
+tuple~PackageEntry~ packages
}
class PackageEntry {
+str source
+str|None host
}
class ResolvedPackage {
+str source_repo
+str|None host
+str|None source_url
}
class MarketplaceBuilder {
+_remote_source_coordinates(entry)
+_resolved_output_host(source_host, source_url)
+_resolve_entry(entry)
}
MarketplaceConfig "1" o-- "many" PackageEntry
MarketplaceBuilder ..> MarketplaceConfig : reads source_base
MarketplaceBuilder ..> PackageEntry : resolves
MarketplaceBuilder ..> ResolvedPackage : emits
flowchart TD
P["parse packages[].source"] --> S["split_host_from_source"]
S --> H{"host present?"}
H -->|"yes (override)"| O["use per-entry host + owner/repo"]
H -->|"no"| B{"sourceBase declared?"}
B -->|"yes"| C["split_source_base -> compose base + source; set source_url"]
B -->|"no"| G["preserve default-host owner/repo behavior"]
O --> R["_get_resolver_for_host -> AuthResolver token select"]
C --> R
G --> R
Recommendation
Ship it. The work is complete, board-ratified, strictly additive, and CI is green across lint, R0801, auth-signal, and the full marketplace test matrix. Track the version-range composition test as the highest-signal follow-up so both resolution branches carry a base-compose regression guard; the ADO fixture and the Section 7.5 cross-reference are cheap insurance for the #676 reuse. None of these block merge -- the maintainer performs the protected-branch merge.
Full per-persona findings
Supply Chain Security Expert
No findings. _parse_source_base enforces the #1288 guard set verbatim (https-only scheme, FQDN host via _HOST_PAT, rejects userinfo/port/query/fragment and trailing .git) and adds validate_path_segments(reject_empty=True) plus SOURCE_BASE_RE for arbitrary-depth segment safety. The FQDN-first relative-source rejection is the correct call: a value that names a host but forms no valid host/owner/repo override is refused rather than silently composed onto the base, removing a confused-deputy exfiltration path. Affirmed -- do not loosen.
Auth Expert
Active. Composed remote entries derive their host from split_source_base and flow through _get_resolver_for_host(source_host), so a non-default base host (e.g. gitlab.example.com, dev.azure.com) is classified and tokened via AuthResolver exactly like a per-entry host override. Bare owner/repo with no base still resolves to the default host. _resolved_output_host correctly returns the base host directly (not _effective_host-normalized) when source_url is set, so the canonical URL is emitted rather than github shorthand. Host->token selection is correct and the ADO-base reuse generality holds.
Doc Writer
Active. No findings. Section 7.5 of manifest-schema.md documents sourceBase shape, override-vs-compose semantics, arbitrary path depth, and the FQDN-first rejection rationale; publish-to-a-marketplace.md shows the authoring workflow; the packaged package-authoring.md skill resource is updated per repo Rule 4; CHANGELOG has an [Unreleased] entry. No drift.
Python Architect
Active.
-
[recommended] Build-composition coverage exercises only the explicit-
refpath attests/unit/marketplace/test_marketplace_source_base.py.
Aversion:semver entry undersourceBasewould lock the compose-onto-base behavior in_resolve_version_rangeas well, since both branches now threadsource_host/source_url.
Suggested: add a parametrized version-range case assertingresolved.source_urlandsource_repo.The refactor itself is clean:
_remote_source_coordinatesand_resolved_output_hostisolate the new routing decision, and hoisting_validate_source_valueintoyml_schema(consumed byyml_editor) eliminates the prior duplicated-validator drift risk. No file-length or R0801 concerns (CI green).
Test Coverage Expert
Active.
- [recommended] The new test module covers additive/no-base compatibility, override-vs-compose precedence, single-segment rejection without base, FQDN-first rejection, the 10-case guard-violation matrix, and the editor relative-source path -- strong. The one gap is the
version:resolution branch for composition.
Suggested: mirrortest_composes_relative_source_onto_base_for_resolution_and_outputwith aversion: "^1.0.0"entry.
DevX UX Expert
Active.
- [nit] Authoring ergonomics are good:
sourceBase+ bare relative names removes per-entry base repetition, and the rejection error enumerates the four override forms so an author who hits the confused-deputy guard can recover without reading the spec. Consider surfacing the "use an explicit host-prefixed override or full https URL" hint in the error string itself (docs already state it).
CLI Logging Expert
Active.
- [nit] Validation messages are ASCII-only, specific per violation (userinfo / port / query / fragment /
.git/ FQDN / empty-segment), and consistent with existingMarketplaceYmlErrorphrasing. No logging of token or credential material on the per-host resolver path. No action required.
OSS Growth Hacker
Active. No findings. This directly unlocks the canonical enterprise pattern from #1519 (self-hosted GitLab with subgroup nesting) and lets large internal marketplaces drop per-entry base repetition -- a concrete adoption and contributor-onboarding lever for enterprise users.
Performance Expert -- inactive
Diff touches only parse-time (yml_schema.py, yml_editor.py) and build-time composition (builder.py, output_mappers.py); it does not touch deps/**, cache/**, install/phases/**, install/pipeline.py, install/resolve.py, or transport, and claims no perf win. Per-host resolvers remain cached. No performance lens required.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
|
Nice — the panel's
WhyComposition here is build-time ( resolver = RefResolver(offline=offline) # no host, no token
refs = resolver.list_remote_refs(entry.source) # entry.source is the *relative* pathso it issues Repromarketplace:
owner: { name: t }
sourceBase: https://gitlab.example.com/group/sub/team/project
packages:
- name: my-package
source: my-package
version: "^0.1.0"apm pack # succeeds: marketplace.json has the composed URL + sha
apm marketplace check # fails: "Git failed during ls-remote (exit 128)"This matters because Fix shapeBecause composition is build-time, I worked through exactly this on a draft branch (#1731): a shared At minimum, a |
…ted shapes Folds the apm-review-panel and Copilot follow-ups for PR #1736 as regression traps on the sourceBase parse/compose surface, no production change: - version-range branch: assert a `version:` entry composes onto sourceBase and routes through the composed owner/repo coordinate (the prior coverage only exercised the explicit-`ref` branch). Mutation-break verified: dropping source_url in _resolve_version_range makes the test fail. - ADO-shaped base: assert https://dev.azure.com/org/project/_git validates and composes a relative repo, so the field shape stays general enough for the #1010 ADO-reuse-sourceBase path. - dotted subgroup: assert a relative `team.tools/repo` (dotted owner) composes onto the base rather than being treated as a host override, locking the accepted behavior the Copilot inline questioned. addresses CEO follow-up FU-1 (version-range compose) and FU-2 (ADO fixture); addresses Copilot inline on src/apm_cli/marketplace/yml_schema.py. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Shepherd-driver: ready for maintainer review (CI green, mergeable)
Folded in this run
Copilot signals reviewed
Consumer-install confirm (issue #1010 / #1014 surface) -- FLAGGED, out of this PR's surfaceConfirmed via code trace: this PR fixes DISCOVERY/AUTHORING -- This fix lives in the CONSUMER install resolver path, OUTSIDE this PR's Deferred (out-of-scope follow-ups)
Regression-trap evidence (mutation-break gate)
Lint contract
CIAll required checks green on Mergeability statusCaptured from
Convergence1 outer iteration; 1 Copilot round. Final panel verdict |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | sourceBase cross-module coupling is a nit; frozen-dataclass + Strategy mapper pattern is idiomatic and sound. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI echo, rich helpers, CommandLogger, DiagnosticCollector, or progress output surfaces changed; validation errors follow existing MarketplaceYmlError style. |
| DevX UX Expert | 0 | 0 | 2 | sourceBase composes intuitively for producers; no user-facing CLI surface or error wording regressed. |
| Supply Chain Security Expert | 0 | 0 | 1 | sourceBase validation covers critical attack surfaces: https-only, FQDN host, no userinfo/port/query/fragment/.git, and path segment guards. |
| OSS Growth Hacker | 0 | 0 | 2 | Enterprise sourceBase is a strong multi-host adoption story; current docs and changelog cover it without needing README churn. |
| Auth Expert | 0 | 0 | 0 | sourceBase-derived hosts route through builder._get_resolver_for_host(source_host) and the existing per-host AuthResolver path; #1010 owns consumer-install resolver follow-up. |
| Doc Writer | 0 | 3 | 1 | Docs are accurate on validation/edge cases but should state explicitly that two-segment owner/repo also composes under sourceBase. |
| Test Coverage Expert | 0 | 0 | 0 | New test_marketplace_source_base.py covers schema guards, composition branches, output mapping, editor behavior, overrides, ADO shape, and dotted subgroups. |
| Performance Expert | 0 | 0 | 2 | No hot-path regression: sourceBase composition is in-memory string work with no extra network RTTs or filesystem I/O. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Doc Writer] Annotate or clarify the two-segment
owner/repoexample in the publish guide so producers understand it composes onto sourceBase rather than acting as an override. -- Test evidence (test_dotted_subgroup_relative_source_composes_onto_base, passed) confirms the behavior. Without an explicit note, producers may misreadowner/repoas GitHub shorthand and get unexpected results. - [Doc Writer] Add a sentence to manifest-schema section 7.5 stating that any non-override relative source, including
owner/repo, composes onto sourceBase. -- The common two-segment case is only implied; making it explicit closes the discoverability gap for the reference doc. - [OSS Growth Hacker] Lead the changelog entry with the enterprise benefit before naming the sourceBase mechanism. -- Release readers convert faster when the first words describe the outcome rather than the feature name.
Architecture
classDiagram
direction LR
class MarketplaceConfig {
<<ValueObject>>
+source_base str or None
+packages tuple[PackageEntry]
}
class PackageEntry {
<<ValueObject>>
+source str
+host str or None
+is_local bool
}
class MarketplaceBuilder {
<<Orchestrator>>
+_remote_source_coordinates(entry) tuple
+_resolve_entry(entry) ResolvedPackage
}
class ResolvedPackage {
<<ValueObject>>
+source_repo str
+host str or None
+source_url str or None
}
class OutputMappers {
<<Strategy Functions>>
+_remote_source_url(pkg) str or None
}
class YmlSchema {
<<Validation Module>>
+split_source_base()
+_parse_source_base()
+_validate_source_value()
}
class YmlEditor {
<<Round-trip Editor>>
+add_plugin_entry()
}
MarketplaceConfig *-- PackageEntry : contains
MarketplaceBuilder ..> MarketplaceConfig : reads source_base
MarketplaceBuilder ..> ResolvedPackage : produces source_url
OutputMappers ..> ResolvedPackage : emits URL or shorthand
YmlEditor ..> YmlSchema : reuses validation
class MarketplaceConfig:::touched
class MarketplaceBuilder:::touched
class ResolvedPackage:::touched
class OutputMappers:::touched
class YmlSchema:::touched
class YmlEditor:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[apm pack marketplace] --> B[load apm.yml]
B --> C{package source local?}
C -->|yes| D[emit local source unchanged]
C -->|no| E{entry host or HTTPS override?}
E -->|yes| F[use per-entry host and repo]
E -->|no| G{sourceBase set?}
G -->|yes| H[split sourceBase into host and path]
H --> I[compose base path plus package source]
G -->|no| J[preserve default owner/repo behavior]
F --> K[resolve refs with host-specific resolver]
I --> K
J --> K
K --> L[ResolvedPackage with source_url when composed]
L --> M[Claude/Codex output mappers]
M --> N{source_url present?}
N -->|yes| O[emit canonical HTTPS URL]
N -->|no| P[emit host URL or github shorthand]
Recommendation
Merge now. CI is green, all panelists agree on soundness, and no blocking or required findings exist. Track the doc-writer's two publish-guide clarifications as a fast follow-up PR -- they improve producer discoverability but do not affect correctness or security.
Full per-persona findings
Python Architect
- [nit] Cross-module import of private helpers
_parse_source_base/_validate_source_valuefrom yml_schema into yml_editor atsrc/apm_cli/marketplace/yml_editor.py
Prefixing with underscore signals module-private scope. When yml_editor imports these, it couples to an unstable internal API. If validation evolves, the private contract may change without notice. This is a nit because the sibling-module coupling radius is bounded.
Suggested: Rename toparse_source_base/validate_source_valueand export via__all__, or extract to a shared marketplace validation module.
CLI Logging Expert
No findings.
DevX UX Expert
- [nit] Producer guide could clarify
owner/repopath semantics under sourceBase
A producer seeing sourceBase may wonder whether relative paths likeowner/repoare resolved against the base or are default-host shorthand. One sentence or inline YAML comment naming base + relative = full URL would avoid trial-and-error. - [nit] Private-looking helper names imported cross-module reduce contributor discoverability
Underscore-prefixed helpers imported across sibling modules signal module-private APIs. Renaming or exporting would better convey the intended shared validation contract.
Supply Chain Security Expert
- [nit] Trailing slash normalization removes only one slash before
reject_emptycatches the rest
A multi-trailing-slash value remains invalid becausevalidate_path_segmentswithreject_emptycatches empty path segments.rstrip('/')would be a cosmetic clarity improvement, not a security fix.
OSS Growth Hacker
- [nit] Changelog hook could lead with enterprise outcome before naming sourceBase
Release readers convert faster when the first words describe the benefit (enterprise marketplace paths without repetition) rather than the mechanism. - [nit] A before/after YAML snippet would make the producer value more immediately visible at
docs/src/content/docs/producer/publish-to-a-marketplace.md
Enterprise users scanning docs benefit from seeing repeated full paths collapse into one sourceBase plus short sources. This is conversion polish, not required correctness.
Auth Expert
No findings.
Doc Writer
- [recommended] publish guide example uses
acme-org/pinned-packageunder sourceBase without explaining that it composes onto the base atdocs/src/content/docs/producer/publish-to-a-marketplace.md:109
With sourceBase set, plainowner/repois not an override; the builder composes it onto the base. A reader may mistakeacme-org/pinned-packagefor default GitHub shorthand unless the example comments the composed result or uses an explicit host-prefixed override.
Suggested: Annotate the entry as composed, change it togithub.com/acme-org/pinned-packageif it is meant to demonstrate an override, or make it single-segment like the first example.
Proof (passed):tests/unit/marketplace/test_marketplace_source_base.py::TestSourceBaseBuildComposition::test_dotted_subgroup_relative_source_composes_onto_base-- proves: A two-segment source composes onto sourceBase rather than acting as an override. [devx,portability-by-manifest]
assert resolved.source_repo == 'platform/marketplaces/team.tools/repo' - [recommended] manifest schema section 7.5 does not explicitly say
owner/repocomposes when sourceBase is set atdocs/src/content/docs/reference/manifest-schema.md:812
Section 7.5 documents single-segment composition and override forms, but the common two-segmentowner/repocase is only implied. The implementation composes any non-override relative source, includingowner/repoand deeper paths.
Suggested: Add a sentence: when sourceBase is set, any source that is not a host-prefixed, full HTTPS URL, or local./override composes onto the base, includingowner/repoand deeper relative paths. - [recommended] Override-rule prose is duplicated across docs and apm-guide at
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md
The override rule now appears in the producer guide, manifest schema, and apm-guide. The reference should be canonical, with shorter summaries elsewhere to reduce drift.
Suggested: Keep the sample and point readers to manifest-schema.md section 7.5 for the full sourceBase rules. - [nit] Appendix B revision history was not bumped for the new normative sourceBase field at
docs/src/content/docs/reference/manifest-schema.md:963
The manifest schema reference keeps a revision history for schema-surface additions. sourceBase is a normative field and should have a row.
Suggested: Add a revision row notingmarketplace.sourceBaseand Section 7.5 semantics.
Test Coverage Expert
No findings.
Performance Expert
- [nit]
split_source_basecalled per marketplace entry adds negligible repeated string work
A short string split per package is below the noise floor even for large marketplaces and does not change network or filesystem cost. - [nit]
urlparseand regex validation of sourceBase is a one-shot parse cost
Validation runs once during YAML load on a short string and is not on the resolver network hot path.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Clean additive design: frozen dataclass composition, cached lazy-init, context-aware validation. No architectural faults. |
| CLI Logging Expert | 0 | 0 | 2 | No CLI output surface touched; marketplace validation errors follow the existing MarketplaceYmlError pattern correctly. Ship. |
| DevX UX Expert | 0 | 2 | 2 | sourceBase ergonomics are strong: before/after docs, actionable error on confused-deputy, and apm marketplace add respects base. Two recommended polish items. |
| Supply Chain Security Expert | 0 | 0 | 1 | sourceBase validation is defense-in-depth layered: HTTPS-only, FQDN, rejects dangerous URL components, validate_path_segments, confused-deputy guard. No blocking issues. |
| OSS Growth Hacker | 0 | 1 | 2 | Before/after YAML example in the publishing guide is a strong conversion asset; CHANGELOG wording is user-outcome-first. Ship. |
| Auth Expert | 0 | 0 | 1 | sourceBase host extraction correctly feeds into _get_resolver_for_host; no auth bypass or credential leak introduced. |
| Doc Writer | 0 | 1 | 2 | Docs accurately mirror sourceBase semantics with a clean canonical-in-7.5 hierarchy; minor non-bloat and skill-link polish only. Ship. |
| Test Coverage Expert | 0 | 0 | 1 | sourceBase feature ships with solid unit plus integration coverage for the Claude mapper; Codex mapper path is a low-risk nit. |
| Performance Expert | 0 | 0 | 1 | No hot-path overhead or cache regression; sourceBase composition is cached string concat in the producer-only marketplace build pipeline. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add Codex output mapper integration assertion for sourceBase-composed URLs -- Missing assertion on a multi-harness-support surface. Low risk (shared helper) but closes a regression-trap gap for the second harness.
- [Python Architect] Replace getattr(yml, 'source_base', None) with direct attribute access -- Frozen dataclass guarantees field existence; getattr would silently swallow a future rename refactor.
- [DevX UX Expert] Add confused-deputy error hint suggesting sourceBase or owner/repo form -- Actionable error messages reduce support burden for enterprise adopters hitting the generic rejection.
- [Doc Writer] De-duplicate sourceBase composition illustration in publish guide -- Pick one teaching device (Before/After wins) to avoid guide bloat as the page grows.
- [Auth Expert] Lowercase host in split_source_base return value -- Defensive normalization for case-sensitive token cache keys. Low risk today but prevents a class of subtle bugs.
Architecture
classDiagram
direction LR
class MarketplaceConfig {
<<ValueObject>>
+source_base str or None
+packages tuple[PackageEntry]
}
class PackageEntry {
<<ValueObject>>
+source str
+host str or None
+is_local bool
}
class MarketplaceBuilder {
<<Facade>>
-_source_base_parts _SourceBaseCoords or None
-_source_base_parts_loaded bool
+_get_source_base_parts() _SourceBaseCoords
+_remote_source_coordinates(entry) tuple
+compose_marketplace_json(resolved) dict
}
class _SourceBaseCoords {
<<ValueObject>>
+host str
+path_prefix str
+source_base str
+org_hint str
}
class MarketplaceOutputMapper {
<<Strategy>>
+compose(config, resolved) MapperResult
}
class ResolvedPackage {
<<ValueObject>>
+source_repo str
+host str or None
+source_url str or None
}
MarketplaceBuilder *-- _SourceBaseCoords : caches
MarketplaceBuilder ..> MarketplaceConfig : reads
MarketplaceBuilder ..> PackageEntry : iterates
MarketplaceBuilder ..> ResolvedPackage : produces
MarketplaceBuilder ..> MarketplaceOutputMapper : delegates
MarketplaceOutputMapper <|-- ClaudeMarketplaceMapper
MarketplaceOutputMapper <|-- CodexMarketplaceMapper
class _SourceBaseCoords:::touched
class MarketplaceBuilder:::touched
class MarketplaceConfig:::touched
class ResolvedPackage:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[apm marketplace build] --> B[MarketplaceBuilder._load_yml]
B --> C[yml_schema.parse_source_base]
C --> D{sourceBase present?}
D -->|no| E[source_base = None on MarketplaceConfig]
D -->|yes| F[NET guard: HTTPS FQDN path validation]
F --> G[MarketplaceConfig.source_base = validated URL]
G --> H[MarketplaceBuilder._get_source_base_parts]
H --> I[split_source_base -> host and path_prefix]
I --> J[Cache _SourceBaseCoords]
J --> K[_remote_source_coordinates]
K --> L{entry.host set?}
L -->|yes| M[Override: return entry host and source]
L -->|no| N{_source_base_parts cached?}
N -->|no| O[Default: return entry source unchanged]
N -->|yes| P[Compose repo_path and source_url]
P --> Q[return base_host repo_path source_url org_hint]
Q --> R[_resolve_entry -> ResolvedPackage]
R --> S[compose_marketplace_json]
S --> T[output mapper emits canonical HTTPS URL]
Recommendation
All nine panelists recommend ship with zero blocking findings. CI is green, the feature is strictly additive with no migration, validation is defense-in-depth, and the PR closes #1519 with credited community design. Merge now; track the Codex-mapper test gap and the getattr cleanup as optional fast-follows in a subsequent PR or the next batch sweep.
Full per-persona findings
Python Architect
- [nit] _SourceBaseCoords could be a module-level NamedTuple instead of a frozen dataclass at
src/apm_cli/marketplace/builder.py:106
Design patterns: _SourceBaseCoords is an internal value object with a derived property (org_hint). A frozen dataclass is fine here. Not worth changing -- the current shape is idiomatic and correct. - [nit] split_source_base trusts its input is already validated but has no assert/docstring guard at
src/apm_cli/marketplace/yml_schema.py:117
split_source_base is exported and documents a parse_source_base-validated precondition. Current call sites validate first, so this is low risk.
Suggested: Add an assert after removeprefix if desired. - [recommended] Builder getattr fallback for source_base is overly defensive given frozen dataclass contract at
src/apm_cli/marketplace/builder.py:386
MarketplaceConfig is a frozen dataclass where source_base always exists. Direct attribute access would surface a rename immediately.
Suggested: Replace getattr(yml, 'source_base', None) with yml.source_base.
CLI Logging Expert
- [nit] sourceBase composition silently succeeds with no verbose-mode trace at
src/apm_cli/marketplace/builder.py
When --verbose is active, the builder could log the composition step so humans debugging resolution can see the routing decision. Existing host routing also lacks verbose traces, so this is polish only.
Suggested: Add logger.debug for sourceBase composition if desired. - [nit] parse_source_base error messages omit the offending value at
src/apm_cli/marketplace/yml_schema.py:524
CLI error message guidance prefers naming the thing that failed. Existing sourceBase errors name the field but not the raw value. This is minor polish.
Suggested: Include raw_source_base in the error string if desired.
DevX UX Expert
- [recommended] Error message for confused-deputy rejection names no concrete next action when sourceBase is absent at
src/apm_cli/marketplace/yml_schema.py
When sourceBase is not set and a user writes source: tool, the generic accepted-forms list could add a hint to set sourceBase or use owner/repo form.
Suggested: Append a hint in source_base is None branch of validate_source_value. - [recommended] Manifest-schema docs lack a quick cross-link from packages[].source table row to Section 7.5 at
docs/src/content/docs/reference/manifest-schema.md
A user scanning the table may miss composition semantics. A parenthetical see Section 7.5 would improve discoverability.
Suggested: Append a section link in the source field description row. - [nit] CHANGELOG entry could say what the user types, not what the feature is at
CHANGELOG.md
Action-first changelog phrasing is slightly easier to scan. - [nit] Publishing guide before/after snippet shows ref but not version at
docs/src/content/docs/producer/publish-to-a-marketplace.md
Most marketplace entries use version ranges; one version example would reinforce both branches.
Supply Chain Security Expert
- [nit] _RELATIVE_SOURCE_RE matches traversal sequences before validate_path_segments rejects them at
src/apm_cli/marketplace/yml_schema.py:96
The code comment correctly documents regexes are shape filters and validate_path_segments is the canonical traversal guard. Tighter regex would be extra defense but current path is safe.
Suggested: Optionally tighten _SEGMENT_PAT to reject bare-dot-only segments at regex layer.
OSS Growth Hacker
- [recommended] Before/after example in publish guide lacks a one-line why-you-care hook above it at
docs/src/content/docs/producer/publish-to-a-marketplace.md
A sentence framing the payoff primes the reader to read the YAML diff as proof rather than more config.
Suggested: Add a one-liner before the Before block. - [nit] CHANGELOG entry parenthetical could be the lead at
CHANGELOG.md
Minor growth polish; current wording already passes. - [nit] Manifest-schema source field description is long; consider a numbered list for scanability at
docs/src/content/docs/reference/manifest-schema.md
Enterprise adopters scanning reference docs may benefit from a compact list. Information is complete and correct.
Auth Expert
- [nit] split_source_base does not lowercase the host before returning it at
src/apm_cli/marketplace/yml_schema.py
DNS hostnames are case-insensitive. Lowercasing extracted host would be defensive for case-sensitive token cache keys or per-host environment lookup. Current lowercase examples and FQDN validation make risk low.
Suggested: return host.lower(), path_prefix
Doc Writer
- [recommended] publish-to-a-marketplace.md demonstrates sourceBase composition twice at
docs/src/content/docs/producer/publish-to-a-marketplace.md
The canonical sample has inline resolution comments and the Before/After block re-illustrates the same behavior. The Before/After block is the stronger teaching device; avoid guide bloat.
Suggested: Keep Before/After and drop inline resolution comments, or vice versa. - [nit] package-authoring skill points to Section 7.5 in plaintext rather than a path link at
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md
The skill already uses relative docs links elsewhere; making this a link improves navigation consistency.
Suggested: Link to ../../../../../docs/src/content/docs/reference/manifest-schema.md#75-marketplacepackages - [nit] Cross-page anchor Microsoft open source compliance: policy docs, license, and trademark notice #75-marketplacepackages is correct; keep it verified on future heading renames at
docs/src/content/docs/producer/publish-to-a-marketplace.md:129
Anchor currently resolves from docs/producer to manifest-schema Section 7.5; no action now.
Proof (manual): A reader clicking the manifest schema link lands on Section 7.5.
[manifest schema](../reference/manifest-schema/#75-marketplacepackages) ; target heading: ### 7.5. marketplace.packages
Test Coverage Expert
- [nit] Codex output mapper sourceBase path lacks dedicated assertion in integration test at
tests/integration/test_marketplace_builder_hermetic.py
The integration test asserts the Claude mapper emits composed sourceBase URLs, while the Codex mapper also uses _remote_source_url without a parallel assertion. Risk is low because both mappers invoke the same helper.
Suggested: Add a codex output spec to the existing sourceBase integration test or a sibling test.
Proof (missing at):tests/integration/test_marketplace_builder_hermetic.py::test_build_writes_source_base_composed_url_to_codex_output-- proves: Codex consumers see the fully composed sourceBase URL for relative package sources [multi-harness-support,portability-by-manifest]
assert codex_output["packages"][0]["source"]["url"] == "https://gitlab.example.com/platform/marketplaces/team/tool"
Performance Expert
- [nit] _resolve_entry calls both _load_yml() and _remote_source_coordinates which also calls _get_source_base_parts -> _load_yml() at
src/apm_cli/marketplace/builder.py:582
Both calls resolve to a cached singleton, so the cost is one extra pointer comparison per entry. Not actionable now.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Sync the Stage 2 complexity/file-length refactor branch with main's 22 feature commits (Hermes #1726, Kiro IDE #1741, multi-host dep identity #1735, same-repo remote path deps #1732, git_file_transport #1740, revision pins #1738, marketplace sourceBase/source parity/inherit description #1736/#1739/#1755, pack --archive .zip #1720, mcp optional registry inputs #1734, and the v0.19.0/v0.20.0 releases). Conflict resolution preserved both sides: main's new features ported through the branch's extracted sibling modules, branch's tightened ruff thresholds (max-statements=120, max-branches=40, max-complexity=35, max-returns=8, max-args=12) and 800-line file limit retained. All 7 CI-mirror lint gates pass; full unit suite green (17099 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
feat(marketplace): add sourceBase for package sources
TL;DR
Adds a marketplace-level
sourceBasefield so marketplace authors can name package repositories relative to an enterprise git base path. Per-entry host-prefixed, full HTTPS URL, and local sources remain overrides, so this is additive whensourceBaseis absent. This implements @leocamello's ratified Option A design and closes #1519.Closes #1519
Problem (WHY)
host/owner/repo(deeply-nested GitLab subgroups, arbitrary git hosts) #1519).sourceBaseis a source-routing input, so the issue required that "the security guards from fix: add support for custom github endpoints (host.tld/owner/repo) as apm.yml entrypoint for command apm pack (marketplace) #1288 ... must apply tosourceBaseas strictly as they do to per-entry source values" (marketplace.packages[].source: generalize beyondhost/owner/repo(deeply-nested GitLab subgroups, arbitrary git hosts) #1519).owner/repono longer silently routes to github.com when a base is declared" (marketplace.packages[].source: generalize beyondhost/owner/repo(deeply-nested GitLab subgroups, arbitrary git hosts) #1519).Approach (WHAT)
marketplace.sourceBasewith HTTPS/FQDN/path validation and context-awarepackages[].sourcevalidation../local sources independent from the base.sourceBasemanifests on the existingowner/repo-> default-host path.Implementation (HOW)
src/apm_cli/marketplace/yml_schema.pySOURCE_BASE_RE,source_baseonMarketplaceConfig, strictsourceBaseparsing, and context-aware package source validation.src/apm_cli/marketplace/yml_editor.pysrc/apm_cli/marketplace/builder.pysrc/apm_cli/marketplace/output_mappers.pytests/unit/marketplace/test_marketplace_source_base.py[Unreleased].Diagram
The diagram shows the new build-time routing decision. Only host-less remote entries compose onto
sourceBase; overrides bypass it.flowchart LR A["marketplace.sourceBase"] --> C["packages source"] C --> D{"local path"} D -->|"yes"| L["emit local source unchanged"] D -->|"no"| H{"host or HTTPS URL present"} H -->|"yes"| O["use per-entry override"] H -->|"no"| B{"base declared"} B -->|"yes"| N["compose base plus source"] B -->|"no"| G["preserve default-host owner/repo behavior"]Trade-offs
source_urlonly on resolved packages rather than changing the resolver contract; this keeps the resolver URL-driven.github.com/owner/repoinside asourceBasemarketplace when the package really lives on public GitHub; this is the intended confused-deputy fix.sourceBaseout of generated marketplace JSON; consumers receive standard Claude/Codex source objects.Benefits
sourceBaseretain their previous parse and output behavior.Validation
Scenario evidence
sourceBase.tests/unit/marketplace/test_marketplace_source_base.py::TestSourceBaseBuildComposition::test_composes_relative_source_onto_base_for_resolution_and_outputtests/unit/marketplace/test_marketplace_source_base.py::TestSourceBaseBuildComposition::test_host_prefixed_source_overrides_source_basetests/unit/marketplace/test_marketplace_source_base.py::TestSourceBaseSchema::test_rejects_source_base_security_guard_violationsowner/repobehavior.tests/unit/marketplace/test_marketplace_source_base.py::TestSourceBaseSchema::test_absent_source_base_keeps_owner_repo_source_unchangedCommands run
How to test
sourceBase: https://gitlab.example.com/group/marketplaceto a marketplace block.packages[].source: tool-namewith areforversion.apm pack --dry-run --offlineagainst cached refs or a test repo and confirm the output source URL includes/group/marketplace/tool-name.source: github.com/owner/repoand confirm it emits as an override rather than under the base.sourceBaseand confirm bareowner/repoentries still parse as before.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com