Conversation
…#464 §A) Direct follow-up to #445 (hoisting). Pacquet now reads `.modules.yaml` on the frozen-lockfile install path and compares the recorded layout-driving fields against the current install's effective config. A drift on any axis errors out instead of silently leaving the prior layout behind. Closes #464 §A. Mirrors upstream pnpm's `validateModules` + `checkCompatibility` ([`94240bc046`](https://github.com/pnpm/pnpm/tree/94240bc046)). ## Axes covered - **`hoist_pattern`** → `HOIST_PATTERN_DIFF`. The most user-facing axis: changing `pnpm-workspace.yaml`'s `hoistPattern` between installs now errors instead of leaving stale `<vs>/node_modules/` symlinks behind. - **`public_hoist_pattern`** → `PUBLIC_HOIST_PATTERN_DIFF`. - **`included` (dependency groups)** → `INCLUDED_DEPS_CONFLICT`, with the recorded vs requested groups in the error payload. - **`virtual_store_dir_max_length`** → `VIRTUAL_STORE_DIR_MAX_LENGTH_DIFF`. - **`store_dir`** → `UNEXPECTED_STORE`. - **`virtual_store_dir`** → `UNEXPECTED_VIRTUAL_STORE_DIR`. - **`layout_version`** is enforced at deserialize time by `LayoutVersion::try_from`, so a mismatch surfaces as `ReadModulesError` before this validator runs. Drift checks fire in upstream's order so the first error a user sees matches what pnpm would surface for the same drift. `None` and `Some([])` patterns compare equal (mirrors upstream's `?? []` coalesce). Path equality goes through `Path::eq`, which ignores trailing-slash-style differences — `/store` and `/store/` compare equal as upstream's `path.relative === ''` check does. ## Implementation - New `validate_modules` module + `ValidateModulesError` enum, one variant per axis with `miette` codes matching upstream's `ERR_PNPM_*` symbols and user-facing messages copied verbatim so a user sees the same wording in both tools. - `Install::run` reads the manifest via `read_modules_manifest` (returns `Ok(None)` on first install — no validation), then calls `validate_modules` before the install dispatcher. Two new `InstallError` variants (`ReadModulesManifest`, `ValidateModules`) thread the typed errors out. ## Tests - 10 unit tests in `validate_modules::tests` covering each axis in isolation + the upstream-order tiebreaker + the `None`/`Some([])` and trailing-slash equivalences. - 3 CLI integration tests in `crates/cli/tests/validate_modules.rs`: - `re_install_with_changed_hoist_pattern_errors` — install with default, mutate yaml, re-install → fails with hoist-pattern-diff error message. - `re_install_with_changed_public_hoist_pattern_errors` — same for the public side. - `re_install_with_no_change_succeeds` — guards against the validator firing on every re-install. - Two `known_failures` in `crates/cli/tests/hoist.rs` (`hoist.ts:209` / `:220`) updated: now no-op stubs explaining they're covered indirectly by the new integration test, with their porting plan entries updated accordingly. ## Out of scope - §B: `--force` purge path. Today the validator errors and the user wipes `node_modules/` manually. The `force: bool` plumbing + per-importer `removeContentsOfDir` will land separately. - §C: `virtualStoreOnly` exemption. Pacquet doesn't implement `virtualStoreOnly` install yet — guard added when that mode ships. Tests: `just ready` 922/922 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements ChangesLayout drift validation and install integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/package-manager/src/install.rs`:
- Around line 232-248: The modules manifest validation runs unconditionally;
gate the read_modules_manifest + validate_modules block so it only executes when
the install is in frozen-lockfile mode (check the install config flag, e.g.
config.frozen_lockfile or the appropriate frozen-lockfile boolean on config).
Concretely: wrap the current if let Some(modules) =
read_modules_manifest::<RealApi>(&config.modules_dir) { ... } block in a
conditional that first checks the frozen-lockfile flag on config, and only then
reads and calls validate_modules(&modules, config, included, &workspace_root,
&config.modules_dir), returning the same InstallError variants on failure.
In `@crates/package-manager/src/validate_modules.rs`:
- Around line 75-78: Update the remediation help text so it no longer suggests
`pacquet install --force`; for the diagnostic emitted via
pacquet_package_manager::hoist_pattern_diff (and the other drift variants
referenced in this file) change the help message to instruct users to either
restore the previous hoistPattern in `pnpm-workspace.yaml` or manually remove
`node_modules/` and run a normal reinstall (e.g., `rm -rf node_modules &&
pacquet install`), and apply this exact wording change consistently to all the
other drift variants in this module so no diagnostic recommends `--force`.
🪄 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: 1d30f430-1f25-4c01-8314-5a08853149c9
📒 Files selected for processing (7)
crates/cli/tests/hoist.rscrates/cli/tests/validate_modules.rscrates/package-manager/src/install.rscrates/package-manager/src/lib.rscrates/package-manager/src/validate_modules.rscrates/package-manager/src/validate_modules/tests.rsplans/TEST_PORTING.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/lib.rscrates/cli/tests/validate_modules.rscrates/package-manager/src/validate_modules/tests.rscrates/package-manager/src/install.rscrates/cli/tests/hoist.rscrates/package-manager/src/validate_modules.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
When citing upstream pnpm code anywhere (code comments, doc comments, Markdown docs, PR descriptions, or commit messages), link to a specific commit SHA, not a branch name. Use the first 10 hex characters of the SHA. Branch links like
github.com/<owner>/<repo>/blob/main/...are impermanent; permanent links pin the commit so the reference stays meaningful long after upstream changes. This rule applies to references to any GitHub repository, not only pnpm/pnpm.
Files:
plans/TEST_PORTING.md
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: When porting behavior from pnpm, port the relevant pnpm tests to Rust tests whenever they translate. Matching test coverage is the easiest way to prove behavioral parity.
Consult the test-porting plan inplans/TEST_PORTING.mdbefore adding ported tests. Follow the conventions expected for ports: useknown_failuresmodules, usepacquet_testing_utils::allow_known_failure!at the not-yet-implemented boundary, and temporarily break the subject under test to verify the ported test actually catches the regression. Update TEST_PORTING.md checkboxes as items land.
Follow the test-logging guidance in CODE_STYLE_GUIDE.md: log before non-assert_eq!assertions, usedbg!for complex structures, skip logging for simple scalarassert_eq!assertions.
Files:
crates/cli/tests/validate_modules.rscrates/cli/tests/hoist.rs
🧠 Learnings (3)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/lib.rscrates/cli/tests/validate_modules.rscrates/package-manager/src/validate_modules/tests.rscrates/package-manager/src/install.rscrates/cli/tests/hoist.rscrates/package-manager/src/validate_modules.rs
📚 Learning: 2026-05-07T14:24:47.105Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 391
File: crates/cli/tests/lifecycle_scripts.rs:0-0
Timestamp: 2026-05-07T14:24:47.105Z
Learning: In pnpm/pacquet CLI lifecycle tests, note that `AutoMockInstance::load_or_init` returns an anchor to a shared singleton mock registry process. If a test spawns a secondary `CommandTempCwd::init().add_mocked_registry()` (e.g., to run a reinstall with `--frozen-lockfile`), the secondary/inner `mock_instance` may be dropped safely as long as the primary/outer `mock_instance` remains in scope (so the singleton registry stays alive). Separately retain the inner `TempDir` (e.g., via a `frozen_root` binding) so the workspace lives for the duration of the command.
Applied to files:
crates/cli/tests/validate_modules.rscrates/cli/tests/hoist.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/validate_modules/tests.rs
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #470 +/- ##
==========================================
+ Coverage 88.71% 88.91% +0.20%
==========================================
Files 116 117 +1
Lines 9936 10044 +108
==========================================
+ Hits 8815 8931 +116
+ Misses 1121 1113 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds .modules.yaml validation on install so pacquet errors out (instead of silently keeping a stale node_modules layout) when layout-driving settings drift between runs, mirroring pnpm’s validateModules + checkCompatibility.
Changes:
- Introduces
validate_modules+ValidateModulesErrorwith drift checks for hoist patterns, included dependency groups, store paths, and virtual-store-dir-max-length. - Wires
.modules.yamlread + validation intoInstall::run, surfacing newInstallErrorvariants for read/validate failures. - Adds unit tests for each drift axis and CLI integration tests covering re-install drift scenarios; updates test-porting notes / known-failure stubs accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plans/TEST_PORTING.md | Updates hoist.ts porting notes to point at new validate-modules integration coverage. |
| crates/package-manager/src/validate_modules.rs | New validator + typed diagnostics for .modules.yaml drift axes. |
| crates/package-manager/src/validate_modules/tests.rs | Unit tests covering each drift axis and ordering semantics. |
| crates/package-manager/src/install.rs | Reads .modules.yaml and validates it before proceeding with install. |
| crates/package-manager/src/lib.rs | Exposes the new validate_modules module via re-exports. |
| crates/cli/tests/validate_modules.rs | Adds end-to-end tests asserting re-install errors on hoist-pattern drift. |
| crates/cli/tests/hoist.rs | Replaces two prior known-failure stubs with no-op notes (covered indirectly). |
Comments suppressed due to low confidence (1)
crates/package-manager/src/validate_modules.rs:79
- This diagnostic
helptext points users topacquet install --force, butpacquet installcurrently has no--forceCLI option. Since this string will be user-facing, it should reflect an actually-supported fix (manual deletion, or add the--forceflag + purge behavior as part of this slice). Also consider applying the same update to the otherValidateModulesErrorvariants that mention--force.
#[diagnostic(
code(pacquet_package_manager::hoist_pattern_diff),
help(
"Either run `pacquet install --force` to recreate `node_modules/`, or restore the previous `hoistPattern` in `pnpm-workspace.yaml`."
)
)]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.84261943178,
"stddev": 0.4231240990456764,
"median": 3.9573680402799996,
"user": 2.0738513399999996,
"system": 2.7736587,
"min": 3.13861229228,
"max": 4.34113330728,
"times": [
4.04046318128,
4.029616347279999,
3.76952703828,
4.34113330728,
4.2232561642799995,
3.8851197332800003,
3.58719805128,
3.18686114628,
3.13861229228,
4.22440705628
]
},
{
"command": "pacquet@main",
"mean": 3.2029993817800007,
"stddev": 0.21947528448293058,
"median": 3.16655833728,
"user": 2.05622814,
"system": 2.7799144,
"min": 2.94749576828,
"max": 3.72356056528,
"times": [
3.3303778252800003,
2.9510279602800003,
3.2543414372800004,
3.1247754972800004,
3.1363799722800003,
3.22891811728,
3.1891734682800004,
3.14394320628,
2.94749576828,
3.72356056528
]
},
{
"command": "pnpm",
"mean": 6.48176193928,
"stddev": 0.91824534618488,
"median": 6.025689391779999,
"user": 7.254593739999999,
"system": 3.4176197000000004,
"min": 5.645190246279999,
"max": 8.00503689528,
"times": [
5.85378147128,
5.87481410728,
5.645190246279999,
5.9905017012799995,
6.060877082279999,
5.8958165212799996,
6.11575900128,
7.69781063428,
8.00503689528,
7.67803173228
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.06972139758,
"stddev": 0.11781316938032782,
"median": 1.05611256218,
"user": 0.32106555999999997,
"system": 1.2590037799999998,
"min": 0.9676286321800001,
"max": 1.36259598518,
"times": [
1.09184724318,
1.08669539118,
1.36259598518,
1.06079518918,
0.9676286321800001,
0.98069885118,
1.13167248918,
1.05142993518,
0.99514980318,
0.96870045618
]
},
{
"command": "pacquet@main",
"mean": 2.0857131968799996,
"stddev": 0.5096825199839311,
"median": 1.94987143518,
"user": 0.32697716,
"system": 1.27656088,
"min": 1.31821494418,
"max": 2.68692652318,
"times": [
2.61434093618,
2.68692652318,
1.97289061218,
1.91047715418,
2.59747872718,
1.31821494418,
1.91057673518,
2.55760542618,
1.92685225818,
1.36176865218
]
},
{
"command": "pnpm",
"mean": 3.3718517181800003,
"stddev": 0.9369236273291951,
"median": 2.9064461016800003,
"user": 2.3805929599999995,
"system": 1.6883618799999998,
"min": 2.39209658118,
"max": 5.199676682180001,
"times": [
4.47013879118,
2.5984457831800003,
2.93660641418,
2.39209658118,
2.87628578918,
2.72431831918,
2.74752505118,
3.78446671218,
5.199676682180001,
3.98895705818
]
}
]
} |
…elp text Address PR #470 review feedback (CodeRabbit + Copilot, two distinct issues): - **Gate `.modules.yaml` validation on `frozen_lockfile`** (both reviewers, real bug). Validation was running on every install path, including `pacquet install` (without `--frozen-lockfile`). Hoisting itself is frozen-lockfile-only in pacquet today (`InstallWithoutLockfile::run` returns an empty `HoistedDependencies` map), so the without-lockfile path can't drift on `hoist_pattern` / `public_hoist_pattern` even when yaml flips between runs — the validation error there would surface drift that's irrelevant for that mode. Matches the PR scope (#464 §A is explicitly the frozen-lockfile read-and-error path). - **Drop `--force` from help text** (multiple reviewers). The per-axis `help(...)` strings recommended `pacquet install --force`, but pacquet doesn't expose a `--force` install flag yet (it's tracked under #464 §B). Replaced with the actual recovery path: restore the previous yaml setting, or remove `node_modules/` and re-run `pacquet install --frozen-lockfile`. - For `VirtualStoreDirMaxLengthDiff` specifically: the help points only at the manual-purge path, since pacquet doesn't yet read `virtualStoreDirMaxLength` from `pnpm-workspace.yaml` (the loader pins the upstream default 120 unconditionally) — the user can't restore the recorded value via yaml, only by re-creating `node_modules/`. Doc comment notes this and when the future yaml-key wiring lands the help should switch. - Module docs and the `InstallError::ValidateModules` doc comment also updated to point at the manual recovery path while explaining where the upstream `--force` mapping lives (#464 §B). Tests: `just ready` 922/922 pass; `just dylint` clean. The 3 CLI integration tests in `validate_modules.rs` continue to pass — they all use `--frozen-lockfile` so the new gate doesn't change their behavior. The `re_install_with_no_change_succeeds` test specifically also confirms that running `pacquet install --frozen-lockfile` twice doesn't trip the validation when nothing drifted.
Closes #464 §A.
Direct follow-up to #445 (hoisting). Pacquet now reads
.modules.yamlon the frozen-lockfile install path and errors out on layout drift instead of silently leaving the prior layout behind.Summary
validate_modulesmodule +ValidateModulesErrorenum, one variant per drift axis withmiettecodes matching upstream'sERR_PNPM_*symbols and user-facing messages copied verbatim from upstream so a user sees the same wording in pnpm and pacquet.Install::runreads.modules.yamlviaread_modules_manifest(returnsOk(None)on first install — no validation), then callsvalidate_modulesbefore the install dispatcher.InstallErrorvariants (ReadModulesManifest,ValidateModules) thread the typed errors out.Axes covered
Mirrors upstream pnpm's
validateModules+checkCompatibility.hoist_patternHOIST_PATTERN_DIFFpnpm-workspace.yamlhoistPatternchanged between installspublic_hoist_patternPUBLIC_HOIST_PATTERN_DIFFincludedINCLUDED_DEPS_CONFLICT--prodvirtual_store_dir_max_lengthVIRTUAL_STORE_DIR_MAX_LENGTH_DIFFstore_dirUNEXPECTED_STOREvirtual_store_dirUNEXPECTED_VIRTUAL_STORE_DIRlayout_versionReadModulesErrorDrift checks fire in upstream's order so the first error a user sees matches what pnpm would surface for the same drift.
Equivalences
NoneandSome([])patterns compare equal — mirrors upstream'sequals(modules.publicHoistPattern ?? [], opts.publicHoistPattern ?? [])coalesce.Path::eq, which is component-wise —/storeand/store/compare equal as upstream'spath.relative === ''does.Tests
validate_modules::testscovering each axis in isolation + the upstream-order tiebreaker + theNone/Some([])and trailing-slash equivalences.crates/cli/tests/validate_modules.rs:re_install_with_changed_hoist_pattern_errors— install with default, mutate yaml, re-install → fails with hoist-pattern-diff message in stderr.re_install_with_changed_public_hoist_pattern_errors— same for the public side.re_install_with_no_change_succeeds— guards against the validator firing on every re-install when nothing changed.known_failuresincrates/cli/tests/hoist.rs(hoist.ts:209/:220) updated: now no-op stubs explaining they're covered indirectly by the new integration test, with their porting plan entries updated accordingly.Out of scope (separate slices of #464)
--forcepurge path — today the validator errors and the user wipesnode_modules/manually. Theforce: boolplumbing + per-importerremoveContentsOfDiris the natural follow-up.virtualStoreOnlyexemption — pacquet doesn't implementvirtualStoreOnlyinstall yet; the guard goes in when that mode ships.Test plan
cargo nextest run -p pacquet-package-manager validate_modules::tests— 10/10 passcargo nextest run -p pacquet-cli --test validate_modules— 3/3 passjust ready— 922/922 passcargo doc --workspace --no-depswithRUSTDOCFLAGS="-D warnings"— cleanjust dylint— cleantaplo format --check— cleanWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
.modules.yamlconfiguration and fails with error messages when layout-related settings (hoist patterns, dependency groups, storage directories) differ from previous installs.Tests
.modules.yamlvalidation across configuration changes.