feat(pacquet): wire NpmResolver into install; fix(pick-registry) unscoped npm-alias routing#11760
Conversation
Implements phases A, B, and C of #11756: ports `parseBareSpecifier`, adds the `NpmResolver` struct implementing the `Resolver` trait, and refactors the no-lockfile install into a two-pass resolve-then-install shape backed by a minimum-slice port of `resolveDependencyTree`. `InstallPackageFromRegistry` now takes a pre-resolved `ResolveResult` instead of `(name, version_range)` and drops the legacy `Package::fetch_from_registry` + `Package::pinned_version` calls — all npm-shaped resolution flows through the new chain. --- Written by an agent (Claude Code, claude-opus-4-7).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11760 +/- ##
==========================================
+ Coverage 89.64% 89.93% +0.28%
==========================================
Files 151 154 +3
Lines 17571 18007 +436
==========================================
+ Hits 15752 16194 +442
+ Misses 1819 1813 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0683628903600004,
"stddev": 0.0758818815224874,
"median": 2.07347607196,
"user": 2.93147474,
"system": 2.20464794,
"min": 1.9467888069600001,
"max": 2.18564807196,
"times": [
1.9467888069600001,
1.9855246559600002,
2.18564807196,
2.10464158196,
2.13020075296,
2.02447701096,
2.01243372296,
2.07364059796,
2.1469621559600003,
2.0733115459600002
]
},
{
"command": "pacquet@main",
"mean": 2.0342915701599997,
"stddev": 0.08774039120978055,
"median": 2.00669484546,
"user": 2.9323489400000002,
"system": 2.1942944399999997,
"min": 1.9325673949600002,
"max": 2.2155763689600003,
"times": [
2.08906989796,
1.9976442159600003,
2.2155763689600003,
1.95133647396,
1.9869398989600002,
2.01574547496,
2.07073518996,
2.11522205196,
1.9325673949600002,
1.96807873396
]
},
{
"command": "pnpm",
"mean": 4.205331854860001,
"stddev": 0.05520632780717547,
"median": 4.204450571959999,
"user": 7.94792024,
"system": 2.5200177399999992,
"min": 4.12773910596,
"max": 4.29188418896,
"times": [
4.23697348296,
4.24985584296,
4.26074321696,
4.14762991896,
4.229353071959999,
4.166532762959999,
4.29188418896,
4.16305888496,
4.179548071959999,
4.12773910596
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.5131324011000001,
"stddev": 0.05257262502072029,
"median": 0.49572606580000006,
"user": 0.38242316,
"system": 0.9291861000000001,
"min": 0.48839079730000007,
"max": 0.6601373853,
"times": [
0.6601373853,
0.5228592533,
0.48839079730000007,
0.49574406430000006,
0.49235684430000004,
0.49069214130000005,
0.4896783693,
0.49862068130000003,
0.4971364073,
0.49570806730000005
]
},
{
"command": "pacquet@main",
"mean": 0.500704733,
"stddev": 0.02220709241440307,
"median": 0.49197871630000006,
"user": 0.38110285999999993,
"system": 0.9350179000000001,
"min": 0.48555835830000005,
"max": 0.5604783963,
"times": [
0.5604783963,
0.49614978130000004,
0.49042583030000003,
0.49828163630000005,
0.48555835830000005,
0.49048331030000003,
0.49161744130000007,
0.48974897330000006,
0.5119636113,
0.49233999130000006
]
},
{
"command": "pnpm",
"mean": 2.2560305658,
"stddev": 0.03975500480938133,
"median": 2.2534958703,
"user": 2.98918916,
"system": 1.2860833999999999,
"min": 2.1878246393,
"max": 2.3196055443000003,
"times": [
2.2399799213000002,
2.3196055443000003,
2.2936155903000004,
2.1878246393,
2.2649587263,
2.2518250663000003,
2.2930349243,
2.2081196053000003,
2.2551666743000003,
2.2461749663000004
]
}
]
} |
CI's Doc job runs `cargo doc --no-deps --workspace --document-private-items` under `RUSTDOCFLAGS=-D warnings`; CI's Dylint job runs Perfectionist lints under `-D warnings` too. Neither surfaces in `just ready`. Fix the items the post-push run flagged: * Disambiguate fn-vs-module rustdoc links with `name()` syntax. * Resolve the `Resolver` link in `resolving-deps-resolver` against `pacquet_resolving_resolver_base::Resolver`. * Reorder the `ResolvedTree` derives to `Debug, Default, Clone`. * Rename the resolver-chain generic from `R` to `Chain` and the dist- tag validator's `s` to `selector`, matching the existing `Cache: PackageMetaCache` precedent. * Add a trailing comma to the multi-line `assert!` body. --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs (1)
87-110: ⚡ Quick winAdd a rejection test for mismatched tarball basename.
Please add a case like
.../foo/-/bar-1.0.0.tgzand assertNoneto lock in strict tarball parsing and prevent future regressions.🤖 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/src/parse_bare_specifier/tests.rs` around lines 87 - 110, Add a unit test that ensures parse_bare_specifier rejects tarball URLs whose basename doesn't match the package path: create a test (e.g., tarball_url_with_mismatched_basename_declines) that calls parse_bare_specifier with a URL like "https://registry.npmjs.org/foo/-/bar-1.0.0.tgz" (and similarly for a scoped package variant if desired) and assert the result is None; place it alongside the existing tests in parse_bare_specifier/tests.rs to lock in strict tarball basename validation.
🤖 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.
Inline comments:
In `@pacquet/crates/package-manager/src/install_without_lockfile.rs`:
- Around line 327-330: The current fast-path early return when
ctx.resolved_packages.insert(virtual_store_name.clone()) is false prevents
creating per-parent alias links (node_modules/<alias>) on duplicate
{name,version} hits; keep the dedupe set insertion logic but remove the
unconditional return so that when insert returns false you still run the
parent-local symlink/alias creation path (e.g., call the same
alias/materialization logic used for first-writer cases), guarded by a check
that waits or observes any in-flight first-writer completion (e.g., a
"first-writer" flag or completion future) to avoid races. In practice: retain
the insert check to avoid redundant materialization work, but if insert() is
false, call the alias linking routine used below (the code that creates
node_modules/<alias> for the parent) and short-circuit only the heavy
materialization when an in-flight writer is detected/completed.
In `@pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`:
- Around line 15-18: Introduce a strong newtype for the branded package
identifier (e.g., PackageId(String)) and replace raw String usages with it:
change DirectDep.id and ResolvedPackage.id to PackageId, and change
ResolvedTree.packages from HashMap<String, ResolvedPackage> to
HashMap<PackageId, ResolvedPackage>. Implement common traits (Clone, Debug, Eq,
PartialEq, Hash, Ord) and conversion helpers (From<String>/FromStr, Display) and
derive or implement Serialize/Deserialize so the map still serializes as string
keys; update any constructors, lookups, and tests to construct/compare via
PackageId instead of raw strings. Ensure all uses in files referencing
DirectDep, ResolvedPackage, and ResolvedTree are updated to the new type to
restore type-safety.
In `@pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs`:
- Line 268: The current line that assigns manifest using
serde_json::to_value(picked).ok() swallows serialization errors; replace this
with error propagation so failures become a ResolveError instead of None. Change
the manifest assignment to call serde_json::to_value(picked) and map or convert
the serde_json::Error into your ResolveError variant (e.g., using map_err or the
? operator) so the function returns Err(ResolveError::...) on failure; reference
the manifest variable, serde_json::to_value(picked) call, and the ResolveError
type to implement the proper conversion.
In `@pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs`:
- Around line 169-176: The code currently slices the tarball filename into a
version using scopeless_name_length without verifying the filename actually
begins with "<scopeless-name>-"; update the logic in parse_bare_specifier.rs to
first compute the scopeless name (the last segment of name using
scopeless_name_length), then assert that path_with_no_ext starts with that
scopeless name plus a '-' separator (e.g. starts_with(format!("{}-",
scopeless_name)) or equivalent slice comparison); if the prefix check fails
return None, otherwise extract the version substring after that prefix and
continue to Version::parse(version).ok()? before returning Some(NpmTarballUrl {
name, version: version.to_string() }).
---
Nitpick comments:
In `@pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs`:
- Around line 87-110: Add a unit test that ensures parse_bare_specifier rejects
tarball URLs whose basename doesn't match the package path: create a test (e.g.,
tarball_url_with_mismatched_basename_declines) that calls parse_bare_specifier
with a URL like "https://registry.npmjs.org/foo/-/bar-1.0.0.tgz" (and similarly
for a scoped package variant if desired) and assert the result is None; place it
alongside the existing tests in parse_bare_specifier/tests.rs to lock in strict
tarball basename validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c924d83-145c-4f61-baae-323f496df5b6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlpacquet/crates/config/src/version_policy.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rspacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/resolving-resolver-base/src/resolve.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). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
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-npm-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/config/src/version_policy.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_package_from_registry.rs
🔇 Additional comments (16)
pacquet/crates/resolving-resolver-base/Cargo.toml (1)
14-18: LGTM!pacquet/crates/resolving-resolver-base/src/resolve.rs (1)
13-14: LGTM!Also applies to: 161-172
pacquet/crates/config/src/version_policy.rs (1)
133-150: LGTM!pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs (1)
27-167: LGTM!Also applies to: 179-203
pacquet/crates/resolving-npm-resolver/src/lib.rs (1)
29-30: LGTM!Also applies to: 49-50
pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (1)
83-267: LGTM!Also applies to: 269-334
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs (1)
1-187: LGTM!Cargo.toml (1)
40-40: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
14-38: LGTM!pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
1-34: LGTM!pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
26-35: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
52-97: LGTM!Also applies to: 106-162, 167-187
pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
80-209: LGTM!pacquet/crates/package-manager/src/install_package_from_registry.rs (1)
96-103: LGTM!Also applies to: 109-115, 125-128, 145-152, 185-216
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
79-109: LGTM!Also applies to: 112-171, 182-276
pacquet/crates/package-manager/src/install.rs (1)
512-517: LGTM!
There was a problem hiding this comment.
Pull request overview
This PR wires the pacquet npm registry resolver into the no-lockfile install path by introducing an explicit resolve pass (building a resolved dependency tree) followed by an install/materialization pass that consumes pre-resolved ResolveResults.
Changes:
- Add npm-resolution inputs to
ResolveOptionsand introduceNpmResolver+parse_bare_specifierinpacquet-resolving-npm-resolver. - Add new
pacquet-resolving-deps-resolvercrate that resolves a full dependency tree (flatname@versionmap + edges) with concurrency + dedupe. - Refactor the no-lockfile install flow to resolve first, then install using pre-resolved tarball URL/integrity instead of registry lookups during install.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pacquet/crates/resolving-resolver-base/src/resolve.rs | Extends ResolveOptions with published-by/policy and dry-run flags to support npm resolver behavior. |
| pacquet/crates/resolving-resolver-base/Cargo.toml | Adds resolver-base deps needed for new option types (chrono, pacquet-config). |
| pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs | Ports pnpm bare-specifier parsing to classify npm registry inputs and decline non-npm protocols. |
| pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs | Unit tests for the specifier parser port. |
| pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs | Implements Resolver for npm registry resolution and maps picks into ResolveResult. |
| pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs | Resolver tests (range picking, fallthrough, default tag, latest, policy violation surfacing). |
| pacquet/crates/resolving-npm-resolver/src/lib.rs | Updates crate docs/exports to include the new resolver + parser surface. |
| pacquet/crates/resolving-deps-resolver/src/lib.rs | New crate exporting resolve_dependency_tree used by install to pre-resolve deps. |
| pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs | Implements concurrent tree walking, per-id dedupe gating, and policy-violation collection. |
| pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs | Defines the resolved-tree output types consumed by install. |
| pacquet/crates/resolving-deps-resolver/src/tests.rs | Tests for tree walking, dedupe, and resolver decline behavior. |
| pacquet/crates/resolving-deps-resolver/Cargo.toml | Adds the new deps-resolver crate to the workspace. |
| pacquet/crates/package-manager/src/install.rs | Plumbs an Arc<ThrottledClient> into the no-lockfile install struct for resolver ownership needs. |
| pacquet/crates/package-manager/src/install_without_lockfile.rs | Refactors no-lockfile install into resolve-then-install using resolve_dependency_tree + NpmResolver. |
| pacquet/crates/package-manager/src/install_package_from_registry.rs | Switches installer input from (name, range) to pre-resolved ResolveResult (tarball URL/integrity). |
| pacquet/crates/package-manager/src/install_package_from_registry/tests.rs | Updates tests to resolve via mock registry first, then install from the pre-resolved result. |
| pacquet/crates/package-manager/Cargo.toml | Adds deps-resolver (and default-resolver) dependencies needed for the new flow. |
| pacquet/crates/config/src/version_policy.rs | Makes PackageVersionPolicy clonable for inclusion in ResolveOptions. |
| Cargo.toml | Adds pacquet-resolving-deps-resolver to workspace members. |
| Cargo.lock | Locks new crate/dependency graph changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- install_without_lockfile: place the dedup gate after the per-edge `install_package_from_registry` call instead of before it, so a `(name, version)` reached from two different parents still produces the per-parent `node_modules/<alias>` symlink. Dedup now only short-circuits the children walk, matching the original install loop's semantics before the resolveDependencyTree refactor. - npm_resolver: propagate `serde_json::to_value` errors as `ResolveError` instead of silently dropping the manifest. - install_package_from_registry: use `usize::try_from` for the `unpackedSize` hint so a `u64` larger than the host `usize` on 32-bit targets degrades to `None` rather than truncating. - parse_bare_specifier: anchor the tarball filename version extraction on the scopeless-name prefix so a registry that serves `<reg>/foo/-/bar-1.0.0.tgz` declines instead of being mapped to the wrong `(name, version)` pair. Tests cover the rejection. - package-manager: drop the unused `pacquet-resolving-default-resolver` dependency. `DefaultResolver` isn't wired into install today because the chain has only one entry; routing through it now would surface `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER` for every non-npm specifier (git/file/etc) and regress installs whose per-protocol resolvers haven't been ported yet. Land DefaultResolver wiring alongside the second resolver impl. --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
323-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep progress events per package, not per edge.
Moving the dedupe gate below
InstallPackageFromRegistry::run()fixes alias linking, but it also makes shared deps emitpnpm:progress resolved/importedevery time this edge is visited. That turns a per-package signal into a per-edge signal, so reporter progress will be inflated for aliases and repeated transitive deps.A small fix is to compute
first_visitbefore this call and thread that intoInstallPackageFromRegistryso non-first visits still create the parent-local symlink but skip theResolved/Importedemits.As per coding guidelines,
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."🤖 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/package-manager/src/install_without_lockfile.rs` around lines 323 - 357, Compute whether this is the first visit for the virtual store slot before calling InstallPackageFromRegistry::run (i.e., evaluate first_visit = ctx.resolved_packages.insert(virtual_store_name.clone()) and capture its boolean result), add a first_visit boolean field to the InstallPackageFromRegistry struct (or otherwise thread that flag into the operation), and make InstallPackageFromRegistry skip emitting the pnpm:progress Resolved/Imported events when first_visit is false while still performing the per-parent symlink work; keep the existing dedupe return behavior for early exits but ensure the symlink creation path still runs for non-first visits.
🤖 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.
Outside diff comments:
In `@pacquet/crates/package-manager/src/install_without_lockfile.rs`:
- Around line 323-357: Compute whether this is the first visit for the virtual
store slot before calling InstallPackageFromRegistry::run (i.e., evaluate
first_visit = ctx.resolved_packages.insert(virtual_store_name.clone()) and
capture its boolean result), add a first_visit boolean field to the
InstallPackageFromRegistry struct (or otherwise thread that flag into the
operation), and make InstallPackageFromRegistry skip emitting the pnpm:progress
Resolved/Imported events when first_visit is false while still performing the
per-parent symlink work; keep the existing dedupe return behavior for early
exits but ensure the symlink creation path still runs for non-first visits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fdc333f-ad8c-4019-8b0c-35e3bcdba649
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/install_without_lockfile.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.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). (8)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Dylint
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
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-npm-resolver/src/parse_bare_specifier.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
🔇 Additional comments (6)
pacquet/crates/package-manager/Cargo.toml (1)
14-37: LGTM!pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs (1)
170-176: LGTM!pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs (1)
112-120: LGTM!pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (2)
268-268: LGTM!The serialization error is now correctly propagated as a
ResolveErrorinstead of being silently swallowed. This addresses the previous review feedback.
1-46: LGTM!Also applies to: 48-99, 101-176, 178-231, 233-267, 269-333
pacquet/crates/package-manager/src/install_package_from_registry.rs (1)
215-222: LGTM!
After landing the dedup-after-symlink reordering, every parent→child edge to a shared dep fired the per-package signals (`pnpm:progress resolved`, `pnpm:progress imported`, plus the `DownloadTarballToStore` events) once for each visit. That inflates the reporter's per-package counts and breaks parity with pnpm, which fires the resolved/imported channels exactly once per `(name, version)`. Thread a `first_visit: bool` through `InstallPackageFromRegistry`: * `first_visit == true` runs the full path — emit `resolved`, download + cache + import the tarball, emit `imported`, symlink the parent. * `first_visit == false` only refreshes the per-parent symlink under `<node_modules_dir>/<alias>`. No progress emits, no redundant download/import. `install_without_lockfile.rs` computes `first_visit` from the `resolved_packages.insert(...)` return value and threads it in; the same boolean still gates the children recursion below the call. Adds `second_visit_skips_progress_emits_but_still_links` against the mock registry to pin the contract: a future refactor that moves the gate can't quietly reintroduce per-edge spam. --- Written by an agent (Claude Code, claude-opus-4-7).
* npm_resolver: route registry picks via `(alias, bare_specifier)` so an npm-alias entry like `"foo": "npm:@acme/bar@^1"` resolves through `registries[@acme]` instead of the local alias's empty scope. Ports upstream's bareSpecifier-aware `pickRegistryForPackage` by extending pacquet's `pick_registry_for_package` to take an optional `bare_specifier` and consulting the `npm:@scope/...` form before falling back to the package name's scope. New tests cover the npm-alias scope override and the plain-name fallback. * install_without_lockfile: gate the second-visitor symlink on the first writer's import completion. `ResolvedPackages` is now a `DashMap<String, watch::Sender<bool>>` instead of a bare `DashSet<String>`; the first visitor inserts a `watch` channel, runs the full materialization, and signals `true` afterwards. Concurrent second visitors subscribe to the slot and await the completion bit before issuing `symlink_package`, so a Windows `force_symlink_dir` fall-back to junction creation can't race ahead of the target directory's existence. A dropped first writer (panic / cancel) surfaces as `FirstWriterAborted` so the install fails closed. * install_without_lockfile: thread `minimumReleaseAge` / `minimumReleaseAgeExclude` from `Config` into the resolver's `ResolveOptions`. The previous slice left both unset, so the resolver path silently ignored the policy even though the lockfile-verifier already enforced it. Compiles the exclude patterns via `pacquet_config::version_policy::create_package_version_policy`, matching `build_resolution_verifiers`'s plumbing. Surfaces parse errors as `MinimumReleaseAgeExclude` with code `ERR_PNPM_INVALID_MINIMUM_RELEASE_AGE_EXCLUDE`. --- Written by an agent (Claude Code, claude-opus-4-7).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pacquet/crates/resolving-npm-resolver/src/named_registry.rs`:
- Around line 109-127: The pick_registry_for_package currently falls back to
scope_of(pkg_name) even when bare_specifier is an npm: alias, causing scoped
package names to use the wrong registry; change the logic in
pick_registry_for_package to first call
scope_from_bare_specifier(bare_specifier) and if it returns Some use that scope,
but if bare_specifier is Some and begins with "npm:" do NOT fall back to
scope_of(pkg_name) (treat as unscoped alias and fall through to
registries.get("default")), otherwise (when bare_specifier is None or not an
npm: alias) fall back to scope_of(pkg_name); keep using registries.get(scope)
and registries.get("default").cloned().unwrap_or_default() and add a regression
test alongside the alias tests ensuring an `@scope`: "npm:pkg@..." alias resolves
to default when the npm: target is unscoped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d44349fb-4295-4dff-a30f-e549e4995ca2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.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). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
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-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/named_registry/tests.rspacquet/crates/resolving-npm-resolver/src/named_registry.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
CodeRabbit flagged the local-scope fallback as a bug for `"@private/foo": "npm:lodash@^1"`. The behavior actually matches upstream's `getScope` at <https://github.com/pnpm/pnpm/blob/2a9bd897bf/config/pick-registry-for-package/src/index.ts#L8-L19>: the npm-alias branch only returns a scope when the alias target is itself scoped (`npm:@scope/...`); otherwise it falls through to the local pkgName's scope. The local alias slot is treated as the authoritative signal of which registry the user wants to fetch from (e.g. a private fork of an unscoped package re-published under `@private/`). Adds a regression test (`unscoped_npm_alias_under_scoped_local_name_keeps_local_scope`) locking the upstream-matching behavior in so a future refactor can't quietly diverge from pnpm here. --- Written by an agent (Claude Code, claude-opus-4-7).
`pickRegistryForPackage('@private/foo', 'npm:lodash@^1')` previously
returned `registries["@Private"]`, even though the fetched package
is unscoped `lodash` and doesn't live on the `@private` registry.
The local alias slot is just where the install lands in
`node_modules`; it must not drive registry routing when the npm-
alias target itself is unscoped.
`getScope` now returns `null` in the npm-alias branch when the
target is unscoped (instead of falling through to the local
pkgName's scope), so the call falls through to `registries.default`.
The scoped-target case (`npm:@scope/name`) is unchanged: it still
returns the target's scope, overriding the local key's scope as
before. Adds a TS test covering the bug case.
Mirrored in pacquet's `pick_registry_for_package` (the previously
chained `scope_from_bare_specifier(...).or_else(|| scope_of(pkg_name))`
collapsed into a single branch). The pacquet regression test now
asserts the corrected default-routing behavior.
Thanks to @coderabbitai for catching this.
---
Written by an agent (Claude Code, claude-opus-4-7).
* npm_resolver: drop the dead `LockfileResolution::Registry` branch in `build_resolve_result`. `PackageVersion.dist.tarball` is `String` (non-optional in the picker's payload) so the empty-tarball case the branch guarded never fires, and the install side's `extract_tarball` only handles the `Tarball` shape. The mixed-shape output forced a Registry→URL reconstruction with no payoff. Always emit `Tarball`. * npm_resolver: preserve every `WantedDependency` field in `resolve_latest_impl` instead of rebuilding with `..WantedDependency::default()`. The previous rebuild silently dropped `injected`, `prev_specifier`, and `optional`. Clone the source wanted dep and only synthesize a `bare_specifier` when one isn't already set, matching upstream's `createResolveLatest`. * pick-registry-for-package + pacquet named_registry: add the scoped-local + scoped-target (different scopes) regression case (`"@scope1/foo": "npm:@scope2/bar@^1"` → `registries[@scope2]`) on both sides. The TS suite gained a matching test for symmetry. --- Written by an agent (Claude Code, claude-opus-4-7).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:123
policy_violationsis appended before the globalpackagesdedupe check. If the same resolvedidis encountered via multiple parents, later visitors will still push the samepolicy_violationagain even though the package is already inpackages, leading to duplicate violation entries downstream. Consider only recording the violation on the first visitor (after winning thepackagesgate) or deduping the collection (e.g. by(name, version, code)).
if let Some(violation) = result.policy_violation.clone() {
ctx.policy_violations.lock().await.push(violation);
}
* `resolve_node`'s `Ok(None)` from the resolver chain is no longer
silently flattened away. The resolver-base contract says `None`
means "this resolver declined" — outside `DefaultResolver`'s
internal `??` fall-through, that's a contract violation that
would leave installs missing dependencies (e.g. an unported
`git+ssh://` spec would just disappear from the tree). The tree
walker now surfaces it as a typed `SpecNotSupported` error with
code `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER`, mirroring upstream's
`default-resolver` error and using the same `{alias}@{bare}`
rendering. Updated the regression test to assert the error
rather than an empty tree.
* `resolve_dependency_tree` no longer wraps the context in an
`Arc`. The per-direct-dep futures now borrow `&Ctx` directly;
`try_join_all` keeps the borrow alive for the duration of the
resolve pass, and after the await we move the locked maps out
via `into_inner()`. Removes the `Arc::try_unwrap(...).expect(...)`
that would have panicked on an unexpected outstanding clone — a
panic in a library API is not the right failure mode even when
it "shouldn't happen."
---
Written by an agent (Claude Code, claude-opus-4-7).
`install_subtree` used `Sender::send(true)` to signal that the first writer for a virtual-store slot had finished materializing. But `Sender::send` returns `Err` when the channel has zero receivers — and the watch channel I create per slot has zero receivers at the moment of `send` whenever the first writer finishes before any second visitor subscribes (true for cyclic and diamond graphs: `A → B → A` enters the second `install_subtree(A)` only AFTER the outer writer already ran `IPFR::run` and tried to send). Under `send`, that race silently failed: the sender's stored value stayed `false`, the cycle's inner second-visit subscribed, saw `false`, and `changed().await` blocked forever. CI flagged this on the `should_install_circular_dependencies` test (hung past 6 minutes) plus two `lifecycle_scripts` tests with similar diamond shapes. `send_replace(true)` always writes the value and returns the old one regardless of receiver count, so the freshly-subscribed receiver's first `borrow_and_update()` immediately observes `true` and breaks the wait loop. Added a comment naming the race and why `send_replace` is required. Verified locally: `should_install_circular_dependencies` now passes in 8.5s; full `pacquet-cli` suite (84 tests) passes in 46s. --- Written by an agent (Claude Code, claude-opus-4-7).
`config.minimum_release_age` is `u64` (minutes); the previous `minutes as i64` cast wraps silently when the value exceeds `i64::MAX`, and `chrono::Duration::minutes` can overflow internally before the subtraction even runs. An absurd configured value could produce a cutoff in the wrong direction (everything mature, or nothing mature) rather than failing or ignoring the setting. Use a chain of checked operations at both call sites — the resolve pass in `install_without_lockfile.rs` and the verifier factory in `create_npm_resolution_verifier.rs`: * `i64::try_from(minutes)` → catches the `u64 → i64` boundary. * `chrono::Duration::try_minutes(...)` → catches the minutes-to- internal-units overflow. * `DateTime::checked_sub_signed(...)` → catches the wall-clock underflow. On overflow at any step the cutoff degrades to `None`, leaving the policy inactive for the install instead of fabricating a wrong cutoff. Same shape used at both sites so the resolver-time pick and the lockfile-verification gate degrade identically. --- Written by an agent (Claude Code, claude-opus-4-7).
Summary
Two changes ship together: the bulk is the pacquet refactor described in #11756, plus a TypeScript-side fix to
@pnpm/config.pick-registry-for-packagethat surfaced during review.Pacquet — wire NpmResolver into install (Phases A/B/C of #11756)
parse_bare_specifier.rsandnpm_resolver.rsinpacquet-resolving-npm-resolver.NpmResolverimplements theResolvertrait: parses the bare specifier (including npm-aliasnpm:@scope/name@<spec>and tarball-URL forms — with prefix-anchored name validation), picks a version viapick_package, surfacesminimumReleaseAgeviolations inline viadetect_min_release_age_violation.workspace:specs decline so the chain falls through.published_by/published_by_exclude/dry_runadded toResolveOptions.install_without_lockfile.rsconstructs anNpmResolverat install entry from the config-derived registries map and anInMemoryPackageMetaCachethat's shared across the resolve pass and dropped before the install pass.pacquet-resolving-deps-resolvercrate exposesresolve_dependency_tree: a flatname@version-keyed package map with parent-child edges, concurrent sibling resolution viatry_join_all, per-id dedup gate.install_package_from_registry.rsno longer callsPackage::fetch_from_registry/Package::pinned_version; it takes a pre-resolvedResolveResultand reads tarball URL + integrity offLockfileResolution::Tarball.Additional behaviors landed during review:
minimumReleaseAgepolicy in the resolve pass. Previously only enforced by the lockfile-verification gate; the no-lockfile resolve pass now derivespublished_byand the exclude policy fromConfigso resolver-time picks match the configured policy.SPEC_NOT_SUPPORTED_BY_ANY_RESOLVERsurfaces correctly.resolve_dependency_treenow returns a typed error when the chain returnsOk(None)— silently dropping the edge would leave installs missing transitive deps. Mirrors upstream'sdefault-resolvererror shape.InstallPackageFromRegistrytakes afirst_visit: bool;pnpm:progress resolved/pnpm:progress importedplus the tarball download fire once per(name, version), while the per-parentsymlink_packageruns on every edge. Matches upstream's per-package (not per-edge) reporter contract.ResolvedPackagesis nowDashMap<String, watch::Sender<bool>>; the first writer signals completion afterimport_indexed_dir, so a second visitor'ssymlink_package(which may fall back to a Windows junction requiring an existing target) doesn't race ahead of the materialization. A dropped first-writer task surfaces as a typedFirstWriterAbortederror.pick_registry_for_packageis now bareSpecifier-aware so an entry like"foo": "npm:@acme/bar@^1"routes throughregistries[@acme].TS —
@pnpm/config.pick-registry-for-packageunscoped-target fixA separate bug surfaced during the scope-routing port:
pickRegistryForPackage('@private/foo', 'npm:lodash@^1')was routing throughregistries['@private'], even thoughlodashis unscoped and doesn't live on the@privateregistry.getScopenow returnsnullin the npm-alias branch when the alias target is unscoped (instead of falling through to the local pkgName's scope). Changeset is in.changeset/pick-registry-unscoped-npm-alias.md(patch bump for@pnpm/config.pick-registry-for-packageandpnpm). Added matching tests on both the TS and pacquet sides.Out of scope (left as #11756 follow-ups)
policy_violationfrom the tree (Phase E) — the resolver attaches them per-pick already, but the install layer doesn't yet collect or fail on them.NpmResolveris the only chain entry today; once a second resolver lands,DefaultResolverwill get wired in too.parseBareSpecifier.test.tscorpus port — the parser tests pacquet ships cover the cases the install path exercises; remaining corpus items land alongside Phase F.Closes part of #11756.
Test plan
cargo nextest run -p pacquet-resolving-npm-resolver— 397 tests (parser, resolver, picker, named-registry routing, verifier, trust checks).cargo nextest run -p pacquet-resolving-deps-resolver— 3 tests (walk, dedup, decline → SpecNotSupported).cargo nextest run -p pacquet-package-manager— 264 tests (including 3 mock-registry install-package-from-registry integration tests covering thefirst_visit=true/falsecontract).cargo nextest run -p pacquet-cli --test add— CLI add tests pass end-to-end through the new chain against the mock registry.pnpm --filter @pnpm/config.pick-registry-for-package test— 3 TS tests (existing case + unscoped-target fix + scoped-target wins).just test— full workspace suite passes.just lint(cargo clippy --workspace --all-targets -- --deny warnings) — clean.RUSTDOCFLAGS=-D warnings cargo doc --no-deps --workspace --document-private-items— clean.RUSTFLAGS=-D warnings cargo dylint --all -- --all-targets --workspace— clean (Perfectionist lints).cargo fmt --checkandtaplo format— clean.Written by an agent (Claude Code, claude-opus-4-7).