feat(pacquet): write pnpm-lock.yaml and <vsd>/lock.yaml on fresh install#11816
Conversation
Adds a `dependencies_graph_to_lockfile` adapter that converts the resolver's `DependenciesGraph` into a v9 `Lockfile`, hooks it into the install path so a fresh `pacquet install` produces a wanted lockfile, renames `InstallWithoutLockfile` to `InstallWithFreshLockfile` to match the new behavior, drops the `UnsupportedLockfileMode` branch from the dispatch, and flips the `config.lockfile` default from `false` to `true` to match pnpm. Closes #11813.
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? |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughAdds a resolver→pnpm v9 lockfile adapter and a fresh-install flow (InstallWithFreshLockfile) that resolves dependencies, materializes node_modules, and optionally writes pnpm-lock.yaml; non-frozen installs now route through the fresh-lockfile path. ChangesFresh Lockfile Install Path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
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 #11816 +/- ##
==========================================
+ Coverage 87.48% 87.54% +0.06%
==========================================
Files 202 203 +1
Lines 23704 24022 +318
==========================================
+ Hits 20737 21031 +294
- Misses 2967 2991 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Drop the broken `[ResolvedPackage.optional]` intra-doc link (no such item in scope; the reference is upstream's, and the prose is enough without the link). - Disambiguate two `dependencies_graph_to_lockfile` intra-doc links to the function form so rustdoc doesn't error on the function/module collision. - Flatten an `assert!(prod.contains_key(...))` so rustfmt + dylint agree on the body (rustfmt wanted the call wrapped, dylint wanted a trailing comma; extracting the key into a local resolves both).
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.rs (1)
613-644:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFresh installs currently bypass
--no-runtimeand hoisted linking.This branch now routes every non-frozen install through
InstallWithFreshLockfile, but that path never receivesskip_runtimesornode_linkerand still hard-codes the isolated virtual-store/bin flow. A writable install can therefore still materialize*@runtime:packages under--no-runtime, andnodeLinker: hoistedwill later write hoisted metadata without ever doing the hoisted layout work. Please gate those modes here or teach the fresh-lockfile path to honor them before routing everything through it. Based on learnings: only match pnpm’s behavior exactly.🤖 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.rs` around lines 613 - 644, The fresh-install branch currently always constructs and runs InstallWithFreshLockfile without honoring skip_runtimes or node_linker; update this branch to either (A) pass the skip_runtimes and node_linker settings into InstallWithFreshLockfile (add fields to the struct and thread them through its run logic so it skips runtime packages when skip_runtimes is true and performs hoisted layout when node_linker == NodeLinker::Hoisted), or (B) gate the branch so that when skip_runtimes is true or node_linker is hoisted you do not route through InstallWithFreshLockfile and instead invoke the existing writable-install path that respects those modes; locate the construction/use of InstallWithFreshLockfile in this else branch and modify it accordingly (symbols: InstallWithFreshLockfile, skip_runtimes, node_linker, run) so fresh-lockfile installs match pnpm behavior.
🤖 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.rs`:
- Around line 613-644: The fresh-install branch currently always constructs and
runs InstallWithFreshLockfile without honoring skip_runtimes or node_linker;
update this branch to either (A) pass the skip_runtimes and node_linker settings
into InstallWithFreshLockfile (add fields to the struct and thread them through
its run logic so it skips runtime packages when skip_runtimes is true and
performs hoisted layout when node_linker == NodeLinker::Hoisted), or (B) gate
the branch so that when skip_runtimes is true or node_linker is hoisted you do
not route through InstallWithFreshLockfile and instead invoke the existing
writable-install path that respects those modes; locate the construction/use of
InstallWithFreshLockfile in this else branch and modify it accordingly (symbols:
InstallWithFreshLockfile, skip_runtimes, node_linker, run) so fresh-lockfile
installs match pnpm behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0eaf07b-6d49-4419-a314-2f5c1a397315
📒 Files selected for processing (11)
pacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/link_bins.rspacquet/crates/package-manager/src/store_init.rspacquet/crates/package-manager/src/virtual_store_layout.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: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- 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/package-manager/src/store_init.rspacquet/crates/package-manager/src/lib.rspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/package-manager/src/link_bins.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.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/pnpm_compatibility.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/package-manager/src/store_init.rspacquet/crates/package-manager/src/lib.rspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/package-manager/src/link_bins.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.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/package-manager/src/store_init.rspacquet/crates/package-manager/src/lib.rspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/package-manager/src/link_bins.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/virtual_store_layout.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🔇 Additional comments (8)
pacquet/crates/cli/tests/pnpm_compatibility.rs (1)
48-59: LGTM!Also applies to: 129-137
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
1-453: LGTM!pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
1-434: LGTM!pacquet/crates/config/src/lib.rs (1)
317-323: LGTM!Also applies to: 902-907
pacquet/crates/package-manager/src/lib.rs (1)
10-10: LGTM!Also applies to: 20-20, 42-42, 52-52
pacquet/crates/package-manager/src/link_bins.rs (1)
531-532: LGTM!Also applies to: 567-568
pacquet/crates/package-manager/src/store_init.rs (1)
13-13: LGTM!pacquet/crates/package-manager/src/virtual_store_layout.rs (1)
54-55: LGTM!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
348-352: ⚡ Quick winRefresh stale "without-lockfile" wording left over from the rename.
The module/struct rename
InstallWithoutLockfile→InstallWithFreshLockfile(per the PR summary) didn't get propagated into several inline comments, which now contradict the new identity:
- Line 351:
(install-without-lockfile)— this path does now produce a wanted lockfile on success.- Lines 514-516:
No lockfile here, so no prefetched manifests are available — fall back to the legacy readdir-driven path— there is now a wanted lockfile being written below; the rationale is no input lockfile, not no lockfile produced.- Line 525:
the without-lockfile fallback stays project-local— should say "the fresh-lockfile path".- Line 537:
The without-lockfile path has no installability check— same fix.As per coding guidelines: "Doc comments document the contract … not a re-narration of the body" and "Write code that explains itself." Comments that describe the wrong module name actively mislead readers.
📝 Suggested wording tightening
- // Seed `allPreferredVersions` from the importer manifest. No - // wanted lockfile is available on this path (install-without- - // lockfile), so the lockfile-snapshot arm of upstream's - // `getPreferredVersionsFromLockfileAndManifests` is skipped. + // Seed `allPreferredVersions` from the importer manifest. The + // fresh-lockfile path has no *input* wanted lockfile to read + // from, so the lockfile-snapshot arm of upstream's + // `getPreferredVersionsFromLockfileAndManifests` is skipped.- // No lockfile here, so no prefetched manifests are available — - // fall back to the legacy readdir-driven path (slots discovered + // No input lockfile here, so no prefetched manifests are + // available — fall back to the legacy readdir-driven path + // (slots discovered // by walking `<virtual_store_dir>`, child manifests read from // disk). The frozen-lockfile path skips both via // [`LinkVirtualStoreBins::snapshots`] / `package_manifests`. @@ // The bin linker also doesn't need GVS-aware slot lookups // here: without snapshots there are no GVS slot directories to // compute. Construct a legacy layout so the readdir path // enumerates `config.virtual_store_dir` exactly as before. GVS - // is scoped to frozen-lockfile installs (pnpm/pacquet#432); the - // without-lockfile fallback stays project-local. + // is scoped to frozen-lockfile installs (pnpm/pacquet#432); the + // fresh-lockfile path stays project-local. @@ - // The without-lockfile path has no installability check + // The fresh-lockfile path has no installability check // (no `packages:` metadata to evaluate constraints // against), so the skip set is empty by definition.Also applies to: 514-540
🤖 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 348 - 352, Update the inline comments in the InstallWithFreshLockfile implementation to reflect the rename and correct semantics: replace mentions of "install-without-lockfile" / "without-lockfile" with "install-with-fresh-lockfile" or "fresh-lockfile path", and rephrase comments that say "no lockfile here" or "no prefetched manifests are available" to clarify that there is no input/wanted lockfile provided (we do produce a wanted lockfile on success) and that the fallback is used because there is no input lockfile rather than no lockfile ever being written; also change comments stating "fallback stays project-local" and "has no installability check" to refer to the fresh-lockfile path and ensure doc-comments describe the contract (no input lockfile) instead of re-narrating the implementation.
🤖 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/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 348-352: Update the inline comments in the
InstallWithFreshLockfile implementation to reflect the rename and correct
semantics: replace mentions of "install-without-lockfile" / "without-lockfile"
with "install-with-fresh-lockfile" or "fresh-lockfile path", and rephrase
comments that say "no lockfile here" or "no prefetched manifests are available"
to clarify that there is no input/wanted lockfile provided (we do produce a
wanted lockfile on success) and that the fallback is used because there is no
input lockfile rather than no lockfile ever being written; also change comments
stating "fallback stays project-local" and "has no installability check" to
refer to the fresh-lockfile path and ensure doc-comments describe the contract
(no input lockfile) instead of re-narrating the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: baaa333d-008d-40aa-9dc9-c00e1e09127a
📒 Files selected for processing (3)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.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: 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: Code Coverage
- 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/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.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/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.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/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🔇 Additional comments (3)
pacquet/crates/package-manager/src/install/tests.rs (1)
3316-3322: LGTM!pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (2)
153-185: ⚡ Quick winProvide the original review comment
The<review_comment>text wasn’t included in this message, so it can’t be rewritten. Paste the full comment inside<review_comment>...</review_comment>tags (and any relevant code diff/context if present).
201-218: ⚡ Quick winMissing input: No
<review_comment>...</review_comment>content was provided, so the comment can’t be rewritten. Paste the original review comment (including any diff snippets) to update it.
After a fresh install, also persist the current-lockfile alongside `pnpm-lock.yaml` so the next install can diff each snapshot against it and skip the unchanged slots — the same optimization the frozen path already enables. `InstallWithFreshLockfile::run` now returns an `InstallWithFreshLockfileResult` that surfaces the freshly-built `Lockfile` to the caller. `install.rs` saves it as `<virtual_store_dir>/lock.yaml` after `.modules.yaml` succeeds, mirroring the frozen path's safety property: a manifest-write failure can't leave a current-lockfile pointing at incomplete install state. The wanted lockfile and the current lockfile describe the same resolved graph here — the resolver only walked what the install requested, so no `filter_lockfile_for_current` step is needed. Both writes are gated on `config.lockfile`, matching upstream pnpm's `useLockfile` opt-out.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5408476234800004,
"stddev": 0.15462157178991556,
"median": 2.5013643622800004,
"user": 2.7992381799999997,
"system": 3.8154784999999998,
"min": 2.37844763528,
"max": 2.9098844462800004,
"times": [
2.69009161128,
2.5071646162800003,
2.4744684722800003,
2.51946435428,
2.9098844462800004,
2.49556410828,
2.5606359872800004,
2.44214828528,
2.43060671828,
2.37844763528
]
},
{
"command": "pacquet@main",
"mean": 2.46918359988,
"stddev": 0.10511146593091505,
"median": 2.42899736428,
"user": 2.73340818,
"system": 3.7771707999999995,
"min": 2.3769306822800003,
"max": 2.7021144022800003,
"times": [
2.59174674628,
2.3769306822800003,
2.44719052728,
2.39942209928,
2.41130233128,
2.40423942428,
2.7021144022800003,
2.51886837228,
2.39332901628,
2.44669239728
]
},
{
"command": "pnpm",
"mean": 4.946452239379999,
"stddev": 0.10754342690239742,
"median": 4.91340914928,
"user": 8.424833379999999,
"system": 4.318414,
"min": 4.86280080928,
"max": 5.21804608828,
"times": [
4.86280080928,
4.91541158128,
4.98456384528,
4.88172614728,
4.8665065932800005,
4.91140671728,
4.99963104328,
4.95655521028,
5.21804608828,
4.86787435828
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.69146338696,
"stddev": 0.03725543016499666,
"median": 0.67779749436,
"user": 0.38109413999999997,
"system": 1.5736930800000002,
"min": 0.66189599236,
"max": 0.77323406836,
"times": [
0.77323406836,
0.67753499636,
0.67805999236,
0.68393169036,
0.66892552336,
0.67400708836,
0.66189599236,
0.66819020536,
0.68208783836,
0.74676647436
]
},
{
"command": "pacquet@main",
"mean": 0.6965649234600001,
"stddev": 0.0331158838534993,
"median": 0.68206800536,
"user": 0.37236323999999993,
"system": 1.5849725799999999,
"min": 0.67138371036,
"max": 0.77428649336,
"times": [
0.77428649336,
0.67138371036,
0.72730078336,
0.68266589336,
0.68147011736,
0.67665074636,
0.67216552036,
0.71615302636,
0.68470933836,
0.67886360536
]
},
{
"command": "pnpm",
"mean": 2.63577996426,
"stddev": 0.12243979722329187,
"median": 2.58391270486,
"user": 3.2916098399999996,
"system": 2.26093718,
"min": 2.55703794236,
"max": 2.90485451736,
"times": [
2.63602216336,
2.55703794236,
2.56007765936,
2.90485451736,
2.56104711536,
2.56431157336,
2.5864349553599997,
2.59065221836,
2.81597104336,
2.58139045436
]
}
]
} |
…otEntry `SnapshotEntry.optional` was hard-coded to `false`, so every snapshot looked "non-optional" and `BuildModules` would treat any build failure as fatal even for packages reachable only via `optionalDependencies`. Port upstream pnpm's `ResolvedPackage.optional` propagation: 1. `ResolvedPackage` (the dedup envelope) gains an `optional: bool` field. The walker seeds it from `wanted.optional || parent.optional` on the first visit and AND-folds it with `current_is_optional` on every subsequent visit, so a single non-optional path flips it back to `false` and keeps it there. Mirrors resolveDependencies.ts:1625-1630. 2. `extract_children` now emits a per-child `is_optional` flag — `true` when the entry came from the package's `optionalDependencies` map. 3. `resolve_dependency_tree` and `resolve_importer` look up the importer's `optionalDependencies` set and tag each direct dep, then propagate the flag down the recursion. 4. `DependenciesGraphNode` carries the resolved package's `optional` field so peer-variants of the same `pkgIdWithPatchHash` share it. 5. The lockfile adapter writes `SnapshotEntry.optional = node.optional` instead of hard-coding `false`. Tests: - 4 unit tests on the resolver (direct optional seed, transitive inheritance, AND-fold when a non-optional path exists, transitive via a parent's `optionalDependencies` edge). - 1 unit test on the adapter (`SnapshotEntry.optional` round-trips). - 1 integration test through `Install` asserting a top-level `optionalDependencies` entry produces `optional: true` in the written `pnpm-lock.yaml`. Each test was verified to catch a regression by temporarily breaking the implementation before re-checking it green.
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.rs (1)
629-645:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFresh installs still drop install-mode flags.
This branch always dispatches into
InstallWithFreshLockfile, but it never forwardsskip_runtimesornode_linker. That means a non-frozen--no-runtimeinstall will still resolve/materialize runtime packages, andnodeLinker: hoistedwill still build the isolated.pacquetlayout while.modules.yamlrecordshoisted, leaving the next install/rebuild to interpret the tree incorrectly. Please thread these modes through the fresh installer or fail fast for unsupported combinations before writing state files.🤖 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.rs` around lines 629 - 645, The fresh-install branch calls InstallWithFreshLockfile but never forwards the install-mode flags (skip_runtimes and node_linker), so runtime packages and nodeLinker behavior are incorrect; update the call site to either include these flags in the InstallWithFreshLockfile struct (add fields skip_runtimes and node_linker and pass the current values from the caller) and ensure InstallWithFreshLockfile::run uses them, or perform a pre-check before creating/writing state files to fail fast for unsupported combinations (e.g., when node_linker mismatches or skip_runtimes is set) so state is not written incorrectly; locate the constructor invocation of InstallWithFreshLockfile and the run call to apply the fix.
🤖 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.rs`:
- Around line 629-645: The fresh-install branch calls InstallWithFreshLockfile
but never forwards the install-mode flags (skip_runtimes and node_linker), so
runtime packages and nodeLinker behavior are incorrect; update the call site to
either include these flags in the InstallWithFreshLockfile struct (add fields
skip_runtimes and node_linker and pass the current values from the caller) and
ensure InstallWithFreshLockfile::run uses them, or perform a pre-check before
creating/writing state files to fail fast for unsupported combinations (e.g.,
when node_linker mismatches or skip_runtimes is set) so state is not written
incorrectly; locate the constructor invocation of InstallWithFreshLockfile and
the run call to apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c47ed727-2002-4472-9daf-942cabc0674e
📒 Files selected for processing (11)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/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: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: Analyze (javascript)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-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-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.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/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_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/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
🔇 Additional comments (19)
pacquet/crates/resolving-deps-resolver/src/dependencies_graph.rs (1)
43-51: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (8)
131-141: LGTM!
143-153: LGTM!
265-284: LGTM!
307-312: LGTM!
353-385: LGTM!
396-419: LGTM!
538-566: LGTM!
445-465: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (4)
43-44: LGTM!
151-166: LGTM!
220-228: LGTM!
244-250: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs (1)
500-500: LGTM!pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs (1)
91-106: LGTM!pacquet/crates/resolving-deps-resolver/src/tests.rs (2)
863-889: LGTM!
891-1105: LGTM!pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
365-371: LGTM!Also applies to: 399-399
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
52-72: LGTM!Also applies to: 85-85, 284-284, 457-525
…or --no-runtime The fresh-lockfile dispatch silently dropped `skip_runtimes` and `node_linker` — neither was forwarded into `InstallWithFreshLockfile`, so a fresh `pacquet install` with `--node-linker=hoisted` produced an isolated `node_modules` layout, and `--no-runtime` materialized runtime archives anyway. Pacquet's hoist pass and runtime-snapshot filter both run only against a loaded lockfile (frozen-lockfile path); honoring the flags on a fresh install needs upstream's per-snapshot filter and a hoist pass over the freshly-built graph. Both ports are out of scope for this PR. Refuse the unsupported combinations up front instead, before any reporter event fires or any state file is written, so a follow-up retry under `--frozen-lockfile` against an existing lockfile lands on the supported path. Adds `UnsupportedFreshInstallNodeLinker` and `UnsupportedFreshInstallSkipRuntimes` error variants plus integration tests asserting both that the error fires and that no `pnpm-lock.yaml`, `lock.yaml`, or `.modules.yaml` ends up on disk.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install/tests.rs (1)
3788-3791: ⚡ Quick winAlso assert that workspace state is not written.
Install::runhas one more persisted artifact after these files:.pnpm-workspace-state-v1.json. Without checking that too, these tests don't fully pin the "before writing state" guarantee.🧪 Suggested assertion
assert!(!dir.path().join(Lockfile::FILE_NAME).exists(), "no wanted lockfile written"); assert!(!virtual_store_dir.join(Lockfile::CURRENT_FILE_NAME).exists(), "no current lockfile"); assert!(!modules_dir.join(".modules.yaml").exists(), "no modules manifest"); + assert!( + load_workspace_state(dir.path()).expect("read workspace state").is_none(), + "no workspace state" + );Also applies to: 3836-3839
🤖 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/tests.rs` around lines 3788 - 3791, Add an assertion to also verify the workspace state file is not written: check that dir.path().join(".pnpm-workspace-state-v1.json") does not exist (similar to the existing asserts for Lockfile::FILE_NAME, Lockfile::CURRENT_FILE_NAME, and ".modules.yaml") in the failing-install tests in pacquet/crates/package-manager/src/install/tests.rs (the same change should be mirrored in the other test block around the 3836–3839 range); locate the assertions following the Install::run call and append assert!(!dir.path().join(".pnpm-workspace-state-v1.json").exists(), "no workspace state written").
🤖 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.rs`:
- Around line 127-153: The display messages for
UnsupportedFreshInstallNodeLinker and UnsupportedFreshInstallSkipRuntimes are
incorrect: they say "fresh install" but these errors are raised for any
non-frozen run (i.e., when frozen_lockfile is false). Update the #[display(...)]
strings for UnsupportedFreshInstallNodeLinker (and its node_linker mention) and
UnsupportedFreshInstallSkipRuntimes to refer to a "non-frozen install" (or
"non-frozen run") and keep the guidance about re-running with --frozen-lockfile
or changing the config/flag; ensure the diagnostic code names
(pacquet_package_manager::unsupported_fresh_install_node_linker and
pacquet_package_manager::unsupported_fresh_install_skip_runtimes) and enum
variant names (UnsupportedFreshInstallNodeLinker,
UnsupportedFreshInstallSkipRuntimes) remain unchanged.
---
Nitpick comments:
In `@pacquet/crates/package-manager/src/install/tests.rs`:
- Around line 3788-3791: Add an assertion to also verify the workspace state
file is not written: check that dir.path().join(".pnpm-workspace-state-v1.json")
does not exist (similar to the existing asserts for Lockfile::FILE_NAME,
Lockfile::CURRENT_FILE_NAME, and ".modules.yaml") in the failing-install tests
in pacquet/crates/package-manager/src/install/tests.rs (the same change should
be mirrored in the other test block around the 3836–3839 range); locate the
assertions following the Install::run call and append
assert!(!dir.path().join(".pnpm-workspace-state-v1.json").exists(), "no
workspace state written").
🪄 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: 1c8f1f5f-9abf-495b-96b6-c3884c5a4518
📒 Files selected for processing (2)
pacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/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: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Dylint
- GitHub Check: Lint and Test (windows-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/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.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/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.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/install.rspacquet/crates/package-manager/src/install/tests.rs
🔇 Additional comments (1)
pacquet/crates/package-manager/src/install.rs (1)
280-309: LGTM!
…en installs The `UnsupportedFreshInstall*` errors fire on any `!frozen_lockfile` run, not just first installs — pacquet doesn't honor an existing lockfile without `--frozen-lockfile` yet (stale-lockfile rewrite is a follow-up). So a re-install with a pinned `pnpm-lock.yaml` but no `--frozen-lockfile` would hit the same gate, and the "on a fresh install yet" wording pointed users at the wrong condition. Reword the display strings to "without --frozen-lockfile yet" so the prompt matches the actual gate. Variant names and diagnostic codes stay put — the next round will rename them along with the fresh-install-path semantics. Surfaced by coderabbitai review on #11816.
…write_lock (#11825) The verifier was racing in-flight writers: while `ensure_file` held `cas_write_lock(path)` and was partway through `write_all`, a sibling snapshot's `check_pkg_files_integrity` would stat the same CAS path (no lock), see a partial size, and call `remove_stale_cafs_entry(path)`. The writer's `write_all` then continued against an orphan inode, the writer's `cas_paths` was populated with the now-deleted path, and `link_file` later hit ENOENT — the CI failure shape on #11816 / 0.2.2-7. Fix (Option C): keep `verify_file`'s lock-free fast path (the common case: file unchanged since prior install, `is_modified` false), but acquire `cas_write_lock(path)` before any branch that could call `remove_stale_cafs_entry`. Re-`check_file` under the lock so a writer's full `write_all` lands before we evaluate. Performance: the fast path adds zero overhead. The slow path — files whose mtime is > 100 ms past the recorded `checked_at` — takes one uncontended Mutex acquire per file, sub-microsecond on uncontended locks. Contention only fires when a writer + a verifier hit the same blob simultaneously; the wait is bounded by one `write_all` and trades a millisecond-scale wait for avoiding a network re-fetch. The new integration test in `pacquet-store-dir/tests/` is a deterministic reproducer: it acquires `cas_write_lock` from the test thread (standing in for an in-flight writer), pre-seeds a partial CAS file at the matching path, and runs the verifier in the background. Pre-fix, the verifier unlinks the file while the "writer" is still simulated as in-progress; post-fix, the verifier blocks on the lock until released. To make `cas_write_lock` reachable from the store-dir crate the function was promoted from `fn` to `pub fn` in pacquet-fs.
Summary
Ports the writable-lockfile install path from upstream pnpm so a fresh
pacquet installproduces bothpnpm-lock.yamland<virtual_store_dir>/lock.yaml, instead of discarding the resolver's output. Closes #11813.What lands here:
DependenciesGraph → Lockfileadapter. Ports upstream'supdateLockfile+ thepackages:/snapshots:split thatconvertToLockfileFileapplies on write, fanning the resolver's depPath-keyed graph into the v9 wire shape:importers["."]carriesspecifiers+ classified dep groups, keyed by the manifest's declared alias.packagescarriesPackageMetadataper peer-stripped depPath (pkgIdWithPatchHash).snapshotscarries oneSnapshotEntryper peer-suffixed depPath.pnpm-lock.yamlafter materialization succeeds, and<virtual_store_dir>/lock.yamlafter.modules.yamllands — same ordering and safety property the frozen path uses, so a manifest-write failure can't leave a current-lockfile pointing at incomplete install state. Both writes gate onconfig.lockfile.config.lockfiledefaults totrueso a plainpacquet installwrites a lockfile, matching upstream pnpm'suseLockfiledefault.SnapshotEntry.optionalpropagation. Ports upstream'sResolvedPackage.optionalAND-fold: a snapshot is marked optional only when every path from any importer goes through anoptionalDependenciesedge. Without this,BuildModulestreats every failure as fatal even for deps reached only through an optional consumer.InstallWithoutLockfile→InstallWithFreshLockfile. TheUnsupportedLockfileModeerror variant is gone; non-frozen installs always run the resolver + writer path.node_linker: hoistedorskip_runtimes: true(CLI--no-runtime) now errors up front. Both flags rely on filters that run only against a loaded lockfile (the hoist pass inlink_hoisted_modules, the runtime-snapshot filter in the frozen-lockfile materializer); honoring them on a fresh install needs a port of those filters against the freshly-built graph, which is out of scope here. The gate fires before any reporter event or state file, so a follow-up retry under--frozen-lockfilelands on the supported path.Out of scope
skip_runtimeson the fresh path. Needs upstream's runtime-snapshot filter ported against the in-memory graph.VirtualStoreLayout::legacy; flipping to GVS-when-enabled is a separate design decision.preferFrozenLockfiledispatch. When a lockfile exists but doesn't matchpackage.json, we should re-resolve and rewrite. Tracked separately.Tests
dependencies_graph_to_lockfile::tests::*, 6 tests) — direct-dep shape, dev/optional split, peer-suffixed key generation, optional-children partitioning,transitivePeerDependenciessorting,SnapshotEntry.optionalround-trip.tests::optional_propagation::*, 4 tests) — importer seed, transitive inheritance, AND-fold across mixed paths, manifest-leveloptionalDependenciesedge.fresh_install_writes_pnpm_lock_yaml_with_expected_shapefresh_install_splits_dev_and_prod_dependency_sectionsfresh_install_records_user_written_specifierfresh_install_lockfile_round_trips_through_load_save_loadfresh_install_with_lockfile_disabled_does_not_write_a_lockfilefresh_install_also_writes_current_lockfile_under_virtual_storefresh_install_with_lockfile_disabled_skips_current_lockfile_toofresh_install_marks_optional_snapshots_in_pnpm_lock_yamlfresh_install_refuses_hoisted_node_linker_before_writing_statefresh_install_refuses_skip_runtimes_before_writing_statepnpm_compatibilitytests now clean uppnpm-lock.yamlbetween the pacquet and pnpm halves so the second tool can't pick a different code path than the first.Test plan
cargo nextest run -p pacquet-resolving-deps-resolver— 50 testscargo nextest run -p pacquet-package-manager install::tests— 46 testscargo nextest run -p pacquet-package-manager dependencies_graph_to_lockfile— 6 testscargo nextest run -p pacquet-cli --test pnpm_compatibility— 5 testscargo fmt --all -- --check,just check,just lint,just dylintWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes