Conversation
📝 WalkthroughWalkthroughThis PR implements Global Virtual Store (GVS) support for pacquet's fresh-install flow. It precomputes a per-install VirtualStoreLayout with AllowBuildPolicy handling, threads it through the install context, computes per-package slot directories for pnpm-compatible path layouts, registers projects in the global store, and adds an integration test verifying GVS directory structures. ChangesGlobal Virtual Store Fresh-Install Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11819 +/- ##
=======================================
Coverage 87.57% 87.58%
=======================================
Files 203 203
Lines 24024 24045 +21
=======================================
+ Hits 21040 21060 +20
- Misses 2984 2985 +1 ☔ 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.39882321686,
"stddev": 0.05861264828035116,
"median": 2.4007814935600003,
"user": 2.75615978,
"system": 3.62787568,
"min": 2.29565586456,
"max": 2.46081691456,
"times": [
2.40104104156,
2.39980911456,
2.3092606995600002,
2.46081691456,
2.44653682656,
2.37333356756,
2.45798468556,
2.44327150856,
2.29565586456,
2.4005219455600004
]
},
{
"command": "pacquet@main",
"mean": 2.3314319386600006,
"stddev": 0.043223210168798376,
"median": 2.33093983856,
"user": 2.77211728,
"system": 3.59702748,
"min": 2.2537845195600004,
"max": 2.39879511556,
"times": [
2.2966287755600003,
2.30868295356,
2.33037184556,
2.39879511556,
2.2537845195600004,
2.3066033815600004,
2.33150783156,
2.3397356155600004,
2.3838528725600003,
2.36435647556
]
},
{
"command": "pnpm",
"mean": 4.561310107760001,
"stddev": 0.04211830698377037,
"median": 4.563425258560001,
"user": 7.73609398,
"system": 4.02595538,
"min": 4.5068498445600005,
"max": 4.6255847795600005,
"times": [
4.6209129655600005,
4.56636189856,
4.56048861856,
4.5068498445600005,
4.51499433556,
4.50848688656,
4.57317023556,
4.56035710056,
4.57589441256,
4.6255847795600005
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6931113623,
"stddev": 0.022351986600453372,
"median": 0.6894020049,
"user": 0.3855689,
"system": 1.59301588,
"min": 0.6721115689,
"max": 0.7537375759,
"times": [
0.7537375759,
0.6721115689,
0.6894659099,
0.6903377309000001,
0.6933794649,
0.6815809849,
0.6951641569,
0.6842238589,
0.6817742719000001,
0.6893380999000001
]
},
{
"command": "pacquet@main",
"mean": 0.6999855719,
"stddev": 0.03332183121989024,
"median": 0.6833980588999999,
"user": 0.36504059999999994,
"system": 1.5889379799999999,
"min": 0.6751565749,
"max": 0.7755073639,
"times": [
0.7106824849,
0.6827178179,
0.7755073639,
0.6866072479,
0.6751565749,
0.6825495549,
0.7418564749000001,
0.6806302379,
0.6800696619000001,
0.6840782999
]
},
{
"command": "pnpm",
"mean": 2.401711359,
"stddev": 0.1118042383292205,
"median": 2.3677398599000004,
"user": 2.9555865999999997,
"system": 2.20510068,
"min": 2.3173904059000003,
"max": 2.6904074889,
"times": [
2.3691768369000004,
2.3173904059000003,
2.3252566839,
2.6904074889,
2.4033740379000004,
2.3328835699000003,
2.3663028829000003,
2.4342389939,
2.3267135679,
2.4513691219
]
}
]
} |
…stall path `pacquet install` (no flag, fresh project) was hardcoded to `VirtualStoreLayout::legacy`, so it materialized packages under the project-local `node_modules/.pnpm/` even when `enableGlobalVirtualStore: true` was configured. Closes #11814. A new `build_lockfile_view_from_resolver_graph` adapter converts the resolver's `DependenciesGraph` into the `snapshots:` / `packages:` shape `VirtualStoreLayout::new` already consumes, so all hashing, slot-dir, and bin-linker code is shared with the frozen-lockfile path. The without-lockfile flow now also calls `register_project` against the shared store when GVS is on, mirroring the frozen-lockfile branch. Side effect: aligns the without-lockfile path's per-package save directory with the peer-suffixed slot the children-recursion was already using, so peer-suffixed snapshots no longer split between two unreachable slot directories.
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 (1)
pacquet/crates/cli/tests/install.rs (1)
495-503: ⚡ Quick winUse precise assertion for project-registry entry count.
The frozen-lockfile test asserts exactly one project entry (
assert_eq!(entries.len(), 1)), but this test uses>= 1. For a single-project install, exactly one registry entry should exist.📏 Suggested refinement for precision
- let project_count = projects_entries.count(); - assert!( - project_count >= 1, - "expected at least one project-registry entry under {projects_dir:?}; got {project_count}", - ); + let project_entries: Vec<_> = projects_entries.collect::<Result<_, _>>().expect("read project entries"); + assert_eq!( + project_entries.len(), + 1, + "expected exactly one project-registry entry for a single-project install under {projects_dir:?}", + );This matches the precision of the frozen-lockfile test at line 1868 in
install/tests.rs.🤖 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/cli/tests/install.rs` around lines 495 - 503, The test currently allows >=1 project-registry entries but should assert exactly one for a single-project install; change the assertion that uses project_count (computed from projects_entries.count()) to assert_eq!(project_count, 1) and update the failure message to reflect the expected exact count, keeping references to canonical_store and projects_dir to locate the check.
🤖 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/cli/tests/install.rs`:
- Around line 495-503: The test currently allows >=1 project-registry entries
but should assert exactly one for a single-project install; change the assertion
that uses project_count (computed from projects_entries.count()) to
assert_eq!(project_count, 1) and update the failure message to reflect the
expected exact count, keeping references to canonical_store and projects_dir to
locate the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33fa66fb-57a2-48cb-9302-f5ff27d6407b
⛔ Files ignored due to path filters (1)
pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
pacquet/crates/cli/tests/_utils.rspacquet/crates/cli/tests/install.rspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.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_with_fresh_lockfile.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/testing-utils/src/bin.rs
💤 Files with no reviewable changes (1)
- pacquet/crates/cli/tests/pnpm_compatibility.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: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Compile & Lint
🧰 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/testing-utils/src/bin.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/cli/tests/_utils.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/cli/tests/install.rspacquet/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_with_fresh_lockfile.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/cli/tests/_utils.rspacquet/crates/cli/tests/install.rs
🧠 Learnings (2)
📚 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/testing-utils/src/bin.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/cli/tests/_utils.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/cli/tests/install.rspacquet/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_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/testing-utils/src/bin.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/cli/tests/_utils.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/cli/tests/install.rspacquet/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_with_fresh_lockfile.rs
🔇 Additional comments (17)
pacquet/crates/cli/tests/_utils.rs (1)
4-24: LGTM!pacquet/crates/testing-utils/src/bin.rs (1)
69-82: LGTM!pacquet/crates/package-manager/src/install_package_from_registry.rs (1)
21-62: LGTM!Also applies to: 98-137
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
143-167: LGTM!Also applies to: 226-267, 340-357, 444-458
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (4)
151-156: LGTM!
742-765: LGTM!
797-814: LGTM!Also applies to: 842-842, 629-642
488-531: Silent node-probe fallback toengine_name: Noneis intentional
- The code intentionally degrades to
Noneonspawn_blocking/detect_node_majorfailure (JoinError + detection failure both map toNone), andVirtualStoreLayout::newexplicitly relies on preserving theOptionshape becauseNonevsSome("")produce different GVS hashes.- When
engine_nameisNone,BuildModulesdisables the side-effects cache (engine_name.is_some()gates both read/write), matching the documented “avoid poisoning the cache/GVS hash” behavior used elsewhere (installabilitysynthetic fallback +node_detectedflag).- No warning is currently emitted for this probe-failure path, consistent with the existing design that treats undetected Node as a safe fallback (and intentionally bypasses cache/GVS engine contributions rather than failing or logging noise).
pacquet/crates/package-manager/src/virtual_store_layout.rs (1)
89-96: LGTM!pacquet/crates/package-manager/src/install.rs (2)
640-677: LGTM!
717-735: LGTM!pacquet/crates/package-manager/src/install/tests.rs (1)
82-92: LGTM!pacquet/crates/cli/tests/install.rs (5)
449-470: LGTM!
472-479: LGTM!
481-482: LGTM!
484-493: LGTM!
505-506: LGTM!
Summary
pacquet install(no flag, fresh project) hardcodedVirtualStoreLayout::legacy, so it silently materialized packages into the project-localnode_modules/.pnpm/even whenenableGlobalVirtualStore: truewas set. Now it routes through<store_dir>/v11/links/<scope>/<name>/<version>/<hash>/...like the frozen-lockfile path does.build_lockfile_view_from_resolver_graphadapter (resolver'sDependenciesGraph→snapshots:/packages:maps) so all hashing, slot-dir, and bin-linker code is shared with the frozen-lockfile path throughVirtualStoreLayout::new(...). Stops short of writingpnpm-lock.yaml(the full writable-lockfile adapter is pacquet: port the writable-lockfile install path (resolver → Lockfile adapter) #11813's scope).pacquet_store_dir::register_projecton the workspace root after the fresh-resolve install when GVS is on, mirroring the frozen-lockfile branch so the prune sweep can find this consumer of<store_dir>/links/....Test plan
fresh_install_honors_enable_global_virtual_store(pacquet/crates/cli/tests/install.rs) asserts both the GVS slot path under<store_dir>/v11/links/and the project-registry entry under<store_dir>/v11/projects/afterpacquet installwith no lockfile.cargo nextest run -p pacquet-package-manager— 283 tests pass.cargo nextest run -p pacquet-cli— 94 tests pass, including the existing GVS parity tests (same_global_virtual_store_layout_pure_js,same_global_virtual_store_layout_diamond).just lintclean,just fmtclean.Notes
pacquet_testing_utils::bin::CommandTempCwd::add_mocked_registrynow pinsenableGlobalVirtualStore: falsein the generatedpnpm-workspace.yamlso tests are hermetic regardless of any GVS opt-in the developer has set in their global pnpm config (~/.config/pnpm/config.yamlon XDG-style hosts,~/Library/Preferences/pnpm/config.yamlon macOS by default). The existingenable_gvs_in_workspace_yamlhelper now flips that line in-place rather than appending — pnpm rejects duplicate top-level mapping keys.install/tests.rswas updated:@pnpm/xyz@1.0.0has peer deps on@pnpm/x/@pnpm/y/@pnpm/z, so its slot now correctly lands at the peer-suffixed name (matching the frozen-lockfile path and pnpm). The previous snapshot encoded the pre-existing inconsistency whereinstall_subtreeandinstall_package_from_registrydisagreed on whether to include peers in the slot name.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests
Bug Fixes