feat(view): route 'view versions' to the registry when a default is configured#1938
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates apm view <pkg> versions so that an unlocked shorthand (org/repo) routes to the configured default registry (mirroring apm install), and adds an explicit --registry [NAME] override to force the registry path.
Changes:
- Add registry-routing precedence to
display_versions, including default-registry routing for unlocked shorthands and a new--registry [NAME]flag. - Extend
_display_registry_versionsto accept an optionalregistry_nameoverride and surface a clear error for unknown names. - Add unit tests covering default-registry routing, no-default fallback to git, explicit named-registry forcing, and git-URL escape hatch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/apm_cli/commands/view.py |
Adds --registry [NAME], default-registry routing for unlocked shorthands, and registry-name threading into the registry version listing path. |
tests/unit/test_view_command.py |
Adds unit tests validating the new routing behavior and --registry NAME forcing behavior. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 2 | _effective_default_registry duplicates feature-gate logic from registry_wiring with subtly different user-fallback behavior; _pr alias and double-normalization are minor friction. |
| CLI Logging Expert | 0 | 1 | 3 | Silent auto-registry routing emits no [i] message; users cannot tell why they got registry results instead of git refs. |
| DevX UX Expert | 0 | 5 | 1 | --registry silently dropped without versions field, view.md stale, no source disclosure on auto-route. |
| Supply Chain Security | 0 | 0 | 3 | No blocking security findings; routing is read-only, registry URL is config-derived (no SSRF), feature gate fails closed. |
| OSS Growth Hacker | 0 | 2 | 2 | Strong 'it just works' fix for registry users -- CHANGELOG entry missing and view.md stale; add both to capture the conversion signal. |
| Auth Expert | 0 | 1 | 1 | No credential leakage or auth bypass found; one local-path routing gap fires registry auth when no auth should be needed. |
| Doc Writer | 0 | 4 | 1 | reference/cli/view.md needs --registry in Options and updated routing description; CHANGELOG.md missing feat+fix entries. |
| Test Coverage Expert | 0 | 2 | 1 | 2 recommended gaps: --registry UNKNOWN error path untested; no integration-tier test for default-registry routing. |
| Performance Expert | 0 | 1 | 2 | I/O overhead is noise vs the registry network call; recommended: thread _effective_default_registry result to avoid a second LockFile.read. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[doc-writer + oss-growth-hacker] Add CHANGELOG.md [Unreleased] entry for routing fix and
--registryflag; update view.md Options table, routing description, and Examples section -- APM operating principle requires a CHANGELOG entry for every user-facing behavior change. view.md omits--registry [NAME]from Options and its routing description still implies a lockfile entry is required. Two file edits; primary conversion surface for registry-cohort upgraders and the clearest pre-merge gap. -
[auth-expert] Add local-path early-return to
_is_explicit_git_form(guard on./,../,/prefixes ordep_ref.is_local) --apm view ./local-pkg versionswith a default registry silently dispatches registry credentials for a nonsensical query.dep_ref.is_localis available; the fix is one line and closes a real auth-misfire gap before this reaches production registry users. -
[cli-logging-expert + devx-ux-expert + performance-expert] Capture
_effective_default_registryresult and thread it into_display_registry_versions; emitlogger.inforouting announcement before the auto-route call -- Three personas independently converge on the silent routing gap. The performance-expert's capture-and-thread refactor is the structural prerequisite that makes the announcement trivially correct and eliminates a redundant lockfile read -- one change delivers source disclosure, the perf fix, and closes the UX gap. -
[python-architect] Delegate
_effective_default_registryto registry_wiring (addget_effective_default_registry_for_path(project_root: Path)) to eliminate feature-gate drift risk -- The two implementations diverge on user-fallback behavior. If registry_wiring's feature-gate logic changes, the fix will not propagate to view. A delegation is the correct long-term fix; small enough for a scoped follow-up issue. -
[test-coverage-expert] Add unit test for
--registry UNKNOWN_NAMEerror path; add integration-with-fixtures test for default-registry routing with a real apm.yml and patched RegistryClient -- The unknown-registry error path is completely dark (error message and exit code unverified). The integration-tier gap means the core PR promise is exercised only with triple-mocked internals, below the tier floor for CLI command surface changes.
Architecture
classDiagram
direction LR
class view_module {
<<Module>>
+display_versions(package, logger, project_root, registry)
+_display_registry_versions(package, dep_ref, logger, registry_name)
+_is_explicit_git_form(package) bool
+_effective_default_registry(project_root) Optional~str~
}
class registry_wiring_module {
<<Module>>
+get_effective_default_registry(data) Optional~str~
+should_skip_github_probe_for_dep(dep_ref, default) bool
+routes_unscoped_to_registry(dep) bool
}
class config_loader_module {
<<Module>>
+resolve_effective_registries(regs, default) tuple
}
class feature_gate_module {
<<Module>>
+is_package_registry_enabled() bool
}
class APMPackage {
<<ValueObject>>
+registries Optional~dict~
+default_registry Optional~str~
+from_apm_yml(path) APMPackage
}
class DependencyReference {
<<ValueObject>>
+source Optional~str~
+parse(dep_str) DependencyReference
}
view_module ..> config_loader_module : resolve_effective_registries
view_module ..> feature_gate_module : is_package_registry_enabled
view_module ..> APMPackage : from_apm_yml
view_module ..> DependencyReference : parse
registry_wiring_module ..> config_loader_module : resolve_effective_registries
registry_wiring_module ..> feature_gate_module : is_package_registry_enabled
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[apm view PKG versions] --> B{marketplace ref?}
B -- yes --> C[display marketplace plugin]
B -- no --> D[DependencyReference.parse]
D --> E{--registry passed?}
E -- yes --> F[_display_registry_versions\nregistry_name=value or None]
F --> R1([return])
E -- no --> G[effective_root = project_root or cwd]
G --> H{apm.yml exists?}
H -- yes --> I[_lookup_lockfile_ref]
I --> J{locked source\n== registry?}
J -- yes --> K[_display_registry_versions]
K --> R2([return])
J -- no --> L
H -- no --> L[_is_explicit_git_form?]
L -- False --> M[_effective_default_registry\nfeature gate + apm.yml read]
M --> N{default registry\nconfigured?}
N -- yes --> O[_display_registry_versions]
O --> R3([return])
N -- no --> P
L -- True --> P[GitHubPackageDownloader\nlist_remote_refs]
P --> Q[render refs table]
Recommendation
The routing fix is correct, read-only, and consistent with the apm install precedent. No blocking security findings. Three items should land in this PR before merge: (1) CHANGELOG.md entry + view.md update -- non-negotiable per APM operating principle; (2) local-path guard in _is_explicit_git_form -- closes a real auth-misfire gap; (3) logger.info routing announcement with result-capture refactor -- closes the source-disclosure gap raised by three independent panelists. Once those three are in, the PR is mergeable. The _effective_default_registry delegation to registry_wiring, the integration-tier test, and the unknown-registry unit test should follow as scoped issues.
Full per-persona findings
Python Architect
-
[recommended]
_effective_default_registryduplicates feature-gate logic fromregistry_wiringand diverges on user-fallback behavior atsrc/apm_cli/commands/view.py:485
registry_wiring.get_effective_default_registry already owns this contract. The two functions diverge on user-fallback path. Drift risk: a feature-gate change in registry_wiring will not propagate to view.
Suggested: Addget_effective_default_registry_for_path(project_root: Path)to registry_wiring.py and delegate from view.py. -
[recommended] No comment explaining why
routes_unscoped_to_registryis not used at routing step 3 atsrc/apm_cli/commands/view.py:589
DependencyReference.parse()does NOT setsource='git'for explicit URL forms -- usingroutes_unscoped_to_registrywould silently break the git-URL escape hatch. This constraint is invisible without a comment. -
[nit]
_pris a cryptic 3-char alias; rename toeffective_rootatsrc/apm_cli/commands/view.py:578 -
[nit]
explicit = registry_name or Noneis redundant double-normalization atsrc/apm_cli/commands/view.py:384
CLI Logging Expert
-
[recommended] Auto-route to default registry (case 3) emits no [i] routing announcement at
src/apm_cli/commands/view.py:589
display_versions silently routes a plain shorthand to the registry. Output table changes from Name/Type/Commit to Version/Published with no indication of why. Alogger.infoshould announce the routing decision.
Suggested:logger.info(f"Routing to registry '{_effective_default_registry(_pr)}' (use a git URL to force git path)")before the_display_registry_versionscall. -
[nit] Help example comment column breaks alignment on the
--registry NAMEline atsrc/apm_cli/commands/view.py:655 -
[nit]
(known: ...)in registry error is informal; prefer(available: ...)atsrc/apm_cli/commands/view.py:390 -
[nit] 'bare' in
--registryoption help is CLI slang; prefer 'without a value' atsrc/apm_cli/commands/view.py:683
DevX UX Expert
-
[recommended]
--registryflag is silently dropped when theversionsfield is omitted atsrc/apm_cli/commands/view.py:706
apm view org/repo --registry myreg(withoutversions) falls silently through to local-metadata output with no hint the flag was ignored.
Suggested: Guard:if registry is not None and field != 'versions': logger.warning("--registry is only valid with the 'versions' field") -
[recommended] view.md Options table and commands.md skill guide not updated with
--registry [NAME]atdocs/src/content/docs/reference/cli/view.md:52 -
[recommended] Silent routing change to registry produces no source disclosure in output at
src/apm_cli/commands/view.py:589
Same command produces Version/Published on a machine with a default registry and git refs on clean CI. No indication why. -
[recommended] view.md versions description is stale -- still says lockfile is required for registry routing at
docs/src/content/docs/reference/cli/view.md:41 -
[recommended] Bare
--registryform adds no value over auto-routing; deviates from npm/pip/cargo required-value idiom atsrc/apm_cli/commands/view.py:676 -
[nit] Help example
--registry # From the default registryis misleading once auto-routing is live atsrc/apm_cli/commands/view.py:654
Supply Chain Security
-
[nit] User-supplied
--registry NAMEechoed verbatim in error message without repr() quoting atsrc/apm_cli/commands/view.py:390
Suggested:logger.error(f"Registry {explicit!r} is not configured (known: {known})") -
[nit] SCP form without username (
host:path, no@) not detected as git form; routes to registry atsrc/apm_cli/commands/view.py:481
_is_explicit_git_formrequires BOTH@AND:.github.com:org/reporeturns False. Harmless (produces a 404 from registry) but confusing. -
[nit] Silent
except Exception: passin_effective_default_registryswallows apm.yml parse errors with no diagnostic atsrc/apm_cli/commands/view.py:516
Suggested: Log at debug level before returning None.
OSS Growth Hacker
-
[recommended] No CHANGELOG entry for a user-facing behavior change that resolves a prominent registry-user pain point at
CHANGELOG.md
Suggested: Add under [Unreleased] Fixed: "apm view org/repo versionswith a default registry configured now routes to the configured registry for unlocked shorthands instead of failing with a git auth error.--registry [NAME]added for explicit control. (feat(view): route 'view versions' to the registry when a default is configured #1938)" -
[recommended] docs/reference/cli/view.md does not reflect the new
--registryflag or the default-registry routing behavior atdocs/src/content/docs/reference/cli/view.md -
[nit] preview-and-validate.md describes
view versionsas listing only git tags -- now incomplete for registry producers atdocs/src/content/docs/producer/preview-and-validate.md -
[nit] Help comment
Remote tags/branches (or registry)buries the new registry path for registry-first users atsrc/apm_cli/commands/view.py
Auth Expert
-
[recommended]
_is_explicit_git_formdoes not classify local filesystem paths as git forms; causes registry auth to fire for local-path arguments atsrc/apm_cli/commands/view.py:478
apm view ./local-pkg versionswith a default registry returns False from_is_explicit_git_form, triggering step 3 and dispatching registry credentials for a nonsensical query.dep_ref.is_localis available as a clean guard.
Suggested:if p.startswith('./') or p.startswith('../') or p.startswith('/'): return True -
[nit] CLI help example claims git URL 'forces the git path' but only when
--registryis absent atsrc/apm_cli/commands/view.py:654
When--registryis passed with a full git URL, registry routing fires first. The advertised escape-hatch semantics do not hold in the combined case.
Doc Writer
-
[recommended] reference/cli/view.md Options table missing
--registry [NAME]atdocs/src/content/docs/reference/cli/view.md:54
Suggested:| --registry [NAME] | List versions from a registry. Bare uses lockfile/default; NAME forces a named registry. | -
[recommended] reference/cli/view.md versions routing description is stale after routing fix at
docs/src/content/docs/reference/cli/view.md:41
Still implies lockfile entry is required; step-3 routing (default registry + unlocked shorthand) is not mentioned. -
[recommended] reference/cli/view.md Examples section missing
--registryusage and git-URL escape hatch atdocs/src/content/docs/reference/cli/view.md:56 -
[recommended] CHANGELOG.md [Unreleased] block missing entries for routing fix and new
--registryflag atCHANGELOG.md:8 -
[nit] commands.md view entry missing
--registry [NAME]in options column atpackages/apm-guide/.apm/skills/apm-usage/commands.md:21
Test Coverage Expert
-
[recommended]
--registry UNKNOWN_NAMEerror path has no test attests/unit/test_view_command.py
Every existing--registrytest mocks_display_registry_versionsbefore the error branch is reached. Error message and exit code are both dark. Probed: zero hits in test file for 'unknown', 'not configured', 'registry_not_found'.
Suggested:test_view_versions_unknown_registry_name_exits_with_error: mock registries dict without 'nonexistent', assertexit_code==1and'not configured' in output.
Proof (missing at unit):tests/unit/test_view_command.py::test_view_versions_unknown_registry_name_exits_with_error-- proves:apm view pkg versions --registry unknown-nameexits 1 and names the missing registry in the error -
[recommended] Default-registry routing tested only at unit tier; no integration-with-fixtures coverage at
tests/integration/test_wave6_outdated_view_coverage.py
Core promise tested exclusively at unit tier with triple-mocked internals. Tier-floor for CLI command surface is integration-with-fixtures.
Proof (missing at integration-with-fixtures):tests/integration/test_wave6_outdated_view_coverage.py::TestViewVersionsRegistryRouting::test_shorthand_routes_to_registry_when_default_configured-- proves:apm view myorg/myrepo versionsin a project with a configured default registry calls the registry API, not git -
[nit]
_is_explicit_git_form.git-suffix branch is untested attests/unit/test_view_command.py
URL and SCP forms are exercised indirectly;.gitsuffix has no test. Probed: zero hits for.gitsuffix in test file.
Proof (missing at unit):tests/unit/test_view_command.py::test_view_versions_dot_git_suffix_forces_git_even_with_default_registry-- proves:apm view owner/repo.git versionsroutes to git even when a default registry is configured
Performance Expert
-
[recommended]
_effective_default_registryresult is discarded;_display_registry_versionsre-runsresolve_effective_registriesand re-reads lockfile atsrc/apm_cli/commands/view.py:589
Step 3 calls_effective_default_registryas a boolean guard then discards the result._display_registry_versionsthen rerunsresolve_effective_registriesand does a secondLockFile.read. Fixable with one change: capture and pass asregistry_name=.
Suggested:_default_reg = None if _is_explicit_git_form(package) else _effective_default_registry(_pr); if _default_reg: _display_registry_versions(package, dep_ref, logger, registry_name=_default_reg); return -
[nit] apm.yml
is_file()stat issued twice on same path atsrc/apm_cli/commands/view.py:508 -
[nit]
LockFile.readhas no cache; pre-existing double-read pattern inherited by step-3 path atsrc/apm_cli/commands/view.py
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 #1938 · 737.6 AIC · ⌖ 15.8 AIC · ⊞ 5.5K · ◷
apm-review-panel advisory -- feat/view-versions-registry-routingPanel run: inline (9 personas + CEO synthesis) Orchestrator reservations addressed
Copilot inline review classification
Panel follow-up disposition (fold-vs-defer)
Lint evidence (fold commit a6ecffa)CI evidence
Mergeability snapshot
Panel personaspython-architect, cli-logging-expert, devx-ux-expert, supply-chain-security-expert, oss-growth-hacker, auth-expert, doc-writer, test-coverage-expert, performance-expert (active=false; no hot path touched) Shepherd-driver loop complete. Ready-to-merge pending Phase 6 rebase. |
…onfigured
`apm view <pkg> versions` only consulted the lockfile to decide registry vs
git, so an unlocked shorthand fell through to `git ls-remote` even when a
default registry was configured -- inconsistent with `apm install`, which
routes unscoped shorthands to the default registry.
Routing precedence in display_versions (highest first):
1. --registry [NAME] -- force the registry path (named, or default if bare).
2. lockfile records the package as source: registry (existing behavior).
3. a default registry is configured and the ref is a plain shorthand
(mirrors `apm install`); an explicit git URL stays on the git path.
Adds the --registry [NAME] flag for explicit control and keeps a full git URL
as the escape hatch to force git. _display_registry_versions already resolved
the default registry for unlocked packages; it now also honors an explicit
registry name (clear error when unknown). All paths are read-only -- no
install or lockfile mutation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- _is_explicit_git_form now detects SCP-style refs for any SSH user (e.g. alice@github.com:org/repo), not just git@, plus .git suffixes. Arbitrary-user SCP refs no longer mis-route to the default registry. - _effective_default_registry honors the registries experimental feature gate (is_package_registry_enabled), mirroring install's get_effective_default_registry -- a config.json default no longer routes view->registry when registries are disabled. - Tests: bare --registry parses and forces the registry path; SCP git ref routes to git; feature-gate-off returns no default registry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add local-path early-return to _is_explicit_git_form so ./local-pkg, ../path, and /abs/path are treated as git forms and never dispatch registry credentials for a nonsensical query (auth-expert finding) - Capture _effective_default_registry result at step 3 and thread it as registry_name to _display_registry_versions, eliminating the redundant re-read inside that function; emit logger.info routing announcement so users can tell why they got registry results instead of git refs (cli-logging-expert + devx-ux-expert + performance-expert finding) - Add CHANGELOG [Unreleased] Added entry for routing fix and --registry flag - Update docs/reference/cli/view.md: routing description, Options table with --registry [NAME], and new Examples entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix stale subcommand section in view.md: 'installed registry packages' implied only locked deps could use registry listing; updated to describe the three-signal routing (--registry flag, lockfile source, default registry for plain shorthands) so the doc matches the implementation - Add unit test test_view_versions_unknown_registry_name_exits_with_error: patches resolve_effective_registries to return a dict without the requested name and asserts exit 1 with 'not configured' in output; the error path was previously untested - Add integration tests TestViewVersionsRegistryRouting in test_view_coverage.py: test_registry_flag_routes_to_named_registry exercises the real Click command with an apm.yml fixture, mocking only external I/O; verifies the version table is printed; test_registry_flag_unknown_name_exits_one covers the same error path at integration tier Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a6ecffa to
2991d2e
Compare
|
Rebased onto current main at cdc0495 -> 2991d2e. Conflicting paths resolved (faithful merge of both intents):
Lint contract: Post-push mergeability: Ready for maintainer review. |
…sts, and doc fixes - Warn when --registry is used without the versions field (previously silently ignored); addresses devx-ux panel follow-up. - Add TestDisplayRegistryVersions with 4 direct tests covering: unknown registry name, no-registry-configured, RegistryError propagation, and happy-path output; addresses test-coverage-expert follow-up. - Deduplicate registry-routing description in view.md: Description bullet now forward-references the subcommand paragraph; addresses doc-writer follow-up. - Update commands.md skill resource to expose --registry [NAME] for view; addresses devx-ux follow-up on apm-usage skill drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2991d2e to
8bf45fd
Compare
Summary
apm view <pkg> versionsdecided registry-vs-git only from the lockfile, so an unlocked shorthand fell through togit ls-remoteeven when a default registry was configured. That contradictsapm install, which already routes unscoped shorthands to the default registry:It now consults the default registry for unlocked shorthands, and adds an explicit
--registry [NAME]override.Routing precedence (
display_versions)--registry [NAME]-- force the registry path. Bare uses the lockfile/default; a value names a specific registry (clear error if unknown).source: registry(existing behavior).apm install. An explicit git URL (https://github.com/...) stays on the git path.A full git URL is the escape hatch to force git when a default registry is set (the
viewanalog of the- git:manifest form).Why this is low-risk
All paths here are read-only -- no install, no resolution, no lockfile mutation. This is the read-only sibling of the deferred default-registry-vs-git-transitives discussion, without any of that risk.
_display_registry_versionsalready resolved the default registry for unlocked packages (it just wasn't reached); this wires the routing and adds explicit name support.Behavior
view org/repo versions(unlocked shorthand)view org/repo versions(locked as registry)view org/repo versions --registryview org/repo versions --registry NAMEview https://github.com/org/repo versionsTests
4 new cases in
tests/unit/test_view_command.py: default-registry routing for unlocked shorthand, no-default-still-git,--registry NAMEforces + threads the name, and git-URL-forces-git. 331 view/registry tests pass; ruff clean. Verified live against a JFrog registry (unlocked listing, named registry, unknown-name error, git escape).apm-spec-waiver: read-only display routing -- makes
view versionsconsult the configured default registry consistently withinstall; no install, resolution, or lockfile behavior changes