This repository was archived by the owner on May 14, 2026. It is now read-only.
Conversation
…438 slice 1) Port pnpm's `importIndexedDir(..., { force: true, keepModulesDir: true })` from `fs/indexed-pkg-importer/src/importIndexedDir.ts` as the foundation for the hoisted node-linker. Slice 1 of #438. `import_indexed_dir` lives next to `create_cas_files` in `crates/package-manager/`. Fresh targets delegate straight to `create_cas_files`; existing targets are rebuilt by staging the new contents in a sibling directory, moving any pre-existing `node_modules/` into the stage to preserve nested deps, removing the old directory, then renaming the stage into place. Symlinks and regular files at the target path are unlinked before the fresh-target branch runs. Slice 1 is stand-alone — no install-pipeline wiring yet. Subsequent slices (`linkHoistedModules` analog, hoist algorithm port, `Install::run` branch on `node_linker`) will consume this primitive. Upstream reference: `pnpm/pnpm@94240bc0` — https://github.com/pnpm/pnpm/blob/94240bc0/fs/indexed-pkg-importer/src/importIndexedDir.ts
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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). (6)
📝 WalkthroughWalkthroughAdds import_indexed_dir: materializes CAS-indexed package files into a target directory, branching on path kind and using a staged sibling directory to atomically swap in new contents while optionally preserving an existing node_modules/. Includes rescue logic and extensive tests for edge cases and concurrency. ChangesIndexed Directory Import with Staging/Swap
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new package-manager primitive for hoisted-linker work: importing an “indexed” package into a real directory while overwriting existing contents but preserving an existing node_modules/ subtree. This is intended as a foundational building block for later hoisted-linker slices.
Changes:
- Introduces
import_indexed_dirwith a “fresh target” path (delegates tocreate_cas_files) and an “existing target” stage-and-swap path that preservesnode_modules/. - Exposes the new primitive via
pacquet-package-manager’s public API surface. - Adds stand-alone unit tests covering fresh installs, overwrite semantics,
node_modulespreservation, collisions, hardlink behavior, and basic concurrency.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/package-manager/src/lib.rs | Registers and re-exports the new import_indexed_dir module. |
| crates/package-manager/src/import_indexed_dir.rs | Implements the stage-and-swap import primitive with node_modules preservation and new error types. |
| crates/package-manager/src/import_indexed_dir/tests.rs | Adds unit tests validating overwrite + preserve semantics and several edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
==========================================
- Coverage 86.98% 86.81% -0.18%
==========================================
Files 93 93
Lines 6647 6803 +156
==========================================
+ Hits 5782 5906 +124
- Misses 865 897 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/package-manager/src/import_indexed_dir/tests.rs (1)
274-321: ⚡ Quick winTighten the stage-path collision test.
Using
pkg-aandpkg-bmeans this still passes ifpick_stage_path()becomes deterministic per target name, so it doesn't actually protect the "every call gets a unique sibling path" invariant described in the comment. Reusing the same target basename or asserting onpick_stage_path()directly would make the regression signal much stronger.🤖 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 `@crates/package-manager/src/import_indexed_dir/tests.rs` around lines 274 - 321, The test uses distinct basenames ("pkg-a"/"pkg-b") so a deterministic per-target basename in pick_stage_path() could still pass; change the test to force the same target basename (e.g., create two different parent dirs but use the same final basename such as "pkg") or directly call pick_stage_path() twice and assert the returned sibling paths are different to ensure every call yields a unique sibling path; update the concurrent_imports_into_different_targets_do_not_collide test to either (a) create targets like tmp/.../t1/pkg and tmp/.../t2/pkg and invoke import_indexed_dir on both, or (b) call pick_stage_path() twice (via its public/internal API) and assert the two paths differ.
🤖 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 `@crates/package-manager/src/import_indexed_dir.rs`:
- Around line 137-142: The current stage_and_swap() flow unconditionally removes
the staging tree on any error, which can delete the only remaining copy after a
failed rename; instead implement a safe swap: first rename the existing dir_path
to a backup name (e.g., dir_path.with_extension(".backup")), then rename stage
to dir_path, and only after the final rename succeeds remove the backup
(fs::remove_dir_all on the backup). On any failure during the swap, attempt to
restore by renaming the backup back to dir_path and do not remove stage until
success; update all code paths that call fs::remove_dir_all(&stage) (and the
analogous 172-200 region) to follow this backup-then-rename-then-cleanup pattern
so one valid tree always survives.
- Around line 166-185: The code treats all errors from
fs::symlink_metadata(&target_modules) as "no node_modules", which can silently
drop unreadable node_modules; change the match so that Err(e) is only ignored
when e.kind() == std::io::ErrorKind::NotFound and all other Err(e) values are
propagated as an ImportIndexedDirError::PreserveModulesDir (use the same from:
target_modules, to: stage_modules, error: e shape) before continuing; keep the
existing Ok(meta) if meta.file_type().is_dir() branch and its rename/error
mapping unchanged and ensure stage_modules is constructed once
(stage.join("node_modules")) so you can reference it in the error case.
---
Nitpick comments:
In `@crates/package-manager/src/import_indexed_dir/tests.rs`:
- Around line 274-321: The test uses distinct basenames ("pkg-a"/"pkg-b") so a
deterministic per-target basename in pick_stage_path() could still pass; change
the test to force the same target basename (e.g., create two different parent
dirs but use the same final basename such as "pkg") or directly call
pick_stage_path() twice and assert the returned sibling paths are different to
ensure every call yields a unique sibling path; update the
concurrent_imports_into_different_targets_do_not_collide test to either (a)
create targets like tmp/.../t1/pkg and tmp/.../t2/pkg and invoke
import_indexed_dir on both, or (b) call pick_stage_path() twice (via its
public/internal API) and assert the two paths differ.
🪄 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: e8f87350-39f2-42a0-a31b-d932deb241f8
📒 Files selected for processing (3)
crates/package-manager/src/import_indexed_dir.rscrates/package-manager/src/import_indexed_dir/tests.rscrates/package-manager/src/lib.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: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Dylint
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/lib.rscrates/package-manager/src/import_indexed_dir.rscrates/package-manager/src/import_indexed_dir/tests.rs
🧠 Learnings (2)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/lib.rscrates/package-manager/src/import_indexed_dir.rscrates/package-manager/src/import_indexed_dir/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/import_indexed_dir/tests.rs
…438 slice 1 review) Address two review findings on the new `import_indexed_dir` primitive: 1. Data loss on cleanup. The blanket `remove_dir_all(stage)` after any post-step-3 failure would delete a `node_modules/` that had already been moved into the staging directory. Replace it with a step-by-step error path that, when the preserved `node_modules/` is in stage, renames it back onto `dir_path/node_modules/` before the staging directory is cleaned up. The user's nested deps are never the staging directory's exclusive copy past the rescue point. 2. Swallowed inspect errors. `Ok(_) | Err(_) => {}` for the pre-existing `node_modules/` lookup hid `PermissionDenied` and transient I/O errors, which would let the subsequent `remove_dir_all` silently clobber nested deps. Only `NotFound` is now treated as benign; everything else surfaces as `InspectTarget`. Also fix the `cargo doc --document-private-items` workspace job by disambiguating the intra-doc links to `create_cas_files()` and `link_file()` — both names resolve to a function *and* a module, and rustdoc rejects the ambiguity under `-D warnings`. Tests: - `remove_dir_all_failure_restores_preserved_node_modules` — pins the rescue path by chmod'ing a subdir of `dir_path` to 0o500 so `remove_dir_all` fails after step 3. - `node_modules_inspect_permission_denied_surfaces` — proves `PermissionDenied` on the `node_modules/` inspect returns `InspectTarget` and cleans up staging.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/package-manager/src/import_indexed_dir.rs (1)
194-212:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep the staged
node_modules/when rescue cannot put it back.These failure paths still call
remove_dir_all(&stage)even whenrescue_node_modulescannot restorestage/node_modulestodir_path/node_modules, or whencreate_dir_all(dir_path)fails before the rescue attempt. In both cases that cleanup deletes the only remaining copy of the preserved nested deps.💡 Localized fix
- rescue_node_modules(nm_moved, &stage_modules, &target_modules); - let _ = fs::remove_dir_all(&stage); + let restored = rescue_node_modules(nm_moved, &stage_modules, &target_modules); + if restored { + let _ = fs::remove_dir_all(&stage); + } return Err(ImportIndexedDirError::RemoveExisting { path: dir_path.to_path_buf(), error }); @@ - if nm_moved && fs::create_dir_all(dir_path).is_ok() { - rescue_node_modules(nm_moved, &stage_modules, &target_modules); - } - let _ = fs::remove_dir_all(&stage); + let restored = if nm_moved && fs::create_dir_all(dir_path).is_ok() { + rescue_node_modules(nm_moved, &stage_modules, &target_modules) + } else { + !nm_moved + }; + if restored { + let _ = fs::remove_dir_all(&stage); + } return Err(ImportIndexedDirError::Swap { from: stage, to: dir_path.to_path_buf(), error }); @@ -fn rescue_node_modules(nm_moved: bool, stage_modules: &Path, target_modules: &Path) { +fn rescue_node_modules(nm_moved: bool, stage_modules: &Path, target_modules: &Path) -> bool { if !nm_moved { - return; + return true; } if let Err(error) = fs::rename(stage_modules, target_modules) { tracing::warn!( target: "pacquet::import_indexed_dir", ?stage_modules, ?target_modules, %error, "failed to restore preserved node_modules/ after a partial stage-and-swap; \ - the staged copy is at the source path until cleanup runs", + leaving the staged copy in place for manual recovery", ); + return false; } + true }Also applies to: 222-235
🤖 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 `@crates/package-manager/src/import_indexed_dir.rs` around lines 194 - 212, The staged node_modules can be lost because the code always calls fs::remove_dir_all(&stage) even when rescue_node_modules fails or when fs::create_dir_all(dir_path) fails; update the failure paths around fs::rename and fs::remove_dir_all so we only delete the staged tree when the rescue actually succeeded (or when nm_moved is false). Concretely, change rescue_node_modules usage (in the ImportIndexedDir handling around the fs::rename error and the earlier RemoveExisting path) so rescue_node_modules returns a success indicator (bool or Result) and branch: if nm_moved && create_dir_all(dir_path).is_ok() then call rescue_node_modules and only call fs::remove_dir_all(&stage) when that rescue reports success; if create_dir_all fails or rescue fails, keep the stage directory intact and return the appropriate ImportIndexedDirError::Swap/RemoveExisting without removing &stage. Ensure the same change is applied to the other identical block (lines 222–235).
🤖 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.
Duplicate comments:
In `@crates/package-manager/src/import_indexed_dir.rs`:
- Around line 194-212: The staged node_modules can be lost because the code
always calls fs::remove_dir_all(&stage) even when rescue_node_modules fails or
when fs::create_dir_all(dir_path) fails; update the failure paths around
fs::rename and fs::remove_dir_all so we only delete the staged tree when the
rescue actually succeeded (or when nm_moved is false). Concretely, change
rescue_node_modules usage (in the ImportIndexedDir handling around the
fs::rename error and the earlier RemoveExisting path) so rescue_node_modules
returns a success indicator (bool or Result) and branch: if nm_moved &&
create_dir_all(dir_path).is_ok() then call rescue_node_modules and only call
fs::remove_dir_all(&stage) when that rescue reports success; if create_dir_all
fails or rescue fails, keep the stage directory intact and return the
appropriate ImportIndexedDirError::Swap/RemoveExisting without removing &stage.
Ensure the same change is applied to the other identical block (lines 222–235).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3def5535-9f9b-42a0-89ad-74c1f500dffc
📒 Files selected for processing (2)
crates/package-manager/src/import_indexed_dir.rscrates/package-manager/src/import_indexed_dir/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). (5)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/import_indexed_dir/tests.rscrates/package-manager/src/import_indexed_dir.rs
🧠 Learnings (2)
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/import_indexed_dir/tests.rs
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/import_indexed_dir/tests.rscrates/package-manager/src/import_indexed_dir.rs
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.542566411,
"stddev": 0.18120085827116897,
"median": 2.5104989657,
"user": 2.59279526,
"system": 3.4377771000000004,
"min": 2.3595975386999997,
"max": 3.0190684577,
"times": [
2.6041347227,
2.4221024747,
3.0190684577,
2.5449075286999996,
2.4953513196999997,
2.5372520336999997,
2.4521905126999997,
2.5256466117,
2.3595975386999997,
2.4654129097
]
},
{
"command": "pacquet@main",
"mean": 2.4278898943000002,
"stddev": 0.07336233112681019,
"median": 2.4275294942,
"user": 2.57392406,
"system": 3.3619006,
"min": 2.3366916936999997,
"max": 2.5245525616999998,
"times": [
2.4811947527,
2.3547917827,
2.3999603247,
2.3366916936999997,
2.3773671196999997,
2.5245525616999998,
2.5189407447,
2.4855246397,
2.3447766597,
2.4550986637
]
},
{
"command": "pnpm",
"mean": 5.7673685189,
"stddev": 0.060386920291060624,
"median": 5.7694976857,
"user": 8.414824460000002,
"system": 4.3127346,
"min": 5.6615186617,
"max": 5.8471316307,
"times": [
5.7558004437,
5.8119701397,
5.8453684527,
5.6615186617,
5.7526916207,
5.7297811917,
5.6984457877,
5.8471316307,
5.7877823327,
5.7831949277
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6894869912199999,
"stddev": 0.03671669988806593,
"median": 0.6785652153200001,
"user": 0.35553241999999996,
"system": 1.4944203000000003,
"min": 0.6606604583200001,
"max": 0.7876375693200001,
"times": [
0.7876375693200001,
0.68546889332,
0.69239461032,
0.6703537363200001,
0.66635466732,
0.6805108833200001,
0.6606604583200001,
0.70305830432,
0.67181124232,
0.6766195473200001
]
},
{
"command": "pacquet@main",
"mean": 0.7600168821200001,
"stddev": 0.04860632457110471,
"median": 0.75712953982,
"user": 0.36844252,
"system": 1.5265053,
"min": 0.69254372032,
"max": 0.8424329073200001,
"times": [
0.81362090032,
0.7133948303200001,
0.79896983732,
0.75548616932,
0.70326965732,
0.75877291032,
0.8424329073200001,
0.7682595643200001,
0.75341832432,
0.69254372032
]
},
{
"command": "pnpm",
"mean": 2.3725066152200003,
"stddev": 0.0649832620662973,
"median": 2.38116730432,
"user": 2.83153612,
"system": 2.1741208999999992,
"min": 2.26856430232,
"max": 2.47068268232,
"times": [
2.47068268232,
2.37907712232,
2.31553233332,
2.41809692432,
2.32459121532,
2.31241654332,
2.26856430232,
2.43981695932,
2.3832574863200002,
2.41303058332
]
}
]
} |
…_dir (#438 slice 1) Per maintainer feedback: pnpm uses one function for both node-linkers parameterized by opts. Collapse pacquet's two-function split into the same shape. `create_cas_files` and the previously-separate `import_indexed_dir` fold into a single `import_indexed_dir(.., opts: ImportIndexedDirOpts)` that services both linkers: - Default opts (isolated linker): existing-target short-circuit, fresh-target mkdir + parallel link. Matches what `create_cas_files` did and what the virtual-store callers need. - `force: true` (hoisted linker): stage-and-swap re-import. - `force: true` + `keep_modules_dir: true` (hoisted linker): also preserve `dir_path/node_modules/` with the same rescue logic as before. The two existing isolated-linker callers (`create_virtual_dir_by_snapshot`, `install_package_from_registry`) now call `import_indexed_dir(.., ImportIndexedDirOpts::default())`. The error type renames from `CreateCasFilesError` to `ImportIndexedDirError`. `link_file`'s docstring updates to point at the new entry point. Tests stay equivalent: same behaviors covered, but `FORCE_KEEP` / `FORCE_ONLY` constants make the call shapes explicit. A new `existing_target_short_circuits_under_default_opts` pins the isolated-linker invariant that an existing dir is never re-imported when opts are defaulted, and `force_without_keep_clobbers_node_modules` covers the `force` + `keep_modules_dir: false` parameter combination. Upstream reference: `pnpm/pnpm@94240bc0` — - https://github.com/pnpm/pnpm/blob/94240bc0/fs/indexed-pkg-importer/src/importIndexedDir.ts - https://github.com/pnpm/pnpm/blob/94240bc0/store/controller-types/src/index.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/package-manager/src/import_indexed_dir.rs (1)
285-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid deleting the staging tree when
node_modulesrestoration may have failed.After
nm_moved == true,stagecan contain the only recoverablenode_modulescopy. In both failure branches,stageis removed unconditionally even if restoration cannot run (e.g.,create_dir_all(dir_path)fails) or restoration itself fails.💡 Minimal hardening patch
- if let Err(error) = fs::remove_dir_all(dir_path) { - rescue_node_modules(nm_moved, &stage_modules, &target_modules); - let _ = fs::remove_dir_all(&stage); + if let Err(error) = fs::remove_dir_all(dir_path) { + let restored = rescue_node_modules(nm_moved, &stage_modules, &target_modules); + if !nm_moved || restored { + let _ = fs::remove_dir_all(&stage); + } return Err(ImportIndexedDirError::RemoveExisting { path: dir_path.to_path_buf(), error }); } @@ - if let Err(error) = fs::rename(&stage, dir_path) { - if nm_moved && fs::create_dir_all(dir_path).is_ok() { - rescue_node_modules(nm_moved, &stage_modules, &target_modules); - } - let _ = fs::remove_dir_all(&stage); + if let Err(error) = fs::rename(&stage, dir_path) { + let restored = if nm_moved && fs::create_dir_all(dir_path).is_ok() { + rescue_node_modules(nm_moved, &stage_modules, &target_modules) + } else { + !nm_moved + }; + if restored { + let _ = fs::remove_dir_all(&stage); + } return Err(ImportIndexedDirError::Swap { from: stage, to: dir_path.to_path_buf(), error }); }-fn rescue_node_modules(nm_moved: bool, stage_modules: &Path, target_modules: &Path) { +fn rescue_node_modules(nm_moved: bool, stage_modules: &Path, target_modules: &Path) -> bool { if !nm_moved { - return; + return true; } if let Err(error) = fs::rename(stage_modules, target_modules) { tracing::warn!( @@ "failed to restore preserved node_modules/ after a partial stage-and-swap; \ the staged copy is at the source path until cleanup runs", ); + return false; } + true }🤖 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 `@crates/package-manager/src/import_indexed_dir.rs` around lines 285 - 303, The staging tree (`stage`) is being removed unconditionally even when `nm_moved == true` and restoration of `node_modules` may have failed; update the error branches around fs::remove_dir_all(dir_path) and fs::rename(&stage, dir_path) so that you only call fs::remove_dir_all(&stage) after successful rescue; specifically, when nm_moved is true: attempt to recreate the target dir with fs::create_dir_all(dir_path) and run rescue_node_modules(nm_moved, &stage_modules, &target_modules), and only if that sequence succeeds remove the staging tree (`stage`); if create_dir_all or the rescue step fails, do not delete `stage` and return the appropriate ImportIndexedDirError::{RemoveExisting, Swap} so the recoverable `node_modules` remains intact. Ensure these changes touch the error handling around ImportIndexedDirError::RemoveExisting and ImportIndexedDirError::Swap and keep logic for nm_moved == false unchanged.
🤖 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.
Duplicate comments:
In `@crates/package-manager/src/import_indexed_dir.rs`:
- Around line 285-303: The staging tree (`stage`) is being removed
unconditionally even when `nm_moved == true` and restoration of `node_modules`
may have failed; update the error branches around fs::remove_dir_all(dir_path)
and fs::rename(&stage, dir_path) so that you only call
fs::remove_dir_all(&stage) after successful rescue; specifically, when nm_moved
is true: attempt to recreate the target dir with fs::create_dir_all(dir_path)
and run rescue_node_modules(nm_moved, &stage_modules, &target_modules), and only
if that sequence succeeds remove the staging tree (`stage`); if create_dir_all
or the rescue step fails, do not delete `stage` and return the appropriate
ImportIndexedDirError::{RemoveExisting, Swap} so the recoverable `node_modules`
remains intact. Ensure these changes touch the error handling around
ImportIndexedDirError::RemoveExisting and ImportIndexedDirError::Swap and keep
logic for nm_moved == false unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98b84d39-af74-480c-8ee7-018d1f2a7e46
📒 Files selected for processing (7)
crates/package-manager/src/create_cas_files.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/package-manager/src/import_indexed_dir.rscrates/package-manager/src/import_indexed_dir/tests.rscrates/package-manager/src/install_package_from_registry.rscrates/package-manager/src/lib.rscrates/package-manager/src/link_file.rs
💤 Files with no reviewable changes (1)
- crates/package-manager/src/create_cas_files.rs
✅ Files skipped from review due to trivial changes (1)
- crates/package-manager/src/link_file.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/package-manager/src/import_indexed_dir/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/install_package_from_registry.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/package-manager/src/lib.rscrates/package-manager/src/import_indexed_dir.rs
🧠 Learnings (1)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/install_package_from_registry.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/package-manager/src/lib.rscrates/package-manager/src/import_indexed_dir.rs
🔇 Additional comments (4)
crates/package-manager/src/import_indexed_dir.rs (1)
15-281: LGTM!Also applies to: 308-349
crates/package-manager/src/lib.rs (1)
10-10: LGTM!Also applies to: 33-33
crates/package-manager/src/create_virtual_dir_by_snapshot.rs (1)
1-4: LGTM!Also applies to: 62-62, 94-110
crates/package-manager/src/install_package_from_registry.rs (1)
2-2: LGTM!Also applies to: 61-61, 179-186
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
crates/package-manager/src/import_indexed_dir.rs:304
- On
fs::rename(stage, dir_path)failure, the code only attempts anode_modules/rescue ifcreate_dir_all(dir_path)succeeds, and then it unconditionally removes the staging dir. Ifcreate_dir_allor the rescue rename fails, the subsequentremove_dir_all(stage)can delete the preservedstage/node_modules. To uphold the “no data loss” invariant, consider keeping the stage directory (or at least not deletingstage/node_modules) when rescue is not confirmed to have succeeded, and surface the stage path in the error/logs for manual recovery.
if let Err(error) = fs::rename(&stage, dir_path) {
if nm_moved && fs::create_dir_all(dir_path).is_ok() {
rescue_node_modules(nm_moved, &stage_modules, &target_modules);
}
let _ = fs::remove_dir_all(&stage);
return Err(ImportIndexedDirError::Swap { from: stage, to: dir_path.to_path_buf(), error });
}
crates/package-manager/src/import_indexed_dir.rs:326
- The warning in
rescue_node_modulessays “the staged copy is at the source path until cleanup runs”, but all current callers immediately run a staging cleanup (remove_dir_all(stage)), which would remove that source path if the rescue rename fails. Either avoid cleaning up the stage when rescue fails (preferred, to prevent data loss) or adjust the log message to accurately describe what remains on disk and where recovery is possible.
if let Err(error) = fs::rename(stage_modules, target_modules) {
tracing::warn!(
target: "pacquet::import_indexed_dir",
?stage_modules,
?target_modules,
%error,
"failed to restore preserved node_modules/ after a partial stage-and-swap; \
the staged copy is at the source path until cleanup runs",
);
}
…ks correctly on Windows (#438 slice 1 review) Address two new review findings on the unified import primitive: 1. Rescue failure used to be silently followed by an unconditional `remove_dir_all(stage)`, which would destroy the user's only remaining copy of the preserved `node_modules/` when the restoration rename couldn't run (permissions, transient I/O). `restore_preserved_node_modules` now returns whether the restore actually completed, and the combined cleanup helper `finalize_stage_cleanup_after_failure` only rimrafs the staging directory when the restore succeeded or wasn't needed. On restore failure the staging directory is left on disk and a warning emits the path so an operator can recover manually. 2. The non-directory-with-force unlink path called `fs::remove_file` directly. On Windows that rejects directory symlinks and junctions (the OS treats them as directory-shaped), which would wedge the overwrite path the moment a stale dir symlink occupied the target. `remove_non_dir_dirent` now resolves the link on Windows and routes through `fs::remove_dir` when the target is a directory; Unix continues to use `remove_file` unchanged. The Unix path has no behavior change. Existing tests still pass; the rescue-failure leak path is hard to drive deterministically in a unit test (it requires both `remove_dir_all` and the subsequent `fs::rename` to fail in the same configuration) so the new path is covered by docstring + tracing emit rather than a dedicated test. The Windows path is exercised on CI's windows-latest job.
This was referenced May 13, 2026
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Six items from the Copilot review on #449: 1. **Gate `register_project` on frozen-lockfile** — the call previously fired for any GVS-on install, including the legacy `InstallWithoutLockfile` path that uses `VirtualStoreLayout::legacy` and never references `<store_dir>/links/...`. Tightened to `frozen_lockfile && config.enable_global_virtual_store` so a registry entry only appears for installs that actually consume the shared store. 2. **`VirtualStoreLayout::new` engine: `Option<&str>`** — the param was `&str` and the call site passed `engine_name.as_deref() .unwrap_or("")`, which turned a missing `node` into `Some("")` and hashed to a different value than `None` (the latter omits the `engine` contribution from `calc_graph_node_hash`). Switched the signature to `Option<&str>` and the call site to `engine_name.as_deref()` so the missing-engine case round-trips correctly. Test call sites updated. 3. **`run_emits_imported_event_after_create_cas_files`** — renamed to `run_emits_imported_event_after_import_indexed_dir` (and updated the doc comment) since `create_cas_files` was renamed to `import_indexed_dir` in #441. 4. **`VirtualStoreLayout::package_store_dir` doc** — the field doc still claimed the root was `Config::virtual_store_dir` in both modes. Updated to reflect the post-split design: it's `Config::global_virtual_store_dir` under GVS, `virtual_store_dir` otherwise. 5. **Test manifest placement** — the new `frozen_lockfile_under_gvs_registers_project_and_runs_clean` and `frozen_lockfile_with_gvs_off_skips_project_registry` tests created the manifest at `<tmp>/package.json` while `project_root` was `<tmp>/project/`. `Install::run` derives the registry target from `manifest.path().parent()`, so the registry entry actually pointed at `<tmp>`, not the intended project root. Moved the manifest into `project_root` and replaced the entry-count-only check with a `read_link` + canonicalize assertion that the symlink resolves back to `project_root`. 6. **Preserve yaml-set `globalVirtualStoreDir`** — `apply_global_virtual_store_derivation` unconditionally overwrote the field, so a user-pinned `globalVirtualStoreDir:` in `pnpm-workspace.yaml` was silently lost. Added a `global_virtual_store_dir_explicit` parameter that short-circuits the derivation when yaml provided the value, mirroring the pre-existing `virtual_store_dir_explicit` plumbing. New `yaml_global_virtual_store_dir_wins_over_derivation` test pins the behaviour. All 704 workspace tests pass; `taplo --check`, `just dylint`, `cargo doc -D warnings` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Six items from the Copilot review on #449: 1. **Gate `register_project` on frozen-lockfile** — the call previously fired for any GVS-on install, including the legacy `InstallWithoutLockfile` path that uses `VirtualStoreLayout::legacy` and never references `<store_dir>/links/...`. Tightened to `frozen_lockfile && config.enable_global_virtual_store` so a registry entry only appears for installs that actually consume the shared store. 2. **`VirtualStoreLayout::new` engine: `Option<&str>`** — the param was `&str` and the call site passed `engine_name.as_deref() .unwrap_or("")`, which turned a missing `node` into `Some("")` and hashed to a different value than `None` (the latter omits the `engine` contribution from `calc_graph_node_hash`). Switched the signature to `Option<&str>` and the call site to `engine_name.as_deref()` so the missing-engine case round-trips correctly. Test call sites updated. 3. **`run_emits_imported_event_after_create_cas_files`** — renamed to `run_emits_imported_event_after_import_indexed_dir` (and updated the doc comment) since `create_cas_files` was renamed to `import_indexed_dir` in #441. 4. **`VirtualStoreLayout::package_store_dir` doc** — the field doc still claimed the root was `Config::virtual_store_dir` in both modes. Updated to reflect the post-split design: it's `Config::global_virtual_store_dir` under GVS, `virtual_store_dir` otherwise. 5. **Test manifest placement** — the new `frozen_lockfile_under_gvs_registers_project_and_runs_clean` and `frozen_lockfile_with_gvs_off_skips_project_registry` tests created the manifest at `<tmp>/package.json` while `project_root` was `<tmp>/project/`. `Install::run` derives the registry target from `manifest.path().parent()`, so the registry entry actually pointed at `<tmp>`, not the intended project root. Moved the manifest into `project_root` and replaced the entry-count-only check with a `read_link` + canonicalize assertion that the symlink resolves back to `project_root`. 6. **Preserve yaml-set `globalVirtualStoreDir`** — `apply_global_virtual_store_derivation` unconditionally overwrote the field, so a user-pinned `globalVirtualStoreDir:` in `pnpm-workspace.yaml` was silently lost. Added a `global_virtual_store_dir_explicit` parameter that short-circuits the derivation when yaml provided the value, mirroring the pre-existing `virtual_store_dir_explicit` plumbing. New `yaml_global_virtual_store_dir_wins_over_derivation` test pins the behaviour. All 704 workspace tests pass; `taplo --check`, `just dylint`, `cargo doc -D warnings` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Six items from the Copilot review on #449: 1. **Gate `register_project` on frozen-lockfile** — the call previously fired for any GVS-on install, including the legacy `InstallWithoutLockfile` path that uses `VirtualStoreLayout::legacy` and never references `<store_dir>/links/...`. Tightened to `frozen_lockfile && config.enable_global_virtual_store` so a registry entry only appears for installs that actually consume the shared store. 2. **`VirtualStoreLayout::new` engine: `Option<&str>`** — the param was `&str` and the call site passed `engine_name.as_deref() .unwrap_or("")`, which turned a missing `node` into `Some("")` and hashed to a different value than `None` (the latter omits the `engine` contribution from `calc_graph_node_hash`). Switched the signature to `Option<&str>` and the call site to `engine_name.as_deref()` so the missing-engine case round-trips correctly. Test call sites updated. 3. **`run_emits_imported_event_after_create_cas_files`** — renamed to `run_emits_imported_event_after_import_indexed_dir` (and updated the doc comment) since `create_cas_files` was renamed to `import_indexed_dir` in #441. 4. **`VirtualStoreLayout::package_store_dir` doc** — the field doc still claimed the root was `Config::virtual_store_dir` in both modes. Updated to reflect the post-split design: it's `Config::global_virtual_store_dir` under GVS, `virtual_store_dir` otherwise. 5. **Test manifest placement** — the new `frozen_lockfile_under_gvs_registers_project_and_runs_clean` and `frozen_lockfile_with_gvs_off_skips_project_registry` tests created the manifest at `<tmp>/package.json` while `project_root` was `<tmp>/project/`. `Install::run` derives the registry target from `manifest.path().parent()`, so the registry entry actually pointed at `<tmp>`, not the intended project root. Moved the manifest into `project_root` and replaced the entry-count-only check with a `read_link` + canonicalize assertion that the symlink resolves back to `project_root`. 6. **Preserve yaml-set `globalVirtualStoreDir`** — `apply_global_virtual_store_derivation` unconditionally overwrote the field, so a user-pinned `globalVirtualStoreDir:` in `pnpm-workspace.yaml` was silently lost. Added a `global_virtual_store_dir_explicit` parameter that short-circuits the derivation when yaml provided the value, mirroring the pre-existing `virtual_store_dir_explicit` plumbing. New `yaml_global_virtual_store_dir_wins_over_derivation` test pins the behaviour. All 704 workspace tests pass; `taplo --check`, `just dylint`, `cargo doc -D warnings` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Threads `VirtualStoreLayout` through the frozen-lockfile install pipeline so packages installed under `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path instead of the legacy per-project flat layout. Closes the rest of #432 (Section B + C activation + D entry tests). Pipeline changes (frozen-lockfile path): - `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine_name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup. - `Install::run` calls `pacquet_store_dir::register_project` when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` so a future `pacquet store prune` can find the projects that still reference `<store_dir>/links/...` slots. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. - `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). Field-split deviation from upstream: Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning. Tests: - `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on. - `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write. - All 653 workspace tests pass under the new layout-threaded path. - Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen- lockfile fixture against the registry-mock, which is out of scope here. Based on #441 (`feat/438`); the `import_indexed_dir` rename from that PR is already merged in. The two PRs only co-touch `create_virtual_dir_by_snapshot.rs` — both pieces land cleanly on top of each other.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Six items from the Copilot review on #449: 1. **Gate `register_project` on frozen-lockfile** — the call previously fired for any GVS-on install, including the legacy `InstallWithoutLockfile` path that uses `VirtualStoreLayout::legacy` and never references `<store_dir>/links/...`. Tightened to `frozen_lockfile && config.enable_global_virtual_store` so a registry entry only appears for installs that actually consume the shared store. 2. **`VirtualStoreLayout::new` engine: `Option<&str>`** — the param was `&str` and the call site passed `engine_name.as_deref() .unwrap_or("")`, which turned a missing `node` into `Some("")` and hashed to a different value than `None` (the latter omits the `engine` contribution from `calc_graph_node_hash`). Switched the signature to `Option<&str>` and the call site to `engine_name.as_deref()` so the missing-engine case round-trips correctly. Test call sites updated. 3. **`run_emits_imported_event_after_create_cas_files`** — renamed to `run_emits_imported_event_after_import_indexed_dir` (and updated the doc comment) since `create_cas_files` was renamed to `import_indexed_dir` in #441. 4. **`VirtualStoreLayout::package_store_dir` doc** — the field doc still claimed the root was `Config::virtual_store_dir` in both modes. Updated to reflect the post-split design: it's `Config::global_virtual_store_dir` under GVS, `virtual_store_dir` otherwise. 5. **Test manifest placement** — the new `frozen_lockfile_under_gvs_registers_project_and_runs_clean` and `frozen_lockfile_with_gvs_off_skips_project_registry` tests created the manifest at `<tmp>/package.json` while `project_root` was `<tmp>/project/`. `Install::run` derives the registry target from `manifest.path().parent()`, so the registry entry actually pointed at `<tmp>`, not the intended project root. Moved the manifest into `project_root` and replaced the entry-count-only check with a `read_link` + canonicalize assertion that the symlink resolves back to `project_root`. 6. **Preserve yaml-set `globalVirtualStoreDir`** — `apply_global_virtual_store_derivation` unconditionally overwrote the field, so a user-pinned `globalVirtualStoreDir:` in `pnpm-workspace.yaml` was silently lost. Added a `global_virtual_store_dir_explicit` parameter that short-circuits the derivation when yaml provided the value, mirroring the pre-existing `virtual_store_dir_explicit` plumbing. New `yaml_global_virtual_store_dir_wins_over_derivation` test pins the behaviour. All 704 workspace tests pass; `taplo --check`, `just dylint`, `cargo doc -D warnings` clean.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends pacquet's package-import primitive to cover pnpm's
importIndexedDir(..., { force, keepModulesDir })call shape so thehoisted node-linker (the umbrella of #438) has a primitive to build on.
create_cas_files→import_indexed_dirto matchpnpm's name (its docstring already said it mirrored
tryImportIndexedDir), and adds anImportIndexedDirOptsstructwith
forceandkeep_modules_dir. Both linkers now go throughthe same function — pnpm uses one function for both, so does
pacquet.
force: false, keep_modules_dir: false): theisolated linker's behavior. Existing target short-circuits; fresh
target gets
mkdir+ parallellink_file. Unchanged hot pathfor the two existing call sites
(
create_virtual_dir_by_snapshot,install_package_from_registry).force: true: stage the new contents in a sibling directory,remove the old tree, rename stage into place. A regular file or
symlink occupying the target is unlinked first.
force: true+keep_modules_dir: true: before the swap,dir_path/node_modules/is moved into the staging directory sonested deps survive the rebuild. On any failure after the move,
the staged copy is restored to
dir_path/node_modules/beforestaging is cleaned up — staging never holds the user's only copy
of nested deps. This is the call shape the hoisted linker will
use.
(
linkHoistedModulesanalog, hoist algorithm,Install::runbranch on
node_linker) will passforce + keep_modules_dirfrom the new hoisted path. The default-opts behavior is exercised
in production today via the existing isolated callers.
Upstream reference
Ported from
pnpm/pnpm@94240bc0:fs/indexed-pkg-importer/src/importIndexedDir.ts— the function this ports.store/controller-types/src/index.ts— theImportOptionsshapeImportIndexedDirOptsmirrors.installing/deps-restorer/src/linkHoistedModules.ts:134— the call site that will drive theforce + keepModulesDircombination once the later slices land.Scope omissions (intentional)
moveOrMergeModulesDirssemantics for the case where the indexedfile map itself contains
node_modules/entries. Upstream merges;pacquet errors with
NodeModulesCollision. The hoisted-linker callsite never produces that state in practice (npm and pnpm strip
node_modules/at pack time), so erroring loudly until a realcaller demands the merge is the right call.
safeToSkipshort-circuit. Upstream's pre-existence checkbeyond the default short-circuit lives in
index.tsaround theimporter; that decision belongs at the Slice 5 call site, not in
this primitive.
Performance
The isolated-linker hot path's syscall count is unchanged. The old
if dir_path.exists() { return Ok(()); }(onestat) becomesfs::symlink_metadata(dir_path)(onelstat) plus a matchdispatch;
populate_diris the oldcreate_cas_filesbodyverbatim and gets inlined under release LTO. The new
forcebranches only run for hoisted, which has no callers yet.
One behavior diff: a dangling symlink at the target under default
opts used to fall through to
mkdirand surface an IO error;under the new code it short-circuits as "already populated". The
virtual store doesn't produce dangling symlinks under normal
operation, so this is theoretical.
Test plan
fresh_target_links_files— default opts, no existing target.existing_target_short_circuits_under_default_opts— pinsthe isolated-linker invariant.
force_keep_replaces_files_and_preserves_node_modules— thehoisted call shape.
force_without_keep_clobbers_node_modules—forcewithoutkeep_modules_dirdoes not preserve nested deps.force_keep_without_node_modules_replaces_cleanly— preservebranch with nothing to preserve.
force_replaces_regular_file_target— unlinks before freshinstall.
force_replaces_symlink_target_without_following— unlinksthe link, leaves the pointee.
fresh_target_creates_nested_directories— mkdir pre-passreaches deep entries.
node_modules_collision_in_file_map_errors—force_keep+tarball-shipped
node_modules/surfacesNodeModulesCollisionwithout clobbering the existing tree.
hardlink_method_survives_staging_swap—force+ Hardlinkshares inode with the store source.
remove_dir_all_failure_restores_preserved_node_modules—data-loss rescue path under
force_keep.node_modules_inspect_permission_denied_surfaces—non-
NotFoundinspect errors are not swallowed.concurrent_force_imports_into_different_targets_do_not_collide— staging-path uniqueness across rayon workers.
just ready,cargo doc --document-private-items,taplo format --check,just dylintall clean.f084645.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes
Refactor
Tests