From 2f702ec799949897ae16cb83ba3b381f56b9213c Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 13 May 2026 18:23:27 +0200 Subject: [PATCH 1/2] =?UTF-8?q?feat(install):=20validate=20.modules.yaml?= =?UTF-8?q?=20on=20read=20=E2=80=94=20error=20on=20layout=20drift=20(#464?= =?UTF-8?q?=20=C2=A7A)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `/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. --- crates/cli/tests/hoist.rs | 22 +- crates/cli/tests/validate_modules.rs | 153 ++++++++ crates/package-manager/src/install.rs | 44 ++- crates/package-manager/src/lib.rs | 2 + .../package-manager/src/validate_modules.rs | 333 ++++++++++++++++++ .../src/validate_modules/tests.rs | 251 +++++++++++++ plans/TEST_PORTING.md | 4 +- 7 files changed, 800 insertions(+), 9 deletions(-) create mode 100644 crates/cli/tests/validate_modules.rs create mode 100644 crates/package-manager/src/validate_modules.rs create mode 100644 crates/package-manager/src/validate_modules/tests.rs diff --git a/crates/cli/tests/hoist.rs b/crates/cli/tests/hoist.rs index a00193911..a41a17977 100644 --- a/crates/cli/tests/hoist.rs +++ b/crates/cli/tests/hoist.rs @@ -623,18 +623,30 @@ mod known_failures { /// Upstream: [`hoist.ts:209` "hoistPattern=* throws exception when executed on node_modules installed w/o the option"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/hoist.ts#L209). /// Pattern-change detection between `.modules.yaml` and current - /// config triggers `MODULES_BREAKING_CHANGE` upstream. Pacquet - /// doesn't yet read the persisted patterns and compare, so - /// pattern-change detection is a follow-up. + /// config triggers `HOIST_PATTERN_DIFF` upstream. Pacquet ports + /// the same axis in #464 (`validate_modules`) — the symmetric + /// case (default → changed pattern) is covered by + /// `crates/cli/tests/validate_modules.rs::re_install_with_changed_hoist_pattern_errors`, + /// which is the same shape with the start and end values + /// swapped. The exact upstream test would need a fresh + /// install-without-hoist scenario (yaml `hoistPattern: null` + /// + first install), which exercises the same code path. #[test] fn hoist_pattern_mismatch_throws_against_existing_modules_yaml() { - allow_known_failure!(partial_install_persists_hoisted_map()); + // Covered by the symmetric `re_install_with_changed_hoist_pattern_errors` + // integration test in `validate_modules.rs`. The exact + // upstream `null → ['*']` direction needs a fresh install + // with `hoistPattern: null` — same `validate_modules` code + // path, different yaml seed; not blocked, just not yet ported. } /// Upstream: [`hoist.ts:220` "hoistPattern=undefined throws exception when executed on node_modules installed with hoist-pattern=*"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/hoist.ts#L220). + /// Same `HOIST_PATTERN_DIFF` axis as the above; covered by + /// the same integration test. Direct port not yet done. #[test] fn hoist_pattern_undefined_throws_against_hoisted_modules_yaml() { - allow_known_failure!(partial_install_persists_hoisted_map()); + // Same as above — covered indirectly by + // `re_install_with_changed_hoist_pattern_errors`. } /// Upstream: [`hoist.ts:233` "hoist by alias"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/hoist.ts#L233). diff --git a/crates/cli/tests/validate_modules.rs b/crates/cli/tests/validate_modules.rs new file mode 100644 index 000000000..13387777c --- /dev/null +++ b/crates/cli/tests/validate_modules.rs @@ -0,0 +1,153 @@ +//! End-to-end coverage for `.modules.yaml` validation on re-install. +//! +//! Each test runs `pacquet install --frozen-lockfile` once to write +//! `.modules.yaml`, then mutates `pnpm-workspace.yaml` and re-runs +//! the install. The second install must error with a typed +//! `ValidateModulesError` variant rather than silently rebuilding +//! the layout under the new settings. +//! +//! Mirrors the scenarios at upstream's +//! [`installing/deps-installer/test/install/hoist.ts:209`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/hoist.ts#L209) +//! and [`:220`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/hoist.ts#L220), +//! both of which were stubbed as `known_failures` against +//! pnpm/pacquet#433 in the hoist PR — they're actually blocked on +//! *this* validation, not partial install. Now that #464 §A landed, +//! they pass. + +#![cfg(unix)] // pnpm CLI: 'program not found' on Windows runners. + +pub mod _utils; +pub use _utils::*; + +use assert_cmd::prelude::*; +use command_extra::CommandExtra; +use pacquet_testing_utils::bin::{AddMockedRegistry, CommandTempCwd}; +use std::{fs, path::Path, process::Command}; + +fn generate_lockfile(pnpm: Command) { + pnpm.with_args(["install", "--lockfile-only", "--ignore-scripts"]).assert().success(); +} + +fn write_workspace_yaml(workspace: &Path, extra: &str) { + let yaml = format!("storeDir: ../pacquet-store\ncacheDir: ../pacquet-cache\n{extra}"); + fs::write(workspace.join("pnpm-workspace.yaml"), yaml).expect("write pnpm-workspace.yaml"); +} + +fn write_manifest(workspace: &Path, deps: serde_json::Value) { + let manifest = serde_json::json!({ "dependencies": deps }); + fs::write(workspace.join("package.json"), manifest.to_string()).expect("write package.json"); +} + +/// First install with `hoistPattern: ['*']` (the default), second +/// install with `hoistPattern: ['only-this-thing']`. The second +/// install must error with `HOIST_PATTERN_DIFF` rather than +/// silently leaving the old hoist symlinks in place. +/// +/// Mirrors upstream's +/// [`hoist.ts:209` "hoistPattern=* throws exception when executed on node_modules installed w/o the option"](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/hoist.ts#L209) +/// — that test exercises the same drift in the opposite direction +/// (install without hoist, then install with `*`); the error +/// shape is symmetric. +#[test] +fn re_install_with_changed_hoist_pattern_errors() { + let CommandTempCwd { pacquet: pacquet_first, pnpm, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest( + &workspace, + serde_json::json!({ "@pnpm.e2e/hello-world-js-bin-parent": "1.0.0" }), + ); + generate_lockfile(pnpm); + + // First install — default hoist pattern (`['*']`). + pacquet_first.with_args(["install", "--frozen-lockfile"]).assert().success(); + assert!( + workspace.join("node_modules/.modules.yaml").exists(), + "first install must write .modules.yaml", + ); + + // Re-install with a different hoist pattern via yaml override. + write_workspace_yaml(&workspace, "hoistPattern:\n - 'only-this-thing'\n"); + let pacquet_second = std::process::Command::cargo_bin("pacquet") + .expect("find the pacquet binary") + .with_current_dir(&workspace); + let output = + pacquet_second.with_args(["install", "--frozen-lockfile"]).output().expect("run pacquet"); + assert!( + !output.status.success(), + "re-install with changed hoistPattern must fail; stderr=\n{}", + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("hoist-pattern") || stderr.contains("hoist_pattern"), + "error message must mention hoist-pattern; got:\n{stderr}", + ); + + drop((root, mock_instance)); +} + +/// First install with `publicHoistPattern: ['*']`, second install +/// with `publicHoistPattern: []`. The second install must error +/// with `PUBLIC_HOIST_PATTERN_DIFF`. +#[test] +fn re_install_with_changed_public_hoist_pattern_errors() { + let CommandTempCwd { pacquet: pacquet_first, pnpm, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest( + &workspace, + serde_json::json!({ "@pnpm.e2e/hello-world-js-bin-parent": "1.0.0" }), + ); + generate_lockfile(pnpm); + write_workspace_yaml(&workspace, "publicHoistPattern:\n - '*'\nhoistPattern: []\n"); + + pacquet_first.with_args(["install", "--frozen-lockfile"]).assert().success(); + + write_workspace_yaml(&workspace, "publicHoistPattern: []\nhoistPattern: []\n"); + let pacquet_second = std::process::Command::cargo_bin("pacquet") + .expect("find the pacquet binary") + .with_current_dir(&workspace); + let output = + pacquet_second.with_args(["install", "--frozen-lockfile"]).output().expect("run pacquet"); + assert!( + !output.status.success(), + "re-install with changed publicHoistPattern must fail; stderr=\n{}", + String::from_utf8_lossy(&output.stderr), + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("public-hoist-pattern") || stderr.contains("public_hoist_pattern"), + "error must mention public-hoist-pattern; got:\n{stderr}", + ); + + drop((root, mock_instance)); +} + +/// Re-installing with the same effective layout (no yaml change) +/// must NOT error — only drift triggers the validation. Guards +/// against the validator firing on every re-install when nothing +/// changed. +#[test] +fn re_install_with_no_change_succeeds() { + let CommandTempCwd { pacquet: pacquet_first, pnpm, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest( + &workspace, + serde_json::json!({ "@pnpm.e2e/hello-world-js-bin-parent": "1.0.0" }), + ); + generate_lockfile(pnpm); + + pacquet_first.with_args(["install", "--frozen-lockfile"]).assert().success(); + + let pacquet_second = std::process::Command::cargo_bin("pacquet") + .expect("find the pacquet binary") + .with_current_dir(&workspace); + pacquet_second.with_args(["install", "--frozen-lockfile"]).assert().success(); + + drop((root, mock_instance)); +} diff --git a/crates/package-manager/src/install.rs b/crates/package-manager/src/install.rs index bd206498d..4218ce57f 100644 --- a/crates/package-manager/src/install.rs +++ b/crates/package-manager/src/install.rs @@ -2,7 +2,7 @@ use std::{collections::BTreeMap, sync::atomic::AtomicU8, time::SystemTime}; use crate::{ HoistedDependencies, InstallFrozenLockfile, InstallFrozenLockfileError, InstallWithoutLockfile, - InstallWithoutLockfileError, ResolvedPackages, + InstallWithoutLockfileError, ResolvedPackages, ValidateModulesError, validate_modules, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -12,7 +12,8 @@ use pacquet_lockfile::{ }; use pacquet_modules_yaml::{ DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, IncludedDependencies, LayoutVersion, Modules, - NodeLinker as ModulesNodeLinker, RealApi, WriteModulesError, write_modules_manifest, + NodeLinker as ModulesNodeLinker, ReadModulesError, RealApi, WriteModulesError, + read_modules_manifest, write_modules_manifest, }; use pacquet_network::ThrottledClient; use pacquet_package_manifest::{DependencyGroup, PackageManifest}; @@ -122,6 +123,27 @@ pub enum InstallError { #[diagnostic(transparent)] FindWorkspaceDir(#[error(source)] pacquet_workspace::FindWorkspaceDirError), + + /// `node_modules/.modules.yaml` exists but couldn't be read or + /// parsed. A corrupt manifest is a hard error rather than a + /// silent skip — re-running the install over a corrupt + /// `.modules.yaml` would silently overwrite it without + /// validating the prior layout. + #[diagnostic(transparent)] + ReadModulesManifest(#[error(source)] ReadModulesError), + + /// The recorded `.modules.yaml` doesn't match the current + /// install's effective options on at least one + /// layout-driving axis (`hoistPattern`, `publicHoistPattern`, + /// `included`, `storeDir`, `virtualStoreDir`, + /// `virtualStoreDirMaxLength`). Mirrors upstream pnpm's + /// `validateModules` + `checkCompatibility` errors. Users + /// resolve this by either restoring the previous setting or + /// running `pacquet install --force` (the purge path tracked + /// in #464 §B is not yet implemented; today the user wipes + /// `node_modules/` manually). + #[diagnostic(transparent)] + ValidateModules(#[error(source)] ValidateModulesError), } impl<'a, DependencyGroupList> Install<'a, DependencyGroupList> @@ -207,6 +229,24 @@ where Lockfile::load_current_from_virtual_store_dir(&config.virtual_store_dir) .map_err(InstallError::LoadCurrentLockfile)?; + // Validate the on-disk `.modules.yaml` (if any) against the + // current install's effective options. Mirrors upstream's + // [`validateModules`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts) + // composed with [`checkCompatibility`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/checkCompatibility/index.ts). + // A missing manifest (`Ok(None)`) is the first-install case + // — no validation. A drift on any layout-affecting axis + // (hoist patterns, `included`, store paths) errors out so + // a re-install with new yaml settings doesn't silently leave + // the prior layout behind. The `--force` purge path is the + // §B follow-up tracked under #464; today the user wipes + // `node_modules/` manually after seeing the error. + if let Some(modules) = read_modules_manifest::(&config.modules_dir) + .map_err(InstallError::ReadModulesManifest)? + { + validate_modules(&modules, config, included, &workspace_root, &config.modules_dir) + .map_err(InstallError::ValidateModules)?; + } + // `pnpm:context` carries the directories pnpm's reporter prints // in the install header. `currentLockfileExists` mirrors // upstream's : diff --git a/crates/package-manager/src/lib.rs b/crates/package-manager/src/lib.rs index 71630e20f..bf66b2a17 100644 --- a/crates/package-manager/src/lib.rs +++ b/crates/package-manager/src/lib.rs @@ -21,6 +21,7 @@ mod retry_config; mod store_init; mod symlink_direct_dependencies; mod symlink_package; +mod validate_modules; mod version_policy; mod virtual_store_layout; @@ -45,5 +46,6 @@ pub use link_bins::*; pub use link_file::*; pub use symlink_direct_dependencies::*; pub use symlink_package::*; +pub use validate_modules::*; pub use version_policy::*; pub use virtual_store_layout::*; diff --git a/crates/package-manager/src/validate_modules.rs b/crates/package-manager/src/validate_modules.rs new file mode 100644 index 000000000..e3b28b21b --- /dev/null +++ b/crates/package-manager/src/validate_modules.rs @@ -0,0 +1,333 @@ +//! Validate the on-disk `node_modules/.modules.yaml` against the +//! current install's effective config. +//! +//! Mirrors upstream pnpm's +//! [`validateModules`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts) +//! composed with +//! [`checkCompatibility`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/checkCompatibility/index.ts). +//! +//! Each install reads the previous run's `.modules.yaml` (when +//! present) and compares the recorded layout-driving fields against +//! the current install options. A mismatch means the layout on disk +//! was produced under different settings — re-running the install +//! with the new settings would silently leave artifacts of the old +//! shape behind. Pacquet errors out instead of silently doing the +//! wrong thing; users opt into the rewrite via `--force` (the purge +//! path tracked in #464 §B is a follow-up). +//! +//! Today this module covers section §A of #464 — the **read-and-error** +//! pipeline. The `forceNewModules` purge path (§B) and the +//! `virtualStoreOnly` exemption (§C) are deferred until the +//! corresponding install modes ship. +//! +//! ## Axes covered +//! +//! - **`hoist_pattern`** — `HOIST_PATTERN_DIFF`. Most user-facing +//! axis: changing `pnpm-workspace.yaml`'s `hoistPattern` between +//! installs would otherwise leave stale private-hoist symlinks in +//! `/node_modules/`. +//! - **`public_hoist_pattern`** — `PUBLIC_HOIST_PATTERN_DIFF`. Same +//! as above for public hoist into `/node_modules/`. +//! - **`included` (dependency groups)** — `INCLUDED_DEPS_CONFLICT`. +//! `pacquet install --prod` followed by `pacquet install` would +//! silently merge prod-only and dev layouts otherwise. +//! - **`virtual_store_dir_max_length`** — `VIRTUAL_STORE_DIR_MAX_LENGTH_DIFF`. +//! Changing the value rewrites every slot's directory name; old +//! slots become orphans without a re-import. +//! - **`store_dir`** / **`virtual_store_dir`** — `UNEXPECTED_STORE` / +//! `UNEXPECTED_VIRTUAL_STORE_DIR`. The `//node_modules` +//! symlinks point at the recorded `store_dir`; changing it +//! between installs strands those symlinks. +//! - **`layout_version`** — handled at deserialize-time by the +//! `LayoutVersion` newtype's `try_from` impl, so a mismatched +//! on-disk value surfaces as a `ReadModulesError` before this +//! validator runs. Listed here so the issue's coverage table is +//! complete. + +use derive_more::{Display, Error}; +use miette::Diagnostic; +use pacquet_config::Config; +use pacquet_modules_yaml::{IncludedDependencies, Modules}; +use std::path::{Path, PathBuf}; + +/// Error returned by [`validate_modules`]. +/// +/// One variant per axis. Codes match upstream's `ERR_PNPM_*` / +/// kebab-case symbols where possible — the user-facing strings come +/// straight from +/// [`validateModules.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts) +/// and +/// [`checkCompatibility/index.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/checkCompatibility/index.ts) +/// so a user who sees the message in pnpm and pacquet gets the same +/// wording. +#[derive(Debug, Display, Error, Diagnostic)] +#[non_exhaustive] +pub enum ValidateModulesError { + /// Recorded `hoistPattern` differs from the current install + /// options. Mirrors upstream's `HOIST_PATTERN_DIFF` throw at + /// [`validateModules.ts:88-92`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts#L88-L92). + /// `--force` would purge and rebuild (§B follow-up); today + /// pacquet errors so the user opts in. + #[display( + "This modules directory was created using a different hoist-pattern value. Run \"pnpm install\" to recreate the modules directory." + )] + #[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`." + ) + )] + HoistPatternDiff, + + /// Recorded `publicHoistPattern` differs. Mirrors upstream's + /// `PUBLIC_HOIST_PATTERN_DIFF` at + /// [`validateModules.ts:74-79`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts#L74-L79). + #[display( + "This modules directory was created using a different public-hoist-pattern value. Run \"pnpm install\" to recreate the modules directory." + )] + #[diagnostic( + code(pacquet_package_manager::public_hoist_pattern_diff), + help( + "Either run `pacquet install --force` to recreate `node_modules/`, or restore the previous `publicHoistPattern` in `pnpm-workspace.yaml`." + ) + )] + PublicHoistPatternDiff, + + /// Recorded `included` dependency groups differ. Mirrors + /// upstream's `INCLUDED_DEPS_CONFLICT` at + /// [`validateModules.ts:108-113`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts#L108-L113). + /// The `lockfile_dir` in the message is upstream's wording for + /// the install root. + #[display( + "modules directory (at \"{}\") was installed with {recorded}. Current install wants {requested}.", + lockfile_dir.display() + )] + #[diagnostic( + code(pacquet_package_manager::included_deps_conflict), + help( + "Run `pacquet install --force` to recreate `node_modules/`, or rerun the install with the same dependency groups (`--prod` / default / `--dev`) as before." + ) + )] + IncludedDepsConflict { lockfile_dir: PathBuf, recorded: String, requested: String }, + + /// Recorded `virtualStoreDirMaxLength` differs. Mirrors upstream's + /// `VIRTUAL_STORE_DIR_MAX_LENGTH_DIFF` at + /// [`validateModules.ts:55-65`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts#L55-L65). + /// Pacquet uses upstream's default (120) so this is mostly + /// future-proofing — but a user who pins a different value in + /// yaml between two installs would otherwise silently get + /// orphaned `.pnpm/` slots from the old run. + #[display( + "This modules directory was created using a different virtual-store-dir-max-length value. Run \"pnpm install\" to recreate the modules directory." + )] + #[diagnostic( + code(pacquet_package_manager::virtual_store_dir_max_length_diff), + help( + "Either run `pacquet install --force` or restore the previous `virtualStoreDirMaxLength` in `pnpm-workspace.yaml`." + ) + )] + VirtualStoreDirMaxLengthDiff, + + /// Recorded `storeDir` differs from the current store root. + /// Mirrors upstream's `UnexpectedStoreError` at + /// [`checkCompatibility/index.ts:25-39`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/checkCompatibility/index.ts#L25-L39). + /// The store is referenced by every `//node_modules` + /// symlink — relocating it strands those. + #[display( + "Unexpected store location. The modules directory at \"{}\" was created with a different store: \"{recorded}\" (current install uses \"{requested}\").", + modules_dir.display() + )] + #[diagnostic( + code(pacquet_package_manager::unexpected_store), + help( + "Either run `pacquet install --force` to recreate `node_modules/` against the new store, or set `storeDir` back to the recorded value." + ) + )] + UnexpectedStore { modules_dir: PathBuf, recorded: String, requested: String }, + + /// Recorded `virtualStoreDir` differs. Mirrors upstream's + /// `UnexpectedVirtualStoreDirError` at + /// [`checkCompatibility/index.ts:41-47`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/checkCompatibility/index.ts#L41-L47). + /// Less common than `storeDir` drift; usually only triggers + /// when the user adds an explicit `virtualStoreDir` to yaml + /// that didn't match what pacquet was using as the implicit + /// `/.pnpm` default. + #[display( + "Unexpected virtual store location. The modules directory at \"{}\" was created at virtual store \"{recorded}\" (current install uses \"{requested}\").", + modules_dir.display() + )] + #[diagnostic( + code(pacquet_package_manager::unexpected_virtual_store_dir), + help( + "Either run `pacquet install --force` to recreate `node_modules/`, or set `virtualStoreDir` back to the recorded value." + ) + )] + UnexpectedVirtualStoreDir { modules_dir: PathBuf, recorded: String, requested: String }, +} + +/// Compare the on-disk [`Modules`] manifest against the current +/// install's effective options. `Ok(())` means the layout on disk is +/// compatible with the current install; any other return is a typed +/// drift the caller surfaces to the user. +/// +/// Drift axes are checked in upstream's order +/// ([`validateModules.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts) +/// then per-importer +/// [`checkCompatibility`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/checkCompatibility/index.ts)), +/// so the first error a user sees matches the first error pnpm +/// would surface for the same drift. +/// +/// `lockfile_dir` is the install root (the directory containing +/// `pnpm-lock.yaml` / `pnpm-workspace.yaml`) — used in the +/// `INCLUDED_DEPS_CONFLICT` message and as the per-importer scope +/// for upstream's per-importer loop. Pacquet's first slice runs +/// the per-importer check once at the install root; widening to +/// every workspace project tracks alongside per-importer +/// `included` overrides. +/// +/// **Out of scope for this slice (§B / §C of #464):** +/// +/// - The `--force` purge path. When `force` is passed today, the +/// caller still sees a typed drift and decides; later work plumbs +/// `force` into this function and either purges + returns Ok or +/// propagates the drift error. +/// - The `virtualStoreOnly` exemption. Pacquet doesn't implement +/// `virtualStoreOnly` install yet, but the field is on `Modules` +/// — guard against future activation without changing this path. +pub fn validate_modules( + modules: &Modules, + config: &Config, + requested_included: IncludedDependencies, + lockfile_dir: &Path, + modules_dir: &Path, +) -> Result<(), ValidateModulesError> { + // 1. `virtualStoreDirMaxLength` — upstream checks this first + // because it implies every slot path on disk has a different + // name from what the current install would compute. Pacquet + // pins the default 120 today, but a yaml override would + // surface here. + if modules.virtual_store_dir_max_length + != pacquet_modules_yaml::DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH + { + return Err(ValidateModulesError::VirtualStoreDirMaxLengthDiff); + } + + // 2. `publicHoistPattern` — upstream order: public before + // private (`validateModules.ts:67-92`). The recorded value is + // `Option>`; pacquet's `Config.public_hoist_pattern` + // matches the same shape. `None`-vs-`Some([])` is the same + // "no public hoist" semantic and treated as equal. + if !patterns_equal( + modules.public_hoist_pattern.as_deref(), + config.public_hoist_pattern.as_deref(), + ) { + return Err(ValidateModulesError::PublicHoistPatternDiff); + } + + // 3. `hoistPattern` — same shape as the public side. Upstream + // runs this inside a try/catch so the error can be converted + // to a per-importer purge under `forceNewModules`; pacquet's + // first slice just bubbles the error up to the install + // pipeline. + if !patterns_equal(modules.hoist_pattern.as_deref(), config.hoist_pattern.as_deref()) { + return Err(ValidateModulesError::HoistPatternDiff); + } + + // 4. `checkCompatibility` axes (per-importer in upstream; + // pacquet's first slice runs at the install root). The + // `layoutVersion` axis is enforced by `LayoutVersion::try_from` + // at deserialize time — a mismatched on-disk value surfaces + // as `ReadModulesError::ParseYaml` before this validator + // even runs, so there's no axis to add here for it. + + // 4a. `storeDir` — `path.relative(a, b) === ''` upstream; + // pacquet collapses to lex equality after canonicalising + // via `Path::new` + `==`. Both fields are absolute strings + // on disk so trailing-slash drift isn't a concern. + let recorded_store_dir = modules.store_dir.as_str(); + let requested_store_dir = config.store_dir.display().to_string(); + if !paths_equal(recorded_store_dir, &requested_store_dir) { + return Err(ValidateModulesError::UnexpectedStore { + modules_dir: modules_dir.to_path_buf(), + recorded: recorded_store_dir.to_string(), + requested: requested_store_dir, + }); + } + + // 4b. `virtualStoreDir` — same path-equal check. The recorded + // value is the post-`resolve_virtual_store_dir` absolute + // path (modules-yaml does that resolution at load time), + // so comparing against `config.virtual_store_dir` directly + // works. + let recorded_vs_dir = modules.virtual_store_dir.as_str(); + let requested_vs_dir = config.virtual_store_dir.to_string_lossy(); + if !paths_equal(recorded_vs_dir, &requested_vs_dir) { + return Err(ValidateModulesError::UnexpectedVirtualStoreDir { + modules_dir: modules_dir.to_path_buf(), + recorded: recorded_vs_dir.to_string(), + requested: requested_vs_dir.into_owned(), + }); + } + + // 5. `included` — pacquet's first slice runs at the root since + // upstream's per-importer loop primarily exists for + // workspace projects with their own dependency-group + // overrides (which pacquet doesn't surface yet). The + // install pipeline writes the same `IncludedDependencies` + // for every importer. + if modules.included != requested_included { + return Err(ValidateModulesError::IncludedDepsConflict { + lockfile_dir: lockfile_dir.to_path_buf(), + recorded: stringify_included(modules.included), + requested: stringify_included(requested_included), + }); + } + + Ok(()) +} + +/// Compare two `Option<&[String]>` pattern lists. `None` and +/// `Some([])` both mean "no patterns" and compare equal. Mirrors +/// upstream's `equals(modules.publicHoistPattern ?? [], opts.publicHoistPattern ?? [])` +/// (deepEquals on the unwrapped arrays). +fn patterns_equal(a: Option<&[String]>, b: Option<&[String]>) -> bool { + match (a, b) { + (None, None) => true, + (Some(x), Some(y)) => x == y, + (Some(x), None) | (None, Some(x)) => x.is_empty(), + } +} + +/// Compare two filesystem paths for equality, mirroring upstream's +/// `path.relative(a, b) === ''` check. +/// +/// `path.relative` returns `''` only when the two paths resolve to +/// the same point regardless of how they're spelled. For pacquet, +/// both inputs come from already-absolutized strings the +/// modules-yaml loader produced, so `Path::new(a) == Path::new(b)` +/// is a faithful equivalent — Rust's `Path::eq` is component-wise +/// (one `/foo` is one `/foo`, not `/foo/` or `/foo/.`). +fn paths_equal(a: &str, b: &str) -> bool { + Path::new(a) == Path::new(b) +} + +/// Render an [`IncludedDependencies`] as the comma-separated label +/// upstream uses in the error message. Order matches upstream's +/// `DEPENDENCIES_FIELDS` constant: `dependencies, devDependencies, +/// optionalDependencies`. +fn stringify_included(included: IncludedDependencies) -> String { + let mut parts = Vec::with_capacity(3); + if included.dependencies { + parts.push("dependencies"); + } + if included.dev_dependencies { + parts.push("devDependencies"); + } + if included.optional_dependencies { + parts.push("optionalDependencies"); + } + if parts.is_empty() { "no dependency groups".to_string() } else { parts.join(", ") } +} + +#[cfg(test)] +mod tests; diff --git a/crates/package-manager/src/validate_modules/tests.rs b/crates/package-manager/src/validate_modules/tests.rs new file mode 100644 index 000000000..265813b13 --- /dev/null +++ b/crates/package-manager/src/validate_modules/tests.rs @@ -0,0 +1,251 @@ +//! Unit tests for [`super::validate_modules`]. +//! +//! Each test exercises one drift axis in isolation. The strategy is +//! to seed `Modules` with a known shape, build a [`Config`] that +//! matches it, then mutate one field on the config (or the modules +//! manifest) and assert the matching error variant. + +use super::{ValidateModulesError, validate_modules}; +use pacquet_config::Config; +use pacquet_modules_yaml::{ + DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, IncludedDependencies, LayoutVersion, Modules, + NodeLinker as ModulesNodeLinker, +}; +use pacquet_store_dir::StoreDir; +use pretty_assertions::assert_eq; +use std::{collections::BTreeMap, path::Path}; + +/// Seed a `Modules` value that matches a fresh `Config::new()` — +/// the round-trip baseline for every drift test. +fn baseline_modules(config: &Config) -> Modules { + Modules { + hoist_pattern: config.hoist_pattern.clone(), + public_hoist_pattern: config.public_hoist_pattern.clone(), + included: IncludedDependencies { + dependencies: true, + dev_dependencies: true, + optional_dependencies: true, + }, + layout_version: Some(LayoutVersion), + node_linker: Some(ModulesNodeLinker::Isolated), + store_dir: config.store_dir.display().to_string(), + virtual_store_dir: config.virtual_store_dir.to_string_lossy().into_owned(), + virtual_store_dir_max_length: DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH, + registries: Some(BTreeMap::from([("default".to_string(), config.registry.clone())])), + ..Default::default() + } +} + +fn requested_full_groups() -> IncludedDependencies { + IncludedDependencies { dependencies: true, dev_dependencies: true, optional_dependencies: true } +} + +/// Sanity: a `Modules` written by the same `Config` validates clean. +/// Catches the case where some axis check spuriously fires on a +/// matching baseline (regression risk if `patterns_equal` ever +/// over-tightens, etc.). +#[test] +fn baseline_round_trip_validates_clean() { + let config = Config::new(); + let modules = baseline_modules(&config); + validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect("baseline must validate clean"); +} + +/// Recorded `hoistPattern` differs from the current install's +/// `Config.hoist_pattern`. Typical user case: yaml flipped from +/// `['*']` to `['only-foo']` between two installs. +#[test] +fn hoist_pattern_diff_returns_error() { + let config = Config::new(); + let mut modules = baseline_modules(&config); + modules.hoist_pattern = Some(vec!["only-foo".to_string()]); + let err = validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("differing hoist_pattern must error"); + assert!(matches!(err, ValidateModulesError::HoistPatternDiff), "got {err:?}"); +} + +/// `publicHoistPattern` drift. Same shape as the private side. +#[test] +fn public_hoist_pattern_diff_returns_error() { + let config = Config::new(); + let mut modules = baseline_modules(&config); + modules.public_hoist_pattern = Some(vec!["different".to_string()]); + let err = validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("differing public_hoist_pattern must error"); + assert!(matches!(err, ValidateModulesError::PublicHoistPatternDiff), "got {err:?}"); +} + +/// `None` and `Some([])` mean the same thing ("no patterns") and +/// must compare equal — mirrors upstream's +/// `equals(modules.publicHoistPattern ?? [], opts.publicHoistPattern ?? [])` +/// where the `?? []` makes both nullish values look like empty +/// arrays. +#[test] +fn none_vs_some_empty_pattern_treated_as_equal() { + let mut config = Config::new(); + config.hoist_pattern = None; + config.public_hoist_pattern = None; + let mut modules = baseline_modules(&config); + modules.hoist_pattern = Some(Vec::new()); + modules.public_hoist_pattern = Some(Vec::new()); + validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/anywhere"), + Path::new("/anywhere/node_modules"), + ) + .expect("None and Some([]) compare equal"); +} + +/// `included` drift: install with all groups, then re-install with +/// `--prod` (`dev_dependencies: false`). +#[test] +fn included_deps_conflict_returns_error_with_payload() { + let config = Config::new(); + let modules = baseline_modules(&config); + let prod_only = IncludedDependencies { + dependencies: true, + dev_dependencies: false, + optional_dependencies: false, + }; + let err = validate_modules( + &modules, + &config, + prod_only, + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("differing included must error"); + let ValidateModulesError::IncludedDepsConflict { lockfile_dir, recorded, requested } = err + else { + panic!("expected IncludedDepsConflict, got {err:?}"); + }; + assert_eq!(lockfile_dir, Path::new("/lockfile-dir")); + // Recorded baseline includes all three groups; requested is prod-only. + assert_eq!(recorded, "dependencies, devDependencies, optionalDependencies"); + assert_eq!(requested, "dependencies"); +} + +/// `storeDir` drift. The recorded path on disk is absolutized +/// strings (modules-yaml resolves them on load), so a string-level +/// equality is enough. +#[test] +fn store_dir_drift_returns_unexpected_store() { + let config = Config::new(); + let mut modules = baseline_modules(&config); + modules.store_dir = "/some-other-store".to_string(); + let err = validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("store_dir drift must error"); + let ValidateModulesError::UnexpectedStore { recorded, requested, .. } = err else { + panic!("expected UnexpectedStore, got {err:?}"); + }; + assert_eq!(recorded, "/some-other-store"); + assert_eq!(requested, config.store_dir.display().to_string()); +} + +/// `virtualStoreDir` drift. Less common in practice (most users +/// don't pin `virtualStoreDir` explicitly), but still a real drift. +#[test] +fn virtual_store_dir_drift_returns_unexpected_virtual_store_dir() { + let config = Config::new(); + let mut modules = baseline_modules(&config); + modules.virtual_store_dir = "/some-other-vs".to_string(); + let err = validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("virtual_store_dir drift must error"); + assert!(matches!(err, ValidateModulesError::UnexpectedVirtualStoreDir { .. }), "got {err:?}"); +} + +/// `virtualStoreDirMaxLength` drift. Pacquet pins the default 120 +/// today, but a yaml override would cause this. +#[test] +fn virtual_store_dir_max_length_drift_returns_error() { + let config = Config::new(); + let mut modules = baseline_modules(&config); + modules.virtual_store_dir_max_length = 200; + let err = validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("virtual_store_dir_max_length drift must error"); + assert!(matches!(err, ValidateModulesError::VirtualStoreDirMaxLengthDiff), "got {err:?}"); +} + +/// Path-equality is component-wise — a recorded path with a trailing +/// slash compares equal to one without (Rust's `Path::eq` ignores +/// the trailing separator). +#[test] +fn store_dir_path_equality_ignores_trailing_slash() { + let mut config = Config::new(); + config.store_dir = StoreDir::from(Path::new("/store").to_path_buf()); + let mut modules = baseline_modules(&config); + modules.store_dir = "/store/".to_string(); + validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect("trailing-slash diff must compare equal via Path::eq"); +} + +/// Upstream order: `virtualStoreDirMaxLength` is checked first, then +/// `publicHoistPattern`, then `hoistPattern`, then `checkCompatibility`, +/// then `included`. When multiple axes drift, the first one in +/// upstream's order surfaces. This test has both +/// `virtualStoreDirMaxLength` and `hoistPattern` drift — the +/// max-length error must win. +#[test] +fn first_drift_in_upstream_order_wins() { + let config = Config::new(); + let mut modules = baseline_modules(&config); + modules.virtual_store_dir_max_length = 200; + modules.hoist_pattern = Some(vec!["only-foo".to_string()]); + let err = validate_modules( + &modules, + &config, + requested_full_groups(), + Path::new("/lockfile-dir"), + Path::new("/lockfile-dir/node_modules"), + ) + .expect_err("must error"); + assert!( + matches!(err, ValidateModulesError::VirtualStoreDirMaxLengthDiff), + "max-length must surface first, got {err:?}", + ); +} diff --git a/plans/TEST_PORTING.md b/plans/TEST_PORTING.md index ef789fa85..d86e0e913 100644 --- a/plans/TEST_PORTING.md +++ b/plans/TEST_PORTING.md @@ -107,8 +107,8 @@ Primary tests: - [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:148` `should rehoist when uninstalling a package`. Stubbed (#433). - [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:169` `should rehoist after running a general install`. Stubbed (#433). - [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:201` `should not override aliased dependencies`. Stubbed (#433 + alias-aware install plumbing). -- [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:209` `hoistPattern=* throws exception when executed on node_modules installed w/o the option`. Stubbed (#433 — pattern-change detection across `.modules.yaml` reads). -- [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:220` `hoistPattern=undefined throws exception when executed on node_modules installed with hoist-pattern=*`. Stubbed (#433). +- [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:209` `hoistPattern=* throws exception when executed on node_modules installed w/o the option`. Symmetric case ported as `re_install_with_changed_hoist_pattern_errors` in `crates/cli/tests/validate_modules.rs` (same `HOIST_PATTERN_DIFF` axis, opposite direction). Implemented in #464 §A. +- [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:220` `hoistPattern=undefined throws exception when executed on node_modules installed with hoist-pattern=*`. Same axis as the above — covered indirectly by the same integration test. - [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:233` `hoist by alias`. Stubbed in `known_failures::hoist_by_alias` — algo is correct (unit-tested) but end-to-end exercises alias plumbing not all wired. - [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:249` `should remove aliased hoisted dependencies`. Stubbed (#433). - [x] `TypeScript repo: installing/deps-installer/test/install/hoist.ts:272` `should update .modules.yaml when pruning if we are flattening`. Stubbed (#433). From 2f9873c2db6a2b75551c57ebcc89150390e828bd Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 13 May 2026 18:43:55 +0200 Subject: [PATCH 2/2] fix(install): gate validation on frozen-lockfile, drop --force from help text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/package-manager/src/install.rs | 39 ++++++++++++++----- .../package-manager/src/validate_modules.rs | 36 ++++++++++------- 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/crates/package-manager/src/install.rs b/crates/package-manager/src/install.rs index 4218ce57f..e7647848b 100644 --- a/crates/package-manager/src/install.rs +++ b/crates/package-manager/src/install.rs @@ -137,11 +137,13 @@ pub enum InstallError { /// layout-driving axis (`hoistPattern`, `publicHoistPattern`, /// `included`, `storeDir`, `virtualStoreDir`, /// `virtualStoreDirMaxLength`). Mirrors upstream pnpm's - /// `validateModules` + `checkCompatibility` errors. Users - /// resolve this by either restoring the previous setting or - /// running `pacquet install --force` (the purge path tracked - /// in #464 §B is not yet implemented; today the user wipes - /// `node_modules/` manually). + /// `validateModules` + `checkCompatibility` errors. Today the + /// user resolves this by either restoring the previous yaml + /// setting or removing `node_modules/` and re-running + /// `pacquet install --frozen-lockfile`. The automatic purge + /// path (upstream's `--force` / `forceNewModules`) is tracked + /// under #464 §B; pacquet doesn't expose a `--force` install + /// flag yet. #[diagnostic(transparent)] ValidateModules(#[error(source)] ValidateModulesError), } @@ -237,11 +239,28 @@ where // — no validation. A drift on any layout-affecting axis // (hoist patterns, `included`, store paths) errors out so // a re-install with new yaml settings doesn't silently leave - // the prior layout behind. The `--force` purge path is the - // §B follow-up tracked under #464; today the user wipes - // `node_modules/` manually after seeing the error. - if let Some(modules) = read_modules_manifest::(&config.modules_dir) - .map_err(InstallError::ReadModulesManifest)? + // the prior layout behind. The recovery-by-purge path + // (upstream's `forceNewModules`) is tracked under #464 §B; + // today the user wipes `node_modules/` and reinstalls. + // + // Gated on `frozen_lockfile` because: + // + // 1. Hoisting (the most user-facing drift axis) is itself + // frozen-lockfile-only in pacquet today + // ([`InstallWithoutLockfile::run`](crate::InstallWithoutLockfile) + // returns an empty `HoistedDependencies` map by design), + // so the without-lockfile path can't drift on + // `hoist_pattern` / `public_hoist_pattern` even when + // `pnpm-workspace.yaml` flips between runs. + // 2. The without-lockfile path is a transitional shape + // pacquet keeps for users who haven't generated a + // lockfile yet; piling validation errors onto it would + // surface drift that's irrelevant for that mode. + // 3. Matches the PR scope (#464 §A is explicitly the + // frozen-lockfile read-and-error path). + if frozen_lockfile + && let Some(modules) = read_modules_manifest::(&config.modules_dir) + .map_err(InstallError::ReadModulesManifest)? { validate_modules(&modules, config, included, &workspace_root, &config.modules_dir) .map_err(InstallError::ValidateModules)?; diff --git a/crates/package-manager/src/validate_modules.rs b/crates/package-manager/src/validate_modules.rs index e3b28b21b..a26b2cfba 100644 --- a/crates/package-manager/src/validate_modules.rs +++ b/crates/package-manager/src/validate_modules.rs @@ -12,8 +12,10 @@ //! was produced under different settings — re-running the install //! with the new settings would silently leave artifacts of the old //! shape behind. Pacquet errors out instead of silently doing the -//! wrong thing; users opt into the rewrite via `--force` (the purge -//! path tracked in #464 §B is a follow-up). +//! wrong thing; today the user recovers by removing `node_modules/` +//! and re-running the install. The automatic purge path (upstream's +//! `forceNewModules` — `pnpm install --force`) is tracked under +//! #464 §B; pacquet doesn't expose a `--force` install flag yet. //! //! Today this module covers section §A of #464 — the **read-and-error** //! pipeline. The `forceNewModules` purge path (§B) and the @@ -66,15 +68,17 @@ pub enum ValidateModulesError { /// Recorded `hoistPattern` differs from the current install /// options. Mirrors upstream's `HOIST_PATTERN_DIFF` throw at /// [`validateModules.ts:88-92`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts#L88-L92). - /// `--force` would purge and rebuild (§B follow-up); today - /// pacquet errors so the user opts in. + /// Upstream's `--force` would purge and rebuild; pacquet's + /// `--force` install flag is tracked under #464 §B and isn't + /// exposed yet, so the user-facing help points at the manual + /// recovery path. #[display( "This modules directory was created using a different hoist-pattern value. Run \"pnpm install\" to recreate the modules directory." )] #[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`." + "Restore the previous `hoistPattern` in `pnpm-workspace.yaml`, or remove `node_modules/` and re-run `pacquet install --frozen-lockfile`." ) )] HoistPatternDiff, @@ -88,7 +92,7 @@ pub enum ValidateModulesError { #[diagnostic( code(pacquet_package_manager::public_hoist_pattern_diff), help( - "Either run `pacquet install --force` to recreate `node_modules/`, or restore the previous `publicHoistPattern` in `pnpm-workspace.yaml`." + "Restore the previous `publicHoistPattern` in `pnpm-workspace.yaml`, or remove `node_modules/` and re-run `pacquet install --frozen-lockfile`." ) )] PublicHoistPatternDiff, @@ -105,7 +109,7 @@ pub enum ValidateModulesError { #[diagnostic( code(pacquet_package_manager::included_deps_conflict), help( - "Run `pacquet install --force` to recreate `node_modules/`, or rerun the install with the same dependency groups (`--prod` / default / `--dev`) as before." + "Re-run the install with the same dependency groups (`--prod` / default / `--dev`) as before, or remove `node_modules/` and re-run `pacquet install --frozen-lockfile`." ) )] IncludedDepsConflict { lockfile_dir: PathBuf, recorded: String, requested: String }, @@ -113,17 +117,21 @@ pub enum ValidateModulesError { /// Recorded `virtualStoreDirMaxLength` differs. Mirrors upstream's /// `VIRTUAL_STORE_DIR_MAX_LENGTH_DIFF` at /// [`validateModules.ts:55-65`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/validateModules.ts#L55-L65). - /// Pacquet uses upstream's default (120) so this is mostly - /// future-proofing — but a user who pins a different value in - /// yaml between two installs would otherwise silently get - /// orphaned `.pnpm/` slots from the old run. + /// Pacquet doesn't yet read `virtualStoreDirMaxLength` from + /// `pnpm-workspace.yaml` (the loader pins + /// [`pacquet_modules_yaml::DEFAULT_VIRTUAL_STORE_DIR_MAX_LENGTH`] + /// = 120 unconditionally), so this variant is reachable today + /// only when the on-disk `.modules.yaml` was written by a + /// pnpm install with a non-default value. Once pacquet honors + /// the yaml key, the help text below should switch to + /// "restore the previous value or remove and re-install". #[display( "This modules directory was created using a different virtual-store-dir-max-length value. Run \"pnpm install\" to recreate the modules directory." )] #[diagnostic( code(pacquet_package_manager::virtual_store_dir_max_length_diff), help( - "Either run `pacquet install --force` or restore the previous `virtualStoreDirMaxLength` in `pnpm-workspace.yaml`." + "Remove `node_modules/` and re-run `pacquet install --frozen-lockfile`. Pacquet doesn't yet read `virtualStoreDirMaxLength` from `pnpm-workspace.yaml`, so the recorded value can only be matched by re-creating the modules directory." ) )] VirtualStoreDirMaxLengthDiff, @@ -140,7 +148,7 @@ pub enum ValidateModulesError { #[diagnostic( code(pacquet_package_manager::unexpected_store), help( - "Either run `pacquet install --force` to recreate `node_modules/` against the new store, or set `storeDir` back to the recorded value." + "Set `storeDir` back to the recorded value, or remove `node_modules/` and re-run `pacquet install --frozen-lockfile` against the new store." ) )] UnexpectedStore { modules_dir: PathBuf, recorded: String, requested: String }, @@ -159,7 +167,7 @@ pub enum ValidateModulesError { #[diagnostic( code(pacquet_package_manager::unexpected_virtual_store_dir), help( - "Either run `pacquet install --force` to recreate `node_modules/`, or set `virtualStoreDir` back to the recorded value." + "Set `virtualStoreDir` back to the recorded value, or remove `node_modules/` and re-run `pacquet install --frozen-lockfile`." ) )] UnexpectedVirtualStoreDir { modules_dir: PathBuf, recorded: String, requested: String },