fix(pacquet/package-manager): build workspace state from project list, not lockfile#11946
Conversation
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (9)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughbuild_workspace_state and build_projects_map were changed to accept the precomputed ChangesWorkspace projects from in-memory manifests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11946 +/- ##
=======================================
Coverage 87.93% 87.94%
=======================================
Files 227 227
Lines 27741 27720 -21
=======================================
- Hits 24393 24377 -16
+ Misses 3348 3343 -5 ☔ 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": 1.9606140617199999,
"stddev": 0.10626806904123207,
"median": 1.94871090212,
"user": 2.8041329999999998,
"system": 3.30512146,
"min": 1.82115577712,
"max": 2.22291815012,
"times": [
1.97418629712,
1.99133876312,
1.96418940112,
1.92693841212,
1.87943541812,
1.98738327712,
1.82115577712,
2.22291815012,
1.9332324031199999,
1.9053627181200001
]
},
{
"command": "pacquet@main",
"mean": 1.9648815245200002,
"stddev": 0.07014412069486586,
"median": 1.9563398266199998,
"user": 2.7727329,
"system": 3.2661323599999994,
"min": 1.86028563512,
"max": 2.09002599712,
"times": [
2.09002599712,
2.02357251812,
1.91019426312,
1.86028563512,
2.04595580112,
1.97035438612,
1.92396805612,
1.95741266812,
1.95526698512,
1.91177893512
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.64707307392,
"stddev": 0.02119321637655272,
"median": 0.64039558242,
"user": 0.35773252,
"system": 1.3509480600000001,
"min": 0.63146364992,
"max": 0.7057388469200001,
"times": [
0.7057388469200001,
0.64094749892,
0.6398436659200001,
0.6414283209200001,
0.6512561079200001,
0.6396236609200001,
0.63146364992,
0.64356946792,
0.6390272389200001,
0.6378322809200001
]
},
{
"command": "pacquet@main",
"mean": 0.6456031747200001,
"stddev": 0.019184321406219132,
"median": 0.6404206054200001,
"user": 0.36291892000000003,
"system": 1.35775496,
"min": 0.6285298489200001,
"max": 0.6953312339200001,
"times": [
0.65462746392,
0.6299800259200001,
0.64161015992,
0.6392310509200001,
0.6495622519200001,
0.6953312339200001,
0.64262269392,
0.63617174192,
0.6285298489200001,
0.63836527592
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2279022816,
"stddev": 0.038722953714916276,
"median": 2.2338647783,
"user": 3.8411947199999994,
"system": 3.0281572399999996,
"min": 2.1744865633000003,
"max": 2.2929564153000004,
"times": [
2.2531309553,
2.1964312473,
2.2461420563,
2.2371208673000003,
2.2929564153000004,
2.1836467413,
2.1978863983,
2.2666128823000005,
2.1744865633000003,
2.2306086893000003
]
},
{
"command": "pacquet@main",
"mean": 2.2440500532,
"stddev": 0.0348898225392244,
"median": 2.2497482598000005,
"user": 3.8301325199999994,
"system": 3.0283048399999997,
"min": 2.1857863263,
"max": 2.2997240293,
"times": [
2.2997240293,
2.2642369753000002,
2.2559675803,
2.1857863263,
2.2656580993000004,
2.2430812673,
2.2435289393000004,
2.2030550033000003,
2.2691070563,
2.2103552553
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4548371797400004,
"stddev": 0.027273813886860874,
"median": 1.4609234613400002,
"user": 1.7138468,
"system": 1.84969636,
"min": 1.3965592203400001,
"max": 1.4900935883400002,
"times": [
1.4680479933400001,
1.42166281934,
1.46470954134,
1.4900935883400002,
1.45333152134,
1.4793701373400001,
1.4605289053400001,
1.3965592203400001,
1.4527500533400002,
1.46131801734
]
},
{
"command": "pacquet@main",
"mean": 1.4623849039400003,
"stddev": 0.045439116165343206,
"median": 1.4416310513400001,
"user": 1.7225962,
"system": 1.8526376600000003,
"min": 1.4323517993400001,
"max": 1.57292742434,
"times": [
1.4393480923400002,
1.45264341234,
1.43543805434,
1.4396294013400002,
1.57292742434,
1.4323517993400001,
1.51305368834,
1.43683126434,
1.44363270134,
1.45799320134
]
}
]
} |
|
Bencher Report| Branch | pr/11946 |
| 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,227.90 ms(-34.85%)Baseline: 3,419.57 ms | 4,103.48 ms (54.29%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,454.84 ms(-41.50%)Baseline: 2,487.01 ms | 2,984.41 ms (48.75%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,960.61 ms(-9.10%)Baseline: 2,156.96 ms | 2,588.35 ms (75.75%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 647.07 ms(-2.32%)Baseline: 662.46 ms | 794.95 ms (81.40%) |
a98b116 to
544ff4f
Compare
…, not lockfile `build_projects_map` derived workspace projects from `lockfile.importers.keys()`. On the fresh-install path the wanted lockfile is `None` (no `pnpm-lock.yaml` on disk yet), so the function fell into its no-lockfile arm and recorded only the root importer — even for an 87-project workspace like babylon. That broke the [`optimistic_repeat_install`](https://github.com/pnpm/pnpm/blob/6b3ba4d337/pacquet/crates/package-manager/src/optimistic_repeat_install.rs) fast path on the *next* install: `project_structure_matches` compared the recorded project list (`len = 1`) against the workspace projects the resolver discovered (`len = 87`), returned `false`, and the install fell through to the full resolve + verify path even though the on-disk state was already a no-op. ## Fix Upstream pnpm's [`createWorkspaceState`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/workspace/state/src/createWorkspaceState.ts#L14-L27) takes `allProjects: ProjectsList` as input and emits one entry per project — the lockfile isn't involved. Pacquet already builds the matching `project_manifests: &[(PathBuf, &PackageManifest)]` at the top of `Install::run` (for the optimistic-repeat fast path); thread it through to `build_workspace_state` instead of re-deriving from the lockfile. The new `build_projects_map` is half the size of the old one and can't fall into a "lockfile missing" arm — the project list always comes from the same scan the rest of the install uses. ## Impact Re-bench against the vlt babylon fixture (87-project monorepo) shows every \`*+node_modules\` cell collapsing from "very slow" to "faster than pnpm": | Variation | Before | After | |---|---:|---:| | \`node_modules\` | 37.87x | 0.09x (11x faster than pnpm) | | \`lockfile+node_modules\` | 25.52x | 0.07x (14x faster) | | \`cache+node_modules\` | 11.55x | 0.15x (6.7x faster) | | \`cache+lockfile+node_modules\` | 11.35x | 0.17x (5.9x faster) | The fast path only failed for workspace installs whose wanted lockfile was absent on the first install of the iteration — i.e. exactly the vlt benchmark's \`node_modules\` and \`*+node_modules\` shape. Found while validating #11944's claim that the babylon DNF cells would collapse on the next pacquet release (#11902). ## Tests Ports the two scenarios from upstream's [\`createWorkspaceState.test.ts\`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/workspace/state/test/createWorkspaceState.test.ts) into \`install::tests::build_workspace_state_tests\`: - \`empty_project_list_produces_empty_projects_map\` — zero projects in, empty \`projects\` map out, timestamp still populated. - \`records_every_workspace_project_keyed_by_root_dir\` — four projects in, four entries in \`projects\`, each keyed by \`root_dir\`. Verified to fail on the pre-fix shape (returned 1 entry instead of 4) when \`build_projects_map\` is temporarily restricted to the root.
544ff4f to
106d60b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install.rs (1)
1303-1314: ⚡ Quick winUpdate
build_projects_mapdocs to match the new in-memory contract.This doc block still describes lockfile/importer traversal and read-failure warning behavior, but the function now builds directly from
project_manifests. Please align the contract text with the current implementation.As per coding guidelines: “Doc comments (
///,//!) document the contract … They are not a re-narration of the body.”🤖 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 1303 - 1314, Update the doc comment for build_projects_map to reflect the new in-memory contract: state that the function builds the WorkspaceState projects map directly from the provided project_manifests (no lockfile/importer traversal or disk reads), that it does not perform any file I/O or emit read-failure warnings, and mention how the root importer reuses the in-memory manifest rather than re-reading from disk; keep the comment focused on the function's contract rather than re-describing implementation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pacquet/crates/package-manager/src/install.rs`:
- Around line 1303-1314: Update the doc comment for build_projects_map to
reflect the new in-memory contract: state that the function builds the
WorkspaceState projects map directly from the provided project_manifests (no
lockfile/importer traversal or disk reads), that it does not perform any file
I/O or emit read-failure warnings, and mention how the root importer reuses the
in-memory manifest rather than re-reading from disk; keep the comment focused on
the function's contract rather than re-describing implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ab66dd7-d3cf-4894-9054-ba8927bda267
📒 Files selected for processing (2)
pacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/package-manager/src/install/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/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/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/install.rs
🧬 Code graph analysis (2)
pacquet/crates/package-manager/src/install/tests.rs (2)
pacquet/crates/config/src/lib.rs (2)
Config(941-1397)NodeLinker(44-57)pacquet/crates/package-manifest/src/lib.rs (2)
PackageManifest(67-70)PackageManifest(72-260)
pacquet/crates/package-manager/src/install.rs (2)
pacquet/crates/package-manifest/src/lib.rs (2)
path(143-145)PackageManifest(67-70)pacquet/crates/workspace-state/src/lib.rs (3)
ProjectEntry(42-47)WorkspaceState(57-65)now_millis(220-225)
🔇 Additional comments (2)
pacquet/crates/package-manager/src/install.rs (1)
671-672: LGTM!Also applies to: 913-914, 1316-1327, 1343-1347
pacquet/crates/package-manager/src/install/tests.rs (1)
814-894: LGTM!
…er refactor The doc comment still described the old lockfile-driven shape that read each sub-importer's package.json from disk. Now that the function takes pre-loaded `(PathBuf, &PackageManifest)` entries, update the contract to reflect what actually happens: pure in-memory mapping, no I/O, no read-failure warnings.
Summary
build_projects_mapderived workspace projects fromlockfile.importers.keys(). On the fresh-install path the wanted lockfile isNone(nopnpm-lock.yamlon disk yet), so the function fell into its no-lockfile arm and recorded only the root importer — even for an 87-project workspace like babylon.That broke the
optimistic_repeat_installfast path on the next install:project_structure_matchescompared the recorded project list (len = 1) against the workspace projects the resolver discovered (len = 87), returnedfalse, and the install fell through to the full resolve + verify path even though the on-disk state was already a no-op.Fix
Upstream pnpm's
createWorkspaceStatetakesallProjects: ProjectsListas input and emits one entry per project — the lockfile isn't involved. Pacquet already builds the matchingproject_manifests: &[(PathBuf, &PackageManifest)]at the top ofInstall::run(for the optimistic-repeat fast path); thread it through tobuild_workspace_stateinstead of re-deriving from the lockfile.The new
build_projects_mapis half the size of the old one and can't fall into a "lockfile missing" arm — the project list always comes from the same scan the rest of the install uses.Impact
Re-bench against the vlt
babylonfixture (87-project monorepo) shows every*+node_modulescell collapsing from "very slow" to "faster than pnpm":node_moduleslockfile+node_modulescache+node_modulescache+lockfile+node_modulesThese were the four worst cells in the vlt benchmark chart for babylon (all flagged DNF before #11944 fixed the underlying panic). After #11944 babylon stopped DNF'ing but stayed 11-37× slower because of this workspace-state writing bug.
The fast path only failed for workspace installs whose wanted lockfile was absent on the first install of the iteration — i.e. exactly the vlt benchmark's
node_modulesand*+node_modulesshape. Single-project installs always recorded the root correctly because the no-lockfile fallback already emitted the root entry.Found while validating #11944's claim that the babylon DNF cells would collapse on the next pacquet release (#11902).
Tests
Ports the two scenarios from upstream's
createWorkspaceState.test.tsintoinstall::tests::build_workspace_state_tests:empty_project_list_produces_empty_projects_map— zero projects in, emptyprojectsmap out, timestamp still populated. Mirrors pnpm'screateWorkspaceState() on empty list.records_every_workspace_project_keyed_by_root_dir— four projects in, four entries inprojects, each keyed byroot_dir. Mirrors pnpm'screateWorkspaceState() on non-empty list. Verified to fail on the pre-fix shape (returned 1 entry instead of 4) whenbuild_projects_mapis temporarily restricted to the root.Test plan
build_projects_mapto.take(1)).cargo nextest run -p pacquet-package-manager— 335/335 pass.cargo clippy --locked --workspace --all-targets -- --deny warnings— clean.cargo fmt --check— clean.node_modules/.pnpm-workspace-state-v1.jsonafter a fresh install now contains 87 projects (was 1); 2nd install fires the no-op fast path in 0.097s.bench run --pms=pnpm,pacquet --fixtures=babylon --variation={node_modules,cache+node_modules,lockfile+node_modules,cache+lockfile+node_modules}— every cell now under 1.0× and per-iteration times stable at 0.06–0.11s (was 0.09–29s).Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit