From 57fc52fe705d49de7ccc87ec50f4a91abf486236 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Thu, 16 Jul 2026 18:16:37 +0200 Subject: [PATCH] fix(pacquet): validate workspace lockfile importers Check every workspace project manifest against its matching lockfile importer before taking the frozen path. This makes normal installs refresh stale member importers and makes frozen installs reject them. Also restore the empty intent list in the versioning ledger so release-plan validation can parse the committed file. Closes pnpm/pnpm#13080. --- .changeset/ledger.yaml | 2 +- .../pacquet-workspace-lockfile-freshness.md | 5 ++ pnpm/crates/cli/tests/workspace_install.rs | 75 ++++++++++++++++++- pnpm/crates/lockfile/src/freshness.rs | 10 +-- pnpm/crates/lockfile/src/freshness/tests.rs | 36 ++++----- pnpm/crates/package-manager/src/install.rs | 46 +++++++----- pnpr/crates/pnpr/src/resolver/resolve.rs | 3 +- 7 files changed, 128 insertions(+), 49 deletions(-) create mode 100644 .changeset/pacquet-workspace-lockfile-freshness.md diff --git a/.changeset/ledger.yaml b/.changeset/ledger.yaml index 0504f52671..cb2d195b40 100644 --- a/.changeset/ledger.yaml +++ b/.changeset/ledger.yaml @@ -161,7 +161,7 @@ pacquet@12.0.0-alpha.12: - pacquet-symlink-dangling-junction-parent pacquet@12.0.0-alpha.13: dir: pnpm/npm/pnpm - intents: + intents: [] pacquet@12.0.0-alpha.9: dir: pnpm/npm/pnpm intents: diff --git a/.changeset/pacquet-workspace-lockfile-freshness.md b/.changeset/pacquet-workspace-lockfile-freshness.md new file mode 100644 index 0000000000..7d79e0a039 --- /dev/null +++ b/.changeset/pacquet-workspace-lockfile-freshness.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +Fixed installs to detect manifest changes in workspace members and reject stale lockfiles when using `--frozen-lockfile` [pnpm/pnpm#13080](https://github.com/pnpm/pnpm/issues/13080). diff --git a/pnpm/crates/cli/tests/workspace_install.rs b/pnpm/crates/cli/tests/workspace_install.rs index 5796091d9e..3603196393 100644 --- a/pnpm/crates/cli/tests/workspace_install.rs +++ b/pnpm/crates/cli/tests/workspace_install.rs @@ -19,7 +19,11 @@ use pacquet_testing_utils::{ bin::{AddMockedRegistry, CommandTempCwd}, fs::is_symlink_or_junction, }; -use std::fs; +use std::{fs, path::Path, process::Command}; + +fn pacquet_at(workspace: &Path) -> Command { + Command::cargo_bin("pnpm").expect("find the pnpm binary").with_current_dir(workspace) +} /// A workspace with two sibling projects, each pulling in a /// different mocked package, runs through the fresh-resolve path and @@ -141,6 +145,75 @@ fn fresh_resolve_walks_every_workspace_importer() { drop((root, mock_instance)); } +#[test] +fn changed_workspace_importer_invalidates_lockfile() { + let CommandTempCwd { root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + fs::write( + workspace.join("package.json"), + serde_json::json!({ "name": "root", "private": true }).to_string(), + ) + .expect("write root package.json"); + + let workspace_yaml_path = workspace.join("pnpm-workspace.yaml"); + let mut workspace_yaml = + fs::read_to_string(&workspace_yaml_path).expect("read pnpm-workspace.yaml"); + workspace_yaml.push_str("packages:\n - 'pkg-a'\n - 'pkg-b'\n"); + fs::write(&workspace_yaml_path, workspace_yaml).expect("write pnpm-workspace.yaml"); + + fs::create_dir(workspace.join("pkg-a")).expect("mkdir pkg-a"); + fs::write( + workspace.join("pkg-a/package.json"), + serde_json::json!({ "name": "pkg-a", "version": "1.0.0" }).to_string(), + ) + .expect("write pkg-a/package.json"); + fs::create_dir(workspace.join("pkg-b")).expect("mkdir pkg-b"); + fs::write( + workspace.join("pkg-b/package.json"), + serde_json::json!({ "name": "pkg-b", "version": "1.0.0" }).to_string(), + ) + .expect("write pkg-b/package.json"); + + pacquet_at(&workspace).with_arg("install").assert().success(); + + fs::write( + workspace.join("pkg-a/package.json"), + serde_json::json!({ + "name": "pkg-a", + "version": "1.0.0", + "dependencies": { "pkg-b": "workspace:*" }, + }) + .to_string(), + ) + .expect("update pkg-a/package.json"); + + let frozen_output = pacquet_at(&workspace) + .with_args(["install", "--frozen-lockfile"]) + .output() + .expect("run frozen install"); + let frozen_stderr = String::from_utf8_lossy(&frozen_output.stderr); + assert!( + !frozen_output.status.success(), + "frozen install accepted a stale workspace importer\nstderr:\n{frozen_stderr}", + ); + assert!( + frozen_stderr.contains("pacquet_package_manager::outdated_lockfile"), + "frozen install returned the wrong error\nstderr:\n{frozen_stderr}", + ); + + pacquet_at(&workspace).with_arg("install").assert().success(); + let linked_pkg = workspace.join("pkg-a/node_modules/pkg-b"); + assert!( + is_symlink_or_junction(&linked_pkg).expect("query pkg-b link"), + "normal install did not link the dependency added to pkg-a", + ); + assert!(linked_pkg.join("package.json").exists(), "pkg-b link is dangling"); + + drop((root, mock_instance)); +} + /// When the workspace root and a non-root importer both depend on the /// same workspace package via `workspace:*`, each importer's resolved /// `link:` target is relative to *its own* directory — pnpm writes diff --git a/pnpm/crates/lockfile/src/freshness.rs b/pnpm/crates/lockfile/src/freshness.rs index 361aab043f..3c36242f79 100644 --- a/pnpm/crates/lockfile/src/freshness.rs +++ b/pnpm/crates/lockfile/src/freshness.rs @@ -372,9 +372,6 @@ fn all_catalogs_are_up_to_date( /// when the lockfile is up-to-date; returns `Err(StalenessReason)` /// describing the first detected mismatch otherwise. /// -/// Single-importer only today (pacquet doesn't have workspace support -/// — see [#431]). Callers thread the root importer entry directly. -/// /// What is checked (in order, short-circuiting on the first failure): /// /// 1. Flat-record specifier diff against `devDependencies ∪ @@ -389,18 +386,13 @@ fn all_catalogs_are_up_to_date( /// Scoped to what pacquet supports today: no `auto-install-peers` /// pre-pass (pacquet has no separate auto-install-peers mode), no /// `excludeLinksFromLockfile` (`link:` resolutions aren't supported -/// yet — [#431] territory), and no version-range-satisfies check for +/// yet), and no version-range-satisfies check for /// file: / tarball deps (out of scope here). -/// -/// [#431]: https://github.com/pnpm/pacquet/issues/431 pub fn satisfies_package_manifest( importer: &ProjectSnapshot, manifest: &PackageManifest, - importer_id: &str, is_ignored_optional: &dyn Fn(&str) -> bool, ) -> Result<(), StalenessReason> { - let _ = importer_id; // reserved for the multi-importer path once lands. - // Phase 1: flat-record diff against the manifest's union of // dependency fields. Compares the importer's specifiers to the // manifest's existing deps (devs + prod + optional flattened diff --git a/pnpm/crates/lockfile/src/freshness/tests.rs b/pnpm/crates/lockfile/src/freshness/tests.rs index 3faad40bfb..335d2e5f93 100644 --- a/pnpm/crates/lockfile/src/freshness/tests.rs +++ b/pnpm/crates/lockfile/src/freshness/tests.rs @@ -56,7 +56,7 @@ fn matching_manifest_and_lockfile_satisfies() { } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } #[test] @@ -82,7 +82,7 @@ fn manifest_adds_dep_returns_specifier_diff() { } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); let StalenessReason::SpecifiersDiffer(diff) = err else { panic!("expected SpecifiersDiffer, got {err:?}"); @@ -117,7 +117,7 @@ fn manifest_drops_dep_returns_specifier_diff() { } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); let StalenessReason::SpecifiersDiffer(diff) = err else { panic!("expected SpecifiersDiffer, got {err:?}"); @@ -147,7 +147,7 @@ fn manifest_bumps_specifier_returns_specifier_diff() { } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); let StalenessReason::SpecifiersDiffer(diff) = err else { panic!("expected SpecifiersDiffer, got {err:?}"); @@ -187,7 +187,7 @@ fn matching_across_all_three_dep_fields_satisfies() { "optionalDependencies": { "fsevents": "^2.0.0" } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } /// Lockfile has no `importers["."]` entry — even though pacquet's @@ -230,7 +230,7 @@ fn dep_moves_between_fields_returns_dep_specifier_mismatch() { "dependencies": { "typescript": "^5.0.0" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::DepSpecifierMismatch { .. }), @@ -280,7 +280,7 @@ fn cross_field_swap_with_same_cardinalities_caught_by_per_field_check() { "devDependencies": { "react": "^17.0.2" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::DepSpecifierMismatch { .. }), @@ -312,7 +312,7 @@ fn publish_directory_mismatch_returns_publish_directory_mismatch() { "dependencies": { "react": "^17.0.2" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::PublishDirectoryMismatch { .. }), @@ -343,7 +343,7 @@ fn dependencies_meta_mismatch_returns_dependencies_meta_mismatch() { "dependencies": { "foo": "^1.0.0" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::DependenciesMetaMismatch { .. }), @@ -415,7 +415,7 @@ fn dependencies_meta_empty_object_equivalent_to_absent() { "dependenciesMeta": {} }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } #[test] @@ -440,7 +440,7 @@ fn publish_directory_match_satisfies() { "dependencies": { "foo": "1.0.0" } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } #[test] @@ -465,7 +465,7 @@ fn same_dep_in_prod_and_dev_counts_under_prod() { }"#, ); assert!( - satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok(), + satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok(), "manifest listing foo in prod+dev must satisfy a lockfile that records it under prod only", ); } @@ -492,7 +492,7 @@ fn same_dep_in_prod_and_optional_counts_under_optional() { }"#, ); assert!( - satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok(), + satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok(), "manifest listing foo in prod+optional must satisfy a lockfile that records it under optional only", ); } @@ -518,7 +518,7 @@ fn importer_empty_dev_dependencies_equivalent_to_absent() { "dependencies": { "foo": "^1.0.0" } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } // --------------------------------------------------------------------------- @@ -1296,7 +1296,7 @@ fn ignored_optional_filtered_out_of_manifest_diff() { }"#, ); let is_ignored: &dyn Fn(&str) -> bool = &|name: &str| name == "foo"; - assert!(satisfies_package_manifest(importer, &manifest, ".", is_ignored).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, is_ignored).is_ok()); } /// Polarity: without the filter the same fixture must fail. @@ -1323,7 +1323,7 @@ fn ignored_optional_without_filter_surfaces_as_drift() { "optionalDependencies": { "foo": "^1.0.0" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("without the filter the manifest's extra `foo` must surface as drift"); assert!( matches!(err, StalenessReason::SpecifiersDiffer(_)), @@ -1377,7 +1377,7 @@ fn ignored_optional_does_not_apply_to_dev_dependencies() { }"#, ); let is_ignored: &dyn Fn(&str) -> bool = &|name: &str| name == "foo"; - assert!(satisfies_package_manifest(importer, &manifest, ".", is_ignored).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, is_ignored).is_ok()); } /// Pins the group gate: removing the `matches!(... Prod | Optional)` @@ -1404,5 +1404,5 @@ fn ignored_optional_dev_only_lockfile_entry_kept() { }"#, ); let is_ignored: &dyn Fn(&str) -> bool = &|name: &str| name == "foo"; - assert!(satisfies_package_manifest(importer, &manifest, ".", is_ignored).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, is_ignored).is_ok()); } diff --git a/pnpm/crates/package-manager/src/install.rs b/pnpm/crates/package-manager/src/install.rs index dd8aa827ee..bb0b590fd6 100644 --- a/pnpm/crates/package-manager/src/install.rs +++ b/pnpm/crates/package-manager/src/install.rs @@ -813,7 +813,8 @@ where current_lockfile.as_ref().and_then(|current| { check_lockfile_freshness( current, - manifest, + &workspace_root, + &project_manifests, config, &catalogs, ignore_manifest_check, @@ -956,10 +957,17 @@ where // Run the freshness gates; on failure surface a fatal // InstallError via `FreshnessCheckError`'s `From` impl. // The check is run for its side effect (the typed - // outcome) — the borrowed lockfile / manifest are consumed + // outcome) — the borrowed lockfile / manifests are consumed // again inside the frozen branch below. - check_lockfile_freshness(lockfile, manifest, config, &catalogs, ignore_manifest_check) - .map_err(InstallError::from)?; + check_lockfile_freshness( + lockfile, + &workspace_root, + &project_manifests, + config, + &catalogs, + ignore_manifest_check, + ) + .map_err(InstallError::from)?; true } else if update_checksums { false @@ -974,7 +982,8 @@ where if prefer_frozen_lockfile { match check_lockfile_freshness( lockfile, - manifest, + &workspace_root, + &project_manifests, config, &catalogs, ignore_manifest_check, @@ -1778,7 +1787,8 @@ where /// `ignoredOptionalDependencies`) still runs. fn check_lockfile_freshness( lockfile: &Lockfile, - manifest: &PackageManifest, + workspace_root: &Path, + project_manifests: &[(PathBuf, &PackageManifest)], config: &Config, catalogs: &Catalogs, ignore_manifest_check: bool, @@ -1790,17 +1800,17 @@ fn check_lockfile_freshness( return Ok(()); } - // Pacquet has only one importer today ( tracks workspaces), - // so the root project is the only thing to verify; once - // workspaces land this becomes a per-project loop over - // `lockfile.importers`. - check_importer_satisfies( - lockfile, - manifest, - Lockfile::ROOT_IMPORTER_KEY, - config, - parsed_overrides_opt.as_deref(), - ) + for (project_dir, manifest) in project_manifests { + let importer_id = pacquet_workspace::importer_id_from_root_dir(workspace_root, project_dir); + check_importer_satisfies( + lockfile, + manifest, + &importer_id, + config, + parsed_overrides_opt.as_deref(), + )?; + } + Ok(()) } /// Parse `pnpm.overrides` from the config. Values can use the @@ -1916,7 +1926,7 @@ pub(crate) fn check_importer_satisfies( .unwrap_or_default(); let is_ignored_optional: &dyn Fn(&str) -> bool = &|name: &str| ignored_set.contains(name); - satisfies_package_manifest(importer, manifest_for_freshness, importer_id, is_ignored_optional) + satisfies_package_manifest(importer, manifest_for_freshness, is_ignored_optional) .map_err(FreshnessCheckError::Stale) } diff --git a/pnpr/crates/pnpr/src/resolver/resolve.rs b/pnpr/crates/pnpr/src/resolver/resolve.rs index 196b103cb7..02821c4e49 100644 --- a/pnpr/crates/pnpr/src/resolver/resolve.rs +++ b/pnpr/crates/pnpr/src/resolver/resolve.rs @@ -272,8 +272,7 @@ pub fn fresh_frozen_input_lockfile(config: &Config, request: &ResolveRequest) -> }); std::fs::write(&manifest_path, serde_json::to_vec(&manifest_json).ok()?).ok()?; let manifest = PackageManifest::from_path(manifest_path).ok()?; - satisfies_package_manifest(importer, &manifest, Lockfile::ROOT_IMPORTER_KEY, &|_: &str| false) - .ok()?; + satisfies_package_manifest(importer, &manifest, &|_: &str| false).ok()?; Some(lockfile.clone()) }