fix(deps): support multi-host dependency identity#1735
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses multi-host dependency correctness and UX by making lockfile identity host-aware (while keeping github.com keys stable), aligning single-file download auth boundaries with clone auth, adding an explicit type: gitlab host signal for bespoke GitLab instances, and improving download error clarity (only 404 falls through; non-404/network errors surface with endpoint context).
Changes:
- Introduces a shared host-aware dependency unique key (
build_dependency_unique_key) and threadshost_typethrough dependency parsing, lockfile round-trips, host classification, and backend selection. - Updates generic-host file download behavior to use per-dependency auth resolution (avoiding managed PAT headers on generic HTTP downloads) and improves verbose/error diagnostics for raw/API attempts.
- Adds regression tests plus documentation updates for
type: gitlaband the lockfile key migration rule.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/models/dependency/reference.py |
Adds shared host-aware unique-key builder and host_type parsing for object-form deps. |
src/apm_cli/deps/lockfile.py |
Persists host_type and uses shared unique-key logic for lockfile indexing. |
src/apm_cli/core/auth.py |
Extends host classification + auth resolution cache keys to incorporate explicit host_type. |
src/apm_cli/deps/host_backends.py |
Threads host_type into backend selection/classification to route GitLab correctly. |
src/apm_cli/deps/github_downloader.py |
Ensures GitLab routing decisions consider host_type in host classification. |
src/apm_cli/deps/github_downloader_validation.py |
Aligns validation host classification with host_type. |
src/apm_cli/deps/download_strategies.py |
Uses per-dependency auth boundary for file downloads; improves fallback/error surfacing; adds helper error builders. |
src/apm_cli/install/validation.py |
Makes GitLab detection in validation respect explicit host_type. |
tests/test_lockfile.py |
Adds regression tests for host-aware lockfile keys and host_type round-trip. |
tests/test_github_downloader.py |
Updates/extends tests for token boundaries, GitLab routing via type: gitlab, and non-404 error surfacing. |
tests/unit/deps/test_download_strategies_selection.py |
Updates host mock to reflect generic-host token boundary behavior. |
tests/unit/deps/test_download_strategies_phase3.py |
Same as above for phase3 coverage. |
packages/apm-guide/.apm/skills/apm-usage/dependencies.md |
Documents type: gitlab and lockfile key migration behavior. |
packages/apm-guide/.apm/skills/apm-usage/authentication.md |
Documents GitLab host classification options including type: gitlab. |
docs/src/content/docs/reference/lockfile-spec.md |
Documents host_type and the new host-aware lockfile key rule. |
docs/src/content/docs/getting-started/authentication.md |
Adds type: gitlab as an explicit GitLab classification mechanism. |
docs/src/content/docs/consumer/manage-dependencies.md |
Documents object-form type: gitlab and provides examples/usage guidance. |
CHANGELOG.md |
Adds an Unreleased “Fixed” entry describing the multi-host lockfile + auth + error improvements. |
Copilot's findings
- Files reviewed: 18/18 changed files
- Comments generated: 3
| host_value = (host or "").strip() | ||
| if host_value and host_value.lower() != "github.com": | ||
| return f"{host_value}/{key}" | ||
| return key |
| | `repo_url` | string | yes | Canonical repo URL (e.g. `github.com/owner/repo`). Unique key for the entry, except for virtual and local entries (see below). | | ||
| | `host` | string | no | FQDN when not inferable from `repo_url` (e.g. for registry proxies or non-GitHub hosts). | | ||
| | `host_type` | string | no | Explicit host-kind hint, currently `gitlab`, copied from object-form `type: gitlab`. | |
|
|
||
| ### Fixed | ||
|
|
||
| - `apm install` now keeps same-path dependencies from different git hosts distinct in `apm.lock.yaml`, aligns generic-host file-download auth with clone auth, and surfaces non-404 download failures with host and endpoint context. (#773) |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Shared key helper removes duplication; host-blind vs host-qualified split is clean and documented. |
| Auth Expert | 0 | 1 | 0 | host_type threaded everywhere and folded into the resolve cache key; GitLab token precedence correct; unknown types fail closed. |
| Supply Chain Security | 0 | 1 | 0 | Host-qualified identity closes a cross-host overwrite vector; generic-host downloads stop leaking managed/credential-fill tokens. |
| CLI Logging Expert | 0 | 1 | 1 | Download errors now name host + endpoint + status/exception; verbose breadcrumbs unified across hosts. |
| DevX UX Expert | 0 | 1 | 0 | Migration-safe and additive; determinism preserved; orphan/install UX unaffected by the new key. |
| Test Coverage Expert | 0 | 0 | 1 | TDD-first; every behavior trapped. Error-message URL parsing in tests is format-brittle. |
| Performance Expert | 0 | 0 | 0 | No added round-trips (resolve is cached); fail-fast on hard download errors trims wasted attempts. |
| OSS Growth Hacker | 0 | 0 | 0 | type: gitlab opens self-managed GitLab/Gitea adoption; amplify in release notes. |
| Doc Writer | 0 | 0 | 0 | lockfile-spec migration section, type: gitlab, CHANGELOG, and apm-usage guide all updated; no drift. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Supply Chain Security] Keep the credential-fill posture shift visible -- private generic-host (Gitea/Gogs) file reads via the HTTP download path now require a clone path or a host backend signal. -- The trade-off is documented, but it is the one behavior change an existing user could feel; a prominent migration line reduces surprise 401/empty-read reports.
- [Python Architect] The two
get_canonical_dependency_string()bodies (lockfile.py and reference.py) are now near-identical. -- Consider sharing the host-blind branch the same waybuild_dependency_unique_key()was extracted, before it drifts or trips the R0801 duplication gate later. - [Test Coverage / CLI Logging] Download-error tests parse URLs out of the message via
split()+startswith("https://")+rstrip("."). -- Asserting on structured context (host, endpoint, status) instead of message scraping would survive future wording tweaks. - [Auth Expert]
typecurrently accepts onlygitlaband raises on anything else. -- Track the extension path (e.g. futuregitea/bitbuckethints) so the fail-closedValueErroris a conscious gate rather than a future foot-gun.
Architecture
classDiagram
class DependencyReference {
+str repo_url
+str host
+str host_type
+int port
+get_unique_key() str
+get_canonical_dependency_string() str
}
class LockedDependency {
+str repo_url
+str host
+str host_type
+get_unique_key() str
+get_canonical_dependency_string() str
}
class build_dependency_unique_key {
<<function>>
+repo_url
+host
+source
+virtual_path
+registry_prefix
}
class AuthResolver {
+classify_host(host, port, host_type) HostInfo
+resolve(host, org, port, host_type) AuthContext
+resolve_for_dep(dep_ref) AuthContext
}
DependencyReference --> build_dependency_unique_key : host-qualified key
LockedDependency --> build_dependency_unique_key : host-qualified key
AuthResolver ..> DependencyReference : reads host_type
class DependencyReference:::core
class build_dependency_unique_key:::core
flowchart TD
manifest[apm.yml dependency] --> ref[DependencyReference]
ref --> typehint{explicit type gitlab?}
typehint -->|yes| gitlab[GitLab REST + GITLAB_APM_PAT chain]
typehint -->|no| classify[AuthResolver.classify_host]
ref --> keyfn[build_dependency_unique_key]
keyfn --> hostq{host == github.com or unset?}
hostq -->|yes| barekey[bare owner/repo lock key]
hostq -->|no| qualkey[host/owner/repo lock key]
barekey --> lock[apm.lock.yaml]
qualkey --> lock
ref --> orphan[get_canonical_dependency_string]
orphan --> match[host-blind orphan / Source match]
classify --> token[_resolve_dep_token / _resolve_dep_auth_ctx]
token --> dl[single-file download = clone auth boundary]
Recommendation
Merge once you are comfortable with the documented credential-fill posture shift (follow-up 1) -- it is the only change an existing user could feel, and it is intentional and secure-by-default. Everything else is polish: track the shared-helper duplication (follow-up 2) and the test message-scraping brittleness (follow-up 3) as low-priority cleanups. CI is green, tests are TDD-first and comprehensive, and the migration is additive. Note this PR shares lockfile.py with #1738 (#1209); whichever lands second should rebase.
Full per-persona findings
Python Architect
- [recommended] Shared
build_dependency_unique_key()is consumed by bothDependencyReference.get_unique_key()andLockedDependency.get_unique_key()atsrc/apm_cli/models/dependency/reference.pyandsrc/apm_cli/deps/lockfile.py.
Good extraction: it removes the prior copy-paste of the key logic and makes the host-qualification rule single-sourced. The registry-prefix carve-out (proxy host is transport, not identity) is correctly documented and preserves the manifest/lockfile key correspondence.
Suggested: none -- this is the right shape. - [nit] The two
get_canonical_dependency_string()implementations are now near-identical (local_path / virtual_path / repo_url) acrosslockfile.pyandreference.py.
Below the R0801 10-line threshold today, but a candidate for the same extraction treatment before it drifts.
Auth Expert
- [recommended]
host_typeis threaded throughclassify_host,resolve,resolve_for_dep,backend_for, and validation, and is folded into theresolve()cache key tuple atsrc/apm_cli/core/auth.py.
Includinghost_typein the cache key is the correct call -- it prevents a hinted classification from poisoning an unhinted sibling entry for the same (host, port, org). GitLab token precedence (GITLAB_APM_PAT->GITLAB_TOKEN-> credential fill) is preserved, and an unsupportedtyperaisesValueError(fail closed).
Supply Chain Security
- [recommended] Host-qualified lockfile identity (
host/owner/repofor non-default hosts) closes a cross-host key-overwrite vector, anddownload_github_filenow resolves tokens via_resolve_dep_token/_resolve_dep_auth_ctxatsrc/apm_cli/deps/download_strategies.py.
Net secure-by-default tightening: generic hosts no longer receive APM-managed PATs, and the previously-forwarded host-scoped credential-fill token is no longer attached on the HTTP file-download path. The trade-off (private generic-host file reads must use clone or a host backend signal) is documented in the PR body and docs. No blocking concern; flagged as a follow-up for user-facing migration visibility.
CLI Logging Expert
- [recommended] New
_build_download_http_error/_build_download_network_errorhelpers surface host + endpoint + status (or exception type), and verbose breadcrumbs now log Contents API attempts uniformly (previously gated onnot is_github_host).
This directly satisfies the download-error-clarity item: a non-404 failure now names the failing endpoint instead of degrading to a generic missing-file message. ASCII-clean. - [nit] Error strings end with a trailing period that tests strip (
rstrip(".")); harmless but couples message punctuation to test parsing.
DevX UX Expert
- [recommended] Migration is additive and deterministic: existing
github.comkeys stay byte-stable, non-default hosts become visible only in their own key, and host casing is normalized so it cannot mint duplicate keys (tests/test_lockfile.py::test_get_unique_key_lowercases_non_default_host).
Because orphan/Source matching uses the host-blind key, the install/orphan UX is unchanged for existing users -- the new key surface is invisible unless a non-default host is actually in play.
Test Coverage Expert
- [nit] Coverage is TDD-first and traps every claimed behavior: collision (
test_add_dependency_keeps_same_repo_from_different_hosts), github.com preservation, host lowercasing, host_type round-trip, bespoke GitLab routing (test_object_form_type_gitlab_routes_bespoke_host_to_gitlab_api), non-404 surfacing, and the generic token boundary.
The only soft spot: error-path tests reconstruct URLs by scraping the message string; asserting on structured fields would be more robust. (The ~16 localtest_download_*failures are pre-existing network/env artifacts, green in CI -- not attributable to this PR.)
Performance Expert
No findings. The file-download path swaps a direct resolve() call for the cached _resolve_dep_token / _resolve_dep_auth_ctx boundary (no added round-trips), and narrowing raw-URL fallback to 404-only trims wasted attempts on hard failures. classify_host with host_type stays pure/cheap.
OSS Growth Hacker
No findings. Adoption note only: type: gitlab plus host-qualified identity makes APM viable for self-managed GitLab and multi-host Gitea/Gogs estates without hostname heuristics -- a concrete contributor/adopter segment worth amplifying in the release announcement.
Doc Writer
No findings. The lockfile-spec gains a "Lockfile identity keys" migration section, type: gitlab is documented across manage-dependencies.md, authentication.md, and the apm-usage guide, and the CHANGELOG carries a user-facing "Fixed" entry. No code/doc drift detected.
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>
Folds copilot-pull-request-reviewer follow-ups: case-insensitive host key normalization, lockfile-spec doc update, changelog PR number. Refs #773 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The initial #773 implementation conflated two distinct identity functions, shipping the PR with failing CI: - get_canonical_dependency_string() (host-blind canonical for the apm_modules/ filesystem layout + orphan detection) had been made to delegate to the host-qualified get_unique_key(), breaking deps-list orphan/Source matching for non-default-host deps. Restored its host-blind body. - get_unique_key() gated "local" detection on source=="local" while every other method on DependencyReference gates on is_local; a local ref with is_local=True but unset source returned repo_url instead of local_path. Now derives the local signal from is_local, consistent with get_identity()/get_canonical_dependency_string(). - Registry-proxy deps (registry_prefix set, e.g. an Artifactory mirror) were host-qualified, breaking the manifest/lockfile key correspondence used by reinstall + orphan detection. build_dependency_unique_key() now short-circuits to the bare logical key when registry_prefix is set; LockedDependency/DependencyReference pass their prefix through. Added LockedDependency.get_canonical_dependency_string() (host-blind, mirrors DependencyReference) and switched deps-list orphan/Source matching to it so non-default-host deps match their installed dir. Tests: updated lockfile-roundtrip lookups for non-default hosts to the host-qualified key (intended #773 behavior, asserted by the author's own lockfile tests); refreshed deps-list/insecure mocks to stub the new host-blind accessor; fixed auth-phase3/validation dep stubs to set host_type. Full unit suite green (16856 passed); the only remaining local integration failures are the pre-existing test_download_* network artifacts that pass in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e gate Addresses apm-review-panel follow-ups 2 and 4. Extracts the near-identical host-blind canonical-string body shared by DependencyReference and LockedDependency into build_canonical_dependency_string(), mirroring the build_dependency_unique_key() extraction. Each caller passes its own is_local signal (DependencyReference.is_local vs LockedDependency.source == "local") so the two identity models keep their distinct local-detection semantics and the Phase-6 regression fixes are preserved. Also documents the deliberate fail-closed gate in _parse_host_type so the future gitea/bitbucket extension path is a conscious choice rather than a foot-gun. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses apm-review-panel follow-up 3. Replaces the brittle
split()/startswith("https://")/rstrip(".") URL scraping in the
generic-host download-error tests with a shared _error_url_components()
helper that returns parsed (scheme, hostname, path) tuples, per the
tests/ urllib.parse convention. The assertions now survive future
error-message wording or punctuation tweaks.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d410382 to
ac5a4c6
Compare
Shepherd-driver: conflict resolved + panel follow-ups foldedThis PR was Conflict resolutionRebased onto current Folded in this run
DeferredNone within scope. The Lint contract
CIAll required checks green at Mergeability
|
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Multi-host identity split is architecturally clean; defensive getattr access on a declared field is the main smell. |
| CLI Logging Expert | 0 | 0 | 3 | Error messages now include host and endpoint context; only message-format nits remain. |
| DevX UX Expert | 0 | 2 | 2 | Lockfile migration is invisible and type: gitlab is discoverable; generic-host error recovery could guide users better. |
| Supply Chain Security Expert | 0 | 2 | 2 | Token containment is stronger and identity collisions are resolved; lockfile host_type validation would harden reads. |
| OSS Growth Hacker | 0 | 0 | 1 | Docs and error UX are strong multi-host adoption signals; CHANGELOG density is the polish gap. |
| Auth Expert | 0 | 1 | 2 | Token precedence and credential containment are correct; no auth bypass or regression found. |
| Doc Writer | 0 | 2 | 1 | Docs are accurate and discoverable; two single-source-of-truth tidy-ups remain. |
| Test Coverage Expert | 0 | 1 | 1 | Critical lockfile, download-error, and GitLab-routing paths have traps; direct host_type unit coverage is missing. |
| Performance Expert | 0 | 2 | 1 | No wall-time regression; non-404 early exit is a net improvement; duplicated auth resolution is optional cleanup. |
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 unit tests for
classify_host(host_type="gitlab")and unsupportedhost_typeValueError-- missing guardrail on a secure-by-default routing decision; cheap to add, high regression-trap value. - [Supply Chain Security Expert] Validate
host_typeagainst the allowlist inLockedDependency.from_dict()-- crafted lockfiles with unknownhost_typefail later today; earlier validation gives clearer lockfile-specific errors. - [DevX UX Expert] Add a recovery hint for generic-host 401/403 when no credentials were sent -- private generic-host users need to know which explicit signal opts into credential flow.
- [Python Architect] Replace
getattr(dep_ref, "host_type", None)with direct attribute access where typed -- repeated defensive access on a declared field masks type-contract errors and weakens static analysis. - [Doc Writer] Cross-reference the authentication page from
manage-dependencies.mdinstead of re-stating the token chain -- duplicated token-order prose can drift as host support expands.
Architecture
classDiagram
direction LR
class DependencyReference {
<<Dataclass>>
+repo_url str
+host str or None
+host_type str or None
+port int or None
+get_unique_key() str
+get_canonical_dependency_string() str
}
class LockedDependency {
<<Dataclass>>
+repo_url str
+host str or None
+host_type str or None
+port int or None
+registry_prefix str or None
+get_unique_key() str
+get_canonical_dependency_string() str
}
class AuthResolver {
<<Strategy>>
+classify_host(host, port, host_type) HostInfo
+resolve(host, org, port, host_type) AuthContext
+resolve_for_dep(dep_ref) AuthContext
}
class HostInfo {
<<ValueObject>>
+host str
+kind str
+api_base str
+port int or None
}
class DownloadStrategies {
+download_github_file(dep_ref)
+download_gitlab_file(dep_ref)
}
class GitHubDownloader {
+_resolve_dep_token(dep_ref)
+_resolve_dep_auth_ctx(dep_ref)
+_is_generic_dependency_host(dep_ref) bool
}
DependencyReference ..> AuthResolver : host_type input
LockedDependency ..> DependencyReference : round trip
DownloadStrategies ..> AuthResolver : resolves tokens
GitHubDownloader *-- DownloadStrategies : composition
flowchart TD
A[apm install or deps list] --> B[DependencyReference]
B --> C{type gitlab?}
C -->|yes| D[host_type = gitlab]
C -->|no| E[host_type unset]
D --> F[build_dependency_unique_key]
E --> F
F --> G{host is github.com or unset?}
G -->|yes| H[key = owner/repo]
G -->|no| I[key = host/owner/repo]
H --> J[apm.lock.yaml]
I --> J
B --> K[AuthResolver.resolve_for_dep]
K --> L[classify_host with host_type]
L --> M{GitLab-class host?}
M -->|yes| N[GitLab REST plus GitLab token chain]
M -->|no| O[Generic raw or API attempts without managed PAT]
B --> P[build_canonical_dependency_string]
P --> Q[host-blind deps-list/orphan matching]
Recommendation
The PR is ready to land with follow-ups. No panelist found a release-stopping defect; security posture is net-positive and lockfile migration is invisible to existing users. The highest-signal follow-up is adding unit tests for classify_host with host_type; track that and lockfile deserialization validation as immediate follow-up work. The remaining items are housekeeping debt that can ride the next cycle.
Full per-persona findings
Python Architect
- [recommended]
getattr(dep_ref, 'host_type', None)used repeatedly for a declared dataclass field atsrc/apm_cli/deps/host_backends.py:566
host_typeis declared onDependencyReferenceandLockedDependency. Defensive access hides type errors that static analysis could catch.
Suggested: Replace withdep_ref.host_typewhere typed, ordep_ref.host_type if dep_ref is not None else Nonefor optional refs. - [nit] Auth cache key tuple grew without a named shape at
src/apm_cli/core/auth.py:393
The cache key is now(host, port, host_type, org). A named tuple or frozen dataclass would be more self-documenting if this grows again.
CLI Logging Expert
- [nit] Download error helpers return trailing punctuation at
src/apm_cli/deps/download_strategies.py
Helper punctuation limits composability if future callers append context.
Suggested: Drop trailing periods from helper return strings and let callers terminate. - [nit] Network error format uses
type(error).__name__, which can be opaque for nested urllib3 exceptions atsrc/apm_cli/deps/download_strategies.py
A one-line first line fromstr(error)would improve verbose diagnosability. - [nit] Verbose Contents API breadcrumb now fires for
github.comatsrc/apm_cli/deps/download_strategies.py:819
More complete verbose output, but noisier for manifests with many GitHub deps.
DevX UX Expert
- [recommended] Error messages include raw URLs that expose internal implementation endpoints at
src/apm_cli/deps/download_strategies.py
Default errors should prioritize what failed, why, and what to do next; endpoint URLs are often better in verbose output.
Suggested: Use a concise default summary and reserve URL/endpoint details for verbose output. - [recommended] No guidance in the error path when a private generic-host dependency fails unauthenticated at
src/apm_cli/deps/download_strategies.py
Generic hosts no longer receive PAT headers. A 401/403 should explain that credentials were not sent and name the opt-in path.
Suggested: Add a recovery hint for generic-host 401/403 without Authorization. - [nit]
type: gitlabhas docs discoverability but no CLI discoverability
Future install recovery or scaffolding could surface this signal. - [nit] Unsupported
typevalue error could list accepted values atsrc/apm_cli/models/dependency/reference.py
Package-manager errors are easier to fix when they include valid options.
Supply Chain Security Expert
- [recommended]
host_typeis not validated when reading lockfiles atsrc/apm_cli/deps/lockfile.py
LockedDependency.from_dict()accepts the raw value and relies on later classification to fail closed.
Suggested: Validate against{None, "gitlab"}during deserialization. - [recommended] Auth cache key includes
host_type; future allowlist expansion should keep cardinality bounded atsrc/apm_cli/core/auth.py:393
Current parser rejects unknown values, so this is not exploitable today. - [nit] Download errors include full URLs, which future host integrations should keep token-free at
src/apm_cli/deps/download_strategies.py
APM sends tokens in headers today, so this is safe now. - [nit] Defensive
getattr(dep_ref, 'host_type', None)masks type-contract errors atsrc/apm_cli/deps/host_backends.py
The field is now first-class.
OSS Growth Hacker
- [nit] CHANGELOG entry packs several user-visible changes into one sentence at
CHANGELOG.md:24
Splitting lockfile identity, PAT containment, and error surfacing into separate bullets would improve upgrade scanning.
Suggested: Split into one bullet per user-visible behavior.
Auth Expert
- [nit]
classify_hostcould use lowercased host when building self-managed GitLabapi_baseatsrc/apm_cli/core/auth.py:270
DNS is case-insensitive, so this is harmless; usinghwould be defensive. - [nit] Duplicate
classify_hostcalls occur across generic-host checks and auth resolution atsrc/apm_cli/deps/github_downloader.py:409
Cached and not correctness-affecting, but future cleanup could reduce noise. - [recommended] Defensive
getattr(dep_ref, 'host_type', None)is used next to directdep_ref.portaccess atsrc/apm_cli/deps/download_strategies.py:636
This signals unclear interface boundaries.
Suggested: Usedep_ref.host_typedirectly where typed, or comment why duck-typing is intentional.
Doc Writer
- [recommended]
manage-dependencies.mdre-states the GitLab token chain instead of cross-referencing the authentication page atdocs/src/content/docs/consumer/manage-dependencies.md
Duplicated token-order prose can drift.
Suggested: Link to the GitLab-class hosts section instead of listing token order inline. - [recommended]
lockfile-specidentity section omits the registry-proxy short-circuit atdocs/src/content/docs/reference/lockfile-spec.md
build_dependency_unique_key()returns the bare key whenregistry_prefixis set; the docs should state that exception.
Suggested: Add one sentence that registry-proxy entries keep the bare logical key. - [nit] The new
type: gitlabparagraph packs several rules into one dense block atdocs/src/content/docs/consumer/manage-dependencies.md
Splitting usage, routing, and private-read caveats would improve scanability.
Test Coverage Expert
- [recommended] No unit test exercises
classify_host()with the newhost_typeparameter or its error path atsrc/apm_cli/core/auth.py:278
grep found noclassify_host.*host_typetests. The downloader path covers happy routing, but the direct contract lacks unit guardrails.
Suggested: AddTestClassifyHosttests forhost_type="gitlab"and unsupportedhost_type.
Proof (missing at):tests/unit/test_auth.py::TestClassifyHost::test_host_type_gitlab_reclassifies_bespoke_host-- proves: Explicit host_type hint routes a bespoke hostname to GitLab classification without env vars [secure-by-default,vendor-neutral]
assert AuthResolver.classify_host('code.acme.com', host_type='gitlab').kind == 'gitlab' - [nit]
resolve_for_dephas no test verifyinghost_typeis forwarded toclassify_hostattests/unit/core/test_auth_phase3.py:296
Downloader integration covers this indirectly, but a symmetric unit assertion would catch future refactors.
Suggested: Addtest_resolve_for_dep_threads_host_type.
Proof (missing at):tests/unit/core/test_auth_phase3.py::TestResolveForDep::test_resolve_for_dep_threads_host_type-- proves: host_type on DependencyReference flows through resolve_for_dep into host classification [secure-by-default]
dep.host_type = 'gitlab'; ctx = resolver.resolve_for_dep(dep); assert ctx.host_info.kind == 'gitlab'
Performance Expert
- [recommended]
_resolve_dep_tokenplus_resolve_dep_auth_ctxcan repeat auth resolution for non-generic hosts atsrc/apm_cli/deps/download_strategies.py:748
Wall-time cost is negligible but avoidable.
Suggested: Call_resolve_dep_auth_ctxonce and derivetokenfromctx.token. - [recommended] Auth cache key now includes
host_type; verify no unintended fragmentation atsrc/apm_cli/core/auth.py:393
gitlab.comwith and without explicit host_type could create two equivalent cache entries. - [nit] Non-404 early exit on raw URL is a net performance win
The new behavior avoids a wasted Contents API request after non-404 raw failures.
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 | 0 | 0 | Clean host-type threading with shared identity helpers and a named auth cache key; ship. |
| CLI Logging Expert | 0 | 0 | 0 | Error helpers and recovery messages are structured, consistent, and verbose-gated. |
| DevX UX Expert | 0 | 0 | 0 | type: gitlab and generic-host recovery are discoverable and actionable. |
| Supply Chain Security Expert | 0 | 0 | 0 | host_type allowlists fail closed; generic-host PAT denial and bare-PAT redaction are correctly wired. |
| OSS Growth Hacker | 0 | 0 | 0 | CHANGELOG leads with the no-collision user outcome; multi-host support strengthens positioning. |
| Auth Expert | 0 | 0 | 0 | AuthResolver routing, cache discrimination, and token precedence are sound. |
| Doc Writer | 0 | 0 | 0 | Docs and apm-guide resources match the implementation; cross-links and anchors resolve. |
| Test Coverage Expert | 0 | 0 | 0 | Critical surfaces have regression traps, including integration coverage for type: gitlab. |
| Performance Expert | 0 | 0 | 0 | No hot-path regression; host_type dispatch is local and cache cardinality is bounded. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class AuthResolver {
<<Strategy>>
+classify_host(host, port, host_type) HostInfo
+resolve(host, org, port, host_type) AuthContext
+resolve_for_dep(dep_ref) AuthContext
}
class AuthCacheKey {
<<ValueObject>>
+host str | None
+port int | None
+host_type str
+org str
}
class DependencyReference {
+host_type str | None
+get_unique_key() str
+get_canonical_dependency_string() str
}
class LockedDependency {
+host_type str | None
+get_unique_key() str
+get_canonical_dependency_string() str
}
class DownloadDelegate {
+download_github_file() bytes
+download_gitlab_file() bytes
}
AuthResolver *-- AuthCacheKey : indexes
AuthResolver ..> DependencyReference : resolve_for_dep
DependencyReference ..> LockedDependency : persists
DownloadDelegate ..> AuthResolver : resolves auth
flowchart TD
A[apm.yml dependency] --> B[DependencyReference.parse_from_dict]
B --> C{type: gitlab?}
C -->|yes| D[host_type = gitlab]
C -->|no| E[host_type = none]
D --> F[build_dependency_unique_key]
E --> F
F --> G{github.com or unset host?}
G -->|yes| H[bare owner/repo key]
G -->|no| I[host/owner/repo key]
D --> J[AuthResolver.resolve_for_dep]
J --> K[classify_host with host_type]
K --> L{GitLab kind?}
L -->|yes| M[GitLab REST/token chain]
L -->|no| N[generic path with no managed PAT]
H --> O[apm.lock.yaml]
I --> O
Recommendation
Ship now. All foldable review findings have been folded into this PR, CI is green, and no in-scope follow-ups remain. The separate lockfile refactor work in #1738 / #1209 stays outside this PR's scope.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert
No findings.
Doc Writer
No findings.
Test Coverage Expert
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.
…ges (#1757) * chore: release v0.20.0 Bump pyproject.toml + uv.lock to 0.20.0 and roll the [Unreleased] CHANGELOG block into [0.20.0] - 2026-06-11. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.20.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(integration): refresh stale integration tests after v0.20.0 changes The release integration suite (which gates releases, not per-PR runs) surfaced 29 failures after v0.20.0 was tagged. Each was a stale test that had not been updated to match an intentional source behavior change merged this cycle. Source is correct throughout; only tests change here. Clusters and causing PRs: - A (#1735): download_github_file now calls _host._resolve_dep_auth_ctx; mock host helpers never stubbed it, forcing the wrong auth path. Added the stub to make_host/_make_host in 4 download test files. Also #1735 made generic raw-URL network errors raise RuntimeError instead of falling back to the Contents API -- rewrote 2 stale fallback tests to assert the raise. - B (#1742): apm compile -t copilot suppresses empty AGENTS.md shells when .github/instructions/*.md exists. Flipped 3 assertions to expect no AGENTS.md and assert the instruction files are present. - C (#1739): marketplace fetches moved to a shared requests.Session (_HTTP_SESSION.get); updated 6 tests to mock that seam. - D (#1734): optional registry env inputs are omitted without an override; updated 2 placeholder tests to assert the var is absent. - E (#1734): same omit-optional change; assert var not in result. - F (#1720): tar symlink rejection wording changed; updated the regex. Verified locally: 738 passed, 16 skipped (token-gated e2e). The two runnable cluster-B e2e tests (mixed_deps, guardrailing) pass against the v0.20.0 build; ado_e2e is identical logic, validated in CI. Full lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r changes (#1758) * chore: release v0.20.0 Bump pyproject.toml + uv.lock to 0.20.0 and roll the [Unreleased] CHANGELOG block into [0.20.0] - 2026-06-11. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.20.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(integration): refresh stale integration tests after v0.20.0 changes The release integration suite (which gates releases, not per-PR runs) surfaced 29 failures after v0.20.0 was tagged. Each was a stale test that had not been updated to match an intentional source behavior change merged this cycle. Source is correct throughout; only tests change here. Clusters and causing PRs: - A (#1735): download_github_file now calls _host._resolve_dep_auth_ctx; mock host helpers never stubbed it, forcing the wrong auth path. Added the stub to make_host/_make_host in 4 download test files. Also #1735 made generic raw-URL network errors raise RuntimeError instead of falling back to the Contents API -- rewrote 2 stale fallback tests to assert the raise. - B (#1742): apm compile -t copilot suppresses empty AGENTS.md shells when .github/instructions/*.md exists. Flipped 3 assertions to expect no AGENTS.md and assert the instruction files are present. - C (#1739): marketplace fetches moved to a shared requests.Session (_HTTP_SESSION.get); updated 6 tests to mock that seam. - D (#1734): optional registry env inputs are omitted without an override; updated 2 placeholder tests to assert the var is absent. - E (#1734): same omit-optional change; assert var not in result. - F (#1720): tar symlink rejection wording changed; updated the regex. Verified locally: 738 passed, 16 skipped (token-gated e2e). The two runnable cluster-B e2e tests (mixed_deps, guardrailing) pass against the v0.20.0 build; ado_e2e is identical logic, validated in CI. Full lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(release): refresh release-validation harness for v0.20.0 behavior changes The release-validation shell harness carries its own copies of behavior assertions that duplicate the integration suite. Two of them went stale this cycle from the same PRs that broke the integration tests (#1757): - GH-AW compat (#1720): `apm pack --archive` now emits .zip by default; the archive check grepped only `build/*.tar.gz`. Accept either extension, testing each glob independently (a single `ls a b` exits non-zero when either pattern is unmatched, even if the other matches). - Hero scenario 2 / AGENTS.md (#1742): copilot `apm compile` omits the empty AGENTS.md shell when installed instructions already live under `.github/instructions/`. The check insisted AGENTS.md exist; now accept AGENTS.md OR a populated `.github/instructions/`, mirroring the merged pytest fix in test_guardrailing_hero_e2e.py. Same fixes applied to the Windows .ps1 (AGENTS.md only; it has no archive check). Predicates validated locally against apm v0.20.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
fix(deps): support multi-host dependency identity
TL;DR
This PR implements the board-approved #773 items 1-4: host-aware lockfile identity, consistent per-dependency token resolution for file downloads, explicit GitLab host signaling, and clearer download-attempt errors. Existing
github.comlockfile keys remain byte-stable as bareowner/repo; only non-default hosts add the host segment. Closes #773.Important
Lockfile migration note: existing
github.comkeys are preserved as-is. New non-default host entries usehost/owner/repo, sogithub.com/team/skillsandgitea.myorg.com/team/skillscan coexist without colliding.Problem (WHY)
get_unique_key()returned the bare repo path, so two hosts with the same path could "collide silently inapm.lock"._download_github_fileresolved tokens differently from clone, causing "misleading 401 errors when a GitHub PAT is sent to a Gitea / GitLab server".code.acme.com.Approach (WHAT)
build_dependency_unique_key()keepsgithub.comimplicit and prefixes only non-default hosts._resolve_dep_token()/_resolve_dep_auth_ctx(), matching clone boundaries.type: gitlab; the hint flows through auth, backends, validation, and lockfile replay.apm-usageguide documenttype: gitlaband the lockfile key migration rule.Implementation (HOW)
src/apm_cli/models/dependency/reference.py,src/apm_cli/deps/lockfile.pyhost_typeparse/round-trip support.src/apm_cli/core/auth.py,src/apm_cli/deps/host_backends.pysrc/apm_cli/deps/download_strategies.py,src/apm_cli/deps/github_downloader.pytype: gitlabto GitLab REST, and surfaced endpoint-specific errors.src/apm_cli/install/validation.py,src/apm_cli/deps/github_downloader_validation.pytests/**CHANGELOG.mdDiagram
The diagram shows the new dependency flow from manifest identity to host-aware lockfile key and download backend.
flowchart LR manifest[Manifest dependency] --> ref[DependencyReference] ref --> key{Host value} key -->|github.com or unset| ghkey[Lock key owner/repo] key -->|non-default host| hostkey[Lock key host/owner/repo] ref --> type{Explicit type gitlab} type -->|yes| gitlab[GitLab REST and GitLab token chain] type -->|no| generic[Generic raw/API attempts without managed PAT]Trade-offs
github.comentries, but non-default hosts now become visible in the in-memory key.type: gitlabis explicit instead of heuristic or probe-based, avoiding network capability detection in manifest parsing.Benefits
owner/repoon two hosts no longer overwrites a lockfile entry.Validation
Scenario evidence
github.comremains implicittests/test_lockfile.pytests/test_github_downloader.py::TestGiteaRawUrlDownload,tests/unit/deps/test_download_strategies_*tests/test_github_downloader.py::TestGiteaGogsApiVersionNegotiation::test_object_form_type_gitlab_routes_bespoke_host_to_gitlab_apitests/test_github_downloader.py::TestGiteaRawUrlDownloadCommands run
How to test
apm.ymlwithgithub.com/team/skillsandgitea.myorg.com/team/skills, runapm install, and confirm both lockfile entries remain present.apm install --verbose; confirm raw/API attempts are logged and no GitHub PAT header is sent.type: gitlabfor a bespoke hostname and confirm GitLab REST URLs are used.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com