fix(marketplace): enhance repository input parsing for GitLab subgroups and HTTPS URLs#1034
Conversation
Antonin-Rouxel-LaPoste-BGPN
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Enhances apm marketplace add repository input parsing to support GitLab subgroup paths (N-segment owners) and full HTTP(S) repository URLs, addressing the limitation that previously only accepted 2- or 3-segment shorthands.
Changes:
- Replace rigid 2/3-segment parsing with a unified parser that supports N-segment subgroup paths and full HTTP(S) URLs.
- Update user-facing hint text to mention the HTTPS URL form.
- Add unit tests covering GitLab subgroup shorthand, HTTPS URL parsing,
.gitstripping, and rejection cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/apm_cli/commands/marketplace.py |
Implements the new unified parsing logic for marketplace repo inputs and updates CLI messaging. |
src/apm_cli/marketplace/errors.py |
Expands the “not registered” hint to mention full HTTPS URLs. |
tests/unit/marketplace/test_marketplace_commands.py |
Adds new test cases for subgroup shorthands, HTTPS URLs, .git stripping, and invalid inputs. |
| owner = "/".join(parts[1:-1]) | ||
| repo_name = parts[-1] | ||
| else: | ||
| # OWNER/.../REPO (no host prefix, any number of segments) | ||
| owner = "/".join(parts[:-1]) | ||
| repo_name = parts[-1] |
There was a problem hiding this comment.
This parser now allows multi-segment owner values even when the resolved host is the GitHub API backend (default github.com). Downstream, _github_contents_url() interpolates source.owner directly into /repos/{owner}/{repo}/..., so an owner containing / will generate a malformed/ambiguous API path and can accidentally target the wrong repo/path. Consider rejecting owner values containing / when resolved_host is a GitHub host (or otherwise ensuring the client builds URLs safely for multi-segment owners).
| if host and host.lower() != url_host: | ||
| logger.error( | ||
| f"Invalid host: '{parts[0]}'. " | ||
| f"Use 'OWNER/REPO' or 'HOST/OWNER/REPO' format." | ||
| f"Conflicting host: --host '{host}' vs '{url_host}' in URL." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Host comparisons use host.lower() without stripping whitespace (e.g. --host "gitlab.com "), which can raise a false "Conflicting host" error even though the normalized host is the same. Consider normalizing the --host value once up front (strip + lower) and using that for both conflict checks and later validation.
| """Register a marketplace from OWNER/REPO, HOST/OWNER/.../REPO, or a full HTTPS URL.""" | ||
| logger = CommandLogger("marketplace-add", verbose=verbose) |
There was a problem hiding this comment.
The CLI now accepts full HTTP(S) URLs and N-segment subgroup paths, but documentation still shows only OWNER/REPO and HOST/OWNER/REPO forms. Please update the relevant Starlight docs pages (e.g. docs/src/content/docs/guides/marketplaces.md, docs/src/content/docs/reference/cli-commands.md, docs/src/content/docs/guides/marketplace-authoring.md) and the APM guide resource (packages/apm-guide/.apm/skills/apm-usage/commands.md) to include the HTTPS URL form and subgroup examples.
| # ------------------------------------------------------------------ | ||
| # GitLab subgroup / deep-path support | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| @patch("apm_cli.marketplace.client.fetch_marketplace") | ||
| @patch("apm_cli.marketplace.client._auto_detect_path") | ||
| def test_add_gitlab_subgroup_shorthand(self, mock_detect, mock_fetch, runner): | ||
| """HOST/group/subgroup/.../repo shorthand stores all intermediate segments in owner.""" | ||
| from apm_cli.commands.marketplace import marketplace |
There was a problem hiding this comment.
The new parsing behavior around host detection vs OWNER/REPO (especially for owners containing dots like foo.bar/repo) is not covered by tests here. Adding a regression test that asserts foo.bar/repo is treated as owner="foo.bar", repo="repo" (not as a host-prefixed shorthand) would prevent reintroducing the ambiguity fix.
|
@microsoft-github-policy-service agree company="La Poste Groupe" |
APM Review Panel Verdict: REJECT
Required before merge (12 items)
Nits (11 items, skip if you want)
CEO arbitrationPR #1034 ships a genuinely valuable feature -- paste-from-browser HTTPS URLs and GitLab subgroup paths -- but it arrives with three categories of blocking defect that must be resolved before merge. First, and most critically, the implementation accepts (redacted) scheme URLs with no warning, no opt-in flag, and no documentation of the risk. Two panelists (DevX/UX and Supply Chain Security) flag this independently: from a UX standpoint it breaks the established --allow-insecure contract every other APM fetch path honors; from a security standpoint it is a direct MITM injection point for marketplace manifests. These two angles reinforce each other and are not in conflict -- the fix is the same: reject (redacted) at the parser boundary and document that only https:// is accepted. Second, the PR introduces a percent-encoding bypass in validate_path_segments ('%2E%2E' evades the '..' check) and multi-segment owner strings interpolated into API URLs without per-segment encoding, leaving '?', '#', and '@' characters as live URL-injection vectors. These are supply-chain correctness issues, not edge cases. Third, the 50-line parsing block embedded in the Click handler conflates command orchestration with URL/path resolution logic, making the new behavior untestable in isolation; extraction to _parse_repo_argument() with a defined return type is a structural prerequisite for the security fixes above, since it creates one auditable entry point. Two process gaps must close in the same PR: cli-commands.md must reflect the new HOST/OWNER/.../REPO and full-URL input forms (the docs currently show only the fixed 3-segment shape, making the new feature undiscoverable from help alone), and CHANGELOG.md must carry an [Unreleased] entry. Neither is cosmetic -- the docs gap means GitLab users who need subgroup support cannot find the syntax without reading source, and the CHANGELOG gap makes the feature invisible to evaluators deciding whether to upgrade. The recommended path forward is: (1) land the _parse_repo_argument() extraction first as it is the foundation for auditable security fixes; (2) add urllib.parse.unquote() normalization before validate_path_segments and per-segment urllib.parse.quote() encoding before URL construction; (3) restrict accepted schemes to https:// only, raising a clear actionable error for (redacted) (4) update cli-commands.md and CHANGELOG.md; (5) replace the placeholder '...' stub test bodies with real assertions. The feature strategy is correct and the growth signal is real -- ship it clean. Dissent resolved: No genuine inter-panelist disagreement exists. The (redacted) finding surfaces from both DevX/UX (UX contract) and Supply Chain Security (MITM risk) and the two rationales are complementary -- a single fix satisfies both. CLI Logging Expert's PathTraversalError message finding and Python Architect's structural extraction finding are also complementary: extraction to _parse_repo_argument() creates the natural site for a single user-readable error raise, resolving both concerns simultaneously. Growth/positioning note: The paste-from-browser UX (full HTTPS URL input) is a concrete, demonstrable hook for a 'GitLab teams: APM works for you' campaign. OSS Growth Hacker recommends a standalone social beat -- a tweet thread or dev.to post anchored to the MANIFESTO 'Portability over Vendor Lock-in' principle -- rather than bundling the feature into a release roundup. Schedule it as soon as the fixed PR merges. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class add {
<<IOBoundary>>
+repo: str
+name: str
+branch: str
+host: str
+verbose: bool
}
class MarketplaceSource {
<<ValueObject>>
+host: str
+owner: str
+repo: str
+branch: str
+name: str
}
class CommandLogger {
<<Base>>
+error()
+progress()
+complete()
}
class validate_path_segments {
<<Pure>>
}
class PathTraversalError {
<<Exception>>
}
class is_valid_fqdn {
<<Pure>>
}
class default_host {
<<Pure>>
}
class fetch_marketplace {
<<IOBoundary>>
}
class _auto_detect_path {
<<IOBoundary>>
}
class MarketplaceNotFoundError {
<<Exception>>
}
add --> CommandLogger : uses
add --> MarketplaceSource : constructs
add --> fetch_marketplace : calls
add --> _auto_detect_path : calls
add --> validate_path_segments : calls
add --> is_valid_fqdn : calls
add --> default_host : calls
validate_path_segments ..> PathTraversalError : raises
MarketplaceNotFoundError --> MarketplaceSource : references
class add:::touched
class PathTraversalError:::touched
class validate_path_segments:::touched
class MarketplaceNotFoundError:::touched
classDef touched fill:#ffe0b2,stroke:#e65100
flowchart TD
A([CLI: apm marketplace add REPO]) --> B[add -- marketplace.py:368]
B --> C{starts with https:// ?}
C -- yes --> D[urlparse repo_input pure parse]
D --> E{is_valid_fqdn url_host ?}
E -- no --> ERR1[logger.error + sys.exit 1]
E -- yes --> F{--host conflicts?}
F -- yes --> ERR2[logger.error + sys.exit 1]
F -- no --> G[split path to owner and repo_name]
C -- no --> H[split by slash to parts]
H --> I{len parts ge 3 and is_valid_fqdn parts0 ?}
I -- yes --> J[resolved_host=parts0 owner=join parts1 to -1 repo_name=parts-1]
I -- no --> K[owner=join parts to -1 repo_name=parts-1]
G --> L[validate_path_segments owner and repo_name]
J --> L
K --> L
L -- PathTraversalError --> ERR3[logger.error str exc + sys.exit 1]
L -- ok --> M{resolved_host is None?}
M -- yes --> N{--host flag set?}
N -- yes --> O[is_valid_fqdn host check]
O -- invalid --> ERR4[logger.error + sys.exit 1]
O -- valid --> P[resolved_host = normalized host]
N -- no --> Q[resolved_host = default_host FS read env/config]
M -- no --> R[validate --name flag via _is_valid_alias]
P --> R
Q --> R
R -- invalid --> ERR5[logger.error + sys.exit 1]
R -- ok --> S[_auto_detect_path probe_source NET HTTP probe]
S -- None --> ERR6[logger.error + sys.exit 1]
S -- path --> T[fetch_marketplace fetch_source NET download JSON]
T --> U[three-tier alias resolution: --name > manifest.name > repo_name]
U --> V[add_marketplace source FS write registry]
V --> W([logger.complete exit 0])
Design patterns
Required findings: (see aggregated list above) Nits:
CLI Logging ExpertRequired findings: (see aggregated list above) Nits:
DevX UX ExpertRequired findings: (see aggregated list above) Nits:
Supply Chain Security ExpertRequired findings: (see aggregated list above) Nits:
Auth ExpertInactive -- PR changes marketplace source URL parsing only and does not modify AuthResolver, token management, or credential resolution logic (touched files: src/apm_cli/commands/marketplace.py, src/apm_cli/marketplace/errors.py, tests only). OSS Growth HackerRequired findings: (see aggregated list above) Nits:
Side-channel: This is the right feature to anchor a 'GitLab teams: APM works for you' tweet thread or dev.to post. The paste-from-browser UX (full HTTPS URL) is the concrete, demonstrable hook. Recommend scheduling this as a standalone social beat rather than bundling it into a release roundup. Verdict computed deterministically: 12 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
added
panel-rejected
|
Following the b9df59a -- refactor parsing to delegate to
|
| Panel finding | Status |
|---|---|
Multi-segment owner interpolated into API URL without per-segment encoding (client.py) |
Not addressed -- source.owner containing / is passed as-is into the GitHub Contents API URL; ?, #, @ characters in a segment could inject into the request |
Percent-encoded traversal bypass (%2E%2E bypasses validate_path_segments) |
Not verified -- DependencyReference.parse() handles URL parsing but I have not confirmed unquote() is called before the traversal checks |
APM Review Panel Verdict: REJECT
Required before merge (7 items)
Nits (13 items, skip if you want)
CEO arbitrationThis PR carries a genuinely compelling UX primitive -- "paste the browser URL directly" -- but ships with three independent security findings that must be resolved before merge. Supply Chain and Auth Expert arrived at the same root vulnerability from complementary angles: the current implementation accepts any arbitrary FQDN, and the AuthResolver then forwards GitHub credentials ( On The GitLab-examples-that-silently-fail issue is the most strategically damaging finding in this panel. A user who follows the documented GitLab examples will register a broken marketplace with no upfront signal, which violates community trust far more than a delayed feature. The examples must either be removed until GitLab fetch support lands, or the CLI must emit a prominent host-kind warning at registration time. The CHANGELOG and docs wording must be corrected before merge regardless of which path is chosen; the launch narrative for the "paste URL" feature is worth protecting, and a wave of confused GitLab users in the first week would poison it. Dissent resolved: DevX UX Expert marked the GitLab-examples-that-don't-work finding as REQUIRED (two items); OSS Growth Hacker treated the same documentation gap as NIT-level. The CEO sides with DevX. A user who reads the docs, follows the GitLab examples, and receives a silent registration-success before a broken fetch is a user who files a bug, loses trust, and churns. The fix must block merge, not land later. Growth/positioning note: The "paste the browser URL, skip the reformatting" UX primitive is the kind of micro-delight that earns organic sharing among DevEx and platform engineering audiences. Protect the narrative now: fix the CHANGELOG sentence structure (split the limitation onto its own line), replace the Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class add_command {
<<IOBoundary>>
+add(repo, name, branch, host, allow_insecure, verbose)
}
note for add_command "Entry point: apm marketplace add"
class DependencyReference {
<<ValueObject>>
+repo_url str
+host Optional[str]
+explicit_scheme Optional[str]
+reference Optional[str]
+is_local bool
+is_virtual bool
+is_insecure bool
+parse(dep_str) DependencyReference
}
class MarketplaceSource {
<<ValueObject>>
+name str
+owner str
+repo str
+branch str
+host str
+path Optional[str]
}
class MarketplaceRegistry {
<<IOBoundary>>
+add_marketplace(source)
}
class MarketplaceClient {
<<IOBoundary>>
+fetch_marketplace(source) Manifest
+_auto_detect_path(source) Optional[str]
}
class CommandLogger {
<<Base>>
+start(msg)
+error(msg)
+success(msg)
+verbose_detail(msg)
+warning(msg)
}
class PathTraversalError {
<<Exception>>
}
class is_valid_fqdn {
<<Pure>>
+is_valid_fqdn(s) bool
}
class add_command:::touched
class DependencyReference:::touched
add_command ..> DependencyReference : parse(repo)
add_command *-- CommandLogger : uses
add_command ..> MarketplaceSource : constructs
add_command ..> MarketplaceRegistry : add_marketplace
add_command ..> MarketplaceClient : fetch + detect
add_command ..> is_valid_fqdn : validates --host
DependencyReference ..> PathTraversalError : raises
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm marketplace add REPO"]) --> B
B["Validate --host via is_valid_fqdn\nmarketplace.py:385"] -->|invalid| ERR1(["sys.exit(1)"])
B -->|valid| C
C["DependencyReference.parse(repo)\nreference.py:1059"] -->|PathTraversalError| ERR2(["sys.exit(1)"])
C -->|ValueError| ERR3(["sys.exit(1)"])
C -->|ok| D
D{"dep.is_local or dep.is_virtual?\nmarketplace.py:412"} -->|yes| ERR4(["sys.exit(1)"])
D -->|no| E
E{"dep.is_insecure and not allow_insecure?\nmarketplace.py:421"} -->|yes| ERR5(["sys.exit(1) -- HTTP rejected"])
E -->|no| F
F["Extract owner/repo_name\nfrom dep.repo_url.split('/')\nmarketplace.py:432-434"] --> G
G{"host conflict?\nrepo_has_explicit_host and host != dep.host\nmarketplace.py:441"} -->|yes| ERR6(["sys.exit(1)"])
G -->|no| H
H["resolved_host = host or dep.host\nresolved_branch = branch or dep.reference or main\nmarketplace.py:450-452"] --> I
I["Validate --name flag via _is_valid_alias\nmarketplace.py:459"] -->|invalid| ERR7(["sys.exit(1)"])
I -->|valid| J
J["[NET] _auto_detect_path(probe_source)\nmarketplace/client.py"] -->|None| ERR8(["sys.exit(1) -- no marketplace.json"])
J -->|path found| K
K["[NET] fetch_marketplace(fetch_source, force_refresh=True)\nmarketplace/client.py"] --> L
L["Resolve display_name: --name > manifest.name > repo_name\nmarketplace.py:502-519"] --> M
M["[FS] add_marketplace(source)\nmarketplace/registry.py"] --> N
N(["logger.success -- registered"])
Design patterns
No findings. CLI Logging ExpertRequired:
Nits:
DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertRequired:
Nits:
Auth ExpertRequired:
Nits:
OSS Growth HackerNo findings. Nits:
Verdict computed deterministically: 7 required findings across 5 active panelists (cli-logging-expert: 1, devx-ux-expert: 2, supply-chain-security-expert: 2, auth-expert: 2; python-architect and oss-growth-hacker: 0). APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
…aths, trusted-host gate Re-applies the intent of issue microsoft#1027 on the post-refactor command surface (src/apm_cli/commands/marketplace/__init__.py is the new home of 'apm marketplace add' after microsoft#1024 split the package). Addresses all 7 required findings from the second APM Review Panel verdict. Behaviour changes ----------------- * New parser '_parse_marketplace_repo' accepts: - OWNER/REPO (unchanged) - HOST/OWNER/REPO (unchanged) - HOST/group/sub/.../REPO (NEW -- nested GHES sub-paths) - https://HOST/owner/.../repo[.git] (NEW -- paste from browser) * http:// is rejected at parse time (no --allow-insecure escape hatch). * Path-traversal sequences (incl. percent-encoded '%2E%2E') are rejected via validate_path_segments after urllib.parse.unquote. * New '_is_trusted_marketplace_host' gate rejects non-GitHub hosts at registration time -- only github.com, *.ghe.com, and the host configured via GITHUB_HOST are accepted. GitLab / Bitbucket / arbitrary FQDNs get an actionable 'not yet supported' error instead of silently 404-ing at fetch time and forwarding GITHUB_TOKEN / GITHUB_APM_PAT to the wrong host. * '--host' flag conflict detection still works on every input form, including HTTPS URLs. Panel findings addressed (second REJECT verdict, run 25169913031) ----------------------------------------------------------------- * [supply-chain-security] '--allow-insecure' MITM surface -> flag dropped; HTTP rejected at parse time * [supply-chain-security] no trusted-host allowlist -> _is_trusted_marketplace_host gate (github.com / *.ghe.com / GITHUB_HOST) * [auth-expert] GitHub tokens forwarded to GitLab via _resolve_token -> trusted-host gate prevents the request from ever being built * [auth-expert] GitHub Contents API URL constructed for non-GitHub hosts -> rejected before _auto_detect_path is called * [devx-ux] GitLab examples advertised a silently-broken workflow -> docs replaced with supported-host callout above the Examples block; only github.com / GHES examples shown * [devx-ux] 'unsupported fetches' note buried after argument descriptions -> blockquote moved to immediately precede the Examples section * [cli-logging] inconsistent symbol='error' usage -> the new add() block matches the surrounding file's logger style (which does not pass symbol='error' on add-command errors); finding was self-relative to the deleted marketplace.py and no longer applies Tests ----- 9 new tests cover: HTTPS URL parsing, .git stripping, nested GHES sub-path, non-GitHub host rejection (URL + shorthand), HTTP rejection, path-traversal rejection (literal + percent-encoded), --host conflict with URL, single-segment URL rejection. tests/unit/marketplace/test_marketplace_commands.py: 32 pass. Full unit suite: 6877 pass, 0 fail (the 6 pre-existing build_integration / azure_skills failures are present on origin/main and unrelated). Real-asset proof ---------------- $ apm marketplace add https://github.com/addyosmani/agent-skills --name addy-rework [*] Registering marketplace 'addy-rework'... Repository: addyosmani/agent-skills [+] Marketplace 'addy-rework' registered (1 plugins) $ apm marketplace add https://gitlab.com/mycompany/myorg/specs-and-standards/internal-marketplace [x] Host 'gitlab.com' is not yet supported for 'apm marketplace add'. ... Closes microsoft#1027 Co-authored-by: Antonin Rouxel <anrouxel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
fix/marketplace-url
branch
from
a65b11e to
6879f21
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| marketplace, ["add", "https://gitlab.com/acme/team/plugin-marketplace"] | ||
| ) | ||
| assert result.exit_code != 0 | ||
| assert "gitlab.com" in result.output |
|
|
||
| result = runner.invoke(marketplace, ["add", "gitlab.com/acme/team/plugin-marketplace"]) | ||
| assert result.exit_code != 0 | ||
| assert "gitlab.com" in result.output |
… guard in _fetch_file Addresses APM Review Panel REJECT verdict (2026-04-30): - [cli-logging-expert] Add symbol='error' to all logger.error() calls in marketplace add command and symbol='warning' to logger.warning() so the [x] / [!] traffic-light symbols render consistently. Covers the two required call sites flagged by the panel plus the related warning nit. - [auth-expert / supply-chain-security-expert] Add defense-in-depth host-kind guard at the top of _fetch_file. Marketplace registration already gates non-trusted hosts, but if a legacy registry entry or future caller bypasses that gate we MUST NOT issue a GitHub Contents API request to a non-GitHub host -- doing so would attach Authorization: token <github_pat> headers to requests aimed at unrelated hosts and leak credentials. Fail closed with MarketplaceFetchError for kind='generic' / 'ado'. Also gate the Authorization header on host_info.kind so the credential-attach site is locally auditable. GitLab examples were already absent from cli-commands.md and the unsupported-hosts blockquote already immediately precedes the Examples section. The HTTP rejection at parse time and trusted-host registration gate were already in place from the prior rework. Tests: full unit suite green (6939 passed). New tests cover the generic-host rejection path and confirm github.com still passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added
panel-review
DevX UX Expert Review{
"persona": "devx-ux-expert",
"active": true,
"required": [
{
"id": "ux-r1",
"title": "Trusted-host error is too long and buries the next action",
"location": "src/apm_cli/marketplace/errors.py -- trusted-host error",
"detail": "The error for unsupported hosts runs 3 sentences before the user learns what to do. The final clause about credential forwarding reads like an internal design note, not user guidance. A user hitting this at 11pm needs one clear action. Proposed structure: line 1 = what failed, line 2 = supported values, line 3 = one concrete next step. The credential-forwarding rationale belongs in --help or docs, not stderr.",
"suggestion": "Error: Host '{resolved_host}' is not supported.\nSupported hosts: github.com, *.ghe.com, or the host set via GITHUB_HOST.\nSet GITHUB_HOST or use a supported host, then re-run the command."
},
{
"id": "ux-r2",
"title": "MarketplaceNotRegisteredError recovery hint uses old-style shorthand only",
"location": "src/apm_cli/marketplace/errors.py -- MarketplaceNotRegisteredError",
"detail": "The error says 'Run apm marketplace add OWNER/REPO (or pass a full HTTPS URL)'. The parenthetical buries the most copy-paste-friendly form. A user who arrived via a browser URL will paste the URL; they should see that form first. Either list both forms equally or lead with the URL form since it is unambiguous.",
"suggestion": "Run 'apm marketplace add https://github.com/OWNER/REPO' or 'apm marketplace add OWNER/REPO' to register it, or 'apm marketplace list' to see registered marketplaces."
},
{
"id": "ux-r3",
"title": "Conflicting-host error does not tell the user which flag to drop in context",
"location": "src/apm_cli/marketplace/errors.py -- _parse_marketplace_repo conflicting host",
"detail": "The message says 'Drop --host or use a matching value' but does not show what the matching value would be. A user who copy-pasted a GHES URL and also has --host set in a config file will not know which value wins or what to type. The concrete next action must be a runnable command.",
"suggestion": "Conflicting host: --host '{host_flag}' does not match '{embedded_host}' in '{raw}'.\nTo fix: drop --host and run: apm marketplace add {raw}"
}
],
"nits": [
{
"id": "ux-n1",
"detail": "cli-commands.md usage block lists '(host/redacted) -- the [.git] bracket notation is familiar to git docs but not to npm/pip users. Consider a plain prose note instead: 'The .git suffix is stripped automatically.'"
},
{
"id": "ux-n2",
"detail": "The invalid-format error lists three valid forms in a parenthetical. If a user types 'apm marketplace add gitlab.com/org/repo' they get the unsupported-host error, not the format error -- but the format error still lists HOST/OWNER/REPO as valid. Ensure the format error is only shown for genuinely malformed input, not for rejected-but-syntactically-valid hosts."
},
{
"id": "ux-n3",
"detail": "commands.md table cell 'Register a marketplace (also accepts HOST/OWNER/REPO, nested HOST/group/sub/.../REPO, or full HTTPS URL)' is too wide for terminal help rendering. Trim to: 'Register a marketplace (OWNER/REPO, HOST/OWNER/REPO, or full HTTPS URL)' -- the nested sub-path case is implied by the HOST/OWNER/REPO form."
},
{
"id": "ux-n4",
"detail": "HTTP rejection error says 'Use HTTPS for marketplace registration.' -- adding the corrected URL makes it one copy-paste away: 'Use HTTPS instead: https://{raw_without_scheme}'"
},
{
"id": "ux-n5",
"detail": "The docs note on supported hosts says 'Native non-GitHub support is tracked separately' without a link or issue number. A user who needs GitLab support has nowhere to go. Add a link or issue reference."
}
]
}Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
APM Review Panel Verdict: REJECT
Required before merge (7 items)
Nits (18 items, skip if you want)
CEO arbitrationThe panel converges strongly on two themes: a real security bypass and a cluster of user-facing message quality problems. On security, supply-chain-security-expert identified a concrete, exploitable gap -- percent-encoded traversal sequences (%2E%2E) pass validate_path_segments undetected in the shorthand branch because unquote() is applied only in the HTTPS path. This is not theoretical; it is a direct bypass of the path-traversal guard the PR explicitly promises. The fix is surgical and the PR's overall traversal-guard design is otherwise sound. Python-architect's required finding is architecturally related: the PR introduces _is_trusted_marketplace_host as a second, independent implementation of host-trust classification that runs on the credential-leak guard, while AuthResolver.classify_host is the established single source of truth. These two implementations agree today but share no invariant, making future drift silent and dangerous on the most sensitive code path in the feature. On user-facing messages, python-architect, cli-logging-expert, and devx-ux-expert all independently required improvements to the untrusted-host error: it is too long, buries the actionable fix, and includes security rationale that belongs in documentation rather than the default error path. Three independent panelists raising the same message as a required finding is unusually strong convergence and confirms this is not a stylistic preference but a usability defect. The PR's core logic -- accepting full HTTPS URLs, parsing nested HOST/group/.../REPO shorthands, stripping .git suffixes, and blocking HTTP -- is well-designed and the security intent throughout is clear. The required findings are all fixable without redesigning the feature. None of the panelists questioned the approach; they questioned specific implementation details that fall short of the stated security guarantees. Dissent resolved: python-architect (required) vs. auth-expert (nit) on dual trust-boundary severity. Sided with python-architect: credential-leakage guards must share a single authoritative classification call rather than relying on two independent implementations that happen to agree today. The cost of the fix is negligible; the risk of silent future divergence is not. Growth/positioning note: oss-growth-hacker correctly flags that "paste any GitHub URL directly" is the headline feature and should lead the CHANGELOG entry. The current framing ("accepts full HTTPS URLs and nested HOST/group/sub/.../REPO shorthands") undersells the friction-reduction story, which is the most shareable aspect of this release. Per-persona findings (full)Python ArchitectclassDiagram
class AuthResolver {
<<Strategy>>
+classify_host(host) HostInfo
+try_with_fallback(host, fn, org)
}
class HostInfo {
<<ValueObject>>
+host: str
+kind: str
+has_public_repos: bool
+api_base: str
}
class _parse_marketplace_repo {
<<function>>
+repo: str
+host_flag: str or None
returns: tuple(owner, repo_name, embedded_host)
}
class _is_trusted_marketplace_host {
<<function -- DUPLICATES AuthResolver>>
+host: str
returns: bool
}
class add_command {
<<ClickCommand>>
uses _parse_marketplace_repo
uses _is_trusted_marketplace_host
}
class _fetch_file {
<<function in client.py>>
uses AuthResolver.classify_host
}
class _is_trusted_marketplace_host:::touched
class add_command:::touched
class _parse_marketplace_repo:::touched
class _fetch_file:::touched
AuthResolver --> HostInfo
add_command --> _parse_marketplace_repo
add_command --> _is_trusted_marketplace_host
_is_trusted_marketplace_host ..> AuthResolver : should delegate (currently duplicates)
_fetch_file --> AuthResolver
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm marketplace add REPO_ARG --host HOST"] --> B["_parse_marketplace_repo(repo, host_flag)"]
B --> C{"URL scheme?"}
C -- "(redacted) --> D["[x] ValueError: Insecure HTTP URL rejected"]
C -- "https://" --> E["urlparse + _up.unquote(path) + strip .git"]
C -- "shorthand" --> F["raw.split('/') -- NO unquote (BUG)"]
E --> G["validate_path_segments on owner + repo"]
F --> G
G --> H{"FQDN first segment?"}
H -- yes --> I["embedded_host = segments[0]; segments = segments[1:]"]
H -- no --> J["embedded_host = None"]
I --> K["resolve effective host"]
J --> K
K --> L["_is_trusted_marketplace_host(resolved_host)"]
L -- "False" --> M["[x] logger.error: host not supported; sys.exit(1)"]
L -- "True" --> N["[FS] fetch_marketplace(source)"]
N --> O["_fetch_file: AuthResolver.classify_host(source.host)"]
O --> P{"kind in github/ghe_cloud/ghes?"}
P -- "No" --> Q["[x] MarketplaceFetchError: refuse to fetch"]
P -- "Yes" --> R["[NET] GitHub Contents API with Authorization header"]
R --> S["add_marketplace to registry"]
Design patterns
Required findings: 1 (see above) CLI Logging ExpertRequired findings: 2 (see above) DevX UX ExpertRequired findings: 3 (see above) Supply Chain Security ExpertRequired findings: 1 (percent-encoded traversal bypass in shorthand path, see above) Auth ExpertRequired findings: 0 OSS Growth HackerRequired findings: 0 Verdict computed deterministically: 7 required findings across 6 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
Persona-gated round 3 fold for microsoft#1034. 7 findings on 38dc498: - _is_trusted_marketplace_host removed; routes via AuthResolver.classify_host (single source of truth shared with marketplace/client.py fetch-time guard) - Percent-encoded traversal (%2E%2E) bypass closed in shorthand branch (mirrors HTTPS branch _up.unquote() before split) - PathTraversalError message rewritten action-first, no double-explanation - Untrusted-host error rewritten: 3 lines (outcome, supported, action), security rationale removed from default error path - Conflicting-host error includes runnable next command (apm marketplace add <raw>) - MarketplaceNotFoundError surfaces copy-pasteable HTTPS URL form first, shorthand form demoted Regression tests: - test_marketplace_host_classification_via_auth_resolver - test_untrusted_host_error_has_action_in_first_sentence - test_path_traversal_error_message_no_double_exception_text - test_conflicting_host_error_includes_runnable_command - MarketplaceNotFoundError test asserts URL form precedes shorthand Lint clean (ruff format + check). 6920 unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (3 items)
Nits (15 items, skip if you want)
CEO arbitrationPR #1034 is structurally sound and ready to merge once the three required findings are addressed. The devx-ux-expert correctly flags that the untrusted-host error and the MarketplaceNotFoundError both fail GHES users at the Recovery lens: a user who typed a GHES URL cannot copy-paste their way to a fix, which directly contradicts the stated goal of this PR. These are not polish items -- they are correctness failures against the PR's own stated objective of actionable errors. The supply-chain finding (double-encoded traversal bypass) is a genuine security regression introduced by the The "single source of truth" consolidation is directionally correct but incomplete. Growth/positioning note: The "gives you the exact command to run when something goes wrong" pattern is a headline differentiator. Once the GHES copy-paste failures are fixed and the pattern works correctly end-to-end, this becomes a story: APM is the CLI that never leaves you guessing. Codify the pattern in CONTRIBUTING.md in this PR -- not as a deferred task -- so it is enforceable from day one. Dissent resolved: No panelist elevated a nit to required against another panelist's explicit judgment. The python-architect, cli-logging-expert, and devx-ux-expert are in thematic alignment on the PathTraversalError detail loss. The supply-chain panelist experienced a delayed return; its finding was received and is reflected in the required count above. The security-gate consolidation is semantically equivalent for the common cases per auth-expert analysis. Per-persona findings (full)Python ArchitectclassDiagram
class AuthResolver {
<<Strategy>>
+classify_host(host) HostInfo
+resolve(host) AuthContext
}
class HostInfo {
<<ValueObject>>
+kind: str
+display_name: str
}
class MarketplaceCommand {
+add(repo, name, branch, host, verbose)
-_parse_marketplace_repo(repo, host_flag) tuple
-_TRUSTED_MARKETPLACE_HOST_KINDS tuple
}
class MarketplaceClient {
+fetch_marketplace(source)
}
class MarketplaceErrors {
+MarketplaceNotFoundError
+PathTraversalError
}
class MarketplaceCommand:::touched
class MarketplaceErrors:::touched
classDef touched fill:#fff3b0,stroke:#d47600
MarketplaceCommand --> AuthResolver : classify_host (registration-time guard)
MarketplaceClient --> AuthResolver : classify_host (fetch-time guard)
MarketplaceCommand --> MarketplaceErrors : raises/catches
AuthResolver --> HostInfo : returns
flowchart TD
A["apm marketplace add REPO"] --> B["_parse_marketplace_repo()"]
B --> B1{HTTPS URL?}
B1 -- yes --> B2["urllib.parse + path strip"]
B1 -- no --> B3["_up.unquote(raw) then split"]
B2 --> C["validate_path_segments()"]
B3 --> C
C -->|PathTraversalError| ERR1["logger.error: path-traversal, exit 1"]
C -->|ValueError| ERR2["logger.error: str exc, exit 1"]
C --> D["resolve effective host"]
D --> E["AuthResolver.classify_host(resolved_host)"]
E --> F{kind in TRUSTED_KINDS?}
F -- no --> ERR3["logger.error: host not supported, exit 1"]
F -- yes --> G["fetch_marketplace + add_marketplace"]
G --> H["success"]
Design patterns
CLI Logging ExpertRequired: none. Nits:
DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertRequired:
Nits:
Auth ExpertRequired: none. Nits:
OSS Growth HackerRequired: none. Nits:
Strategic note: "Gives you the exact command to run when something goes wrong" is a headline differentiator. Once enforced across the full CLI it becomes a release story, not just a bug fix. Verdict computed deterministically: 3 required findings across 6 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
Address all 3 round-3 required findings: 1. supply-chain (required): Double-encoded traversal (%252E%252E) bypassed validate_path_segments after a single _up.unquote. Hardened the guard itself (option b from the panel) by iteratively unquoting each segment up to 8 passes before checking the reject set. All callers (cowork lockfile paths, virtual paths, dependency strings) now benefit without per-site URL-decode logic. Updated the cowork test that previously documented the security gap to assert PathTraversalError for both single- and double-encoded payloads. 2. devx-ux (required): Untrusted-host error was not copy-pasteable for GHES users (no concrete export, ambiguous "re-run the command"). Rewrote it as a multi-line recovery block that interpolates the resolved host into 'export GITHUB_HOST=...' and the original repo into 'apm marketplace add ...', both shlex-quoted to prevent shell metacharacter issues. 3. devx-ux (required): MarketplaceNotFoundError hardcoded github.com in its URL hint, leaving GHES users with a copy-paste URL that would never work for them. Added a 'host' parameter (defaulting to github.com to preserve public-cloud behaviour) and updated registry callers to pass default_host(). Also addressed the convergent nit raised by 5/6 panelists in the local mirror review: shlex.quote(raw) in the conflicting-host suggestion (was a round-3 supply-chain nit), and shlex.quote(resolved_host) in the new export line for symmetry with quoted_repo. Tests: 6926 passed in unit suite. New tests cover the double-encoded guard, GHES copy-paste path, and host-aware MarketplaceNotFoundError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added
panel-review
🎯 APM Review Panel — PR #1034Feature: Panel SummaryThis PR addresses issue #1027 cleanly and with a security-first posture. The implementation is well-structured, thoroughly tested, and introduces meaningful defense-in-depth hardening beyond the core feature request. ✅ StrengthsSecurity architecture
Developer experience
Test coverage
🔍 Minor ObservationsThese are non-blocking but worth tracking:
ConclusionThe implementation is production-ready. The two security hardening items (#1027 URL parsing, iterative traversal decode) are correct and tested. The three observations above are quality improvements for a follow-up — none blocks merge. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
added
panel-approved
Note
Closes #1027
TL;DR
apm marketplace addwas limited to exactly 2 or 3 path segments, making it impossible to register a GitLab marketplace hosted under subgroups (e.g.mycompany/myorg/specs-and-standards/repo). This PR replaces the rigid segment counter with a unified parser that accepts any N-segment shorthand path and full HTTPS URLs.Problem
Why the old parser failed
The previous implementation split the input on
/and checkedlen(parts) == 2orlen(parts) == 3. Any deeper path — which GitLab subgroups require — was rejected with "Expected 'OWNER/REPO'".Two separate root causes were reported in #1027:
OWNER/REPO(2 parts) orHOST/OWNER/REPO(3 parts) were accepted.Important
The
MarketplaceSourcemodel already storesowneras a plain string with no segment-count constraint, so the fix is entirely in the parser; no schema migration is required.Approach
OWNER/REPOHOST/OWNER/REPOHOST/group/sub/.../REPOOWNER/group/sub/.../REPOhttps://host/group/sub/.../repo[.git]The rule is: everything except the last segment is
owner; the last segment isrepo. This mirrors howdependency/reference.pyalready handles generic hosts forapm add(see_resolve_shorthand_to_parsed_url), giving both surfaces consistent behaviour.Implementation
Files changed
src/apm_cli/commands/marketplace.py—addcommand parserReplaced the 2/3-segment
if/elif/elseblock with a three-branch strategy:Path-traversal sequences (
..,.) in the parsedownerandrepo_nameare validated through the existingvalidate_path_segmentsguard (required by the path-security rules incopilot-instructions.md). Conflicting--hostflags are still caught in all branches.src/apm_cli/marketplace/errors.py—MarketplaceNotFoundErrorUpdated the user-facing hint to mention the HTTPS URL form.
tests/unit/marketplace/test_marketplace_commands.pyAdded 6 new test cases to
TestMarketplaceAdd:HOST/group/sub/.../repo).gitsuffix stripping--hostflag with HTTPS URLFlow diagram
The diagram below shows the updated parse strategy inside the
addcommand.flowchart TD A(["apm marketplace add INPUT"]) --> B{"starts with https://?"} B -- yes --> C["urlparse / strip .git"] C --> D{"path segments >= 2?"} D -- no --> ERR1(["error: need OWNER/REPO"]) D -- yes --> E["owner = all-but-last\nrepo = last segment"] B -- no --> F["split on / filter empty"] F --> G{"segments >= 2?"} G -- no --> ERR2(["error: need OWNER/REPO"]) G -- yes --> H{"first segment is valid FQDN?"} H -- yes --> I{"segments >= 3?"} I -- no --> ERR3(["error: HOST/OWNER/REPO required"]) I -- yes --> J["host = first\nowner = middle segments\nrepo = last"] H -- no --> K["owner = all-but-last\nrepo = last segment"] E & J & K --> L["validate_path_segments\nowner + repo_name"] L --> M["resolve --host flag\nor default_host"] M --> N(["_auto_detect_path + register"])Trade-offs
owneris a multi-segment string —MarketplaceSource.ownermay now be"mycompany/myorg/specs-and-standards". The model already stored it as a plain string and the_github_contents_urlbuilder inlines it directly, so the API URL is assembled correctly. No other callers were found to assume a single-segment owner.MarketplaceSourcestorable viaapm marketplace add.http://accepted alongsidehttps://— kept for parity withdependency/reference.py; production usage is expected to be HTTPS-only.Validation
21/21 unit tests pass
Full unit suite — 6656 tests, 0 failures
Live invocation with a 4-segment subgroup path
Parser correctly sets
owner = "solutions-distributeurs/yz_-alf_framework/sandbox",repo = "github-copilot-agents",host = "gitlab.udd.net.intra.laposte.fr". (Fetch aborted manually — no live GitLab API support yet.)How to test
uv run pytest tests/unit/marketplace/test_marketplace_commands.py -v— all 21 tests greenuv run apm marketplace add gitlab.com/myorg/subgroup/my-marketplace --name my-mkt— parses cleanly, fails at fetch (expected without a live GitLab API)uv run apm marketplace add https://gitlab.com/myorg/subgroup/my-marketplace.git --name my-mkt— same result,.gitstrippeduv run apm marketplace add gitlab.com/myorg/repo --host github.com— exits with "Conflicting host" erroruv run apm marketplace add gitlab.com/myorg/../evil/repo— exits with traversal-rejection error