feat(package-manager): write .pnpm-workspace-state-v1.json after install#11665
Merged
Conversation
pnpm's verifyDepsBeforeRun gate bails out with "Cannot check whether dependencies are outdated" as soon as the workspace state file is missing, so a node_modules tree materialized by pacquet always tripped the check and forced a reinstall. Port @pnpm/workspace.state to a new pacquet-workspace-state crate and write the file at the end of Install::run so pnpm can fast-path the freshness check after pacquet has done the install. Closes the gap behind the pnpm_config_verify_deps_before_run: false workaround in 7ff112b.
|
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). (9)
📝 WalkthroughWalkthroughThis PR adds a new ChangesWorkspace state persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Comment |
Re-align the dependency-block padding to taplo's expected width — CI's `taplo format --check` flagged the 13-character padding the manual draft shipped with.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install.rs (1)
724-728: ⚡ Quick winAvoid re-resolving workspace root inside
build_workspace_state.
Install::runalready resolvesworkspace_rootwith error propagation. Recomputing it here and swallowing errors can silently fall back tomanifest_dir, which risks inconsistent project keys vs. write location in edge cases. Thread&workspace_rootinto this helper instead.♻️ Suggested refactor
-fn build_workspace_state( +fn build_workspace_state( + workspace_root: &std::path::Path, config: &Config, node_linker: NodeLinker, included: IncludedDependencies, manifest: &PackageManifest, lockfile: Option<&Lockfile>, ) -> WorkspaceState { - let manifest_dir = manifest.path().parent().expect("manifest path always has a parent dir"); - let workspace_root = pacquet_workspace::find_workspace_dir(manifest_dir) - .ok() - .flatten() - .unwrap_or_else(|| manifest_dir.to_path_buf()); - let allow_builds = (!config.allow_builds.is_empty()).then(|| { config.allow_builds.iter().map(|(k, v)| (k.clone(), serde_json::Value::Bool(*v))).collect() }); WorkspaceState { last_validated_timestamp: now_millis(), - projects: build_projects_map(&workspace_root, manifest, lockfile), + projects: build_projects_map(workspace_root, manifest, lockfile),update_workspace_state( &workspace_root, - &build_workspace_state(config, node_linker, included, manifest, lockfile), + &build_workspace_state(&workspace_root, config, node_linker, included, manifest, lockfile), ) .map_err(InstallError::WriteWorkspaceState)?;🤖 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 724 - 728, The helper build_workspace_state must not re-resolve the workspace root; instead, change its signature to accept a &workspace_root (Path or PathBuf reference) and use that value rather than calling pacquet_workspace::find_workspace_dir inside build_workspace_state (remove the manifest_dir/workspace_root recomputation and the unwrap_or_else fallback). Update the call site in Install::run to pass the already-resolved workspace_root (propagating errors as currently done there) so workspace resolution is centralized and no silent fallback to manifest_dir occurs.
🤖 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 724-728: The helper build_workspace_state must not re-resolve the
workspace root; instead, change its signature to accept a &workspace_root (Path
or PathBuf reference) and use that value rather than calling
pacquet_workspace::find_workspace_dir inside build_workspace_state (remove the
manifest_dir/workspace_root recomputation and the unwrap_or_else fallback).
Update the call site in Install::run to pass the already-resolved workspace_root
(propagating errors as currently done there) so workspace resolution is
centralized and no silent fallback to manifest_dir occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de6446ca-b30b-4f15-83e3-d353d3645320
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlpacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/workspace-state/Cargo.tomlpacquet/crates/workspace-state/src/lib.rspacquet/crates/workspace-state/src/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Agent
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Dylint
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- 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)
No star imports inside module bodies — write
use super::{Foo, bar}instead ofuse super::*;and the same for any other glob. Exception: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-exports likepub use submodule::*;inlib.rs
Files:
pacquet/crates/workspace-state/src/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/workspace-state/src/lib.rs
🔇 Additional comments (7)
pacquet/crates/workspace-state/Cargo.toml (1)
1-24: LGTM!pacquet/crates/workspace-state/src/lib.rs (1)
14-225: LGTM!pacquet/crates/workspace-state/src/tests.rs (1)
1-98: LGTM!Cargo.toml (1)
38-38: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
32-32: LGTM!pacquet/crates/package-manager/src/install.rs (1)
24-27: LGTM!Also applies to: 148-155, 520-534, 635-705, 730-762
pacquet/crates/package-manager/src/install/tests.rs (1)
16-18: LGTM!Also applies to: 686-796
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11665 +/- ##
==========================================
+ Coverage 89.13% 89.14% +0.01%
==========================================
Files 126 127 +1
Lines 14337 14458 +121
==========================================
+ Hits 12779 12889 +110
- Misses 1558 1569 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Dylint's `perfectionist::macro_trailing_comma` flags single-line macro invocations that end with a trailing comma. Rustfmt's earlier collapse of the multi-line assertion left the comma intact; remove it so the nightly dylint check passes.
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.67203833806,
"stddev": 0.08583373177202643,
"median": 2.66344435116,
"user": 2.59712534,
"system": 3.730567979999999,
"min": 2.5694548196600002,
"max": 2.82177194566,
"times": [
2.60403678066,
2.72496114066,
2.77217629766,
2.58769840866,
2.71187870466,
2.82177194566,
2.5694548196600002,
2.63112424666,
2.69576445566,
2.6015165806600002
]
},
{
"command": "pacquet@main",
"mean": 2.65255090956,
"stddev": 0.08020325402914395,
"median": 2.67723251066,
"user": 2.61847504,
"system": 3.6944637799999995,
"min": 2.5233185816600003,
"max": 2.73387510066,
"times": [
2.72850961466,
2.73387510066,
2.70125381666,
2.60734307866,
2.6532112046600003,
2.6157162936600002,
2.53021886366,
2.71838988266,
2.5233185816600003,
2.71367265866
]
},
{
"command": "pnpm",
"mean": 5.01547437916,
"stddev": 0.051150328625306045,
"median": 5.0038302546599995,
"user": 8.23112784,
"system": 4.21627378,
"min": 4.93624354466,
"max": 5.10539405966,
"times": [
5.10539405966,
5.07839182566,
4.986988128659999,
5.0495809206599995,
5.02725059666,
4.983793921659999,
4.98312501666,
5.02067238066,
4.983303396659999,
4.93624354466
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7473898256400002,
"stddev": 0.04556598222646133,
"median": 0.7297346974400001,
"user": 0.3815628,
"system": 1.62438802,
"min": 0.70279533344,
"max": 0.8318790774400001,
"times": [
0.8318790774400001,
0.7151882904400001,
0.70279533344,
0.7168967354400001,
0.71755070344,
0.71064239344,
0.7419186914400001,
0.8101602564400001,
0.7842523664400001,
0.74261440844
]
},
{
"command": "pacquet@main",
"mean": 0.83145604314,
"stddev": 0.05554386153951669,
"median": 0.8093636929400001,
"user": 0.3954014,
"system": 1.65340802,
"min": 0.7730499014400001,
"max": 0.9332127174400001,
"times": [
0.9332127174400001,
0.7730499014400001,
0.7844849944400001,
0.83858969244,
0.91294734444,
0.81152212544,
0.8072052604400001,
0.80129463944,
0.86581575844,
0.7864379974400001
]
},
{
"command": "pnpm",
"mean": 2.71827581064,
"stddev": 0.12934283433903235,
"median": 2.70415829644,
"user": 3.3927828000000004,
"system": 2.27928042,
"min": 2.58162992544,
"max": 2.99327651844,
"times": [
2.75416880344,
2.68311252344,
2.86460070144,
2.58162992544,
2.64284524544,
2.72713482444,
2.62810254244,
2.58268295244,
2.99327651844,
2.72520406944
]
}
]
} |
github-actions Bot
pushed a commit
to Eyalm321/pnpm
that referenced
this pull request
May 18, 2026
…all (pnpm#11665) * feat(package-manager): write .pnpm-workspace-state-v1.json after install pnpm's verifyDepsBeforeRun gate bails out with "Cannot check whether dependencies are outdated" as soon as the workspace state file is missing, so a node_modules tree materialized by pacquet always tripped the check and forced a reinstall. Port @pnpm/workspace.state to a new pacquet-workspace-state crate and write the file at the end of Install::run so pnpm can fast-path the freshness check after pacquet has done the install. Closes the gap behind the pnpm_config_verify_deps_before_run: false workaround in 7ff112b. * chore(workspace-state): apply taplo formatting Re-align the dependency-block padding to taplo's expected width — CI's `taplo format --check` flagged the 13-character padding the manual draft shipped with. * chore(workspace-state): drop trailing comma in single-line assert_eq! Dylint's `perfectionist::macro_trailing_comma` flags single-line macro invocations that end with a trailing comma. Rustfmt's earlier collapse of the multi-line assertion left the comma intact; remove it so the nightly dylint check passes.
This was referenced May 18, 2026
This was referenced May 25, 2026
This was referenced May 29, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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
@pnpm/workspace.stateto a newpacquet-workspace-statecrate so pacquet writes<workspaceDir>/node_modules/.pnpm-workspace-state-v1.jsonat the end of every install.Install::runright after.modules.yamland the current lockfile are persisted, mirroring upstream'supdateWorkspaceStatecall site.verifyDepsBeforeRungate immediately returnsupToDate: falsewith the issue "Cannot check whether dependencies are outdated" — which is why ci: run install with pacquet #11657 had to disable the check in CI.The file records the project list (read from lockfile importers, falling back to the root manifest),
nodeLinker,dev/production/optional(from the dispatchedIncludedDependencies),autoInstallPeers,dedupePeerDependents,hoistPattern,publicHoistPattern,hoistWorkspacePackages,ignoredOptionalDependencies,patchedDependencies, andallowBuilds. Settings pacquet does not track yet (e.g.dedupeDirectDeps,peersSuffixMaxLength,overrides,packageExtensions) are omitted; pnpm's check only iterates fields present in the JSON, so an absent key is silently skipped.After this lands, the
pnpm_config_verify_deps_before_run: falseenv var introduced in 7ff112b can be dropped from.github/workflows/ci.ymland.github/workflows/test.yml.Test plan
cargo nextest run -p pacquet-workspace-state— 5/5 new unit tests (file path, round-trip, missing-file,nodeLinkerlowercase, omits-None settings).cargo nextest run -p pacquet-package-manager— 268/268 pass (267 existing + newinstall_writes_workspace_stateintegration test that asserts the file shape after a minimal frozen-lockfile install).cargo clippy -p pacquet-workspace-state --all-targets -- --deny warningsandcargo clippy -p pacquet-package-manager --lib -- --deny warningsclean.cargo fmt --checkclean.cargo check --workspaceclean.pnpm_config_verify_deps_before_run: falseand the no-op tests workaround from.github/workflows/{ci,test}.yml.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests
Chores