From f017f7bd0f19f0db5e953ddc7be2bd1d725fbfeb Mon Sep 17 00:00:00 2001 From: Ayush Singh <135635937+Ayush442842q@users.noreply.github.com> Date: Sat, 19 Sep 2026 14:50:28 +0530 Subject: [PATCH] fix(catalog): use the catalog entry when `pnpm add` names no version (#14965) Naming no version asks for whatever the workspace agreed on, so a new dependency the catalog already lists takes the entry rather than the range `latest` happens to produce today. pnpm 11 decides this the same way, in `parseWantedDependencies`, which is why only pnpm 12 reported a mismatch. A range reaching the catalog check still has to equal the entry. Answering `catalog:` swaps the wanted range for the entry's, so a range that merely falls inside it would lose what the dependency asked for, and pnpm widens the entry for nobody. The concurrency and reporting halves of one add test now sit in separate tests. A bare add of a cataloged package no longer requests `latest`, so the two could not share a fixture. Closes pnpm/pnpm#14865 --------- Co-authored-by: Zoltan Kochan --- .../catalog-covers-the-resolved-range.md | 5 + pnpm/crates/cli/tests/suite/catalog.rs | 69 ++++++++ .../package-manager/src/add/specifier.rs | 23 ++- .../src/add/tests/reporting.rs | 165 ++++++++++++++---- .../package-manager/src/catalog_mode.rs | 13 +- .../package-manager/src/catalog_mode/tests.rs | 107 ++++++++++++ 6 files changed, 340 insertions(+), 42 deletions(-) create mode 100644 .changeset/catalog-covers-the-resolved-range.md diff --git a/.changeset/catalog-covers-the-resolved-range.md b/.changeset/catalog-covers-the-resolved-range.md new file mode 100644 index 0000000000..e640414059 --- /dev/null +++ b/.changeset/catalog-covers-the-resolved-range.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +`pnpm add ` without a version now uses the catalog entry when the workspace already catalogs that package. Naming no version asks for whatever the workspace agreed on, so the entry stands even where it differs from the range `latest` would produce. `catalogMode: strict` used to fail with `ERR_PNPM_CATALOG_VERSION_MISMATCH` and `catalogMode: prefer` wrote a direct range into the manifest [pnpm/pnpm#14865](https://github.com/pnpm/pnpm/issues/14865). diff --git a/pnpm/crates/cli/tests/suite/catalog.rs b/pnpm/crates/cli/tests/suite/catalog.rs index fd1b067ac9..868ad994c2 100644 --- a/pnpm/crates/cli/tests/suite/catalog.rs +++ b/pnpm/crates/cli/tests/suite/catalog.rs @@ -807,3 +807,72 @@ fn add_moves_a_catalog_with_a_per_project_lockfile() { drop((root, anchor)); } + +/// Regression test for [pnpm/pnpm#14865](https://github.com/pnpm/pnpm/issues/14865): +/// naming no version asks for whatever the workspace agreed on, so the entry +/// stands even where it is not the range `latest` would have produced. +#[test] +fn strict_add_without_a_version_reuses_a_catalog_behind_the_latest_release() { + let (root, workspace, anchor) = setup(); + write_manifest(&workspace, "{}"); + append_workspace_yaml( + &workspace, + &format!("catalogMode: strict\ncatalog:\n '{FOO}': ^100.0.0\n"), + ); + + run_ok(&workspace, &["add", "--lockfile-only", FOO]); + + assert_eq!(dep_spec(&workspace, FOO).as_deref(), Some("catalog:")); + + drop((root, anchor)); +} + +/// A catalog entry stands even in `manual` mode, where nothing would have +/// moved the dependency into the catalog on its own. +#[test] +fn manual_add_without_a_version_reuses_the_catalog() { + let (root, workspace, anchor) = setup(); + write_manifest(&workspace, "{}"); + append_workspace_yaml( + &workspace, + &format!("catalogMode: manual\ncatalog:\n '{FOO}': ^100.0.0\n"), + ); + + run_ok(&workspace, &["add", "--lockfile-only", FOO]); + + assert_eq!(dep_spec(&workspace, FOO).as_deref(), Some("catalog:")); + + drop((root, anchor)); +} + +#[test] +fn prefer_add_without_a_version_reuses_a_matching_catalog_range() { + let (root, workspace, anchor) = setup(); + write_manifest(&workspace, "{}"); + append_workspace_yaml( + &workspace, + &format!("catalogMode: prefer\ncatalog:\n '{FOO}': ^100.0.0\n"), + ); + + run_ok(&workspace, &["add", "--lockfile-only", FOO]); + + assert_eq!(dep_spec(&workspace, FOO).as_deref(), Some("catalog:")); + + drop((root, anchor)); +} + +#[test] +fn strict_add_without_a_version_reuses_a_matching_catalog_range() { + let (root, workspace, anchor) = setup(); + write_manifest(&workspace, "{}"); + append_workspace_yaml( + &workspace, + &format!("catalogMode: strict\ncatalog:\n '{FOO}': ^100.0.0\n"), + ); + + run_ok(&workspace, &["add", "--lockfile-only", FOO]); + + assert_eq!(dep_spec(&workspace, FOO).as_deref(), Some("catalog:")); + + drop((root, anchor)); +} diff --git a/pnpm/crates/package-manager/src/add/specifier.rs b/pnpm/crates/package-manager/src/add/specifier.rs index 53bba7a736..fb6b010f38 100644 --- a/pnpm/crates/package-manager/src/add/specifier.rs +++ b/pnpm/crates/package-manager/src/add/specifier.rs @@ -145,7 +145,11 @@ pub(super) fn declared_specifier(manifest: &PackageManifest, package_name: &str) /// specifier verbatim (a `catalog:` reference, a range, or an /// exact pin) — `pnpm add ` without a /// version leaves the declared range untouched; -/// - a brand-new dependency fetches and pins the `latest` range. +/// - a brand-new dependency the catalog already lists takes the +/// `catalog:` reference — naming no version asks for whatever the +/// workspace agreed on, which is the entry, not the `latest` range +/// it happens to resolve to today; +/// - any other brand-new dependency fetches and pins the `latest` range. pub(super) async fn bare_save_specifier( package_selector: &str, selector: &AddSelector, @@ -187,9 +191,24 @@ pub(super) async fn bare_save_specifier( .await? .unwrap_or_else(|| normalized_save_specifier(spec))), (None, Some(prev)) => Ok(prev.to_string()), - (None, None) => pick_latest_range(package_name, inputs).await, + (None, None) => match cataloged_specifier(package_name, inputs) { + Some(specifier) => Ok(specifier), + None => pick_latest_range(package_name, inputs).await, + }, } } +/// The `catalog:` reference for a dependency the catalog already lists, or +/// `None` when it lists no entry for it. +fn cataloged_specifier(package_name: &str, inputs: &AddResolveInputs<'_, '_>) -> Option { + let catalog_name = crate::per_dep_catalog_name(None, inputs.save_catalog_name); + inputs.catalogs.get(catalog_name)?.get(package_name)?; + Some(if catalog_name == pnpm_catalogs_types::DEFAULT_CATALOG_NAME { + "catalog:".to_string() + } else { + format!("catalog:{catalog_name}") + }) +} + pub(super) async fn resolve_node_runtime_specifier( version_spec: &str, prev_specifier: Option<&str>, diff --git a/pnpm/crates/package-manager/src/add/tests/reporting.rs b/pnpm/crates/package-manager/src/add/tests/reporting.rs index 4eb018f4e3..844bcc05ab 100644 --- a/pnpm/crates/package-manager/src/add/tests/reporting.rs +++ b/pnpm/crates/package-manager/src/add/tests/reporting.rs @@ -113,7 +113,7 @@ async fn add_routes_scoped_packages_to_configured_scoped_registry() { scoped_latest.assert_async().await; } #[tokio::test] -async fn add_resolves_package_selectors_concurrently_and_reports_in_selector_order() { +async fn add_resolves_package_selectors_concurrently() { static EVENTS: Mutex> = Mutex::new(Vec::new()); EVENTS.lock().unwrap().clear(); @@ -166,39 +166,12 @@ async fn add_resolves_package_selectors_concurrently_and_reports_in_selector_ord drop(requests); } - fn assert_catalog_warning_order(events: &[LogEvent]) { - let warning_messages: Vec<_> = events - .iter() - .filter_map(|event| match event { - LogEvent::Pnpm(log) - if log.level == LogLevel::Warn - && log.message.starts_with("Catalog version mismatch") => - { - Some(log.message.as_str()) - } - _ => None, - }) - .collect(); - assert_eq!( - warning_messages, - [ - r#"Catalog version mismatch for "@one/a": using direct version "1.0.0" instead of catalog version "9.0.0"."#, - r#"Catalog version mismatch for "@two/b": using direct version "1.0.0" instead of catalog version "9.0.0"."#, - r#"Catalog version mismatch for "@three/c": using direct version "1.0.0" instead of catalog version "9.0.0"."#, - ], - ); - } - let dir = tempdir().unwrap(); let project_root = dir.path().join("project"); let modules_dir = project_root.join("node_modules"); let virtual_store_dir = modules_dir.join(".pacquet"); std::fs::create_dir_all(&project_root).unwrap(); - std::fs::write( - project_root.join("pnpm-workspace.yaml"), - "packages:\n - '.'\ncatalog:\n '@one/a': 9.0.0\n '@two/b': 9.0.0\n '@three/c': 9.0.0\n", - ) - .unwrap(); + std::fs::write(project_root.join("pnpm-workspace.yaml"), "packages:\n - '.'\n").unwrap(); let mut manifest = PackageManifest::create_if_needed(project_root.join("package.json")) .expect("create manifest"); @@ -208,7 +181,7 @@ async fn add_resolves_package_selectors_concurrently_and_reports_in_selector_ord config.store_dir = dir.path().join("pacquet-store").into(); config.modules_dir = modules_dir; config.virtual_store_dir = virtual_store_dir; - config.catalog_mode = pnpm_config::CatalogMode::Prefer; + config.catalog_mode = pnpm_config::CatalogMode::Manual; config.minimum_release_age = None; let mut servers = Vec::new(); let mut mocks = Vec::new(); @@ -292,17 +265,139 @@ async fn add_resolves_package_selectors_concurrently_and_reports_in_selector_ord ); } - { - let events = EVENTS.lock().unwrap(); - assert_catalog_warning_order(&events); - } - for (latest, _packument) in mocks { latest.assert_async().await; } drop(servers); } #[tokio::test] +async fn add_reports_catalog_warnings_in_selector_order() { + static EVENTS: Mutex> = Mutex::new(Vec::new()); + EVENTS.lock().unwrap().clear(); + + struct RecordingReporter; + impl Reporter for RecordingReporter { + fn emit(event: &LogEvent) { + EVENTS + .lock() + .unwrap() + .push(event.clone()); + } + } + + let dir = tempdir().unwrap(); + let project_root = dir.path().join("project"); + let modules_dir = project_root.join("node_modules"); + let virtual_store_dir = modules_dir.join(".pacquet"); + std::fs::create_dir_all(&project_root).unwrap(); + std::fs::write( + project_root.join("pnpm-workspace.yaml"), + "packages:\n - '.'\ncatalog:\n '@one/a': 9.0.0\n '@two/b': 9.0.0\n '@three/c': 9.0.0\n", + ) + .unwrap(); + let mut manifest = PackageManifest::create_if_needed(project_root.join("package.json")) + .expect("create manifest"); + + // The slowest selector is named first, so reports that followed the order + // the responses arrived in would come back reversed. + let packages = [("one", "a", 200), ("two", "b", 100), ("three", "c", 0)]; + let mut config = Config::new(); + config.store_dir = dir.path().join("pacquet-store").into(); + config.modules_dir = modules_dir; + config.virtual_store_dir = virtual_store_dir; + config.catalog_mode = pnpm_config::CatalogMode::Prefer; + config.minimum_release_age = None; + let mut servers = Vec::new(); + let mut mocks = Vec::new(); + let mut package_names = Vec::new(); + + for (scope, name, response_delay_ms) in packages { + let package_name = format!("@{scope}/{name}"); + let mut server = mockito::Server::new_async().await; + let registry_url = format!("{}/", server.url()); + config.registries_by_scope.insert(format!("@{scope}"), registry_url.clone()); + + let response_body = package_body(&package_name, ®istry_url); + let packument_path = format!("/@{scope}%2F{name}"); + let packument = server + .mock("GET", packument_path.as_str()) + .with_status(200) + .with_header("content-type", "application/json") + .with_chunked_body(move |writer| { + std::thread::sleep(Duration::from_millis(response_delay_ms)); + writer.write_all(response_body.as_bytes()) + }) + .expect(1) + .create_async() + .await; + + // Each selector names its version. A bare `pnpm add` takes the catalog + // entry outright, which is the case with nothing to report. + package_names.push(format!("{package_name}@1.0.0")); + mocks.push(packument); + servers.push(server); + } + + let config = config.leak(); + let http_client = ThrottledClient::default(); + let resolved_packages = ResolvedPackages::default(); + Add { + manifest: &mut manifest, + options: crate::AddOptions { + resolved_packages: &resolved_packages, + http_client: &http_client, + config, + lockfile: crate::CommandLockfile::loaded(None, None), + package_names: &package_names, + range_spec_style: RangeSpecStyle::Patch, + lockfile_only: true, + }, + resources: crate::AddResources { + tarball_mem_cache: Arc::default(), + http_client_arc: Arc::new(ThrottledClient::default()), + dependency_groups: Some([DependencyGroup::Prod]), + included_groups: None, + save_catalog_name: None, + supported_architectures: None, + }, + } + .run::() + .await + .expect("add should resolve all package selectors"); + + fn catalog_mismatch_warnings(events: &[LogEvent]) -> Vec<&str> { + events + .iter() + .filter_map(|event| match event { + LogEvent::Pnpm(log) + if log.level == LogLevel::Warn + && log.message.starts_with("Catalog version mismatch") => + { + Some(log.message.as_str()) + } + _ => None, + }) + .collect() + } + + { + let events = EVENTS.lock().unwrap(); + assert_eq!( + catalog_mismatch_warnings(&events), + [ + r#"Catalog version mismatch for "@one/a": using direct version "1.0.0" instead of catalog version "9.0.0"."#, + r#"Catalog version mismatch for "@two/b": using direct version "1.0.0" instead of catalog version "9.0.0"."#, + r#"Catalog version mismatch for "@three/c": using direct version "1.0.0" instead of catalog version "9.0.0"."#, + ], + ); + } + + for packument in mocks { + packument.assert_async().await; + } + drop(servers); +} +#[tokio::test] async fn add_reports_resolution_errors_in_selector_order() { let dir = tempdir().unwrap(); let project_root = dir.path().join("project"); diff --git a/pnpm/crates/package-manager/src/catalog_mode.rs b/pnpm/crates/package-manager/src/catalog_mode.rs index 37fbdb7fcf..3491cb8d9d 100644 --- a/pnpm/crates/package-manager/src/catalog_mode.rs +++ b/pnpm/crates/package-manager/src/catalog_mode.rs @@ -229,13 +229,16 @@ fn is_project_relative_path(specifier: &str) -> bool { /// Whether the catalog entry already covers the wanted specifier, so the /// dependency can keep resolving through the catalog: the entry names the -/// same concrete version, or it is a range the wanted version satisfies. +/// same specifier, or it is a range the wanted version satisfies. /// -/// The wanted specifier has to be a concrete version. A wanted range is -/// never covered, because the catalog — not the dependency — decides which -/// version a `catalog:` reference resolves to. +/// A wanted range has to match the entry exactly. Keeping the catalog swaps +/// the wanted range for the entry's, so a merely narrower range would drop +/// what the dependency asked for, and pnpm does not widen the entry to make +/// room for it. A wanted version is different: `pnpm add` moves the catalog +/// onto the version it names. pub(crate) fn catalog_covers(entry: &str, wanted: &str) -> bool { - matches!((Range::parse(entry), Version::parse(wanted)), (Ok(entry), Ok(wanted)) if entry.satisfies(&wanted)) + entry == wanted + || matches!((Range::parse(entry), Version::parse(wanted)), (Ok(entry), Ok(wanted)) if entry.satisfies(&wanted)) } /// The catalog group a dependency belongs to: a previous `catalog:` diff --git a/pnpm/crates/package-manager/src/catalog_mode/tests.rs b/pnpm/crates/package-manager/src/catalog_mode/tests.rs index b3020386e9..d9e396e968 100644 --- a/pnpm/crates/package-manager/src/catalog_mode/tests.rs +++ b/pnpm/crates/package-manager/src/catalog_mode/tests.rs @@ -33,6 +33,16 @@ fn decide( decide_catalog::(mode, None, catalogs, dep, "/repo") } +/// [`decide`] without dropping the warning, for the modes that report a +/// mismatch instead of failing on it. +fn decide_outcome( + mode: CatalogMode, + catalogs: &Catalogs, + dep: &CatalogModeDep<'_>, +) -> Result { + super::decide_catalog_outcome(mode, None, catalogs, dep, "/repo") +} + #[test] fn manual_mode_keeps_the_direct_version() { let catalogs = catalogs(&[("default", &[("is-positive", "1.0.0")])]); @@ -110,6 +120,103 @@ fn strict_errors_when_the_wanted_specifier_is_a_range() { assert_eq!(err.catalog_dep, "is-positive@1.0.0"); } +/// A range that merely falls inside the catalog range is a mismatch, not a +/// match, and deliberately so. +/// +/// Answering `catalog:` replaces the wanted range with the entry's. Were +/// `^2.1.0` treated as covered by `^2.0.0`, the manifest would go on to +/// resolve through `^2.0.0` and could take `2.0.x`, which is exactly what +/// asking for `^2.1.0` ruled out. pnpm does not widen the entry to `^2.1.0` +/// either, since every other project on the entry would inherit that. So the +/// containment direction is not the question: only a range equal to the entry +/// keeps the catalog, and anything else is reported. +#[test] +fn strict_errors_when_the_wanted_range_sits_inside_the_catalog_range() { + let catalogs = catalogs(&[("default", &[("is-positive", "^2.0.0")])]); + let err = decide(CatalogMode::Strict, &catalogs, &dep("is-positive", "^2.1.0")) + .expect_err( + "`catalog:` would resolve through `^2.0.0` and could take a version `^2.1.0` excludes", + ); + assert_eq!( + err, + CatalogVersionMismatchError { + catalog_dep: "is-positive@^2.0.0".to_string(), + wanted_dep: "is-positive@^2.1.0".to_string(), + }, + ); +} + +/// The mirror of [`strict_errors_when_the_wanted_range_sits_inside_the_catalog_range`]: +/// a range the catalog sits inside is no better, since `catalog:` would drop +/// the versions the wanted range adds. +#[test] +fn strict_errors_when_the_wanted_range_holds_the_catalog_range() { + let catalogs = catalogs(&[("default", &[("is-positive", "^2.1.0")])]); + let err = decide(CatalogMode::Strict, &catalogs, &dep("is-positive", "^2.0.0")) + .expect_err( + "`catalog:` would resolve through `^2.1.0` and never take the `2.0.x` the range allows", + ); + assert_eq!( + err, + CatalogVersionMismatchError { + catalog_dep: "is-positive@^2.1.0".to_string(), + wanted_dep: "is-positive@^2.0.0".to_string(), + }, + ); +} + +/// Prefer mode keeps the range the dependency asked for rather than quietly +/// swapping in a catalog that spans different versions. +#[test] +fn prefer_keeps_a_direct_range_that_sits_inside_the_catalog_range() { + let catalogs = catalogs(&[("default", &[("is-positive", "^2.0.0")])]); + let outcome = + decide_outcome(CatalogMode::Prefer, &catalogs, &dep("is-positive", "^2.1.0")).unwrap(); + assert_eq!(outcome.decision, CatalogDecision::KeepDirect); + assert!( + outcome.warning.is_some(), + "the mismatch is reported rather than silently resolved through the catalog", + ); +} + +/// A concrete version is the one case containment settles, because `pnpm add` +/// moves the catalog onto the version it names instead of discarding it. +#[test] +fn a_wanted_version_inside_the_catalog_range_is_still_covered() { + assert!(super::catalog_covers("^2.0.0", "2.1.0")); + assert!(!super::catalog_covers("^2.0.0", "^2.1.0"), "a range is not a version"); + assert!(!super::catalog_covers("^2.1.0", "^2.0.0"), "nor is the wider one"); + assert!(super::catalog_covers("^2.0.0", "^2.0.0"), "only an equal range keeps the catalog"); +} + +#[test] +fn prefer_uses_the_catalog_on_a_matching_range() { + let catalogs = catalogs(&[("default", &[("tailwindcss", "^4.3.3")])]); + let decision = decide(CatalogMode::Prefer, &catalogs, &dep("tailwindcss", "^4.3.3")).unwrap(); + assert_eq!( + decision, + CatalogDecision::Catalog { + manifest_specifier: "catalog:".to_string(), + updated_entry: None + }, + "a range equal to the catalog range reuses the existing catalog entry", + ); +} + +#[test] +fn strict_uses_the_catalog_on_a_matching_range() { + let catalogs = catalogs(&[("default", &[("tailwindcss", "^4.3.3")])]); + let decision = decide(CatalogMode::Strict, &catalogs, &dep("tailwindcss", "^4.3.3")).unwrap(); + assert_eq!( + decision, + CatalogDecision::Catalog { + manifest_specifier: "catalog:".to_string(), + updated_entry: None + }, + "a range equal to the catalog range reuses the existing catalog entry", + ); +} + #[test] fn strict_uses_the_catalog_on_a_matching_concrete_version() { let catalogs = catalogs(&[("default", &[("is-positive", "1.0.0")])]);