fix(pacquet,package-manager): walk every workspace project in fresh-resolve install#11905
Conversation
|
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:
📝 WalkthroughWalkthroughRestructures fresh-lockfile install to resolve every workspace importer: load workspace projects, build importer_manifests, resolve each importer into a merged dependency graph, produce per-importer lockfile entries, run per-importer bin linking, and adjust GVS registration and tests. ChangesMulti-importer fresh-resolve workspace path
Sequence Diagram(s)sequenceDiagram
participant Client
participant InstallWithFreshLockfile
participant resolve_importer
participant DependenciesGraph
participant dependencies_graph_to_lockfile
participant GlobalVirtualStore
Client->>InstallWithFreshLockfile: run(importer_manifests)
InstallWithFreshLockfile->>resolve_importer: resolve_importer(importer_manifest_i)
resolve_importer->>DependenciesGraph: emit nodes (per-importer)
InstallWithFreshLockfile->>DependenciesGraph: merge per-importer graphs
InstallWithFreshLockfile->>dependencies_graph_to_lockfile: build_lockfile(importer_manifests, merged_graph, direct_by_importer)
InstallWithFreshLockfile->>GlobalVirtualStore: register workspace root (best-effort)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 #11905 +/- ##
==========================================
+ Coverage 87.61% 87.66% +0.05%
==========================================
Files 219 220 +1
Lines 26666 26780 +114
==========================================
+ Hits 23364 23478 +114
Misses 3302 3302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0813698216,
"stddev": 0.0883391025648184,
"median": 2.0806421649,
"user": 2.73459796,
"system": 3.3552278600000003,
"min": 1.9377619844,
"max": 2.2698043354000004,
"times": [
2.0968951214000002,
2.1402016654000002,
1.9377619844,
2.0441229264,
2.0305569724000003,
2.0743186774,
2.1208987974,
2.2698043354000004,
2.0869656524,
2.0121720834000003
]
},
{
"command": "pacquet@main",
"mean": 2.0156760836000003,
"stddev": 0.04960481641907888,
"median": 1.9996550498999999,
"user": 2.756370059999999,
"system": 3.38692956,
"min": 1.9683130404,
"max": 2.0994513224,
"times": [
2.0108227824,
2.0949763584000003,
2.0261276614000003,
1.9683130404,
1.9691507074,
2.0430283694,
2.0994513224,
1.9771497464,
1.9884873174,
1.9792535304
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6745418242400001,
"stddev": 0.044310850919803864,
"median": 0.66219082494,
"user": 0.36276392,
"system": 1.4696764,
"min": 0.65187713694,
"max": 0.80004579694,
"times": [
0.80004579694,
0.6595529289400001,
0.66735121294,
0.66314118194,
0.66389601294,
0.6622652289400001,
0.65187713694,
0.6558037359400001,
0.66211642094,
0.65936858594
]
},
{
"command": "pacquet@main",
"mean": 0.66486694484,
"stddev": 0.017803650976564295,
"median": 0.6618010724400001,
"user": 0.36756222,
"system": 1.4696258,
"min": 0.6421068549400001,
"max": 0.70727225494,
"times": [
0.70727225494,
0.65160886994,
0.6581838199400001,
0.67610966994,
0.6700006429400001,
0.65458849294,
0.65737921594,
0.6654183249400001,
0.6421068549400001,
0.6660013019400001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.50709399194,
"stddev": 0.03615962778133472,
"median": 2.5106795546400003,
"user": 4.35471506,
"system": 3.3743764199999995,
"min": 2.4420806766400003,
"max": 2.56034813164,
"times": [
2.53957350664,
2.49282806764,
2.56034813164,
2.5291739996400002,
2.53168050264,
2.4790251476400003,
2.52853104164,
2.48748588564,
2.48021295964,
2.4420806766400003
]
},
{
"command": "pacquet@main",
"mean": 2.5304964525399996,
"stddev": 0.032186319320799184,
"median": 2.53320625764,
"user": 4.363230960000001,
"system": 3.4064678200000005,
"min": 2.4595790656400003,
"max": 2.58012752564,
"times": [
2.53392119164,
2.4595790656400003,
2.53565574864,
2.54092255864,
2.52217078464,
2.50659967464,
2.56273993764,
2.5307567146400003,
2.53249132364,
2.58012752564
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.6980263944,
"stddev": 0.017960931154754077,
"median": 1.6994840429,
"user": 2.0220623200000003,
"system": 2.1646251,
"min": 1.6735413478999999,
"max": 1.7245118999,
"times": [
1.6791626308999998,
1.7051523019,
1.7230177719000002,
1.6828550969,
1.6735413478999999,
1.7245118999,
1.7036446409000001,
1.6953234448999999,
1.7087264619,
1.6843283468999999
]
},
{
"command": "pacquet@main",
"mean": 1.7374809892999998,
"stddev": 0.05716856612507281,
"median": 1.7216004564,
"user": 2.0623148199999997,
"system": 2.1725880999999996,
"min": 1.6730279219,
"max": 1.8425915049,
"times": [
1.7219198139,
1.8285008259,
1.7247565719,
1.8425915049,
1.7212810989,
1.7658578429,
1.6974547308999999,
1.6730279219,
1.6996646929,
1.6997548889
]
}
]
} |
|
Bencher Report| Branch | pr/11905 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,507.09 ms(-42.87%)Baseline: 4,388.55 ms | 5,266.26 ms (47.61%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,698.03 ms(-49.81%)Baseline: 3,382.93 ms | 4,059.51 ms (41.83%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,081.37 ms(-9.11%)Baseline: 2,289.87 ms | 2,747.84 ms (75.75%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 674.54 ms(+2.34%)Baseline: 659.10 ms | 790.92 ms (85.29%) |
…nstall The fresh-resolve install path (no `--frozen-lockfile`, no usable lockfile) only resolved the workspace root manifest, so sibling workspace projects' own dependencies never landed in the lockfile or on disk. Re-run `resolve_importer` per importer with shared install caches (`meta_cache`, `fetch_locker`, `picked_manifest_cache`), merge the per-importer graphs, and emit one `importers[<id>]` entry per project. Mirrors upstream's [`resolveRootDependencies`](https://github.com/pnpm/pnpm/blob/3422cecfd3/installing/deps-resolver/src/resolveDependencies.ts#L327-L437) iteration shape — one shared resolution context, per-importer direct-deps slices. Per-importer `link_bins` so each project gets its own `node_modules/.bin`. GVS `register_project` now loops every importer key the freshly-built lockfile carries, mirroring the frozen path. `importer_dep_version` and `snapshot_dep_ref` learned a `link:` short-circuit so workspace-sibling edges emit `ImporterDepVersion::Link` / `SnapshotDepRef::Link` instead of falling through to the `name@version` parser. Cross-importer `TreeCtx` sharing (full upstream parity: one resolution context with per-importer hoist loops) is deferred — each `resolve_importer` call still has its own context. Network-side caches still amortize packument fetches and JSON parsing across importers; only per-resolve semver matching duplicates. Closes #11901.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pacquet/crates/cli/tests/workspace_install.rs (1)
87-126: ⚡ Quick winAdd debug output before these non-
assert_eq!assertions.These assertions are doing structural checks over paths and lockfile content; adding
dbg!right before them makes CI failures much easier to diagnose.Suggested update
let a_dep = workspace.join("packages/a/node_modules/@pnpm.e2e/hello-world-js-bin-parent"); + dbg!(&a_dep); assert!( is_symlink_or_junction(&a_dep).expect("query packages/a symlink"), "packages/a/node_modules direct-dep symlink missing — sibling importer's deps weren't walked", ); let b_dep = workspace.join("packages/b/node_modules/@pnpm.e2e/hello-world-js-bin"); + dbg!(&b_dep); assert!( is_symlink_or_junction(&b_dep).expect("query packages/b symlink"), "packages/b/node_modules direct-dep symlink missing — sibling importer's deps weren't walked", ); @@ let lockfile_path = workspace.join("pnpm-lock.yaml"); let lockfile = fs::read_to_string(&lockfile_path).expect("read pnpm-lock.yaml"); + dbg!(&lockfile_path); + dbg!(&lockfile);As per coding guidelines "Log before non-
assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/cli/tests/workspace_install.rs` around lines 87 - 126, Add debug output (use dbg! or similar) immediately before each non-assert_eq! structural assertion in the test so CI failures show the inspected values: before the is_symlink_or_junction(&a_dep) and is_symlink_or_junction(&b_dep) checks, before the workspace.join("node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0").exists() and workspace.join("node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0").exists() checks, and before reading/inspecting the lockfile (the lockfile string used for lockfile.contains(...) assertions). Ensure the dbg! calls print the path values and the lockfile string so failing assertions show the actual values.
🤖 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/cli/tests/workspace_install.rs`:
- Around line 123-126: The assertion currently checks
lockfile.contains("hello-world-js-bin-parent") which can match anywhere; change
it to assert that the importer-scoped section for packages/a contains the entry.
Update the test to search within the "importers:\n packages/a" block (e.g., use
a regex or substring search that finds the "importers:\n packages/a" header and
then verifies "hello-world-js-bin-parent" appears after it) instead of a global
contains on lockfile; reference the existing lockfile variable and the string
"hello-world-js-bin-parent" and replace the single assert in
workspace_install.rs with an importer-scoped check.
In `@pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs`:
- Around line 484-492: The loop in compute_corrected_optional is treating
unknown aliases as Prod (using
manifest_alias_to_group(...).get(alias).copied().unwrap_or(DependencyGroup::Prod)),
which wrongly seeds optionality for aliases that aren’t declared in the
manifest; instead, only consider aliases that exist in manifest_alias_to_group:
if manifest_alias_to_group(input.manifest).get(alias) returns None, skip that
alias (do not push to dev_seeds/optional_seeds/prod_seeds). Update the matching
logic in the loop over importer_inputs to only act on Some(group) (referencing
compute_corrected_optional, manifest_alias_to_group, importer_inputs, and
direct_dependencies_by_alias).
---
Nitpick comments:
In `@pacquet/crates/cli/tests/workspace_install.rs`:
- Around line 87-126: Add debug output (use dbg! or similar) immediately before
each non-assert_eq! structural assertion in the test so CI failures show the
inspected values: before the is_symlink_or_junction(&a_dep) and
is_symlink_or_junction(&b_dep) checks, before the
workspace.join("node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin-parent@1.0.0").exists()
and
workspace.join("node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0").exists()
checks, and before reading/inspecting the lockfile (the lockfile string used for
lockfile.contains(...) assertions). Ensure the dbg! calls print the path values
and the lockfile string so failing assertions show the actual values.
🪄 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: f879cb5d-8f78-446a-b36a-dcbe69fb6ea8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pacquet/crates/cli/tests/workspace_install.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_with_fresh_lockfile.rspacquet/crates/workspace/Cargo.tomlpacquet/crates/workspace/src/importer_id.rspacquet/crates/workspace/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). (1)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
🧰 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/workspace/src/importer_id.rspacquet/crates/workspace/src/lib.rspacquet/crates/cli/tests/workspace_install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/dependencies_graph_to_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/workspace_install.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/workspace/src/importer_id.rspacquet/crates/workspace/src/lib.rspacquet/crates/cli/tests/workspace_install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/workspace/src/importer_id.rspacquet/crates/workspace/src/lib.rspacquet/crates/cli/tests/workspace_install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/install.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/workspace/src/importer_id.rspacquet/crates/workspace/src/lib.rspacquet/crates/cli/tests/workspace_install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
🧬 Code graph analysis (6)
pacquet/crates/workspace/src/lib.rs (1)
pacquet/crates/workspace/src/importer_id.rs (4)
returns_dot_for_root(38-40)returns_posix_relative_for_subproject(43-48)nested_subproject(51-53)importer_id_from_root_dir(15-30)
pacquet/crates/cli/tests/workspace_install.rs (2)
pacquet/crates/testing-utils/src/bin.rs (1)
AddMockedRegistry(40-49)pacquet/crates/testing-utils/src/fs.rs (1)
is_symlink_or_junction(45-59)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (3)
dependencies_graph_to_lockfile(87-117)GraphToLockfileOptions(45-69)ImporterLockfileInput(33-42)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (3)
pacquet/crates/package-manager/src/install.rs (1)
a(280-896)pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (3)
GraphToLockfileOptions(45-69)ImporterLockfileInput(33-42)dependencies_graph_to_lockfile(87-117)pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (2)
ResolveImporterOptions(52-94)resolve_importer(119-259)
pacquet/crates/package-manager/src/install.rs (4)
pacquet/crates/lockfile/src/lib.rs (2)
Lockfile(61-91)Lockfile(93-174)pacquet/crates/package-manager/src/symlink_direct_dependencies.rs (1)
importer_root_dir(308-319)pacquet/crates/workspace/src/importer_id.rs (1)
importer_id_from_root_dir(15-30)pacquet/crates/workspace/src/manifest.rs (1)
WorkspaceManifest(47-71)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (2)
pacquet/crates/resolving-deps-resolver/src/dependencies_graph.rs (1)
DependenciesGraph(15-15)pacquet/crates/lockfile/src/snapshot_entry.rs (1)
SnapshotEntry(17-45)
🔇 Additional comments (14)
pacquet/crates/package-manager/src/install.rs (4)
690-727: LGTM!The workspace project loading and
importer_manifestsconstruction correctly:
- Loads projects once via
load_workspace_projectsfor reuse- Always inserts the root importer (
".") with the in-memory manifest (preservingpacquet addbehavior)- Skips re-inserting the root if it appears in
workspace_projects(thecontinueguard at line 721)- Uses
importer_id_from_root_dirfor consistent POSIX-relative path computation
764-786: LGTM!The GVS registration loop correctly mirrors the frozen-lockfile path's behavior: iterates all importers from the freshly-built lockfile, falls back to root when no lockfile was written (
config.lockfile=false), and uses the same best-effort warn-on-failure pattern.
1215-1223: LGTM!The helper correctly distinguishes the three states: no workspace manifest (
Ok(None)), workspace manifest with patterns (Somewith patterns), and workspace manifest without patterns (defaults handled byfind_workspace_projects).
1238-1257: LGTM!The refactored signature correctly accepts the pre-loaded projects slice and propagates the
Nonestate for non-workspace installs, avoiding duplicate disk I/O.pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (6)
94-101: LGTM!The field change from single manifest to
BTreeMap<String, &'a PackageManifest>correctly models the multi-importer input shape, with clear documentation of the importer id format.
497-513: LGTM!Preferred-version seeding correctly collects manifests from all importers, ensuring workspace siblings' direct dependencies contribute to the version tie-break table alongside the root's.
537-601: LGTM!The per-importer resolve loop correctly:
- Derives
project_dirfrom each importer's manifest path for correctworkspace:/link:path resolution- Merges graphs with first-writer-wins (safe since identical depPaths yield equivalent nodes by resolver construction)
- Records direct dependencies per importer for lockfile construction
- Emits per-importer peer-issue warnings
863-875: LGTM!Per-importer bin linking correctly:
- Extracts the modules basename (defaults to
node_modulesif absent)- Computes each importer's project directory via
importer_root_dir- Constructs per-importer
modules_dirandbins_dirpaths- Calls
link_binsfor each, ensuring every workspace project gets its own.bin/populated
969-992: LGTM!The helper correctly collects cache keys from the merged graph using the same key format as the frozen-lockfile path, with appropriate filtering for tarball resolutions that have both integrity and structured
name@version.
1002-1033: LGTM!The helper correctly constructs per-importer
ImporterLockfileInputentries by combining each manifest with its direct-deps map fromdirect_by_importer, then passes the shared merged graph todependencies_graph_to_lockfilefor dedupedpackages:andsnapshots:generation.pacquet/crates/workspace/Cargo.toml (1)
19-19: LGTM!pacquet/crates/workspace/src/importer_id.rs (1)
1-7: LGTM!Also applies to: 10-30, 32-54
pacquet/crates/workspace/src/lib.rs (1)
21-21: LGTM!Also applies to: 28-28
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
1-1: LGTM!Also applies to: 6-7, 10-12, 16-43, 150-152, 212-214, 291-293, 356-358, 399-401, 453-455, 536-538, 625-627, 639-824
…, matching pnpm Pacquet was looping `register_project` over every importer in both the frozen-lockfile and fresh-lockfile branches, but upstream pnpm calls `registerProject(opts.storeDir, opts.lockfileDir)` exactly once per install against the workspace root — store prune walks the workspace's `node_modules/.pnpm/` to find every installed package, so one registry entry per workspace is enough. Consolidate to a single call near the start of `Install::run`, matching pnpm's `getContext` ordering at <https://github.com/pnpm/pnpm/blob/d8a79a9c30/installing/context/src/index.ts#L128>. Also port two upstream-derived tests that the multi-importer rewrite of `compute_corrected_optional` and the per-importer link rendering were previously missing direct coverage for: - `multi_importer_pruner_marks_shared_dep_non_optional_when_any_importer_reaches_via_prod` ports the spirit of pnpm's [`pruneSharedLockfile`](https://github.com/pnpm/pnpm/blob/d8a79a9c30/lockfile/pruner/src/index.ts#L17) cross-importer pooling: a depPath reached via a non-optional path from any importer ends up `optional: false` even when another importer reaches it only via an optional path. - `workspace_sibling_link_renders_per_importer_with_link_ref` exercises the multi-importer `workspace:`-link case — importer A depends on importer B via a `link:`-resolved depPath, both render their own `importers[<id>]` entries, and the link node stays out of `packages:` / `snapshots:`.
Addresses CodeRabbit's review on PR #11905. Pacquet's resolver hoists auto-installed peers into `direct_dependencies_by_alias` even when they aren't in the importer's manifest (see `resolve_importer::direct.extend(...)` after each `hoist_peers` call). `build_importer` correctly excludes those undeclared aliases from the importer's lockfile entry, but `compute_corrected_optional` was seeding the pruner BFS from the full `direct_dependencies_by_alias` and defaulting unknown aliases to `DependencyGroup::Prod`. That diverges from upstream's [`pruneSharedLockfile`](https://github.com/pnpm/pnpm/blob/d8a79a9c30/lockfile/pruner/src/index.ts#L27-L29), which seeds purely from `lockfile.importers[*].{dev,optional,}dependencies` — i.e., from the same set `build_importer` writes. The mismatch forced auto-peers reachable only via an optional parent's chain to `optional: false`, leaking them into non-optional installs. Skip aliases not in the manifest when seeding. The new test `auto_installed_peer_not_declared_in_manifest_is_skipped_from_pruner_seeds` pins the corrected behavior — `peer-x` (auto-installed for an `optionalDependencies` parent) stays `optional: true`, matching pnpm. Verified the test fails against the pre-fix code. Also tightens the multi-importer integration test's lockfile assertion: scope the `hello-world-js-bin-parent` check to the `packages/a:` importer section instead of a global substring match, so the test proves the direct-dep entry — not just any mention in `packages:`.
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)
718-752:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the fresh lockfile into workspace-state generation.
When this branch starts without a lockfile, Line 751 captures the newly built multi-importer lockfile, but the later workspace-state write still reads the pre-install
lockfile. On a fresh workspace install that means.pnpm-workspace-state-v1.jsononly records".", so sibling importers are omitted even though this branch just resolved them. Threadfresh_lockfile.as_ref().or(lockfile)intobuild_workspace_state(...)so the recorded project set matches the install result.🤖 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 718 - 752, The workspace-state is being built from the pre-install `lockfile` instead of the newly resolved multi-importer lockfile returned by InstallWithFreshLockfile; update the call to build_workspace_state to pass the fresh lockfile when present by threading `fresh_lockfile.as_ref().or(lockfile)` (use the `fresh_result.wanted_lockfile` / `fresh_lockfile` from the InstallWithFreshLockfile run) so the generated .pnpm-workspace-state-v1.json reflects the resolved set of importers rather than the stale/empty pre-install lockfile.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
825-844: ⚡ Quick winTrim test doc comments to “why” only.
These blocks over-describe setup/flow that the tests already document. Please keep only the non-obvious rationale (and upstream reference), not a prose replay of the test body.
✂️ Suggested simplification
-/// Multi-importer cross-importer pruner BFS. Ported from upstream -/// pnpm's [...] behavior — `copyPackageSnapshots` pools every importer's -/// ... -/// path is all non-optional. +/// Ported from pnpm pruner behavior: if any importer reaches a shared dep +/// through a non-optional path, the pruned snapshot must be `optional: false`. -/// Multi-importer with a `workspace:` link between two siblings. -/// Ported from the spirit of upstream pnpm's [...] -/// ... -/// `snapshots:`. +/// Ported from pnpm multi-importer workspace-link behavior: importer-to-importer +/// `workspace:*` edges render as `ImporterDepVersion::Link`, without adding the +/// linked sibling to `packages`/`snapshots`.As per coding guidelines "Tests are documentation. Do not duplicate them in prose." and "Doc comments ... are not a re-narration of the body."
Also applies to: 945-953
🤖 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/dependencies_graph_to_lockfile/tests.rs` around lines 825 - 844, The long doc comment starting "Multi-importer cross-importer pruner BFS. Ported from upstream pnpm's `pruneSharedLockfile`..." should be shortened to a brief "why" note: keep the upstream reference and a one-sentence rationale explaining the non-obvious behavior being tested (that importer-level non-optional edges must override optional resolver state), and delete the step-by-step scenario prose (the bullet list and flow replay) since the test body already documents setup and behavior; update the comment block above the test accordingly.
🤖 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 718-752: The workspace-state is being built from the pre-install
`lockfile` instead of the newly resolved multi-importer lockfile returned by
InstallWithFreshLockfile; update the call to build_workspace_state to pass the
fresh lockfile when present by threading `fresh_lockfile.as_ref().or(lockfile)`
(use the `fresh_result.wanted_lockfile` / `fresh_lockfile` from the
InstallWithFreshLockfile run) so the generated .pnpm-workspace-state-v1.json
reflects the resolved set of importers rather than the stale/empty pre-install
lockfile.
---
Nitpick comments:
In `@pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs`:
- Around line 825-844: The long doc comment starting "Multi-importer
cross-importer pruner BFS. Ported from upstream pnpm's `pruneSharedLockfile`..."
should be shortened to a brief "why" note: keep the upstream reference and a
one-sentence rationale explaining the non-obvious behavior being tested (that
importer-level non-optional edges must override optional resolver state), and
delete the step-by-step scenario prose (the bullet list and flow replay) since
the test body already documents setup and behavior; update the comment block
above the test accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22eaab6e-30c0-4775-979b-f2e9a47167ce
📒 Files selected for processing (3)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that 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/tests.rspacquet/crates/package-manager/src/install.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.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/tests.rspacquet/crates/package-manager/src/install.rs
🔇 Additional comments (2)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
846-943: LGTM!Also applies to: 955-1029
pacquet/crates/package-manager/src/install/tests.rs (1)
2146-2177: ⚡ Quick winMake this fixture an actual workspace.
Without a
pnpm-workspace.yaml,find_workspace_dirfalls back tomanifest.path().parent(), so this still passes on the single-project path. Add a minimal workspace manifest here so the test really exercises workspace-root discovery instead of the fallback.[ suggest_recommended_refactor ]
Suggested setup
let web_dir = workspace_root.join("packages/web"); std::fs::create_dir_all(&web_dir).expect("create packages/web"); + std::fs::write( + workspace_root.join("pnpm-workspace.yaml"), + "packages:\n - packages/*\n", + ) + .expect("write pnpm-workspace.yaml"); let manifest_path = workspace_root.join("package.json"); let manifest = PackageManifest::create_if_needed(manifest_path).unwrap();
…tering project
CI failure: `fresh_install_honors_enable_global_virtual_store` started
failing after the previous register_project consolidation. Two
compounding bugs:
1. `register_project` now runs early in `Install::run`, before any
install phase has materialized the store. With the test's
relative `storeDir: ../pacquet-store` in `pnpm-workspace.yaml`,
`config.store_dir.root()` ends up shaped like
`<workspace>/../pacquet-store/v11` — a path that doesn't yet
exist on disk.
2. `path_contains`'s "lexical fallback" wasn't actually lexical —
it called `dunce::canonicalize`, and on failure (path doesn't
exist) it kept the path verbatim and ran `starts_with`. So
`<workspace>/../pacquet-store/v11`.starts_with(`<workspace>`)
returned true, the early-return guard fired, and the call
silently skipped without writing the registry entry.
Two-part fix matching upstream:
- `Install::run` now calls `fs::create_dir_all(store_dir.root())`
before `register_project`, mirroring pnpm's
[`fs.mkdir(opts.storeDir, { recursive: true })`](https://github.com/pnpm/pnpm/blob/d8a79a9c30/installing/context/src/index.ts#L125)
call right before `registerProject`. Once the store exists,
`canonicalize` succeeds and `path_contains` resolves both sides
correctly.
- `path_contains` now lexically normalizes `.` / `..` components
when canonicalize fails. Matches upstream's `is-subdir` semantics
(which uses `path.relative`, purely lexical). New test
`path_contains_resolves_parent_components_when_paths_do_not_exist`
pins the behavior; verified it fails against the pre-fix code.
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 (2)
pacquet/crates/package-manager/src/install.rs (2)
860-863:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPersist workspace state from the importer set that was actually installed.
build_workspace_statederivesprojectsfromlockfile.importers, but this write still passes the pre-installlockfile. On a fresh multi-importer workspace install, that can beNoneor stale, so.pnpm-workspace-state-v1.jsonmisses sibling projects andverifyDepsBeforeRunwill keep treating the workspace as outdated.🤖 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 860 - 863, The workspace-state write is using the pre-install lockfile (so build_workspace_state reads lockfile.importers that may be None/stale); change the call in update_workspace_state(...) to pass the post-install lockfile (or the importer set produced by the install operation) instead of the original `lockfile` so `build_workspace_state` computes `projects` from the actual installed importers; locate the call to update_workspace_state and replace the `lockfile` argument with the updated lockfile variable returned by the installer (or construct the workspace state from that install result) so `.pnpm-workspace-state-v1.json` includes all sibling projects and `verifyDepsBeforeRun` sees the correct state.
899-1002:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRun the frozen-lockfile freshness check across every importer.
check_lockfile_freshnessstill validates onlyimporters["."]. After this multi-importer workspace flow, a siblingpackage.jsoncan drift while the root stays unchanged, and--frozen-lockfile/ auto-frozen can still take the frozen path or even the no-op fast path instead of erroring or falling back to fresh resolution. Based on learnings: when reviewing pacquet Rust port, 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 899 - 1002, The function check_lockfile_freshness currently only inspects the single importer fetched via Lockfile::ROOT_IMPORTER_KEY; change it to iterate over lockfile.importers (or its keys/entries) and run the same per-importer checks for each importer: apply parsed_overrides_opt and create a per-importer manifest_for_freshness (using VersionsOverrider as you do for the root), compute the ignored_set/is_ignored_optional for that importer, and call satisfies_package_manifest for each importer id instead of using the single importer variable; ensure errors map to FreshnessCheckError::Stale as before and preserve the earlier global settings check (pacquet_lockfile::check_lockfile_settings) once.
🤖 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 860-863: The workspace-state write is using the pre-install
lockfile (so build_workspace_state reads lockfile.importers that may be
None/stale); change the call in update_workspace_state(...) to pass the
post-install lockfile (or the importer set produced by the install operation)
instead of the original `lockfile` so `build_workspace_state` computes
`projects` from the actual installed importers; locate the call to
update_workspace_state and replace the `lockfile` argument with the updated
lockfile variable returned by the installer (or construct the workspace state
from that install result) so `.pnpm-workspace-state-v1.json` includes all
sibling projects and `verifyDepsBeforeRun` sees the correct state.
- Around line 899-1002: The function check_lockfile_freshness currently only
inspects the single importer fetched via Lockfile::ROOT_IMPORTER_KEY; change it
to iterate over lockfile.importers (or its keys/entries) and run the same
per-importer checks for each importer: apply parsed_overrides_opt and create a
per-importer manifest_for_freshness (using VersionsOverrider as you do for the
root), compute the ignored_set/is_ignored_optional for that importer, and call
satisfies_package_manifest for each importer id instead of using the single
importer variable; ensure errors map to FreshnessCheckError::Stale as before and
preserve the earlier global settings check
(pacquet_lockfile::check_lockfile_settings) once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed399953-71c8-466d-8aa2-a498b3d519b7
📒 Files selected for processing (2)
pacquet/crates/package-manager/src/install.rspacquet/crates/store-dir/src/project_registry.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: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Doc
- GitHub Check: Analyze (javascript)
- 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/store-dir/src/project_registry.rspacquet/crates/package-manager/src/install.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/store-dir/src/project_registry.rspacquet/crates/package-manager/src/install.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/store-dir/src/project_registry.rspacquet/crates/package-manager/src/install.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/store-dir/src/project_registry.rspacquet/crates/package-manager/src/install.rs
🧬 Code graph analysis (1)
pacquet/crates/package-manager/src/install.rs (2)
pacquet/crates/store-dir/src/store_dir.rs (2)
StoreDir(38-59)root(181-183)pacquet/crates/store-dir/src/project_registry.rs (1)
register_project(88-156)
🔇 Additional comments (5)
pacquet/crates/store-dir/src/project_registry.rs (4)
366-371: LGTM!
373-378: LGTM!
380-396: LGTM!
421-443: LGTM!pacquet/crates/package-manager/src/install.rs (1)
378-405: LGTM!
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/store-dir/src/project_registry.rs (1)
381-394:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLeading
..components are lost for relative paths.When
lexical_normalizeencountersParentDiron an emptyPathBuf,pop()returnsfalseand does nothing. For a relative path like../foo, the leading..is silently dropped, producingfooinstead of../foo. The current call site (line 95) likely only passes absolute paths, so this doesn't affect existing behavior, but the function has a general-purpose contract and should handle all valid Path inputs correctly.🔧 Proposed fix: preserve leading `..` for relative paths
fn lexical_normalize(path: &Path) -> PathBuf { use std::path::Component; let mut out = PathBuf::new(); for component in path.components() { match component { Component::ParentDir => { - out.pop(); + if !out.pop() { + // pop() returned false → path was empty or at root. + // Preserve leading `..` for relative paths. + out.push(component.as_os_str()); + } } Component::CurDir => {} other => out.push(other.as_os_str()), } } out }🤖 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/store-dir/src/project_registry.rs` around lines 381 - 394, lexical_normalize currently drops leading ParentDir components because out.pop() is a no-op when out is empty; change the ParentDir arm so when out.pop() returns false you preserve the parent marker by pushing ".." into out (e.g. if !out.pop() { out.push(".."); }) instead of silently doing nothing; update the match arm in lexical_normalize (handling Component::ParentDir) so relative paths like "../foo" keep their leading ".." while existing absolute-path behavior remains unchanged.
🧹 Nitpick comments (1)
pacquet/crates/store-dir/src/project_registry.rs (1)
428-441: ⚡ Quick winConsider adding test coverage for relative paths with leading
...The test validates that
lexical_normalizecorrectly collapses..in absolute paths, but doesn't cover relative paths with leading..(e.g.,../sibling). Adding a relative-path test case would catch the bug flagged inlexical_normalizeand ensure the fallback path behaves correctly when canonicalization fails on relative inputs.📋 Example test case
#[test] fn path_contains_preserves_leading_parent_in_relative_paths() { let outer = Path::new("workspace"); let inner_sibling = Path::new("workspace/../sibling"); assert!( !path_contains(outer, inner_sibling), "`workspace/../sibling` lexically resolves to `sibling` — outside workspace", ); let inner_child = Path::new("workspace/child"); assert!( path_contains(outer, inner_child), "`workspace/child` is genuinely inside workspace", ); }🤖 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/store-dir/src/project_registry.rs` around lines 428 - 441, Add a unit test that exercises relative paths with a leading `..` to ensure path_contains and lexical_normalize handle them correctly: create a new #[test] (e.g., path_contains_preserves_leading_parent_in_relative_paths) alongside the existing tests in project_registry.rs that sets outer = Path::new("workspace"), inner_sibling = Path::new("workspace/../sibling") and inner_child = Path::new("workspace/child"), then assert that path_contains(outer, inner_sibling) is false and path_contains(outer, inner_child) is true; this will catch bugs in lexical_normalize's handling of relative inputs and the canonicalization fallback used by path_contains.
🤖 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/store-dir/src/project_registry.rs`:
- Around line 381-394: lexical_normalize currently drops leading ParentDir
components because out.pop() is a no-op when out is empty; change the ParentDir
arm so when out.pop() returns false you preserve the parent marker by pushing
".." into out (e.g. if !out.pop() { out.push(".."); }) instead of silently doing
nothing; update the match arm in lexical_normalize (handling
Component::ParentDir) so relative paths like "../foo" keep their leading ".."
while existing absolute-path behavior remains unchanged.
---
Nitpick comments:
In `@pacquet/crates/store-dir/src/project_registry.rs`:
- Around line 428-441: Add a unit test that exercises relative paths with a
leading `..` to ensure path_contains and lexical_normalize handle them
correctly: create a new #[test] (e.g.,
path_contains_preserves_leading_parent_in_relative_paths) alongside the existing
tests in project_registry.rs that sets outer = Path::new("workspace"),
inner_sibling = Path::new("workspace/../sibling") and inner_child =
Path::new("workspace/child"), then assert that path_contains(outer,
inner_sibling) is false and path_contains(outer, inner_child) is true; this will
catch bugs in lexical_normalize's handling of relative inputs and the
canonicalization fallback used by path_contains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7a0bd7e-4eb3-4aa2-8a4e-bd0a0d1cb4c5
📒 Files selected for processing (2)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/store-dir/src/project_registry.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 (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-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/store-dir/src/project_registry.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/store-dir/src/project_registry.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/store-dir/src/project_registry.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/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/store-dir/src/project_registry.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
🧬 Code graph analysis (1)
pacquet/crates/store-dir/src/project_registry.rs (1)
pacquet/crates/cli/tests/store.rs (1)
canonicalize(18-20)
🔇 Additional comments (2)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
938-941: LGTM!pacquet/crates/store-dir/src/project_registry.rs (1)
373-374: LGTM!
… lexical_normalize
Two issues:
1. The multi-line `assert!` in
`multi_importer_pruner_marks_shared_dep_non_optional_when_any_importer_reaches_via_prod`
was missing its trailing comma after `cargo fmt` reformatted it from
one-line to multi-line. Perfectionist's `macro-trailing-comma` rule
(which CI enforces via Dylint) flagged it. Added the comma.
2. CodeRabbit pointed out that `lexical_normalize` silently dropped
leading `..` components because `PathBuf::pop()` is a no-op when the
path is empty. For the current `path_contains` callers (both inputs
are absolute paths) this doesn't matter, but the helper is now a
general-purpose utility and the bug would bite any future caller
passing a relative path.
Replaced the naive `out.pop()` with a match on the trailing
component:
- `Component::Normal(_)` → pop (real segment collapses with `..`)
- `Component::RootDir | Prefix(_)` → drop the `..` (`/..` is `/` per
POSIX)
- else → push `..` (preserve leading `..` chain in relative paths)
Matches Go's `path.Clean` semantics. New test
`lexical_normalize_handles_parent_dir_corner_cases` pins all four
corner cases.
Summary
Closes #11901.
Pacquet's fresh-resolve install path (no
--frozen-lockfile, no usablelockfile) previously only walked the workspace root manifest, so sibling
workspace projects' own dependencies never landed in the lockfile or on
disk. Per the issue, this was the dominant factor in the
babylonbenchmark's 4–7× slowdown across every variation.
This PR makes the fresh-resolve dispatch:
find_workspace_projects(re-used by the existing
workspace:-spec lookup so each manifest isread off disk exactly once).
resolve_importerper importer with its ownproject_dir, sharingthe install-scoped
meta_cache/fetch_locker/picked_manifest_cacheso packument and JSON-parse work is amortized across importers.
DependenciesGraphfordependencies_graph_to_lockfile.importers[<id>]entry per project inpnpm-lock.yaml.link_binsper importer so each project gets its ownnode_modules/.bin.register_projectover every importer key the freshly-builtlockfile carries, mirroring the frozen-lockfile branch.
Mirrors upstream's
resolveRootDependenciesiteration shape: one shared resolution context, per-importer direct-deps
slices.
link:lockfile renderingimporter_dep_versionandsnapshot_dep_reflearned alink:short-circuit so workspace-sibling edges render as
ImporterDepVersion::Link(<rel-path>)/SnapshotDepRef::Link(<rel-path>)instead of failing to parse asname@version. The existing variants on those enums already supportedthe shape — only the conversion needed wiring.
Test plan
pacquet/crates/cli/tests/workspace_install.rsspins up a two-project workspace (
packages/a,packages/b), runs afresh
pacquet install, and asserts each importer'snode_modulessymlinks exist and the lockfile records both
importers[packages/a]and
importers[packages/b].pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs:workspace-link direct dep →
ImporterDepVersion::Link; workspace-linkchild edge →
SnapshotDepRef::Link; multi-importer workspace → oneimporters[<id>]entry per project, shared transitive dep deduped toone snapshot.
cargo nextest run -p pacquet-package-manager— 302/302 pass.cargo nextest run -p pacquet-cli --test add— 5/5 pass(
Install.manifestexception forpacquet add's in-memory manifestmutation: the root importer reuses the in-memory manifest, sibling
importers come from the freshly-read workspace walk).
cargo nextest run -p pacquet-resolving-deps-resolver,pacquet-workspace— clean.cargo clippy --locked --workspace --all-targets -- --deny warnings— clean.
cargo fmt/taplo format/typos pacquet— clean.Out of scope (follow-ups)
TreeCtxsharing. Eachresolve_importercallstill allocates its own
TreeCtx, so transitive packages get resolvedper importer. Network-side caches amortize the HTTP and packument JSON
parsing — only per-resolve semver matching duplicates. Full upstream
parity (one shared resolution context with per-importer hoist loops in
the resolver crate) is a separate refactor that touches
TreeCtx,ResolvedTree,resolve_peers, and ~30 test files.pnpm:package-manifest initialreporter emit. Stillroot-only.
WantedKeywidening to includeproject_dir. Sidestepped by theper-importer
TreeCtx; the cross-importer-sharing follow-up willneed it to keep
workspace:*cache entries unambiguous.Notes for reviewers
Install::rundispatch carves out the root importer to reuseInstall.manifest(rather than the freshly-read root manifest fromfind_workspace_projects).pacquet addmutatesInstall.manifestin memory before calling install; re-reading from disk would walk
the pre-add shape and miss the freshly-added dep. The
addtestscaught this in the first pass.
pacquet-only change and pacquet doesn't ship via the same release
pipeline.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Tests
Refactor
Bug Fixes
Chores