feat(pacquet): port named registries to the install chain#11785
Conversation
📝 WalkthroughWalkthroughThis PR implements end-to-end named-registry alias support: users define registry aliases in ChangesNamed Registry Alias Resolution
Sequence Diagram(s)sequenceDiagram
participant User as User Config
participant ConfigLayer as Config Layer
participant Parser as Specifier Parser
participant Chain as DefaultResolver Chain
participant Scheme as LocalSchemeResolver
participant Named as NamedRegistryResolver
participant Path as LocalPathResolver
participant HTTP as HTTP Registry
User->>ConfigLayer: namedRegistries: {gh: url, work: url}
ConfigLayer->>ConfigLayer: substitute_env(), merge_named_registries()
Parser->>Chain: WantedDependency(gh:`@owner/pkg`)
Chain->>Scheme: resolve()
Scheme-->>Chain: Ok(None)
Chain->>Named: resolve()
Named->>Parser: parse_named_registry_specifier()
Parser-->>Named: NamedRegistryPackageSpec
Named->>HTTP: GET /registry/@owner/pkg
HTTP-->>Named: packument
Named-->>Chain: ResolveResult(resolved_via=named-registry)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds the user-facing `<alias>:` resolver surface so a manifest entry like `"@acme/private": "gh:^1.0.0"` resolves against GitHub Packages (or any other registry the user configures under `namedRegistries:` in `pnpm-workspace.yaml`). Mirrors upstream b61e268d57: - `parse_named_registry_specifier_to_registry_package_spec` parses `<alias>:[@<owner>/]<name>[@<version>]` and `<alias>:<version>` bodies. Rejects scope-without-name with `ERR_PNPM_INVALID_NAMED_REGISTRY_PACKAGE_NAME`. - `merge_named_registries` folds the user map onto pacquet's built-in aliases (`gh:` -> GitHub Packages) and validates URLs at resolver construction (`ERR_PNPM_INVALID_NAMED_REGISTRY_URL`). - `NamedRegistryResolver` is a third `Resolver` alongside the npm / jsr paths, emitting `resolved_via: "named-registry"`. Auth headers flow through the existing per-URL `.npmrc` lookup. - `Config::named_registries` reads `namedRegistries:` from `pnpm-workspace.yaml`, with `${VAR}` substitution on the values (matches upstream's `replaceEnvInStringValues`). - `install_without_lockfile` constructs the merged map once and threads it through both the resolver chain (after npm/git, so configured aliases cannot hijack built-in schemes) and `build_resolution_verifiers` (so tarball-URL prefix routing honours user aliases).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11785 +/- ##
==========================================
- Coverage 89.69% 89.45% -0.25%
==========================================
Files 174 175 +1
Lines 20781 21116 +335
==========================================
+ Hits 18640 18889 +249
- Misses 2141 2227 +86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
- `resolving-local-resolver/src/chain.rs`: drop the redundant explicit `(crate::...)` target on `[resolve_latest_from_local]` (clippy's `redundant_explicit_links` lint flagged it under `RUSTDOCFLAGS=-D warnings`) and demote the `[contains_path_sep]` intra-doc reference to backticks since the helper is private. - `resolving-npm-resolver/src/parse_bare_specifier.rs`: rename `unparseable` to `unparsable` in a doc comment so the workspace typos check passes.
Ports upstream's [`resolving/default-resolver/test/namedRegistry.ts`](https://github.com/pnpm/pnpm/blob/b61e268d57/resolving/default-resolver/test/namedRegistry.ts) and the two `resolveNamedRegistry.test.ts` cases the previous commit hadn't covered. - New `tests/chain.rs` integration tests assert that the explicit `link:` / `workspace:` / `file:` schemes win over a colliding named-registry alias. These pin the local-scheme / local-path split: with combined `LocalResolver`, named-registry would slot *after* both halves and a `link` alias could hijack `link:./pkg`. - `resolves_via_builtin_gh_alias` covers the `gh:` happy path that was previously only exercised via a user-supplied `work:` alias. - `preserves_scoped_pkg_name_when_alias_differs` verifies that the resolver records the dependency under the registry's name, not the local manifest alias. - `user_config_overrides_builtin_gh_alias` covers the GHES override scenario where a user points `gh` at their enterprise host. 11 resolver tests pass (5 unit + 3 chain integration + 3 happy-path scenarios) plus the 12 parser cases already in place.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pacquet/crates/resolving-npm-resolver/tests/chain.rs (2)
89-91: ⚡ Quick winAdd debug context before the structural assertions.
The
matches!checks are the non-assert_eq!assertions here. If one fails, we currently lose the actualresolutionpayload, which makes these regressions harder to triage.As per coding guidelines, "Log before non-
assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!."Also applies to: 121-123, 152-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/resolving-npm-resolver/tests/chain.rs` around lines 89 - 91, Add debug output for the complex `result.resolution` before the structural `assert!` checks so failures show the payload; specifically, after `let result = resolver.resolve(&wanted, &opts).await.expect("resolve");` call and before the `assert!(matches!(result.resolution, LockfileResolution::Directory(_)));` (and the analogous places around lines 121-123 and 152-154), insert a debug dump of the resolution (e.g., call dbg!(&result.resolution) or equivalent logging) while leaving the simple scalar `assert_eq!(result.resolved_via, "local-filesystem");` unchanged.
37-46: ⚡ Quick winMake precedence regressions fail without touching the network.
If the chain order regresses, this helper will try to fetch from
npm.work.example.com, so the failure becomes timeout/DNS-dependent instead of immediate. Flipping the named-registry leg tooffline: truekeeps the happy path unchanged and makes precedence failures deterministic.Proposed tweak
cache_dir: None, - offline: false, + offline: true, prefer_offline: false, ignore_missing_time_field: false,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/resolving-npm-resolver/tests/chain.rs` around lines 37 - 46, The test helper currently sets offline: false which causes a network fetch to npm.work.example.com if chain order regresses; change the named-registry leg's offline flag to true (in the struct where http_client, auth_headers, meta_cache, cache_dir, offline, prefer_offline, ignore_missing_time_field are set) so the helper uses the offline path and makes precedence regressions fail deterministically without touching the network.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pacquet/crates/resolving-npm-resolver/tests/chain.rs`:
- Around line 89-91: Add debug output for the complex `result.resolution` before
the structural `assert!` checks so failures show the payload; specifically,
after `let result = resolver.resolve(&wanted, &opts).await.expect("resolve");`
call and before the `assert!(matches!(result.resolution,
LockfileResolution::Directory(_)));` (and the analogous places around lines
121-123 and 152-154), insert a debug dump of the resolution (e.g., call
dbg!(&result.resolution) or equivalent logging) while leaving the simple scalar
`assert_eq!(result.resolved_via, "local-filesystem");` unchanged.
- Around line 37-46: The test helper currently sets offline: false which causes
a network fetch to npm.work.example.com if chain order regresses; change the
named-registry leg's offline flag to true (in the struct where http_client,
auth_headers, meta_cache, cache_dir, offline, prefer_offline,
ignore_missing_time_field are set) so the helper uses the offline path and makes
precedence regressions fail deterministically without touching the network.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a2ef4c0-94e9-4e3d-bff2-71bb09015765
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
pacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/Cargo.tomlpacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rspacquet/crates/resolving-npm-resolver/src/named_registry_resolver/tests.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rspacquet/crates/resolving-npm-resolver/tests/chain.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-npm-resolver/tests/chain.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry_resolver/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/lib.rs
pacquet/**/{tests,test}/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <Bounds>— only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or#[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/resolving-npm-resolver/tests/chain.rs
🧠 Learnings (1)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-npm-resolver/tests/chain.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry_resolver/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/lib.rs
🔇 Additional comments (18)
pacquet/crates/config/src/lib.rs (1)
380-391: LGTM!Also applies to: 1013-1019, 1106-1115, 1133-1139
pacquet/crates/config/src/workspace_yaml.rs (1)
2-3: LGTM!Also applies to: 125-132, 402-419, 500-502
pacquet/crates/config/src/workspace_yaml/tests.rs (1)
2-2: LGTM!Also applies to: 109-167
pacquet/crates/resolving-npm-resolver/src/named_registry.rs (1)
18-19: LGTM!Also applies to: 26-79
pacquet/crates/resolving-npm-resolver/src/named_registry/tests.rs (1)
5-8: LGTM!Also applies to: 143-184
pacquet/crates/resolving-local-resolver/src/chain.rs (1)
1-13: LGTM!Also applies to: 26-128, 171-212
pacquet/crates/resolving-local-resolver/src/lib.rs (1)
32-32: LGTM!pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
21-27: LGTM!Also applies to: 110-116, 181-249
pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs (1)
15-19: LGTM!Also applies to: 26-29, 155-286, 295-295
pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs (1)
1-2: LGTM!Also applies to: 6-10, 217-475
pacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rs (1)
1-215: LGTM!pacquet/crates/resolving-npm-resolver/src/named_registry_resolver/tests.rs (1)
1-268: LGTM!pacquet/crates/resolving-npm-resolver/src/lib.rs (1)
29-29: LGTM!Also applies to: 47-56
pacquet/crates/package-manager/src/build_resolution_verifiers.rs (1)
99-110: LGTM!pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
42-42: LGTM!pacquet/crates/resolving-npm-resolver/Cargo.toml (1)
36-37: LGTM!pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (2)
330-333: LGTM!
335-335: LGTM!
Summary
Ports the user-facing
<alias>:resolver surface to pacquet so a manifest entry like"@acme/private": "gh:^1.0.0"resolves against GitHub Packages (or any other registry the user configures undernamedRegistries:inpnpm-workspace.yaml).Mirrors upstream
b61e268d57(#11324).What's new in pacquet
parse_named_registry_specifier_to_registry_package_spec): handles<alias>:[@<owner>/]<name>[@<version>]and<alias>:<version>bodies. Rejects scope-without-name withERR_PNPM_INVALID_NAMED_REGISTRY_PACKAGE_NAME.merge_named_registries): folds the user map onto pacquet's built-in aliases (gh:→ GitHub Packages) and validates each URL at resolver construction withERR_PNPM_INVALID_NAMED_REGISTRY_URL.NamedRegistryResolver): thirdResolveralongside the npm / jsr paths, emittingresolved_via: "named-registry". Auth headers flow through the existing per-URL.npmrclookup, so a//npm.pkg.github.com/:_authToken=…entry takes effect forgh:specifiers automatically.Config::named_registries): readsnamedRegistries:frompnpm-workspace.yaml, with${VAR}substitution on the values (mirrors upstream'sreplaceEnvInStringValues).install_without_lockfileconstructs the merged map once and threads it through both the resolver chain (after npm/git, so configured aliases cannot hijack built-in schemes) andbuild_resolution_verifiers(so the tarball-URL prefix routing honours user aliases).Test plan
cargo nextest run -p pacquet-resolving-npm-resolver— 164 tests pass (12 new parser cases, 4 new merger cases, 5 new resolver integration tests withmockito)cargo nextest run -p pacquet-config— 206 tests pass (2 new for the yaml field + env substitution)cargo nextest run -p pacquet-package-manager— 265 tests passjust lint— cleancargo fmt --check— cleanjust test) — 1708 / 1708 passWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests