fix(deps): preserve GitLab sparse-fetch transport - #2939
Conversation
Execute the shared transport plan using prepared remotes and per-attempt authentication. Gate REST on executed effective HTTPS, isolate live checkout identities, and add real-Git, architecture and mutation regression proof. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reuse the real-Git transport contract as executable evidence for req-sc-013 and req-rs-016, rather than waiving the Mode B conformance gate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Configure a real strict HTTPS transport plan and a typed Git failure in REST tests, and include the new sparse-plan rule in the frozen architecture inventory. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Typed transport fixtures and rewrite-target deduplication need correction; the documentation and symlink-test updates should also be completed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/apm_cli/deps/download_strategies.py — This new typed-only catch leaves the existing integration regressions… |
 |
tests/integration/test_gitlab_sparse_transport_contract.py — Skipping solely by OS leaves this new symlink-containment regression untested on Windows runners… |
What changed in this PR
This PR fixes GitLab sparse-fetch transport handling while preserving requested URLs, authentication boundaries, checkout reuse, and authorized REST fallback.
Changes:
- Centralizes transport selection and fallback warnings.
- Executes prepared GitLab attempts with transport-aware authentication.
- Adds regression, architecture, conformance, documentation, and changelog coverage.
| File | Reviewed change |
|---|---|
tests/unit/scripts/test_architecture_runner.py |
Updates architecture rule inventory coverage. |
tests/unit/deps/test_initial_transport_scheme.py |
Tests scheme precedence and warning decisions. |
tests/unit/deps/test_gitlab_sparse_transport_contract.py |
Tests authentication, fallback, REST eligibility, and reuse. |
tests/unit/deps/test_github_downloader_gitlab_routing.py |
Preserves GitLab routing coverage. |
tests/unit/deps/test_download_strategies_selection.py |
Aligns selection tests with prepared attempts. |
tests/unit/deps/test_download_strategies_phase3.py |
Updates REST fallback fixtures. |
tests/unit/core/test_git_transport_policy.py |
Tests credential-helper restrictions. |
tests/test_gitlab_git_transport.py |
Preserves GitLab transport expectations. |
tests/test_github_downloader.py |
Updates shared downloader regressions. |
tests/spec_conformance/test_gitlab_sparse_transport_reqs.py |
Binds transport behavior to conformance requirements. |
tests/integration/test_gitlab_sparse_transport_mutations.py |
Validates behavioral mutations. |
tests/integration/test_gitlab_sparse_transport_contract.py |
Verifies real Git materialization and symlink containment. Nit: improve platform-aware symlink setup. |
tests/integration/test_architecture_owner_rule_mutations.py |
Adds ownership mutation coverage. |
tests/integration/test_architecture_gitlab_sparse_transport.py |
Tests static transport checks. |
src/apm_cli/install/validation.py |
Aligns validation transport selection. |
src/apm_cli/deps/transport_selection.py |
Centralizes transport planning. Moderate: preserve distinct rewrite targets during deduplication. |
src/apm_cli/deps/git_reference_resolver.py |
Aligns ref discovery transport. |
src/apm_cli/deps/download_strategies.py |
Executes transport plans and gates REST fallback. Critical: update existing fixtures to raise typed transport errors. |
src/apm_cli/deps/clone_engine.py |
Reuses shared transport helpers. |
src/apm_cli/core/auth.py |
Applies transport-specific credential policy. Nit: update the canonical authentication documentation for HTTPS-only lookup. |
scripts/architecture_linter/groups/transport_platform.py |
Registers the new rule. |
scripts/architecture_linter/checks/transport_gitlab_sparse.py |
Adds sparse transport architecture checks. |
scripts/architecture_linter/checks/transport_auth_platform.py |
Updates authentication boundary checks. |
packages/apm-guide/.apm/skills/apm-usage/authentication.md |
Updates distributed authentication guidance. |
docs/src/content/docs/consumer/manage-dependencies.md |
Documents sparse transport fallback behavior. |
docs/src/content/docs/consumer/authentication.md |
Documents transport-aware authentication. |
CONFORMANCE.md |
Regenerates conformance counts. |
CONFORMANCE.json |
Regenerates conformance bindings. |
CHANGELOG.md |
Records the GitLab transport fix. |
.apm/architecture/owners/transport-auth-platform.json |
Records transport ownership and evidence. |
Suppressed comments (2)
src/apm_cli/core/auth.py:1191
- Because this condition now suppresses GitLab credential-helper lookup for non-HTTPS remotes, the canonical auth page is stale:
docs/src/content/docs/getting-started/authentication.md:388and its package-source table at line 402 still promiseGITLAB_APM_PAT -> GITLAB_TOKEN -> credential fillwithout the HTTPS-only qualifier. Update that canonical flow/table in this PR so the documented auth contract matches the new resolver behavior.
host_info.kind not in ("generic", "gitlab") or allow_generic_credential_lookup
src/apm_cli/deps/transport_selection.py:450
- The initial and chained attempts retain distinct requested/effective URLs, but
_dedup_attempts()later keys only on(scheme, use_token). If these two candidates have different safeinsteadOftargets (for example, two file or SSH mirrors), both collapse to the same scheme/auth pair and the second prepared attempt is dropped, so opt-in fallback can never reach the alternate mirror. Deduplicate on normalized requested/effective URLs as well and add a regression case for distinct rewrite targets.
initial = [_SSH]
chained = [_AUTH_HTTPS, _PLAIN_HTTPS] if has_token else [_PLAIN_HTTPS]
elif initial_scheme == "https":
initial = [_AUTH_HTTPS] if has_token else [_PLAIN_HTTPS]
chained = [_SSH, _PLAIN_HTTPS] if has_token else [_SSH]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | No blocking architecture concerns; the PR centralizes GitLab sparse transport decisions and ships dual behavioral/static guardrails. |
| CLI Logging Expert | 0 | 1 | 1 | No blocking CLI-output issue; make opt-in GitLab sparse fallback transitions as visible as clone fallback transitions. |
| DevX UX Expert | 0 | 2 | 0 | No blocking DevX issue; the recovery guidance needs two small fixes to avoid sending users or agents to stale advice. |
| Supply Chain Security | 1 | 0 | 0 | GitLab SSH validation still resolves credentials before per-attempt policy; fix that auth-contract gap. |
| OSS Growth Hacker | 0 | 0 | 0 | No growth-surface blockers; docs and changelog stay bounded to the GitLab sparse transport contract without README or strategy scope creep. |
| Auth Expert | 0 | 0 | 0 | GitLab sparse-fetch auth now resolves per effective attempt, keeps SSH/native Git token-free, and limits REST to executed same-origin HTTPS; no auth blockers found. |
| Doc Writer | 0 | 1 | 0 | Docs remain scoped and guidance shrinks by 20 words. Qualify the strict-SSH guarantee to distinguish APM's requested URL from Git's effective rewrite target. |
| Test Coverage | 0 | 0 | 0 | GitLab sparse-fetch behavior is covered by mapped regression, mutation, architecture, conformance, and real-binary transport tests. |
| Performance Expert | 0 | 0 | 0 | No scoped performance or lock regression found; GitLab sparse fetch keeps one transport per prepared identity and retry work is bounded. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] Fold the strict-SSH documentation correction before ship. -- The docs currently overstate that explicit SSH/SCP always keeps the effective fetch on SSH and that Git failure never triggers REST; implementation allows safe same-host SSH-to-HTTPS
insteadOfrewrites, after which exhausted effective HTTPS can authorize REST. - [DevX UX Expert] Fold the packaged apm-usage dependency guidance update before ship. --
packages/apm-guide/.apm/skills/apm-usage/dependencies.mdstill says GitLab path files are fetched over Git, not REST, which contradicts the new Git-first plus restricted REST fallback contract that downstream agents will quote to users. - [DevX UX Expert] Fold the dead fallback docs link fix before ship. -- The custom-port recovery warning points users at a route/anchor that does not exist in this tree; a broken recovery link undercuts the failure-mode UX for the exact transport scenario this PR repairs.
- [CLI Logging Expert] Fold an actual protocol-switch warning for GitLab sparse fallback. -- When opt-in fallback changes the attempted sparse transport, users should see the same kind of yellow transition signal clone fallback provides, especially because an effective HTTPS attempt can affect REST eligibility.
- [Supply Chain Security] Do not fold the validation auth redesign into this PR; capture only as an out-of-scope follow-up if the maintainer wants it later. -- The credential-resolution probe is real but pre-existing at the merge base, not a regression from this sparse-fetch fix. Folding it now would broaden the patch into validation/provider auth behavior that the maintainer explicitly excluded.
Architecture
classDiagram
direction LR
class GitHubPackageDownloader {
<<FacadeOwner>>
+_download_github_file(dep_ref, file_path, ref) bytes
+_build_repo_url(repo_url_base, use_ssh, dep_ref, token) str
}
class DownloadDelegate {
<<Delegate>>
+download_gitlab_file(dep_ref, file_path, ref) bytes
+_download_gitlab_file_via_git(dep_ref, file_path, ref, requested_url, effective_url, git_env, auth_mode) bytes
+_git_file_transport_key(dep_ref, ref, requested_url, effective_url, auth_mode) tuple
+_gitlab_rest_eligible(effective_url, api_base) bool
}
class TransportSelector {
<<Strategy>>
+select(dep_ref, cli_pref, allow_fallback, has_token, candidate_url) TransportPlan
}
class TransportAttempt {
<<ValueObject>>
+scheme str
+use_token bool
+label str
+requested_url str
+effective_url str
}
class TransportPlan {
<<ValueObject>>
+attempts list
+strict bool
+fallback_hint str
}
class InsteadOfResolver {
<<Protocol>>
+resolve(candidate_url) str
+has_exact_rule(candidate_url) bool
}
class GitConfigInsteadOfResolver {
<<Adapter>>
+resolve(candidate_url) str
+has_exact_rule(candidate_url) bool
}
class AuthResolver {
<<Strategy>>
+resolve_for_remote(host, remote_url, org) AuthContext
+git_env_for_remote(ctx, remote_url) dict
+build_native_git_credential_env(host_info, remote_url) dict
}
class AuthContext {
<<ValueObject>>
+token str
+source str
+host_info HostInfo
+auth_scheme str
}
class HostInfo {
<<ValueObject>>
+kind str
+host str
+port int
+api_base str
}
class GitSparseFileTransport {
<<ReusableResource>>
+fetch_file(file_path) bytes
+fetch_file_with_commit(file_path) GitFileFetchResult
+close() None
}
class GitFileTransportError {
<<TypedFailure>>
}
class GitTransportPolicy {
<<ValueObject>>
+use_resolved_credentials bool
+allow_native_credential_lookup bool
+reject_https_downgrade bool
}
GitHubPackageDownloader *-- DownloadDelegate : owns
GitHubPackageDownloader *-- TransportSelector : shared selector
DownloadDelegate ..> TransportSelector : consumes plan
TransportSelector o-- InsteadOfResolver : rewrite policy seam
InsteadOfResolver <|.. GitConfigInsteadOfResolver
TransportSelector ..> TransportPlan : returns
TransportPlan *-- TransportAttempt : ordered attempts
DownloadDelegate ..> AuthResolver : per-attempt auth
AuthResolver ..> AuthContext : returns
AuthContext *-- HostInfo : classifies
AuthResolver ..> GitTransportPolicy : applies
DownloadDelegate *-- GitSparseFileTransport : live checkout cache
GitSparseFileTransport ..> GitFileTransportError : raises typed retry signal
note for TransportSelector "Single owner for initial scheme and ordered fallback plan"
note for AuthResolver "Chain of Responsibility: env token -> gh/CLI where eligible -> native credential helper where policy allows"
class DownloadDelegate:::touched
class TransportSelector:::touched
class TransportAttempt:::touched
class TransportPlan:::touched
class AuthResolver:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["CLI install path -> GitHubPackageDownloader._download_github_file"] --> B["src/apm_cli/deps/download_strategies.py: DownloadDelegate.download_gitlab_file"]
B --> C["initial_transport_scheme(dep_ref, self._host._protocol_pref)"]
C --> D["build_repo_url(..., token='') creates requested candidate_url"]
D --> E["[EXEC] TransportSelector.select(..., has_token=False, candidate_url=candidate_url) probes git insteadOf via GitConfigInsteadOfResolver"]
E --> F["AuthResolver.resolve_for_remote(host, initial.effective_url or requested_url)"]
F --> G["TransportSelector.select(..., has_token=bool(initial_ctx.token)) returns final TransportPlan"]
G --> H{"for attempt in plan.attempts"}
H --> I["requested_url = attempt.requested_url or build_repo_url(..., token='')"]
I --> J["effective_url = attempt.effective_url or requested_url"]
J --> K["AuthResolver.resolve_for_remote(host, effective_url, owner)"]
K --> L{"attempt.use_token?"}
L -->|"yes"| M["AuthResolver.git_env_for_remote(attempt_ctx, effective_url)"]
L -->|"no"| N["AuthResolver.build_native_git_credential_env(host_info, effective_url)"]
M --> O["validate_git_url_rewrite_safety(requested_url, git_env) revalidates before cache reuse"]
N --> O
O --> P["_git_file_transport_key(provider.kind, ref, normalize_repo_url(requested_url), normalize_repo_url(effective_url), auth_mode)"]
P --> Q["[LOCK] _git_file_transports_lock gets or creates GitSparseFileTransport"]
Q --> R["[EXEC][FS][NET] GitSparseFileTransport.fetch_file(file_path): git init, remote add, sparse-checkout, fetch, checkout, read_bytes"]
R -->|"success"| S["return bytes; verbose_callback('Fetched file via git transport: ...')"]
R -->|"GitFileTransportError only"| T["[LOCK] _discard_git_file_transport(key, failed_transport) evicts exact failed instance"]
T --> U["rest_eligible = rest_eligible or _gitlab_rest_eligible(effective_url, host_info.api_base)"]
U --> H
R -->|"security/local I/O/rewrite/non-transport exception"| V["terminal propagation; no retry and no REST"]
H -->|"plan exhausted"| W{"rest_eligible true?"}
W -->|"yes: executed same-origin effective HTTPS"| X["[NET] _download_gitlab_file_via_rest(dep_ref, file_path, ref, verbose_callback)"]
W -->|"no"| Y["RuntimeError: Git transport failed; REST is not authorized by selected plan"]
Recommendation
Advisory recommendation: fold the four in-scope docs/UX corrections now, then re-run the shepherd's exact-head validation path. Do not expand into validation auth redesign, provider-wide cache/auth cleanup, README positioning, or docs restructuring; after the bounded folds and refreshed evidence, this should be ready to ship as a trust-preserving GitLab sparse transport fix.
Full per-persona findings
Python Architect
- [nit] Design pattern inventory for the architecture artifact.
Design patterns - Used in this PR: Strategy / selector --
TransportSelector.selectowns ordered transport attempts, whileDownloadDelegate.download_gitlab_fileconsumes the plan instead of recomputing scheme/fallback decisions. - Used in this PR: Dataclass-as-value-object -- frozen
TransportAttemptandTransportPlancarry requested/effective URL identity, token intent, labels, and strictness through the flow. - Used in this PR: Facade/Delegate --
DownloadDelegateremains the backend-specific boundary underGitHubPackageDownloader, with the GitLab sparse path folded into the existing delegate surface. - Used in this PR: Chain of Responsibility --
AuthResolver.resolve_for_remoteretains one credential-resolution chain per effective remote and routes child Git env creation throughgit_env_for_remoteorbuild_native_git_credential_env. - Pragmatic suggestion: none -- the current shape is the simplest correct design at this bounded scope; further provider-wide auth/cache redesign would cross the maintainer's no-scope-creep reservation.
CLI Logging Expert
- [recommended] Emit a protocol-switch warning when GitLab sparse fallback actually changes transport. at
src/apm_cli/deps/download_strategies.py:1058
CloneEngine warns at the moment an admitted fallback changes protocol, but the new GitLab sparse loop only emits the custom-port prewarning and then advances from a failed attempt to the next attempt silently. For default human output, a cross-protocol retry is a yellow event: the user opted into it, but should know APM is now trying a different transport and potentially unlocking REST eligibility after an effective HTTPS attempt.
Suggested: Track the previous failed attempt label/scheme in download_gitlab_file and, when plan.strict is false and the next attempt uses a different scheme, emit the same shape as CloneEngine:Protocol fallback: <prev label> GitLab sparse fetch of <project_path> failed; retrying with <attempt.label>.Keep credentials redacted. - [nit] Make the custom-port warning fix match all fallback sources. at
src/apm_cli/deps/transport_selection.py:154
The warning says todrop --allow-protocol-fallback, but fallback can also come fromAPM_ALLOW_PROTOCOL_FALLBACK=1or saved config; for an explicit SSH URL,Pin the URL schememay also not stop fallback while the escape hatch remains enabled.
Suggested: Prefer wording like:Disable protocol fallback to fail fast, or declare the endpoint each protocol should use.
DevX UX Expert
- [recommended] Custom-port fallback warning links to a docs route that does not exist in this tree. at
src/apm_cli/deps/transport_selection.py:42
The warning is part of the failure/recovery UX for the new GitLab sparse transport path. APM's failure-mode standard is one concrete next action; a deadSee:URL breaks that recovery path. I checked the docs tree forguides/dependenciesandrestoring-the-legacy-permissive-chainand found no matching page or anchor, while the updated in-scope guidance now lives under consumer/manage-dependencies transport selection.
Suggested: Point_PROTOCOL_FALLBACK_DOCS_URLat the existing transport-selection section, e.g.https://microsoft.github.io/apm/consumer/manage-dependencies/#transport-selection, or add the referenced anchor in the docs as part of this bounded docs update. - [recommended] Shipped apm-usage dependency guidance still says GitLab path files are fetched over git, not REST. at
packages/apm-guide/.apm/skills/apm-usage/dependencies.md:68
The PR intentionally changes the user contract to Git-first with restricted REST fallback after an eligible effective HTTPS attempt. The distributed apm-usage resource still saysGitLab path files are fetched over git transport, not the REST API, which conflicts with the updated consumer docs and can make agents give users obsolete troubleshooting advice.
Suggested: Mirror the bounded wording used indocs/src/content/docs/consumer/manage-dependencies.md: GitLabpath:files use Git first, with restricted REST fallback, then link to the GitLab authentication/fetch policy section.
Supply Chain Security
- [blocking] GitLab SSH validation still performs host-scoped credential resolution before the transport attempt is known. at
src/apm_cli/install/validation.py:454
The PR's security contract says GitLab non-HTTPS attempts must not resolve HTTPS credentials and that authentication is selected per effective attempt. However validation builds a GitLab SSH candidate, then immediately calls auth_resolver.resolve_for_dep(dep_ref) before iterating the selected attempts. With an explicit GitLab SSH/SCP dependency this path can read GITLAB_APM_PAT/GITLAB_TOKEN and invoke git credential fill even though the eventual ls-remote is SSH and git_env_for_remote later strips those credentials. That violates least privilege and the documented 'credential helper when the remote transport permits lookup' invariant, and it leaves the validation surface out of sync with the fixed sparse-fetch path.
Suggested: Mirror download_gitlab_file's per-attempt flow in validation: select an anonymous initial plan from the requested URL, resolve credentials with resolve_for_remote(policy_url) only for attempts whose effective transport permits it, and avoid resolve_for_dep for GitLab SSH, SCP, file, and HTTP attempts. Add a regression test for _validate_package_exists('git@gitlab.com:owner/repo.git#main') with a recording token manager asserting resolve_credential_from_git is not called.
Proof (manual only):(no test ref)-- proves: An explicit GitLab SSH validation path can consult HTTPS credential sources before running the SSH probe. [secure-by-default,governed-by-policy]
dep_ctx = None if is_generic else auth_resolver.resolve_for_dep(dep_ref)
OSS Growth Hacker
No findings.
Auth Expert
No findings.
Doc Writer
- [recommended] Qualify the strict-SSH guarantee for safe Git rewrites at
docs/src/content/docs/consumer/authentication.md:68
The new paragraph says explicit SSH/SCP and SSH preference keep fetches on SSH and Git failure never triggers REST. That is stronger than the implementation: TransportSelector._rewrite_attempt preserves the requested URL but adopts the effective rewrite scheme, including in strict mode. A safe same-host SSH-to-HTTPS rewrite can therefore execute effective HTTPS, and download_gitlab_file authorizes REST after that attempt fails and the plan is exhausted. This also contradicts manage-dependencies.md, which correctly says an explicit scheme does not disable insteadOf configuration. The packaged authentication guidance repeats the unconditional preservation claim. This is a bounded documentation correction, not a request to change transport policy.
Suggested: State that strict mode passes the selected SSH/SCP URL to Git without APM choosing another protocol; safe Git insteadOf rules still apply. Replace 'Git failure never triggers REST' with 'If the effective transport remains SSH, failure never triggers REST, even with a PAT available.' Apply the same distinction in packages/apm-guide/.apm/skills/apm-usage/authentication.md and retain the existing effective-HTTPS eligibility paragraph.
Test Coverage
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Deferred (out-of-scope follow-ups)
- Validation credential-order redesign: scope_boundary_crossed = unchanged merge-base validation behavior; this PR preserves validation behavior and fixes per-attempt auth for GitLab sparse fetch. No issue or project was created.
Copilot signals reviewed
No Copilot review or inline comments exist in the first fetch round.
Address CEO docs and diagnostics follow-ups on PR #2939: distinguish requested SSH from effective Git rewrites, correct packaged REST guidance, repair fallback recovery advice, and expose admitted protocol switches. Add a warning regression and isolated mutation proof without changing validation auth or other provider policy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fix both legacy integration REST fixtures to execute a real HTTPS selector plan and raise only typed Git failures. Probe symlink capability rather than skipping Windows unconditionally, and qualify the canonical sparse-fetch credential documentation. Addresses Copilot review 5167472317 without changing validation auth or shared selector dedup semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CI recovery 1: the shared custom-port warning now correctly explains disabling all fallback configuration sources and points at the live docs route. Update its older clone consumer assertion to this reviewed wording without changing runtime behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | No blocking architecture concern; terminal folds route GitLab sparse fetch through selector/AuthResolver with behavioral plus static guards. |
| CLI Logging Expert | 0 | 0 | 0 | CLI warning folds are verified; no remaining CLI logging issues within the reserved GitLab sparse-fetch scope. |
| DevX UX Expert | 0 | 0 | 0 | GitLab sparse transport UX, warnings, and docs are scoped and actionable; no DevX findings remain. |
| Supply Chain Security | 0 | 0 | 0 | No introduced supply-chain blocker remains in the bounded GitLab sparse-fetch transport scope. |
| OSS Growth Hacker | 0 | 0 | 0 | No growth finding: final docs and changelog stay inside the bounded GitLab sparse-fetch scope; no README, strategy, or positioning work needed. |
| Auth Expert | 0 | 0 | 0 | GitLab sparse auth, credential isolation, rewrite checks, and REST eligibility match the bounded transport contract. |
| Doc Writer | 0 | 0 | 0 | Docs now match the bounded GitLab sparse-fetch transport contract; no doc-writer findings remain. |
| Test Coverage | 0 | 0 | 0 | GitLab sparse transport contracts have exact-head unit, integration, mutation, owner, conformance, and binary e2e coverage. |
| Performance Expert | 0 | 0 | 0 | No introduced performance regression; GitLab sparse plan stays cached/reused, adds no raw ls-remote, and exact-head evidence passes. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class GitHubPackageDownloader { <<FacadeOwner>> +_download_github_file() }
class DownloadDelegate { <<Delegate>> +download_gitlab_file() +_download_gitlab_file_via_git() }
class TransportSelector { <<Strategy>> +select() TransportPlan }
class TransportPlan { <<ValueObject>> +attempts +strict }
class TransportAttempt { <<ValueObject>> +requested_url +effective_url +use_token }
class AuthResolver { <<Strategy>> +resolve_for_remote() +git_env_for_remote() }
class GitSparseFileTransport { <<ReusableResource>> +fetch_file() }
GitHubPackageDownloader *-- DownloadDelegate : owns
DownloadDelegate ..> TransportSelector : consumes plan
TransportSelector ..> TransportPlan : returns
TransportPlan *-- TransportAttempt : ordered attempts
DownloadDelegate ..> AuthResolver : per-attempt auth/env
DownloadDelegate *-- GitSparseFileTransport : keyed requested+effective+auth
note for TransportSelector "Single owner: initial scheme plus ordered fallback plan"
note for AuthResolver "Chain of Responsibility: env/CLI/helper only where policy allows"
class DownloadDelegate:::touched
class TransportSelector:::touched
class AuthResolver:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["CLI install -> GitHubPackageDownloader._download_github_file"] --> B["download_strategies.py: DownloadDelegate.download_gitlab_file"]
B --> C["transport_selection.py: initial_transport_scheme(dep_ref, pref)"]
C --> D["build_repo_url(..., token='') creates requested candidate_url"]
D --> E["[EXEC] TransportSelector.select(..., has_token=False, candidate_url)"]
E --> F["AuthResolver.resolve_for_remote(host, initial effective/requested URL)"]
F --> G["TransportSelector.select(..., has_token=bool(initial_ctx.token))"]
G --> H{"for attempt in plan.attempts"}
H --> I["AuthResolver.resolve_for_remote(host, effective_url) then git_env_for_remote/build_native_git_credential_env"]
I --> J["validate_git_url_rewrite_safety(requested_url, git_env)"]
J --> K["[LOCK] _git_file_transport_key(provider, ref, requested, effective, auth_mode)"]
K --> L["[EXEC][FS][NET] _download_gitlab_file_via_git -> GitSparseFileTransport.fetch_file"]
L -->|"success"| M["return bytes; verbose_callback fetched via git transport"]
L -->|"GitFileTransportError only"| N["[LOCK] _discard_git_file_transport(exact failed instance); maybe warn protocol fallback; maybe mark REST eligible"]
N --> H
L -->|"security/rewrite/local/non-transport failure"| O["terminal exception; no retry and no REST"]
H -->|"exhausted"| P{"executed same-origin effective HTTPS?"}
P -->|"yes"| Q["[NET] _download_gitlab_file_via_rest"]
P -->|"no"| R["RuntimeError: REST not authorized by selected transport plan"]
Recommendation
Ship the reviewed scoped code once the orchestrator observes the still-running CI checks finish green. There are no remaining panel follow-ups to fold in this PR, and the deferred baseline themes should stay outside this PR unless a future maintainer explicitly opens a provider-wide validation or fallback-identity redesign.
Full per-persona findings
Python Architect
- [nit] Architecture pattern inventory for the panel synthesis.
Design patterns - Used in this PR: Strategy / selector --
TransportSelector.selectremains the single owner for initial scheme and ordered fallback attempts, whileDownloadDelegate.download_gitlab_fileconsumes the selected plan instead of recomputing it. - Used in this PR: Dataclass-as-value-object -- frozen
TransportAttemptandTransportPlancarry requested URL, effective URL, token intent, label, and strictness through the GitLab sparse path. - Used in this PR: Facade/Delegate --
GitHubPackageDownloaderkeeps backend orchestration delegated toDownloadDelegate, with GitLab sparse-fetch folded into that existing boundary. - Used in this PR: Chain of Responsibility --
AuthResolver.resolve_for_remoteapplies the credential chain per effective remote, and the GitLab sparse loop then selects managed or native Git env per attempt. - Pragmatic suggestion: none -- the current shape is the simplest correct design at this bounded scope; broad provider-wide auth/cache redesign would cross the maintainer's no-scope-creep reservation.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security
No findings.
OSS Growth Hacker
No findings.
Auth Expert
No findings.
Doc Writer
No findings.
Test Coverage
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Reservations carried from strategic-alignment
- Maintainer explicitly requires no scope creep. Preserve GitLab sparse-fetch transport and its directly coupled authentication, rewrite, REST eligibility, live checkout identity, error, regression-test, documentation and ownership-guard contracts. Do not broaden this into provider-wide auth/cache redesign, new user features, unrelated cleanup, documentation restructuring or project positioning work. -- Addressed by the bounded folds and the two explicit baseline deferrals below.
Folded in this run
- (panel) Qualify strict SSH docs with requested versus effective safe Git rewrite semantics. -- resolved in
246eb0a. - (panel) Correct packaged dependency guidance to Git-first with restricted REST recovery. -- resolved in
246eb0a. - (panel) Point the shared custom-port warning at the live transport-selection documentation. -- resolved in
246eb0a. - (panel) Show actual opt-in cross-protocol GitLab sparse retry transitions, with eight regression cases and M10 mutation proof. -- resolved in
246eb0a. - (panel) Make fallback-disable advice cover CLI flags, environment and saved configuration. -- resolved in
246eb0a. - (copilot) Repair both existing integration REST fixtures with real HTTPS selection, prepared auth and typed Git failures. -- resolved in
7479322. - (copilot) Replace unconditional Windows symlink-test skip with a capability probe. -- resolved in
7479322. - (copilot) Qualify the canonical sparse-fetch credential-chain prose and package-source table. -- resolved in
7479322. - (panel) Update the older clone warning regression to the reviewed remediation and parsed live docs URL after CI exposed its stale expectation. -- resolved in
81bbad6.
Copilot signals reviewed
3979380427-- LEGIT: Both overlooked integration REST fixtures failed: their MagicMock plans executed no attempt, and their sparse mock raised an untyped RuntimeError. Real HTTPS plans and typed errors restore their intended contract.3979380480-- LEGIT: An OS-only symlink skip discarded supported Windows coverage. The test now attempts symlink creation and skips only when the filesystem reports unsupported capability.5167472317-suppressed-auth-docs-- LEGIT: The canonical auth page needed the sparse-fetch effective-HTTPS qualifier. Added bounded prose and source-table guidance without claiming validation credential ordering changed.5167472317-suppressed-rewrite-dedup-- LEGIT: Distinct mirror targets can share the existing scheme/auth dedup key. Its AST is unchanged from merge base; changing this all-provider selector behavior crosses the explicit preservation boundary, so it is deferred.
Both inline threads were answered and resolved using threaded replies. Two fetch rounds consumed; no additional review round was requested.
Deferred (out-of-scope follow-ups)
- (panel) Move pre-existing validation credential resolution behind per-effective-attempt policy. -- scope_boundary_crossed: The resolve_for_dep call is identical at merge base. Changing validation credential ordering exceeds the sparse-fetch repair and contradicts the explicit requirement to preserve existing validation behavior.
- (copilot) Make shared transport-selector dedup distinguish alternate mirror URL identities. -- scope_boundary_crossed: The shared _dedup_attempts AST is identical at merge base. URL-aware dedup changes all-provider clone/ref/validation fallback semantics, beyond executing the existing selector plan in GitLab sparse fetch.
No follow-up issue/project was created; no README, policy baseline, or network configuration changed.
Regression-trap evidence (mutation-break gate)
457 exact-head tests passed in 170.96s with zero skips. All 14 behavioral mutation drivers required a passing baseline and the intended child AssertionError; the separate M9 static suite rejected owner bypasses. The new protocol-switch warning test was also demonstrated failing before the implementation.
Lint contract
Exact HEAD 81bbad6: uv run --frozen --no-sync ruff check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/ exited 0; ruff format --check for the same paths exited 0 (1849 files already formatted). Pylint R0801, auth-signals, architecture boundaries, unchanged GNU Linux YAML/2100-line/relative_to guards, assertion and duplicate ratchets, and conformance orphan check all exited 0. Evidence: shepherd-81bbad6-lint.log and shepherd-81bbad6-linux-guards.log. origin/main was freshly merged (already up to date) before each mirror/push.
CI
The CEO's pending-CI condition above is now satisfied. Observed final HEAD 81bbad6: all 18 check-rollup entries complete (16 SUCCESS, 1 NEUTRAL, 1 SKIPPED). Main CI https://github.com/microsoft/apm/actions/runs/34484187161 and merge gate https://github.com/microsoft/apm/actions/runs/34484187284 succeeded, as did spec, docs, CodeQL, NOTICE and CLA. Recovery count 1: earlier run 34483012748 exposed one stale clone-warning assertion, corrected in 81bbad6 without production changes. Actual exact-head Linux binary acceptance separately passed 5 tests in 40.33s; binary SHA256 e7f73b82b11050daa220a25df7b6914c0bec40f27ebac11fa40464a62600cbdf. GitHub remains MERGEABLE/BLOCKED, not a conflict; no merge or auto-merge was performed.
The actual final Linux amd64 binary reports 0.30.0 (81bbad6); five SSH-semver acceptance tests passed in 40.33s. This is separate executed binary evidence, not an inference from generic CI.
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2939 | 81bbad6 |
ship_now | 2 | 9 | 2 | 2 | green | MERGEABLE | BLOCKED | awaiting maintainer review |
Convergence
Two full synchronous nine-persona advisory passes and separate CEO syntheses; one CI recovery. Final stance: ship_now. Canonical-owner completion v2 passed JSON schema and deterministic exact-head semantic verification for all three touched owners. No merge, auto-merge, queue, closure or superseding PR was performed.
fix(deps): preserve GitLab sparse-fetch transport
TL;DR
GitLab sparse downloads now execute the shared transport plan instead of silently rebuilding an SSH dependency as HTTPS. The requested URL reaches Git unchanged; its effective rewrite target controls authentication and REST eligibility. This implements #2938 and addresses the reproduction in #2929.
Closes #2938.
Fixes #2929.
Important
All required PR checks and the separate Linux x86-64 SSH-semver acceptance run pass on final shepherd candidate
81bbad6c15c9dd45136318c48994c06cb8871314. The binary was actually built from this exact candidate. Package-index configuration remained local; no repository, lockfile, or CI configuration changes were needed.Problem (WHY)
2222became an HTTPS sparse-fetch remote. The new actual-Git-remote assertion fails on both recorded unfixed revisions, not merely on a mocked constructor.The proof follows Agent Skills' execution-first guidance:
"Run the skill against real tasks"
Here the concrete inputs are the reported manifest, real local Git materialization, and named failing assertions on deliberate mutations.
Approach (WHAT)
Implementation (HOW)
The principal execution boundary is
download_gitlab_file.src/apm_cli/deps/transport_selection.pysrc/apm_cli/deps/clone_engine.pysrc/apm_cli/deps/git_reference_resolver.pysrc/apm_cli/install/validation.pysrc/apm_cli/deps/download_strategies.pysrc/apm_cli/core/auth.pyscripts/architecture_linter/checks/transport_gitlab_sparse.pyscripts/architecture_linter/checks/transport_auth_platform.pyscripts/architecture_linter/groups/transport_platform.py.apm/architecture/owners/transport-auth-platform.jsontests/unit/deps/test_initial_transport_scheme.pytests/unit/deps/test_gitlab_sparse_transport_contract.pytests/integration/test_gitlab_sparse_transport_contract.pytests/integration/test_gitlab_sparse_transport_mutations.pytests/integration/test_architecture_gitlab_sparse_transport.pytests/integration/test_architecture_owner_rule_mutations.pytests/test_gitlab_git_transport.pytests/test_github_downloader.pytests/unit/core/test_git_transport_policy.pytests/unit/deps/test_download_strategies_selection.pytests/unit/deps/test_github_downloader_gitlab_routing.pytests/unit/deps/test_download_strategies_phase3.pytests/unit/scripts/test_architecture_runner.pytests/spec_conformance/test_gitlab_sparse_transport_reqs.pyCONFORMANCE.jsonCONFORMANCE.mddocs/src/content/docs/consumer/authentication.mddocs/src/content/docs/consumer/manage-dependencies.mdpackages/apm-guide/.apm/skills/apm-usage/authentication.mdCHANGELOG.mdtests/integration/test_download_strategies_selection.py,test_download_strategies_phase3w5.pytests/unit/test_protocol_fallback_warning.pydocs/src/content/docs/getting-started/authentication.mdpackages/apm-guide/.apm/skills/apm-usage/dependencies.mdDiagrams
Dashed nodes show the newly explicit prepared-identity and REST-authorization boundaries; non-transport exceptions propagate rather than entering the retry path.
flowchart LR subgraph Policy["Shared transport policy"] A["DependencyReference"] --> B["TransportSelector.select"] end subgraph Preparation["Per-attempt preparation"] B --> C["Requested and effective URLs"] C --> D["Requested URL to Git"] C --> E["Effective URL to AuthResolver"] end subgraph Execution["GitLab sparse fetch"] D --> F["Prepared checkout identity"] E --> F F --> G["GitFileTransport"] G -->|"Success"| H["Requested file and ref"] G -->|"Typed Git failure"| I["Exhaust remaining plan"] end subgraph Eligibility["REST eligibility"] I --> J{"Executed same-origin effective HTTPS?"} J -->|"Yes"| K["GitLab REST"] J -->|"No"| L["Propagate failure"] end classDef new stroke-dasharray: 5 5; class C,D,E,F,I,J new;Trade-offs
Benefits
Validation
Final PR candidate:
81bbad6c15c9dd45136318c48994c06cb8871314.Base:
e38261c5db4d893d6ddebc3925742e4e3bd2ba74.Production source and lockfile are byte-identical from the bounded warning/doc fold
246eb0a276c3986bdfde271bd1ea5838a4279b53to the final candidate; subsequent commits only correct tests and documentation.Final shepherd validation
0.30.0 (81bbad6). SSH-semver acceptance: 5 passed in 40.33s, zero skips. SHA256:e7f73b82b11050daa220a25df7b6914c0bec40f27ebac11fa40464a62600cbdf.Earlier candidate evidence (retained for traceability)
The following records apply to
fbbc5a1f283841b51847ca07719b22e4bd5bdff8, not to the final candidate. Its CI run passed both Linux test shards, Lint, Windows Compatibility, Test Architecture Ratchets, PR Binary Smoke, Lifecycle Smoke and Coverage Combine, plus the separate spec, docs, CodeQL, NOTICE, CLA and merge checks.uv run --frozen --no-sync pytest -p no:cacheprovider -q tests/unit/deps/test_download_strategies_phase3.py tests/unit/scripts/test_architecture_runner.py tests/unit/deps/test_gitlab_sparse_transport_contract.py tests/integration/test_gitlab_sparse_transport_contract.py tests/spec_conformance/test_gitlab_sparse_transport_reqs.py:uv run --frozen --no-sync pytest -p no:cacheprovider -q tests/spec_conformance/test_gitlab_sparse_transport_reqs.py tests/quality:The isolated Linux x86-64 container used the existing environment-local package mirror. Dependencies were exported from the frozen lockfile and installed with
uv pip sync --require-hashes, followed by the original frozen sync and build commands. The binary reportedAgent Package Manager (APM) CLI version 0.30.0 (fbbc5a1).git diff --exit-codepassed before and after the build and acceptance run.APM_BINARY_PATH="$PWD/dist/apm-linux-x86_64/apm" uv run --frozen --extra dev pytest -p no:cacheprovider -q tests/integration/test_ssh_semver_transport_contract.py:Local regression and lint evidence
Combined regression, architecture, and mutation selection using
uv run --frozen --no-sync pytest(recorded output excerpt):The two skips are pre-existing live-network cases. Every new proof case executed. The run also emitted pytest temporary-directory cleanup warnings; they did not change the test result.
uv run --frozen --no-sync ruff check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/:uv run --frozen --no-sync ruff format --check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/:The pylint R0801 gate, auth boundary script, architecture boundary script, assertion ratchet, and exact-duplicate ratchet pass.
tests/qualitypassed 63 cases without baseline changes.The three GNU-dependent CI scripts were extracted from
.github/workflows/ci.ymland executed unchanged withbash -e -sin a Linux container against the read-only candidate:All three exited 0. The Mermaid block was rendered successfully with
mmdc.Scenario Evidence
tests/integration/test_gitlab_sparse_transport_contract.py::test_real_git_manifest_remote_bytes_and_single_fetch(regression-trap for #2938 / #2929)tests/unit/deps/test_gitlab_sparse_transport_contract.py::test_strict_ssh_failure_never_resttests/unit/deps/test_gitlab_sparse_transport_contract.py::test_unmanaged_transports_strip_pattests/unit/deps/test_gitlab_sparse_transport_contract.py::test_opt_in_fallback_order_port_and_warning;test_protocol_switch_warning_matches_executed_attemptstests/unit/deps/test_gitlab_sparse_transport_contract.py::test_https_rest_preserves_headers_endpoint_and_reftests/integration/test_gitlab_sparse_transport_contract.py::test_effective_local_rewrite_does_not_authorize_resttests/integration/test_gitlab_sparse_transport_contract.py::{test_traversal_is_terminal_before_fetch,test_cached_symlink_escape_is_terminal,test_local_read_failure_is_terminal}tests/integration/test_gitlab_sparse_transport_contract.py::test_missing_custom_ref_is_not_replacedtests/integration/test_gitlab_sparse_transport_contract.py::test_concurrent_paths_share_one_initial_checkout;tests/unit/deps/test_gitlab_sparse_transport_contract.py::test_failed_eviction_preserves_replacementtest_public_github_anonymous_first.py,test_github_throttle_fallback.py,test_git_transport_policy.py,test_validation_strict_transport.py, andtest_git_reference_resolver.pyin the recorded regression selectionHow to test
uv sync --frozen --extra dev, then runuv run --frozen --extra dev pytest -p no:cacheprovider -q tests/unit/deps/test_initial_transport_scheme.py tests/unit/deps/test_gitlab_sparse_transport_contract.py tests/integration/test_gitlab_sparse_transport_contract.py; expect transport, credential, ref, and materialization assertions to pass.uv run --frozen --extra dev pytest -p no:cacheprovider -q tests/integration/test_architecture_gitlab_sparse_transport.py tests/integration/test_gitlab_sparse_transport_mutations.py; expect passing baselines and intended mutant assertion failures recognized by the driver.tests/qualitygates; use Linux for the unchanged GNU grep checks.uv sync --frozen --extra dev --extra build, thenUV_FROZEN=true uv run --extra dev --extra build bash scripts/build-binary.sh. RunAPM_BINARY_PATH="$PWD/dist/apm-linux-x86_64/apm" uv run --frozen --extra dev pytest -p no:cacheprovider -q tests/integration/test_ssh_semver_transport_contract.py; require actual execution, not missing-binary skips.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com