diff --git a/crates/socket-patch-cli/tests/e2e_vendor_pnpm_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_pnpm_build.rs index 51bc2de52..058be83eb 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_pnpm_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_pnpm_build.rs @@ -1086,8 +1086,8 @@ async fn pnpm_pinned_matrix_vendored_lifecycle_and_manifestless_vex() { run_pnpm_capstone(&pm, VendorDriver::VendorCli).await; run_pnpm_capstone(&pm, VendorDriver::GetUuid).await; } - 7 => off_runtime(|| run_legacy_capstone(&pm, "lockfileVersion: 5.4")), - 8 => off_runtime(|| run_legacy_capstone(&pm, "lockfileVersion: '6.0'")), + 7 => off_runtime(|| run_legacy_capstone(&pm, "lockfileVersion: 5.4", "proj")), + 8 => off_runtime(|| run_legacy_capstone(&pm, "lockfileVersion: '6.0'", "proj")), _ => off_runtime(|| run_unsupported_lock_refusal(&pm)), } } @@ -1488,7 +1488,7 @@ fn pnpm7_real_lifecycle_same_path_frozen_and_moved_checkout_offline() { println!("SKIP: `corepack {PNPM_LEGACY_7}` unavailable"); return; } - run_legacy_capstone(PNPM_LEGACY_7, "lockfileVersion: 5.4"); + run_legacy_capstone(PNPM_LEGACY_7, "lockfileVersion: 5.4", "proj"); } #[test] @@ -1497,7 +1497,40 @@ fn pnpm8_real_lifecycle_same_path_frozen_and_moved_checkout_offline() { println!("SKIP: `corepack {PNPM_LEGACY_8}` unavailable"); return; } - run_legacy_capstone(PNPM_LEGACY_8, "lockfileVersion: '6.0'"); + run_legacy_capstone(PNPM_LEGACY_8, "lockfileVersion: '6.0'", "proj"); +} + +/// Project dir names holding a YAML indicator (#754). Windows forbids `:` +/// in a path component, so the `: ` case runs on unix only. +#[cfg(not(windows))] +const YAML_INDICATOR_DIRS: &[&str] = &["hash #x", "colon: x"]; +#[cfg(windows)] +const YAML_INDICATOR_DIRS: &[&str] = &["hash #x"]; + +/// #754: the absolute specifier lands in the lock under a project path +/// holding YAML indicators. Unquoted, ` #` turned the rest of the path +/// into a comment (ERR_PNPM_OUTDATED_LOCKFILE) and `: ` broke the line +/// (ERR_PNPM_BROKEN_LOCKFILE); the same-path frozen install must pass. +#[test] +fn pnpm7_real_lifecycle_under_yaml_indicator_paths() { + if !has_corepack_pm(PNPM_LEGACY_7) { + println!("SKIP: `corepack {PNPM_LEGACY_7}` unavailable"); + return; + } + for dir in YAML_INDICATOR_DIRS { + run_legacy_capstone(PNPM_LEGACY_7, "lockfileVersion: 5.4", dir); + } +} + +#[test] +fn pnpm8_real_lifecycle_under_yaml_indicator_paths() { + if !has_corepack_pm(PNPM_LEGACY_8) { + println!("SKIP: `corepack {PNPM_LEGACY_8}` unavailable"); + return; + } + for dir in YAML_INDICATOR_DIRS { + run_legacy_capstone(PNPM_LEGACY_8, "lockfileVersion: '6.0'", dir); + } } /// Full lifecycle against the REAL pinned legacy pnpm, spike-proven flags: @@ -1515,9 +1548,9 @@ fn pnpm8_real_lifecycle_same_path_frozen_and_moved_checkout_offline() { /// marker bytes (probe C); /// 5. idempotent re-vendor (byte-stable, already_vendored); /// 6. revert restores both files byte-identical and removes .socket/vendor. -fn run_legacy_capstone(pm: &str, lock_head: &str) { +fn run_legacy_capstone(pm: &str, lock_head: &str, proj_dir: &str) { let tmp = tempfile::tempdir().unwrap(); - let proj = tmp.path().join("proj"); + let proj = tmp.path().join(proj_dir); std::fs::create_dir_all(&proj).unwrap(); let pkg_doc = serde_json::json!({ "name": "pnpm-legacy-capstone", diff --git a/crates/socket-patch-core/src/formats/pnpm/lines.rs b/crates/socket-patch-core/src/formats/pnpm/lines.rs index ce1726f1e..7bc049379 100644 --- a/crates/socket-patch-core/src/formats/pnpm/lines.rs +++ b/crates/socket-patch-core/src/formats/pnpm/lines.rs @@ -129,6 +129,89 @@ pub(crate) fn unquote_value(value: &str) -> &str { } } +/// Spell a string as a block-mapping VALUE the way pnpm's YAML writer +/// (js-yaml `dump`) does: plain when the plain scalar reads back as the +/// same string, single-quoted otherwise, double-quoted (escaped) when it +/// holds a character single quotes can't carry. A value we splice in +/// verbatim must not contain ` #` (the rest becomes a comment) or `: ` +/// (the line stops being valid YAML) unquoted — e.g. an absolute path +/// under a directory named `My Project #2`. +pub(crate) fn yaml_value(value: &str) -> std::borrow::Cow<'_, str> { + use std::borrow::Cow; + if !value.chars().all(is_yaml_printable) { + let mut out = String::with_capacity(value.len() + 2); + out.push('"'); + for c in value.chars() { + match c { + '"' => out.push_str("\\\""), + '\\' => out.push_str("\\\\"), + '\n' => out.push_str("\\n"), + '\t' => out.push_str("\\t"), + '\r' => out.push_str("\\r"), + c if is_yaml_printable(c) => out.push(c), + c if (c as u32) <= 0xFF => out.push_str(&format!("\\x{:02X}", c as u32)), + c if (c as u32) <= 0xFFFF => out.push_str(&format!("\\u{:04X}", c as u32)), + c => out.push_str(&format!("\\U{:08X}", c as u32)), + } + } + out.push('"'); + return Cow::Owned(out); + } + if is_plain_safe(value) { + Cow::Borrowed(value) + } else { + Cow::Owned(format!("'{}'", value.replace('\'', "''"))) + } +} + +/// js-yaml's `isPrintable`: what a single-quoted or plain scalar may hold. +fn is_yaml_printable(c: char) -> bool { + let c = c as u32; + (0x20..=0x7E).contains(&c) + || ((0xA1..=0xD7FF).contains(&c) && c != 0x2028 && c != 0x2029) + || ((0xE000..=0xFFFD).contains(&c) && c != 0xFEFF) + || (0x10000..=0x10FFFF).contains(&c) +} + +/// Would `value` (all printable) read back unchanged as a plain block +/// scalar? Mirrors js-yaml's plain-style rules for block context: no +/// indicator first character, no leading/trailing space or trailing `:`, +/// no `#` after a space, no `:` before a space, and nothing YAML would +/// resolve to a non-string (null, bool, number). +fn is_plain_safe(value: &str) -> bool { + let Some(first) = value.chars().next() else { + return false; + }; + if first == ' ' + || "-?:,[]{}#&*!|>'\"%@`".contains(first) + || value.ends_with(' ') + || value.ends_with(':') + || value.contains(" #") + || value.contains(": ") + { + return false; + } + let lower = value.to_ascii_lowercase(); + let implicit = matches!( + lower.as_str(), + "~" | "null" + | "true" + | "false" + | "yes" + | "no" + | "on" + | "off" + | "y" + | "n" + | ".inf" + | "-.inf" + | "+.inf" + | ".nan" + ) || value.parse::().is_ok() + || (value.starts_with("0x") || value.starts_with("0o") || value.starts_with("0b")); + !implicit +} + /// pnpm quotes `@`-leading keys with single quotes; everything we write is /// otherwise bare. pub(crate) fn yaml_key(key: &str) -> String { @@ -147,3 +230,61 @@ pub(crate) fn yaml_key_like(key: &str, original_repr: &str) -> String { _ => yaml_key(key), } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Values pnpm's writer leaves plain stay byte-identical — the spellings + /// the vendored legacy splice already round-trips (#754's passing cells). + #[test] + fn yaml_value_keeps_plain_safe_values_plain() { + for v in [ + "file:/tmp/w/plain dir/.socket/vendor/npm/u/left-pad-1.3.0.tgz", + "file:/tmp/w/ünïcode/x.tgz", + "file:/tmp/w/a'quote/x.tgz", + "file:/tmp/w/[br]/x.tgz", + "file:/tmp/w/x#y/x.tgz", + "file:C:/Users/x/a:b/x.tgz", + "^1.3.0", + "1.3.0", + "npm:left-pad@1.2.0", + ] { + assert_eq!(yaml_value(v), v, "{v}"); + } + } + + /// ` #` and `: ` would truncate or break a plain scalar, so they are + /// single-quoted exactly as pnpm 7/8 write them (#754). + #[test] + fn yaml_value_single_quotes_comment_and_mapping_indicators() { + assert_eq!( + yaml_value("file:/tmp/w/hash #x/a.tgz"), + "'file:/tmp/w/hash #x/a.tgz'" + ); + assert_eq!( + yaml_value("file:/tmp/w/colon: x/a.tgz"), + "'file:/tmp/w/colon: x/a.tgz'" + ); + assert_eq!( + yaml_value("file:/tmp/My Project #2/it's.tgz"), + "'file:/tmp/My Project #2/it''s.tgz'" + ); + assert_eq!(yaml_value("trailing:"), "'trailing:'"); + assert_eq!(yaml_value(" lead"), "' lead'"); + assert_eq!(yaml_value("@scope/x"), "'@scope/x'"); + assert_eq!(yaml_value("catalog:"), "'catalog:'"); + assert_eq!(yaml_value("true"), "'true'"); + assert_eq!(yaml_value("1.0"), "'1.0'"); + assert_eq!(yaml_value(""), "''"); + } + + /// Characters single quotes can't carry fall back to an escaped + /// double-quoted scalar. + #[test] + fn yaml_value_double_quotes_non_printables() { + assert_eq!(yaml_value("a\tb"), "\"a\\tb\""); + assert_eq!(yaml_value("a\nb\"c\\"), "\"a\\nb\\\"c\\\\\""); + assert_eq!(yaml_value("a\u{7f}"), "\"a\\x7F\""); + } +} diff --git a/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs b/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs index ca815da75..a6570bd83 100644 --- a/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs +++ b/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs @@ -72,7 +72,8 @@ use super::state::{VendorEntry, WiringAction, WiringRecord}; use super::{RevertOpts, RevertOutcome, VendorOutcome, VendorWarning}; use crate::constants::npm_family::PNPM_LOCK; use crate::formats::pnpm::lines::{ - next_block, parse_key_line, section_bounds, split_lines, yaml_key, yaml_key_like, + next_block, parse_key_line, section_bounds, split_lines, unquote_value, yaml_key, + yaml_key_like, yaml_value, }; use crate::formats::pnpm::{sniff_lock_grammar, PnpmLock, PnpmLockGrammar}; @@ -575,12 +576,16 @@ fn edit_specifier_v54( if key != ctx.name { continue; } - if rest == ctx.abs_spec { + // The absolute root can hold YAML indicators (` #`, `: `); spell it + // the way pnpm does, or YAML truncates or rejects the line (#754). + let abs_yaml = yaml_value(ctx.abs_spec); + if rest == abs_yaml { return Ok(false); // in sync } - // Ours at a stale root/uuid (a moved checkout being re-vendored) has - // no original; anything else is the user's range, recorded. - let original = (!ctx.is_ours(rest)).then(|| Value::String(rest.to_string())); + // Ours at a stale root/uuid (a moved checkout being re-vendored, or + // an unquoted spelling an older release wrote) has no original; + // anything else is the user's range, recorded. + let original = (!ctx.is_ours(unquote_value(rest))).then(|| Value::String(rest.to_string())); // Built before the splice below: `key`/`repr` borrow `*line`. let record = WiringRecord { file: PNPM_LOCK.to_string(), @@ -590,7 +595,7 @@ fn edit_specifier_v54( original, new: Some(Value::String(ctx.abs_spec.to_string())), }; - let rewritten = format!(" {}: {}", yaml_key_like(key, repr), ctx.abs_spec); + let rewritten = format!(" {}: {abs_yaml}", yaml_key_like(key, repr)); *line = rewritten; wiring.push(record); return Ok(true); @@ -608,6 +613,8 @@ fn edit_root_deps_v60( ) -> Result<(bool, bool), String> { let mut changed = false; let mut hit = false; + // Quoted the way pnpm writes it when the root holds YAML indicators. + let abs_yaml = yaml_value(ctx.abs_spec); for section in ROOT_DEP_SECTIONS { let Some((start, end)) = section_bounds(lines, section) else { continue; @@ -627,7 +634,7 @@ fn edit_root_deps_v60( let target = old_ver == ctx.version || ctx.is_ours(&old_ver); if target { hit = true; - if old_ver == ctx.spec && old_spec == ctx.abs_spec { + if old_ver == ctx.spec && old_spec == abs_yaml { k = f; continue; // in sync } @@ -651,7 +658,7 @@ fn edit_root_deps_v60( "version": ctx.spec, })), }; - lines[si] = format!(" specifier: {}", ctx.abs_spec); + lines[si] = format!(" specifier: {abs_yaml}"); lines[vi] = format!(" version: {}", ctx.spec); wiring.push(record); changed = true; @@ -967,8 +974,14 @@ fn revert_value_line( if rec.original.as_ref().and_then(Value::as_str) == Some(rest) { return; } - let ours = Some(rest) == rec.new.as_ref().and_then(Value::as_str) - || parse_vendor_path(rest).is_some_and(|p| p.eco == "npm" && p.uuid == entry_uuid); + // `new` holds the value; the line may spell it YAML-quoted. + let ours = rec + .new + .as_ref() + .and_then(Value::as_str) + .is_some_and(|new| rest == new || rest == yaml_value(new)) + || parse_vendor_path(unquote_value(rest)) + .is_some_and(|p| p.eco == "npm" && p.uuid == entry_uuid); if !ours { warnings.push(drifted(format!( "{section} entry `{dep}` was changed since vendoring ({rest}); left alone" @@ -1563,13 +1576,15 @@ packages: "; struct Fixture { - tmp: tempfile::TempDir, + // Held only to keep the tempdir alive for the fixture's lifetime. + _tmp: tempfile::TempDir, + root: PathBuf, record: PatchRecord, } impl Fixture { fn root(&self) -> &Path { - self.tmp.path() + &self.root } /// The canonical root — what the backend embeds in the absolute @@ -1689,8 +1704,18 @@ packages: crate::vendor::test_support::npm_flip_suite!(flip_suite_v6, Fixture, flip_fixture_v6, flip_run); async fn fixture_with(pkg_json: &str, lock: &str) -> Fixture { + fixture_in(None, pkg_json, lock).await + } + + /// [`fixture_with`], with the project in a `dir_name` subdirectory of + /// the tempdir — for project paths whose spelling matters to the lock. + async fn fixture_in(dir_name: Option<&str>, pkg_json: &str, lock: &str) -> Fixture { let tmp = tempfile::tempdir().unwrap(); - let root = tmp.path(); + let root_buf = match dir_name { + Some(d) => tmp.path().join(d), + None => tmp.path().to_path_buf(), + }; + let root = root_buf.as_path(); let installed = root.join("node_modules/left-pad"); tokio::fs::create_dir_all(&installed).await.unwrap(); @@ -1733,7 +1758,11 @@ packages: license: "MIT".to_string(), tier: "free".to_string(), }; - Fixture { tmp, record } + Fixture { + _tmp: tmp, + root: root_buf, + record, + } } fn expect_done( @@ -1951,6 +1980,121 @@ packages: } } + /// Project dir names holding a YAML indicator (#754). Windows forbids + /// `:` in a path component, so the `: ` case runs on unix only. + #[cfg(not(windows))] + const YAML_INDICATOR_DIRS: &[&str] = &["hash #x", "colon: x"]; + #[cfg(windows)] + const YAML_INDICATOR_DIRS: &[&str] = &["hash #x"]; + + /// #754: a project root holding a YAML indicator (` #` truncates a + /// plain scalar into a comment, `: ` makes the line invalid) gets the + /// absolute specifier single-quoted exactly as pnpm 7/8 write it, so a + /// frozen install at that path reads the specifier socket-patch meant. + /// The re-run is in sync (what `vendor --check` relies on) and revert + /// restores the lock byte-for-byte — for BOTH grammars. + #[tokio::test] + async fn absolute_specifier_is_yaml_quoted_under_indicator_paths() { + for dir in YAML_INDICATOR_DIRS { + for (before_lock, after_lock, tag) in [ + (T7_BEFORE_LOCK, T7_AFTER_LOCK, "5.4"), + (T8_BEFORE_LOCK, T8_AFTER_LOCK, "6.0"), + ] { + let fx = fixture_in(Some(dir), T_BEFORE_PKG, before_lock).await; + let (result, entry, _) = expect_done(fx.vendor(false).await); + assert!(result.success, "{tag} {dir}: {:?}", result.error); + let entry = entry.expect("entry"); + + let plain = format!("file:{ROOT_TOKEN}/{}", fx.rel_tgz()); + let abs = format!("file:{}/{}", fx.canon_root_str(), fx.rel_tgz()); + let quoted = format!("'{}'", abs.replace('\'', "''")); + let want = after_lock + .replace(&plain, "ed) + .replace(SPIKE_INTEGRITY, &fx.actual_integrity().await); + let lock_after = fx.read(PNPM_LOCK).await; + assert_eq!(lock_after, want, "{tag} {dir}"); + let want_line = match tag { + "5.4" => format!(" left-pad: {quoted}"), + _ => format!(" specifier: {quoted}"), + }; + assert!( + lock_after.lines().any(|l| l == want_line), + "{tag} {dir}: {want_line}\n{lock_after}" + ); + + // In sync on re-run: nothing recorded, bytes stable. + let (result, again, _) = expect_done(fx.vendor(false).await); + assert!(result.success, "{tag} {dir}: {:?}", result.error); + assert!( + again.is_none(), + "{tag} {dir}: in-sync rerun records nothing" + ); + assert!( + result + .files_verified + .iter() + .all(|v| v.status == VerifyStatus::AlreadyPatched), + "{tag} {dir}" + ); + assert_eq!(fx.read(PNPM_LOCK).await, lock_after, "{tag} {dir}"); + + let outcome = revert_pnpm_legacy(&entry, fx.root(), false).await; + assert!(outcome.success, "{tag} {dir}: {:?}", outcome.error); + assert!( + outcome.warnings.is_empty(), + "{tag} {dir}: {:?}", + outcome.warnings + ); + assert_eq!(fx.read(PNPM_LOCK).await, before_lock, "{tag} {dir}"); + assert_eq!(fx.read(PACKAGE_JSON).await, T_BEFORE_PKG, "{tag} {dir}"); + } + } + } + + /// #754: a lock an earlier release left broken (the absolute specifier + /// spliced unquoted under a ` #` path) is not "in sync" — re-vendoring + /// rewrites it to the quoted spelling and records no original for our + /// own stale value; the first entry's revert still restores the user's + /// range. + #[tokio::test] + async fn unquoted_absolute_specifier_from_an_older_release_is_healed() { + for (before_lock, tag) in [(T7_BEFORE_LOCK, "5.4"), (T8_BEFORE_LOCK, "6.0")] { + let fx = fixture_in(Some("hash #x"), T_BEFORE_PKG, before_lock).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.expect("entry"); + let abs = format!("file:{}/{}", fx.canon_root_str(), fx.rel_tgz()); + let quoted = format!("'{}'", abs.replace('\'', "''")); + let healthy = fx.read(PNPM_LOCK).await; + // What releases before the fix wrote. + let broken = healthy.replace("ed, &abs); + assert_ne!(broken, healthy, "{tag}"); + tokio::fs::write(fx.root().join(PNPM_LOCK), &broken) + .await + .unwrap(); + + let (result, again, _) = expect_done(fx.vendor(false).await); + assert!(result.success, "{tag}: {:?}", result.error); + assert_eq!(fx.read(PNPM_LOCK).await, healthy, "{tag}: healed"); + let again = again.expect("a healing re-run records its wiring"); + let rec = again + .wiring + .iter() + .find(|r| r.kind == KIND_LOCK_SPECIFIER || r.kind == KIND_LOCK_ROOT_DEP_PAIR) + .expect("specifier record"); + assert!( + rec.original.is_none(), + "{tag}: our own stale value is never recorded as the user's original: {:?}", + rec.original + ); + // The first vendor's ledger entry (the one carrying the user's + // original) still reverts the healed lock byte-for-byte. + let outcome = revert_pnpm_legacy(&entry, fx.root(), false).await; + assert!(outcome.success, "{tag}: {:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{tag}: {:?}", outcome.warnings); + assert_eq!(fx.read(PNPM_LOCK).await, before_lock, "{tag}"); + } + } + /// Revert restores BOTH files byte-identical (the packages block moves /// back across the sort boundary to its original slot) and removes the /// artifact dir — for BOTH grammars.