feat(install): validate .modules.yaml on read — error on layout drift (#464 §A)

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).

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));
+}

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,29 @@ 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. 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),
 }
 
 impl<'a, DependencyGroupList> Install<'a, DependencyGroupList>
@@ -207,6 +231,41 @@ 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 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::<RealApi>(&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 <https://github.com/pnpm/pnpm/blob/94240bc046/installing/context/src/index.ts#L196>:

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::*;