feat(install): route CLI shorthands to default registry when configured#1816
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns apm install owner/repo#ref behavior with manifest routing when a default registry is configured: it bypasses the GitHub probe, validates/defers ref handling appropriately, and updates resolver/drift/outdated logic to treat non-semver selectors as exact-match pins.
Changes:
- Add CLI-side default-registry routing bypass (with ref validation) and shared routing predicate (
routes_unscoped_to_registry). - Update registry resolver to exact-match non-semver selectors, reject malformed range-like refs clearly, and fix drift/outdated semantics for non-semver pins.
- Expand unit coverage and update docs to describe the new routing + selector rules.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/commands/install.py |
Adds default-registry-aware probe-skip path and wires effective default registry into resolution. |
src/apm_cli/install/registry_wiring.py |
Introduces helpers for effective default registry detection, probe skipping, and CLI ref validation. |
src/apm_cli/models/apm_package.py |
Extracts shared routing predicate and clarifies routing behavior for malformed refs. |
src/apm_cli/deps/registry/resolver.py |
Implements exact-match behavior for non-semver selectors and clearer errors for malformed range-like refs. |
src/apm_cli/drift.py |
Adds literal-equality fallback for non-semver registry refs to prevent perpetual drift. |
src/apm_cli/deps/registry/outdated.py |
Distinguishes “no selector” vs “invalid range” vs “pinned ref” for registry deps. |
tests/unit/install/test_registry_wiring.py |
Adds unit tests for the new registry_wiring helpers. |
tests/unit/commands/test_install_resolve_refs.py |
Adds regression coverage ensuring marketplace provenance is recorded on the probe-skip path. |
tests/unit/test_apm_yml_registries.py |
Adds/adjusts manifest routing tests for non-semver and malformed selectors. |
tests/unit/test_drift_registry.py |
Adds drift tests for non-semver selector equality vs mismatch. |
tests/unit/registry/test_resolver.py |
Adds resolver tests for exact-match non-semver selectors and malformed-range rejection. |
tests/unit/registry/test_outdated.py |
Adds outdated labeling tests for pinned non-semver refs. |
tests/unit/install/test_architecture_invariants.py |
Updates install.py LOC budget notes for new glue. |
docs/src/content/docs/reference/cli/install.md |
Documents default-registry routing behavior for CLI installs. |
docs/src/content/docs/reference/cli/outdated.md |
Documents new registry (...) source labels. |
docs/src/content/docs/guides/registries.md |
Updates registry guide to reflect exact-match non-semver selector support and new rejection cases. |
packages/apm-guide/.apm/skills/apm-usage/commands.md |
Updates usage guidance for default-registry routing and selector semantics. |
f824b88 to
851f3e1
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 3 | Module shape is sound; 1 recommended (broad exception catch silently eats config errors) and 3 nits. |
| CLI Logging Expert | 0 | 1 | 1 | No blocking issues; validate_registry_ref error leaks routing jargon and buries the fix. |
| DevX UX Expert | 0 | 2 | 3 | Help text example misleads GitHub-first users; object-form version field description stale; error recovery missing copy-paste YAML fragment. |
| Supply Chain Security Expert | 0 | 2 | 1 | Probe-skip is semantically sound; two transparency gaps: non-semver refs silently shadow git, user-config reroute leaves no audit trail. |
| OSS Growth Hacker | 0 | 2 | 3 | Solid enterprise registry UX milestone; missing CHANGELOG entry for 4 user-visible behavior changes including a CI/CD drift fix worth its own story beat. |
| Auth Expert | 0 | 0 | 2 | Bypass is architecturally correct: GitHub probe is meaningless for registry deps; registry auth enforced at download time; no exploitable hole found. |
| Doc Writer | 0 | 2 | 3 | Docs accurately cover new CLI routing and non-semver allowance; four gaps: CHANGELOG absent, version-table label ambiguous, drift-fix implicit, example YAML omits non-semver case. |
| Test Coverage Expert | 0 | 3 | 0 | All 10 behavioral changes have unit coverage; probe-bypass, error-cascade, and non-semver drift fix each lack integration-with-fixtures confirmation. |
| Performance Expert | 0 | 1 | 4 | Probe-skip saves 100-500ms per registry dep. No blocking regressions. Three nits: extra is_semver_range on semver drift path, wasted version_strings alloc in non-semver success path, uncached ~/.apm/apm.yml read. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [OSS Growth Hacker] Add CHANGELOG entries for all four user-visible behavior changes: shorthand routing to default registry, non-semver labels as valid registry selectors, the perpetual re-download drift fix, and the new
apm outdatedsource labels. -- Upgraders have no signal that the CI/CD drift fix arrived; the CHANGELOG is the community's durable trust record and this gap compounds with each release cycle. - [DevX UX Expert] Restore
apm install org/pkg1as the primary help-text example and add the registry case (apm install org/pkg1#1.0.0) as a secondary example line. -- Promoting the registry form as the sole example implies a version selector is always required, which is false for GitHub-path users who remain the majority audience of the install command. - [Supply Chain Security Expert] Emit a one-line
_rich_warningwhen a non-semver ref is exact-matched against registry versions, and a_rich_infoline when the default registry is resolved from user config rather than projectapm.yml. -- Both silent routing paths are transparency gaps that undercut user trust and create real footguns on shared or compromised machines. - [Test Coverage Expert] Add integration tests for three unconfirmed cross-module flows: probe-skip install pipeline (outcome=missing, portability-by-manifest), error-path cascade (missing ref + default registry exits 1), and non-semver pin idempotency (no re-download on repeat install). -- All three carry outcome=missing evidence on portability-by-manifest or devx surfaces; without integration fixtures these promises survive refactors only by luck, not by automated guardrail.
- [CLI Logging Expert] Tighten the
validate_registry_referror message to the primary signal only, moving thegit:escape-hatch alternative behind--verbose. -- The current error leaks internal routing jargon and appends a power-user alternative that fails the signal-to-noise test for the majority of users who just need the primary actionable fix.
Architecture
classDiagram
direction TB
class apm_package {
<<Module>>
+routes_unscoped_to_registry(dep) bool
~_route_unscoped_to_default_registry(dep_list, reg) None
}
class registry_wiring {
<<Module>>
+get_effective_default_registry(data) str
+should_skip_github_probe_for_dep(dep_ref, registry) bool
+validate_registry_ref(dep_ref) tuple
}
class InstallCommand {
<<Module>>
~_resolve_package_references(pkgs, default_registry)
~_validate_and_add_packages_to_apm_yml()
}
class RegistryPackageResolver {
<<Strategy>>
~_pick_version(dep_ref, versions) VersionEntry
+download_package(repo_ref, target_path) PackageInfo
}
class DependencyReference {
<<ValueObject>>
+source str
+is_local bool
+reference str
+registry_name str
+ref_kind str
}
class identity_module {
<<Module>>
+InvalidSemverRangeError
~_looks_like_invalid_semver_range(spec) bool
}
class drift {
<<Module>>
+detect_ref_change(dep_ref, locked_dep) bool
}
class cache_pin {
<<Module>>
+find_unpinned_remote_deps(lockfile) list
~_is_markable(dep) bool
}
InstallCommand ..> registry_wiring : lazy-imports 3 helpers
registry_wiring ..> apm_package : routes_unscoped_to_registry
registry_wiring ..> identity_module : InvalidSemverRangeError
RegistryPackageResolver ..> identity_module : _looks_like_invalid_semver_range
apm_package *-- DependencyReference : creates
InstallCommand ..> DependencyReference : reads
drift ..> DependencyReference : reads
cache_pin ..> DependencyReference : reads source
classDef touched fill:#fff3b0,stroke:#d47600
class apm_package:::touched
class registry_wiring:::touched
class InstallCommand:::touched
class RegistryPackageResolver:::touched
class drift:::touched
class cache_pin:::touched
flowchart TD
A(["apm install owner/repo#ref"])
B["_validate_and_add_packages_to_apm_yml\ncommands/install.py"]
C["load_yaml apm.yml"]
D["get_effective_default_registry\ninstall/registry_wiring.py"]
E{"feature gate\nenabled?"}
F{"registries.default\nin apm.yml?"}
G["resolve_effective_registries\nuser ~/.apm/config.json"]
I["default_registry = name or None"]
J["_resolve_package_references\ndefault_registry=..."]
K["DependencyReference.parse"]
L["should_skip_github_probe_for_dep\nregistry_wiring.py"]
M{"default_registry\nis None?"}
N["routes_unscoped_to_registry\napm_package.py"]
O{"source not git or registry\nand not is_local?"}
P["skip = True"]
Q["validate_registry_ref\nregistry_wiring.py"]
R{"reference\npresent?"}
S["dep_ref.ref_kind -- raises\nInvalidSemverRangeError?"]
T["invalid_outcomes.append\nerror"]
U["package_accessible = True"]
V["_validate_package_exists\nGitHub API probe"]
W["persist_dependency_list\nwrite apm.yml"]
A --> B --> C --> D --> E
E -- disabled --> I
E -- enabled --> F
F -- yes --> I
F -- no --> G --> I
I --> J --> K --> L --> M
M -- None --> V
M -- not None --> N --> O
O -- no --> V
O -- yes --> P --> Q --> R
R -- absent --> T
R -- present --> S
S -- raises --> T
S -- ok --> U
U --> W
V --> W
sequenceDiagram
participant CLI as commands/install.py
participant RW as install/registry_wiring.py
participant AP as models/apm_package.py
participant GH as GitHub API
CLI->>RW: get_effective_default_registry(data)
Note over RW: feature gate + project apm.yml + user config.json
RW-->>CLI: default_registry = corp-main
loop for each package CLI arg
CLI->>RW: should_skip_github_probe_for_dep(dep_ref, corp-main)
RW->>AP: routes_unscoped_to_registry(dep_ref)
AP-->>RW: True
RW-->>CLI: skip=True
alt Registry path -- skip probe
CLI->>RW: validate_registry_ref(dep_ref)
RW-->>CLI: (True, empty) ref is valid
Note over CLI: package_accessible = True
else Normal path
CLI->>GH: GET /repos/owner/repo
GH-->>CLI: 200 OK or 404
end
end
Note over CLI: persist_dependency_list_if_changed
Recommendation
The core implementation is architecturally sound: auth is clean, the probe-skip is a confirmed net positive, and all 10 behavioral changes have unit coverage. Address the CHANGELOG gap and the help-text example regression before merge -- both are one-commit fixes with outsized community impact. The remaining followups (silent-routing transparency warnings, integration test fixtures, and error-message ergonomics) are real improvements that belong in a fast-follow or the next sprint but do not gate a feature that is already correct, well-covered at unit tier, and ready to anchor the enterprise registry story.
Full per-persona findings
Python Architect
-
[recommended]
get_effective_default_registryswallows all exceptions from user-config path, silently disabling routing on malformed config atsrc/apm_cli/install/registry_wiring.py
The innerexcept Exceptionaroundresolve_effective_registries({}, None)catches TypeError, AttributeError, json.JSONDecodeError from a corrupt~/.apm/config.json. User gets None with no diagnostic. Narrow to(ImportError, OSError, ValueError)and add_rich_warningfor unexpected errors.
Suggested: Replace bareexcept Exception: return Nonewith typed catches and_rich_warning(f'registry config load failed: {exc}')for unexpected errors. -
[nit]
_looks_like_invalid_semver_rangeis a private symbol now imported by 3 modules; promote to public or re-export viasemver.pyatsrc/apm_cli/deps/registry/resolver.py
Three cross-site consumers make this de-facto public API under a private name.semver.pyis the natural re-export point. -
[nit]
routes_unscoped_to_registrygetattr(dep, 'is_local', True)default is opposite toDependencyReference.is_local=False; add intent comment atsrc/apm_cli/models/apm_package.py
Suggested:getattr(dep, 'is_local', True) # fail-safe: attribute-less objects treated as local, never routed to registry -
[nit]
validate_registry_refreuses git-routingref_kindproperty to detect malformed semver without an explanatory comment atsrc/apm_cli/install/registry_wiring.py
Suggested:# Accesses ref_kind solely to trigger InvalidSemverRangeError for malformed range selectors; non-semver literals pass through per API spec sec 1.3.
CLI Logging Expert
-
[recommended]
validate_registry_referror exposes internal routing concept and buries the fix with a niche secondary alternative atsrc/apm_cli/install/registry_wiring.py
"would route to the default registry" is pipeline jargon; thegit:URL escape hatch is power-user noise for the 95% case.
Suggested: Tighten to: 'version selector required for registry packages: no "#version" found. Add a version (e.g. {hint})'. Movegit:escape hatch behind--verbose. -
[nit]
InvalidSemverRangeErrormessage says "ref field" (YAML key jargon) but user supplied a CLI#versionselector atsrc/apm_cli/install/registry_wiring.py
Suggested: Wrapstr(exc)to: "malformed version selector '^1.0': expected a valid semver range (e.g. '^1.2.0' or '1.0.0')"
DevX UX Expert
-
[recommended] Help text example changed from
apm install org/pkg1toapm install org/pkg1#1.0.0, implying #version is always required -- false for GitHub-path users atsrc/apm_cli/commands/install.py
Suggested: Restoreapm install org/pkg1as primary; addapm install org/pkg1#1.0.0 # registry-routed package (requires default registry)as secondary. -
[recommended] Object-form
versionfield says "Exact version or semver range" only -- stale now non-semver selectors are valid atdocs/src/content/docs/guides/registries.md
Suggested: Add "or non-semver label (main, stable) for exact-match against the registry catalogue." -
[nit] No-version-selector error missing a copy-pasteable YAML fragment at
src/apm_cli/install/registry_wiring.py -
[nit] No at-install-time CLI signal when existing git shorthand deps silently reroute to registry at
src/apm_cli/commands/install.py -
[nit]
apm outdatedstatus "unknown" for "registry (pinned ref)" reads as error, not intentional pin atsrc/apm_cli/deps/registry/outdated.py
Suggested: Usestatus: pinnedorstatus: exact-ref.
Supply Chain Security Expert
-
[recommended] Non-semver ref (e.g.
#main) becomes silent registry version selector with no user-facing notice atsrc/apm_cli/install/registry_wiring.py
A registry publisher can publish a version named "main", "HEAD", "latest" and intercept users who previously used that shorthand as a git ref.
Suggested: When reference is non-semver, emit: "note: '#main' is not a semver selector; it will be exact-matched against registry versions, not used as a git ref." -
[recommended] User-level registry default silently reroutes all unscoped GitHub shorthands with no install-time notice at
src/apm_cli/install/registry_wiring.py
A compromised~/.apm/config.jsonwould silently reroute all installs to an attacker-controlled registry.
Suggested: When default_registry resolves from user config, emit: "routing unscoped shorthands to registry 'name' (user config default)". -
[nit] Bare
except Exception: return Noneswallows config errors silently atsrc/apm_cli/install/registry_wiring.py
Suggested: Add debug-level logging:logging.getLogger(__name__).debug('registry default unavailable: %s', exc)
OSS Growth Hacker
-
[recommended] No CHANGELOG entry for any of the 4 user-visible behavior changes at
CHANGELOG.md -
[recommended] Pitfall section in registries.md does not mention that CLI
apm install owner/repo#refis also silently rerouted atdocs/src/content/docs/guides/registries.md -
[nit] No-version-selector exit-1 behavior not mentioned in registries.md pitfall section
-
[nit] PR description has no user-facing "What does this mean?" paragraph; all 8 test plan checkboxes unchecked on a non-draft PR
Auth Expert
-
[nit] User-level default resolves against empty project map; project-only registry names intentionally excluded but undocumented at
src/apm_cli/install/registry_wiring.py -
[nit]
is_local=Truedefault is conservative but undocumented in function docstring atsrc/apm_cli/models/apm_package.py
Suggested: Append: "Objects that lack anis_localattribute are treated as local (returns False) -- fail-closed for unknown dep types."
Doc Writer
-
[recommended] No CHANGELOG entry for three behavioral changes (convergent with oss-growth-hacker) at
CHANGELOG.md -
[recommended]
registry (pinned ref)row in outdated.md does not mention the drift bug fix atdocs/src/content/docs/reference/cli/outdated.md
Suggested: "Previously, such deps reported perpetual 'outdated'; they now report 'unknown' since no higher version can be inferred from a pinned label." -
[nit] Version table row "Exact semver -- range-matched" is contradictory at
docs/src/content/docs/guides/registries.md -
[nit] Example YAML only shows semver selectors; non-semver case unillustrated at
docs/src/content/docs/guides/registries.md -
[nit] Sample outdated output missing all three new source label patterns at
docs/src/content/docs/reference/cli/outdated.md
Test Coverage Expert
-
[recommended] Probe-skip install pipeline covered at unit tier only; no integration test drives the cross-module flow
Proof (missing at integration-with-fixtures):tests/integration/test_registry_install_shorthand_e2e.pydoes not exist -- proves:apm install owner/repo#1.0.0withregistries.defaultroutes to registry, skips GitHub probe, writessource=registrydep [portability-by-manifest, devx] -
[recommended] Error-path cascade (missing ref + default registry -> exit 1) unit-tested on string shape only; no integration test drives the CLI invocation
Proof (missing at integration-with-fixtures): same location -- proves:apm install owner/repowithregistries.defaultexits 1 with "version selector required" [devx] -
[recommended] Non-semver drift bug-fix has correct unit regression trap; install-pipeline idempotency not integration-confirmed
Unit testtest_non_semver_ref_matching_locked_is_not_driftcorrectly captures the bug. Integration tier (no re-download on repeat install) not confirmed.
Proof (missing at integration-with-fixtures):tests/integration/test_registry_drift_idempotency_e2e.pydoes not exist -- proves: repeated install with non-semver pin does not re-download [portability-by-manifest, devx]
Performance Expert
-
[recommended] Probe-skip saves 100-500ms per registry dep -- confirmed net positive. No action needed.
-
[nit] Extra
is_semver_rangecall on semver drift path (~0.3-0.5ms at 100 deps) atsrc/apm_cli/drift.py. Not blocking. -
[nit]
version_stringsalloc wasted in non-semver success path (negligible) atsrc/apm_cli/deps/registry/resolver.py. Not blocking. -
[nit]
get_effective_default_registryfallback reads~/.apm/apm.ymlon every call atsrc/apm_cli/install/registry_wiring.py. Acceptable at current granularity. Add guard comment that this must NOT be moved inside a per-dep loop. -
[nit] Lazy imports inside hot-path functions are noise-level after first call via sys.modules cache.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1816 · sonnet46 22M · ◷
851f3e1 to
18731f0
Compare
|
Panel findings addressed in the latest push. Fixed (recommended):
Fixed (nit):
Not fixed (integration tests, 3x recommended): Not fixed (performance nits, 4x nit): |
When a default registry is configured (project registries.default or registry.<name>.default true in ~/.apm/config.json), plain owner/repo#ref shorthand entries in apm.yml route to that registry instead of probing GitHub. A version selector (#<ref>) is required; omitting it exits 1. Non-semver selectors (stable, main, a branch name, or any opaque string) are exact-matched against the registry's published version list. Use the git: URL form in apm.yml to force the GitHub path. Also fixes registry deps with non-semver version selectors reporting perpetual outdated -- the drift check now uses literal equality for non-semver registry pins rather than range comparison. Also corrects stale tar.gz/tarball refs and publish UX gaps after zip default (#1779): bundles now accept .zip (default) or legacy .tar.gz. Closes #1816 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
18731f0 to
87db95f
Compare
When a default registry is configured (project registries.default or registry.<name>.default true in ~/.apm/config.json), plain owner/repo#ref shorthand entries in apm.yml route to that registry instead of probing GitHub. A version selector (#<ref>) is required; omitting it exits 1. Non-semver selectors (stable, main, a branch name, or any opaque string) are exact-matched against the registry's published version list. Use the git: URL form in apm.yml to force the GitHub path. Also fixes registry deps with non-semver version selectors reporting perpetual outdated -- the drift check now uses literal equality for non-semver registry pins rather than range comparison. Also corrects stale tar.gz/tarball refs and publish UX gaps after zip default (#1779): bundles now accept .zip (default) or legacy .tar.gz. apm-spec-waiver: client-side routing heuristic, no new normative requirement on registries or producers; deferred to spec v0.2 registry-routing section Closes #1816 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
87db95f to
553af64
Compare
When a default registry is configured (project registries.default or registry.<name>.default true in ~/.apm/config.json), plain owner/repo#ref shorthand entries in apm.yml route to that registry instead of probing GitHub. A version selector (#<ref>) is required; omitting it exits 1. Non-semver selectors (stable, main, a branch name, or any opaque string) are exact-matched against the registry's published version list. Use the git: URL form in apm.yml to force the GitHub path. Also fixes registry deps with non-semver version selectors reporting perpetual outdated -- the drift check now uses literal equality for non-semver registry pins rather than range comparison. Also corrects stale tar.gz/tarball refs and publish UX gaps after zip default (#1779): bundles now accept .zip (default) or legacy .tar.gz. apm-spec-waiver: client-side routing heuristic, no new normative requirement on registries or producers; deferred to spec v0.2 registry-routing section Closes #1816 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
553af64 to
d82a68f
Compare
Resolve strangler-fig conflicts in install.py, hook_integrator.py, and builder.py: keep HEAD's relocated structure and fold main's behavioural deltas into the sibling helper modules that now own the code. - #1816 registry-routing: port default_registry param + probe-skip bypass into install/pkg_resolution.py (RULE B seam for update_existing_dependency_entry_if_needed re-exported from commands/install.py). - #1840 cursor hooks "version": 1 top-level default: port into integration/hook_transforms.py + injection in hook_integrator.py. - #1841 pack {name} placeholder fix: port name=entry.name into marketplace/_builder_resolve.py. Fix merge-introduced ruff complexity regressions in #1816 code: extract _registry_manifest_range_or_row (outdated.py PLR0911), _resolve_package_accessibility + _intercept_marketplace_ref (pkg_resolution.py C901/PLR0915). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
When a default registry is configured (
registries.defaultinapm.ymlorregistry.<name>.default truein~/.apm/config.json),apm install owner/repo#refnow routes to that registry instead of probing GitHub -- matching the behavior of the same dep declared inapm.ymldirectly.install.py-- probe-skip bypass added before_validate_package_exists; a missing or malformed version selector is rejected (with a clear error) beforeapm.ymlis writtenregistry_wiring.py-- three new helpers:get_effective_default_registry(gate-first, validates project-default membership),should_skip_github_probe_for_dep,validate_registry_refapm_package.py-- extractroutes_unscoped_to_registryas the single shared routing predicate; defer malformed-ref rejection to resolve time soapm compileandapm outdateddo not abort on a bad refresolver.py--_pick_versionnow exact-matches non-semver selectors (main,stable,v1.4.2) per HTTP API spec §1.3 instead of rejecting themdrift.py-- literal-equality fallback for non-semver registry pins fixes perpetual drift (everyapm installwas re-downloading)outdated.py-- distinguishregistry (no version selector)/registry (invalid manifest range)/registry (pinned ref)source labelsTest plan
apm install owner/repo#1.0.0with default registry set writes a registry dep toapm.yml(no GitHub probe)apm install owner/repo(no#ref) with default registry set exits 1 with "version selector required"apm install owner/repo#^1.0(malformed range) with default registry set exits 1 with "invalid semver range"apm install owner/repo#mainwith default registry set writes a registry dep; resolver exact-matchesmainagainst published versionsapm installon a locked non-semver registry pin (#main) does not re-download when the lockfile already recordsmainapm outdatedshowsregistry (pinned ref)for#main, notregistry (invalid manifest range)uv run python -m pytest tests/unit/🤖 Generated with Claude Code