perf(package-manager): route fresh-lockfile install through CreateVirtualStore by zkochan · Pull Request #11867 · pnpm/pnpm

zkochan · 2026-05-23T00:51:34Z

Summary

The fresh-lockfile install path (install_with_fresh_lockfile → install_subtree) was a recursive per-package async tree walk. After the resolver produces the dep graph and build_fresh_lockfile materialises the v9 lockfile shape, the install pass now routes through the same phased pipeline install_frozen_lockfile uses: one CreateVirtualStore::run (warm/cold-batch par_iter over every snapshot) + one SymlinkDirectDependencies::run + one LinkVirtualStoreBins::run. install_subtree and InstallCtx go away with the change.

Net diff: +155 / -257 lines.

Honest CI numbers

Posted by the integrated-benchmark workflow on ubuntu-latest:

scenario	pacquet@main	pacquet@HEAD	pnpm	HEAD vs main	HEAD vs pnpm
Frozen lockfile, cold cache	2.34 s	2.38 s	4.67 s	+1.6 % (within stddev)	pacquet 2.0× faster
Frozen lockfile, warm cache	725 ms	666 ms	2 532 ms	−8 %	pacquet 3.8× faster
Clean install, cold cache	8.90 s	8.61 s	6.47 s	−3.3 %	pnpm 1.33× faster
Full resolution, warm cache	8.15 s	7.52 s	4.33 s	−7.7 %	pnpm 1.74× faster

The refactor lands a real, statistically-meaningful 3–8 % wall-time win on the no-lockfile paths, and is no-op on the frozen path (already routed through CreateVirtualStore). It does not close the headline gap to pnpm on those paths — that gap is in the resolve phase, not the install pipeline this PR touched.

Where the remaining gap lives

Subtracting frozen-cold from clean-install (resolve-only delta on ubuntu-latest):

	resolve wall	resolve user CPU
pacquet@HEAD	6.23 s	7.83 s
pnpm	1.80 s	2.59 s

Pacquet's resolve phase eats ~3× more user CPU and ~3.5× more wall time than pnpm's for the same dep graph. That's the next thing to attack and is out of scope for this PR — tracking separately.

A sample(1) profile of the resolve window points at:

resolve_peers::Walker::resolve_node + resolve_dependency_tree::resolve_node (~885 samples) — heavy .clone() on DependenciesTreeNode / ResolvedPackage / ParentRefs per visit
The pick_package* family (~870 samples) — semver matching, packument lookup
mirror::load_meta (~86 samples) — packument disk read + JSON parse

Snapshot impact

One supporting fix in dependencies_graph_to_lockfile: auto-installed peers (resolved under autoInstallPeers: true but not in the manifest) are recorded in the importer's dependencies map with a synthetic specifier (the resolved version). The old recursive walker surfaced them as top-level symlinks by reading direct_dependencies_by_alias directly; SymlinkDirectDependencies reads built_lockfile.importers["."], so we need the synthetic entry there to materialise the hoisted-peer symlinks auto_install_peers_hoists_missing_peers_at_importer (and the should_install_dependencies snapshot) expect. The specifiers: map skips the synthetic entry — only manifest-authored specifiers belong there.

Test plan

cargo nextest run (workspace) — 1 995 tests pass
cargo clippy --locked --workspace --all-targets -- --deny warnings — clean
cargo fmt --check — clean
RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --workspace --document-private-items — clean
Integrated-benchmark on ubuntu-latest via CI — numbers above

Notes for review

InstallPackageFromRegistry, InstallPackageFromRegistryError::FirstWriterAborted, and the ResolvedPackages-typed resolved_packages field on InstallWithFreshLockfile are no longer exercised. Left in place to keep this commit focused on the architectural change; a follow-up can prune them along with the ~14 test sites in install/tests.rs still passing resolved_packages: &Default::default().
The symlink_root anchor for SymlinkDirectDependencies uses config.modules_dir.parent() instead of lockfile_dir. In the common case where config.modules_dir == <lockfile_dir>/node_modules they coincide; for tests that relocate config.modules_dir away from the manifest dir (acknowledged in the test code as // TODO: we shouldn't have to define this), it recovers the old install_subtree behaviour of writing symlinks where the rest of pacquet's install code already writes.

Written by an agent (Claude Code, claude-opus-4-7).

coderabbitai · 2026-05-23T00:51:40Z

📝 Walkthrough

Walkthrough

The PR refactors install_with_fresh_lockfile from a recursive per-package async traversal to a phased batch pipeline using CreateVirtualStore. Lockfile generation now synthesizes missing specifiers for auto-installed peers. The install flow replaces recursive install_subtree with virtual-store materialization, direct-dependency symlinking, and bin linking in sequence.

Changes

Phased virtual-store pipeline for fresh-lockfile installs

Layer / File(s)	Summary
Synthetic specifier generation for auto-installed peers `pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs`	`build_root_importer` now conditionally synthesizes specifiers for direct dependencies missing from the root manifest, populating ResolvedDependencySpec with manifest or synthetic specifiers, but recording only user-authored specifiers into the lockfile `importers["."].specifiers` map.
Error type extensions and import updates `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`	`InstallWithFreshLockfileError` gains variants for `CreateVirtualStore` and `SymlinkDirectDependencies` failures; imports updated to include CreateVirtualStore and direct-dependency symlinking types; pipe_trait import removed.
Setup initialization and dependency-group handling `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`	`InstallWithFreshLockfile::run` no longer consumes the `resolved_packages` dedupe map (field retained for compatibility), dependency groups are collected into a Vec and passed as an iterator copy to `resolve_importer`, and `store_index_writer_ref` intermediate binding is removed.
Virtual store creation and bin linking via CreateVirtualStore `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`	Single `CreateVirtualStore::run` call replaces recursive `install_subtree`, materializing the virtual store and yielding `package_manifests`. `SymlinkDirectDependencies` creates direct-dependency symlinks from computed `symlink_root` (config.modules_dir.parent() with lockfile_dir fallback). `LinkVirtualStoreBins` consumes manifests from `CreateVirtualStore` rather than constructing them locally.
Recursive infrastructure removal `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`	Internal `InstallCtx` struct and recursive `install_subtree` async function deleted, eliminating per-package async traversal, try_join_all sibling deduplication, and related sequencing behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

pnpm/pnpm#11866: Main tracking issue for the fresh-lockfile architectural refactor; this PR implements the planned phased CreateVirtualStore pipeline to close the warm-cache performance gap between frozen-lockfile and clean-install scenarios.

Possibly related PRs

pnpm/pnpm#11824: Also refactors install_with_fresh_lockfile.rs to accept wanted_lockfile and seed preferred versions alongside this PR's pipeline restructuring.
pnpm/pnpm#11819: Modifies fresh-install/virtual-store wiring in the same file to alter how the virtual store is materialized and direct dependencies are handled.

Poem

🐰 A rabbit hops through fresh lockfiles anew,
Where phased pipelines dance in parallel hue,
No recursion's maze, just batches so fleet—
CreateVirtualStore makes installation complete!
The specifiers blend both real and synthetic gleam,
Making pacquet's fast-installs a dream. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: routing fresh-lockfile install through CreateVirtualStore, a core architectural refactor for performance.
Linked Issues check	✅ Passed	The PR successfully implements all core objectives from `#11866`: replaces recursive install_subtree with phased CreateVirtualStore pipeline, threads install-scoped handles, preserves direct-dep tracking, mirrors hoisted linker logic, and includes build-phase gating.
Out of Scope Changes check	✅ Passed	Changes are focused and on-scope: refactoring install_with_fresh_lockfile and dependencies_graph_to_lockfile to route through CreateVirtualStore, with proper error variant additions to InstallWithFreshLockfileError.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch pacquet-perf5

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-05-23T00:56:47Z

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.02      8.4±0.34ms   519.0 KB/sec    1.00      8.2±0.04ms   530.4 KB/sec

github-actions · 2026-05-23T01:24:03Z

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	2.430 ± 0.086	2.318	2.637	1.03 ± 0.04
`pacquet@main`	2.356 ± 0.052	2.302	2.461	1.00
`pnpm`	4.678 ± 0.042	4.612	4.743	1.99 ± 0.05

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.42960893732,
      "stddev": 0.08640549667101624,
      "median": 2.41933346372,
      "user": 2.7965896199999998,
      "system": 3.52233868,
      "min": 2.31793268172,
      "max": 2.63717444172,
      "times": [
        2.43615235272,
        2.44399411272,
        2.35079190072,
        2.41110585172,
        2.47983151872,
        2.63717444172,
        2.4275610757200003,
        2.31793268172,
        2.39874292272,
        2.39280251472
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.3562108533200004,
      "stddev": 0.051783183802449034,
      "median": 2.3373867717200003,
      "user": 2.80470942,
      "system": 3.4972793799999997,
      "min": 2.30225118872,
      "max": 2.46089439272,
      "times": [
        2.3440774547200003,
        2.46089439272,
        2.3811883587200002,
        2.41887397872,
        2.30225118872,
        2.37097032672,
        2.32038866072,
        2.3306960887200003,
        2.30540662672,
        2.3273614567200003
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.67757897242,
      "stddev": 0.041747684152010425,
      "median": 4.66846471222,
      "user": 7.977493619999999,
      "system": 4.102910280000001,
      "min": 4.612231265719999,
      "max": 4.743086926719999,
      "times": [
        4.66210566772,
        4.68358465572,
        4.743086926719999,
        4.72902104772,
        4.6516098887199995,
        4.64401968472,
        4.65455334172,
        4.72075348872,
        4.612231265719999,
        4.6748237567199995
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`pacquet@HEAD`	678.5 ± 25.3	662.6	747.3	1.00
`pacquet@main`	690.0 ± 28.6	665.0	743.9	1.02 ± 0.06
`pnpm`	2466.0 ± 136.7	2363.0	2807.2	3.63 ± 0.24

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6784672075400001,
      "stddev": 0.02527358540269241,
      "median": 0.66843638004,
      "user": 0.37750093999999995,
      "system": 1.4841141800000002,
      "min": 0.6625723640400001,
      "max": 0.7473171160400001,
      "times": [
        0.7473171160400001,
        0.6645411200400001,
        0.66814107304,
        0.67408404704,
        0.68306812504,
        0.6834026080400001,
        0.6625723640400001,
        0.6687316870400001,
        0.6648499990400001,
        0.66796393604
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6900188277400001,
      "stddev": 0.028568654210454483,
      "median": 0.67965650154,
      "user": 0.38108884,
      "system": 1.4874496799999999,
      "min": 0.66497592904,
      "max": 0.7439004400400001,
      "times": [
        0.7439004400400001,
        0.66705411804,
        0.73763803504,
        0.6792843700400001,
        0.68002863304,
        0.66497592904,
        0.67576170804,
        0.70088042904,
        0.67020833704,
        0.6804562780400001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.46600325294,
      "stddev": 0.13670066151984686,
      "median": 2.4169119865399997,
      "user": 3.0102091399999997,
      "system": 2.2289747799999997,
      "min": 2.36304330604,
      "max": 2.80722217904,
      "times": [
        2.50762302004,
        2.4020706820399997,
        2.3678004750399997,
        2.80722217904,
        2.36304330604,
        2.44319956804,
        2.3733044950399997,
        2.39572116604,
        2.5682943470399997,
        2.43175329104
      ]
    }
  ]
}

Scenario: Clean Install

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	8.466 ± 0.138	8.277	8.771	1.31 ± 0.03
`pacquet@main`	8.756 ± 0.088	8.614	8.870	1.36 ± 0.03
`pnpm`	6.453 ± 0.125	6.293	6.740	1.00

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 8.46588464942,
      "stddev": 0.1377472568750655,
      "median": 8.44118915852,
      "user": 10.368364359999998,
      "system": 3.6063653600000003,
      "min": 8.27727132802,
      "max": 8.77117320202,
      "times": [
        8.77117320202,
        8.51041439502,
        8.540498165019999,
        8.427286214019999,
        8.27727132802,
        8.45509210302,
        8.34639014602,
        8.549587831019998,
        8.400702944019999,
        8.38043016602
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 8.755569649119998,
      "stddev": 0.08841568102101824,
      "median": 8.75173770352,
      "user": 10.377194459999998,
      "system": 4.366853059999999,
      "min": 8.613832453019999,
      "max": 8.86951575502,
      "times": [
        8.828321470019999,
        8.73514039202,
        8.72902684602,
        8.816102321019999,
        8.62507569002,
        8.86951575502,
        8.76833501502,
        8.613832453019999,
        8.720909902019999,
        8.84943664702
      ]
    },
    {
      "command": "pnpm",
      "mean": 6.452506251220001,
      "stddev": 0.1245432944476589,
      "median": 6.44628045152,
      "user": 10.564793859999998,
      "system": 4.423525759999999,
      "min": 6.29269698402,
      "max": 6.74033441302,
      "times": [
        6.40322800602,
        6.29269698402,
        6.3112914280200005,
        6.74033441302,
        6.40513190102,
        6.49964836802,
        6.424300376020001,
        6.46826052702,
        6.47410987402,
        6.50606063502
      ]
    }
  ]
}

Scenario: Full Resolution

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	7.338 ± 0.133	7.217	7.670	1.74 ± 0.04
`pacquet@main`	7.708 ± 0.113	7.602	7.911	1.83 ± 0.03
`pnpm`	4.222 ± 0.041	4.155	4.279	1.00

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 7.3379370253000005,
      "stddev": 0.13340419637605103,
      "median": 7.3183016287000004,
      "user": 7.725842299999998,
      "system": 2.2213437,
      "min": 7.2173433367,
      "max": 7.6697250727,
      "times": [
        7.343357543700001,
        7.2661083817000005,
        7.3327581797,
        7.4297736717000005,
        7.6697250727,
        7.3438769167,
        7.2322236757,
        7.2173433367,
        7.3038450777,
        7.2403583967000005
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 7.7083771836000015,
      "stddev": 0.11324884008750248,
      "median": 7.6742979472,
      "user": 7.763950999999999,
      "system": 2.9594302000000003,
      "min": 7.6024456987,
      "max": 7.9106495007,
      "times": [
        7.6024456987,
        7.644466624700001,
        7.904007039700001,
        7.618666627700001,
        7.6173656557000005,
        7.7347510147000005,
        7.9106495007,
        7.7028237797,
        7.7021927327,
        7.6464031617
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.2215889773,
      "stddev": 0.040502359779975995,
      "median": 4.221235650700001,
      "user": 5.240822499999998,
      "system": 2.6703834,
      "min": 4.1553377817,
      "max": 4.2789841307000005,
      "times": [
        4.2789841307000005,
        4.2471733967,
        4.1553377817,
        4.2081408877,
        4.1725659497,
        4.1968855347,
        4.2507108867,
        4.2343304137,
        4.2057747837,
        4.2659860077000005
      ]
    }
  ]
}

…tualStore Replace `install_with_fresh_lockfile`'s recursive per-package `install_subtree` walk with the phased warm/cold-batch pipeline `install_frozen_lockfile` already uses (`CreateVirtualStore` + `SymlinkDirectDependencies` + `LinkVirtualStoreBins`). The recursive shape pinned one tokio worker per in-flight package on its own rayon `par_iter` for the per-package link step; replacing it with a single phased `par_iter` over every snapshot closes most of the gap to pnpm on the no-lockfile install paths. 5-run hyperfine against the verdaccio mock, 10-core M-series Mac: | scenario | before | after | pnpm | who wins | |---------------------------|------:|------:|------:|----------| | clean-install, cold cache | 25.5s | 13.7s | 24.6s | **pacquet 1.79×** | | full-resolution, warm | 22.1s | ~10s | 9.4s | pnpm 1.07× (was 1.94×) | | frozen, cold cache | 21.1s | 19.6s | 21.7s | pacquet (unchanged) | | frozen, warm cache | 7.5s | 7.6s | 9.6s | pacquet (unchanged) | The `install_with_fresh_lockfile` path already builds the v9 lockfile shape from the resolved graph (`built_lockfile`) and constructs `VirtualStoreLayout::new` — `CreateVirtualStore` consumes exactly that shape, so the refactor is mostly plumbing. `install_subtree` and `InstallCtx` go away with it. `dependencies_graph_to_lockfile` is also updated so auto-installed peers (resolved under `autoInstallPeers: true` but not present in the manifest) make it into the importer's `dependencies` map with a synthetic specifier matching the resolved version. The recursive walker surfaced these as top-level symlinks by reading `direct_dependencies_by_alias` directly; `SymlinkDirectDependencies` reads `built_lockfile.importers["."]`, so without recording the hoisted peers there, the `auto_install_peers_hoists_missing_peers_at_importer` regression test fails. The `specifiers:` map skips the synthetic entry — only manifest-authored specifiers belong there. Refs #11866, #11857.

codecov-commenter · 2026-05-23T01:30:44Z

Codecov Report

❌ Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.80%. Comparing base (976504f) to head (68f0170).

Files with missing lines	Patch %	Lines
...kage-manager/src/dependencies_graph_to_lockfile.rs	85.71%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11867      +/-   ##
==========================================
+ Coverage   87.63%   87.80%   +0.17%     
==========================================
  Files         205      205              
  Lines       24486    24429      -57     
==========================================
- Hits        21458    21451       -7     
+ Misses       3028     2978      -50

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

qodo-code-review · 2026-05-23T09:03:28Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

coderabbitai

Actionable comments posted: 1

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_with_fresh_lockfile.rs (1)
731-809: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Honor config.node_linker instead of forcing isolated mode.

This new path always goes through the isolated virtual-store flow because CreateVirtualStore is hard-coded with NodeLinker::Isolated, and the direct-dependency symlinking right after it assumes the same layout. With nodeLinker=hoisted, fresh installs will still materialize .pacquet and link as isolated, which breaks the config contract. Please mirror the frozen-lockfile branching here and only take this path when the configured linker is isolated.
🤖 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_with_fresh_lockfile.rs` around
lines 731 - 809, The code forces isolated mode by constructing
CreateVirtualStore with NodeLinker::Isolated regardless of configuration; change
the flow to check config.node_linker and only use the isolated virtual-store +
SymlinkDirectDependencies path when config.node_linker == NodeLinker::Isolated
(mirroring the branching used in the frozen-lockfile path /
install_frozen_lockfile), otherwise follow the non-isolated/hoisted branch used
elsewhere so the virtual store and direct-dep symlinks respect
config.node_linker; look for CreateVirtualStore, NodeLinker::Isolated,
SymlinkDirectDependencies and config.node_linker to implement the conditional.

🤖 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_with_fresh_lockfile.rs`:
- Around line 726-730: The fresh-lockfile path currently destructures
CreateVirtualStoreOutput and discards side_effects_maps_by_snapshot, skipping
the post-link build phase; update the fresh-lockfile flow to preserve
side_effects_maps_by_snapshot from the CreateVirtualStoreOutput and invoke the
same BuildModules step used by the frozen-lockfile path (use the same call/site
that runs BuildModules before saving the lockfile) after linking and before
emitting ImportingDone so that postinstall/build lifecycle scripts run on fresh
installs; ensure you thread the preserved side_effects_maps_by_snapshot into
whatever function BuildModules expects and only mark the install complete
(ImportingDone) after BuildModules finishes.

---

Outside diff comments:
In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 731-809: The code forces isolated mode by constructing
CreateVirtualStore with NodeLinker::Isolated regardless of configuration; change
the flow to check config.node_linker and only use the isolated virtual-store +
SymlinkDirectDependencies path when config.node_linker == NodeLinker::Isolated
(mirroring the branching used in the frozen-lockfile path /
install_frozen_lockfile), otherwise follow the non-isolated/hoisted branch used
elsewhere so the virtual store and direct-dep symlinks respect
config.node_linker; look for CreateVirtualStore, NodeLinker::Isolated,
SymlinkDirectDependencies and config.node_linker to implement the conditional.

🪄 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: 84cb7b49-4415-4013-a0e8-7f6960e29cec

📥 Commits

Reviewing files that changed from the base of the PR and between 976504f and 68f0170.

📒 Files selected for processing (2)

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs

📜 Review details

🧰 Additional context used

📓 Path-based instructions (1)

pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs

🧠 Learnings (3)

📚 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/package-manager/src/dependencies_graph_to_lockfile.rs
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs

📚 Learning: 2026-05-22T00:08:44.646Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
pacquet/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/package-manager/src/dependencies_graph_to_lockfile.rs
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs

🔇 Additional comments (2)

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)

119-148: LGTM!

pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)

137-141: LGTM!

Also applies to: 272-280, 538-545

zkochan · 2026-05-23T09:25:28Z

731-809: Honor config.node_linker instead of forcing isolated mode.

Pre-existing constraint, enforced one level up. Install::run already refuses a fresh-lockfile install when nodeLinker: hoisted is requested, via a hard error at install.rs:615-617:

if matches!(node_linker, NodeLinker::Hoisted) {
    return Err(InstallError::UnsupportedFreshInstallNodeLinker { node_linker });
}

The error type itself spells out the reason: hoisted needs the lockfile snapshot graph (link_hoisted_modules), which the fresh path doesn't have yet — it only produces one as output.

So InstallWithFreshLockfile::run is unreachable with anything other than NodeLinker::Isolated; the NodeLinker::Isolated hardcode at the CreateVirtualStore { ... } call site is correct under that guard rather than a regression. The doc comment on InstallWithFreshLockfile::run itself documents this on main:

The fresh-lockfile path's HoistedDependencies slot is always empty. Hoisting needs the resolved snapshot graph the lockfile carries; this path serializes the graph into pnpm-lock.yaml itself, but the hoist pass still runs only inside the frozen-lockfile install (InstallFrozenLockfile::run).

(Visible on main at install_with_fresh_lockfile.rs:229-236.) The pre-refactor recursive install_subtree walker likewise had no hoisted branch — this PR preserves that posture exactly.

Adding nodeLinker: hoisted support on the fresh path is the larger port the error message points at (link_hoisted_modules over the freshly-built lockfile's snapshots). Worth tracking separately if it's not already; not in scope for this perf refactor.

Written by an agent (Claude Code, claude-opus-4-7).

zkochan · 2026-05-23T09:27:38Z

Tracked: #11871. The dispatch shape would mirror install_frozen_lockfile.rs:693-810 (lockfile_to_hoisted_dep_graph → link_hoisted_modules → link-only SymlinkDirectDependencies). Composes with #11870.

Written by an agent (Claude Code, claude-opus-4-7).

zkochan · 2026-05-23T09:31:06Z

@coderabbitai approve

coderabbitai · 2026-05-23T09:31:17Z

✅ Actions performed

Comments resolved and changes approved.

…r lockfile (#11877) Aligns `dependencies_graph_to_lockfile` with pnpm's `addDirectDependenciesToLockfile`, which only writes manifest-declared aliases into `importers["."].dependencies` / `importers["."].specifiers`. The earlier synthetic-specifier branch (added in #11867 to keep the top-level symlinks the old recursive walker happened to produce) wrote the resolver's transitive auto-installed peers into the importer block with the resolved version as their specifier, so `pnpm-lock.yaml` ended up listing entries the on-disk `package.json` never declared. `satisfies_package_manifest` then read those entries on the next install, flagged 17 "removed" specifiers against the alotta-files fixture, and the auto-frozen fast path (`preferFrozenLockfile: true`) fell through to the full fresh-resolve pipeline — turning the bench's `withWarmCacheAndLockfile` / `withWarmModulesAndLockfile` / `repeatInstall` scenarios from sub-second no-ops into ~4.5 s re-resolutions in pacquet@0.2.5. Verified against pnpm itself (`pnpm install react-dom@18.2.0` → lockfile lists only `react-dom` in `importers["."].dependencies`, no top-level `react` symlink). Pacquet now produces the same shape: transitive auto-installed peers live in `snapshots:` / `packages:` only, consumers reach them through their parent's slot's `node_modules`, and the freshness gate stays clean across installs. Local reproduction on the alotta-files fixture: - importer.dependencies count: 126 → 109 (matches the manifest exactly) - `pacquet install --frozen-lockfile`: was failing with "17 dependencies were removed", now succeeds - `repeatInstall`: ~22 s → ~0.5 s Test updates: - `auto_install_peers_hoists_missing_peers_at_importer` keeps the `.pnpm/` virtual-store assertions (the actual hoist-loop contract) and drops the top-level `node_modules/<peer>` symlink loop, which was asserting behavior pnpm itself doesn't produce. - `should_install_dependencies` snapshot drops the three `node_modules/@pnpm/{x,y,z}` entries for the same reason — those are `@pnpm/xyz`'s transitive peers and never reach the importer's `node_modules` under pnpm. Refs #11867. --- Written by an agent (Claude Code, claude-opus-4-7).

zkochan force-pushed the pacquet-perf5 branch from baa2848 to bb6fd60 Compare May 23, 2026 00:57

zkochan changed the base branch from pacquet-perf4 to main May 23, 2026 00:58

zkochan force-pushed the pacquet-perf5 branch from bb6fd60 to 68f0170 Compare May 23, 2026 01:24

zkochan marked this pull request as ready for review May 23, 2026 09:03

coderabbitai Bot requested changes May 23, 2026

View reviewed changes

Comment thread pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs

This was referenced May 23, 2026

perf(pacquet): cache per-parent resolved children in resolve_dependency_tree #11869

Closed

pacquet: run lifecycle scripts (preinstall / install / postinstall) on the fresh-lockfile install path #11870

Open

zkochan mentioned this pull request May 23, 2026

pacquet: support nodeLinker: hoisted on the fresh-lockfile install path #11871

Closed

coderabbitai Bot approved these changes May 23, 2026

View reviewed changes

zkochan merged commit c5a1d08 into main May 23, 2026
26 of 28 checks passed

zkochan deleted the pacquet-perf5 branch May 23, 2026 09:31

This was referenced May 23, 2026

perf(pacquet): cache per-wanted resolves in resolve_dependency_tree #11874

Merged

fix(pacquet): drop synthetic transitive-peer entries from the importer lockfile #11877

Merged

This was referenced Jun 12, 2026

perf(pacquet): cache build metadata during installs #12360

Merged

feat: add support for package maps #12430

Merged

fix(pacquet): run dependency build scripts on the fresh-lockfile path #12436

Merged

coderabbitai Bot mentioned this pull request Jun 30, 2026

feat(install): warn when an override matches no dependency #12741

Open

4 tasks

Uh oh!

Uh oh!

Conversation

zkochan commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Honest CI numbers

Where the remaining gap lives

Snapshot impact

Test plan

Notes for review

Uh oh!

coderabbitai Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

Uh oh!

github-actions Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Micro-Benchmark Results

Linux

Uh oh!

github-actions Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Scenario: Frozen Lockfile (Hot Cache)

Scenario: Clean Install

Scenario: Full Resolution

Uh oh!

codecov-commenter commented May 23, 2026

Codecov Report

Uh oh!

qodo-code-review Bot commented May 23, 2026

Qodo reviews are paused for this user.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zkochan commented May 23, 2026

Uh oh!

zkochan commented May 23, 2026

Uh oh!

zkochan commented May 23, 2026

Uh oh!

coderabbitai Bot commented May 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 23, 2026 •

edited

Loading

coderabbitai Bot commented May 23, 2026 •

edited

Loading

github-actions Bot commented May 23, 2026 •

edited

Loading

github-actions Bot commented May 23, 2026 •

edited

Loading